From 451c5c32c9c83e643708d91a97df11aef67d171d Mon Sep 17 00:00:00 2001 From: Donny Xia Date: Fri, 21 Aug 2020 12:05:11 -0700 Subject: [PATCH] code review --- kyaml/fn/runtime/container/container.go | 24 ++--------- kyaml/fn/runtime/container/container_test.go | 42 +++++++++++++------ kyaml/fn/runtime/runtimeutil/functiontypes.go | 25 ++++++++--- kyaml/runfn/runfn.go | 28 ++++++++----- kyaml/runfn/runfn_test.go | 4 +- 5 files changed, 73 insertions(+), 50 deletions(-) diff --git a/kyaml/fn/runtime/container/container.go b/kyaml/fn/runtime/container/container.go index 223290a3d..9f34f1512 100644 --- a/kyaml/fn/runtime/container/container.go +++ b/kyaml/fn/runtime/container/container.go @@ -123,18 +123,7 @@ import ( //    ├── deployment_foo.yaml //    └── service_bar.yaml type Filter struct { - - // Image is the container image to use to create a container. - Image string `yaml:"image,omitempty"` - - // Network is the container network to use. - Network string `yaml:"network,omitempty"` - - // StorageMounts is a list of storage options that the container will have mounted. - StorageMounts []runtimeutil.StorageMount `yaml:"mounts,omitempty"` - - // User username used to run the application in container, - User string + runtimeutil.ContainerSpec `json:",inline" yaml:",inline"` Exec runtimeexec.Filter } @@ -174,13 +163,8 @@ func (c *Filter) getCommand() (string, []string) { // was run from the cli. network := "none" - if c.Network != "" { - network = c.Network - } - // run as nobody by default - user := c.User - if user == "" { - user = "nobody" + if c.Network.Name != "" { + network = c.Network.Name } args := []string{"run", "--rm", // delete the container afterward @@ -188,7 +172,7 @@ func (c *Filter) getCommand() (string, []string) { "--network", network, // added security options - "--user", user, + "--user", c.User.String(), "--security-opt=no-new-privileges", // don't allow the user to escalate privileges // note: don't make fs readonly because things like heredoc rely on writing tmp files } diff --git a/kyaml/fn/runtime/container/container_test.go b/kyaml/fn/runtime/container/container_test.go index fd2d99bc3..bd6f0e6b3 100644 --- a/kyaml/fn/runtime/container/container_test.go +++ b/kyaml/fn/runtime/container/container_test.go @@ -38,7 +38,12 @@ metadata: "--user", "nobody", "--security-opt=no-new-privileges", }, - instance: Filter{Image: "example.com:version"}, + instance: Filter{ + ContainerSpec: runtimeutil.ContainerSpec{ + Image: "example.com:version", + User: "nobody", + }, + }, }, { @@ -56,7 +61,15 @@ metadata: "--user", "nobody", "--security-opt=no-new-privileges", }, - instance: Filter{Image: "example.com:version", Network: "test-1"}, + instance: Filter{ + ContainerSpec: runtimeutil.ContainerSpec{ + Image: "example.com:version", + Network: runtimeutil.ContainerNetwork{ + Name: "test-1", + }, + User: "nobody", + }, + }, }, { @@ -79,12 +92,15 @@ metadata: "--mount", fmt.Sprintf("type=%s,source=%s,target=%s,readonly", "tmpfs", "", "/local/"), }, instance: Filter{ - Image: "example.com:version", - StorageMounts: []runtimeutil.StorageMount{ - {MountType: "bind", Src: "/mount/path", DstPath: "/local/"}, - {MountType: "bind", Src: "/mount/pathrw", DstPath: "/localrw/", ReadWriteMode: true}, - {MountType: "volume", Src: "myvol", DstPath: "/local/"}, - {MountType: "tmpfs", Src: "", DstPath: "/local/"}, + ContainerSpec: runtimeutil.ContainerSpec{ + Image: "example.com:version", + StorageMounts: []runtimeutil.StorageMount{ + {MountType: "bind", Src: "/mount/path", DstPath: "/local/"}, + {MountType: "bind", Src: "/mount/pathrw", DstPath: "/localrw/", ReadWriteMode: true}, + {MountType: "volume", Src: "myvol", DstPath: "/local/"}, + {MountType: "tmpfs", Src: "", DstPath: "/local/"}, + }, + User: "nobody", }, }, }, @@ -104,8 +120,10 @@ metadata: "--security-opt=no-new-privileges", }, instance: Filter{ - Image: "example.com:version", - User: "root", + ContainerSpec: runtimeutil.ContainerSpec{ + Image: "example.com:version", + User: "root", + }, }, }, } @@ -203,7 +221,7 @@ metadata: } } func TestFilter_String(t *testing.T) { - instance := Filter{Image: "foo"} + instance := Filter{ContainerSpec: runtimeutil.ContainerSpec{Image: "foo"}} if !assert.Equal(t, "foo", instance.String()) { t.FailNow() } @@ -234,7 +252,7 @@ func TestFilter_ExitCode(t *testing.T) { func TestIgnoreEnv(t *testing.T) { os.Setenv(tmpDirEnvKey, "") - fltr := Filter{Image: "example.com:version"} + fltr := Filter{ContainerSpec: runtimeutil.ContainerSpec{Image: "example.com:version"}} _, args := fltr.getCommand() for _, arg := range args { if arg == tmpDirEnvKey { diff --git a/kyaml/fn/runtime/runtimeutil/functiontypes.go b/kyaml/fn/runtime/runtimeutil/functiontypes.go index 038bdb80e..7eb1c7056 100644 --- a/kyaml/fn/runtime/runtimeutil/functiontypes.go +++ b/kyaml/fn/runtime/runtimeutil/functiontypes.go @@ -18,11 +18,23 @@ const ( var functionAnnotationKeys = []string{FunctionAnnotationKey, oldFunctionAnnotationKey} +// ContainerUser is a type for username/uid used in container +type ContainerUser string + +func (u *ContainerUser) String() string { + return string(*u) +} + +func (u *ContainerUser) IsEmpty() bool { + return string(*u) == "" +} + +const ( + UserNobody ContainerUser = "nobody" +) + // FunctionSpec defines a spec for running a function type FunctionSpec struct { - // Network is the name of the network to use from a container - Network string `json:"network,omitempty" yaml:"network,omitempty"` - DeferFailure bool `json:"deferFailure,omitempty" yaml:"deferFailure,omitempty"` // Container is the spec for running a function as a container @@ -54,13 +66,16 @@ type ContainerSpec struct { StorageMounts []StorageMount `json:"mounts,omitempty" yaml:"mounts,omitempty"` // User is the username/uid that application runs as in continer - User string `json:"user,omitempty" yaml:"user,omitempty"` + User ContainerUser `json:"user,omitempty" yaml:"user,omitempty"` } // ContainerNetwork type ContainerNetwork struct { // Required specifies that function requires a network Required bool `json:"required,omitempty" yaml:"required,omitempty"` + + // Name is the name of the network to use from a container + Name string `json:"name,omitempty" yaml:"name,omitempty"` } // StarlarkSpec defines how to run a function as a starlark program @@ -113,7 +128,7 @@ func GetFunctionSpec(n *yaml.RNode) *FunctionSpec { } if fn := getFunctionSpecFromAnnotation(n, meta); fn != nil { - fn.Network = "" + fn.Container.Network.Name = "" fn.StorageMounts = []StorageMount{} return fn } diff --git a/kyaml/runfn/runfn.go b/kyaml/runfn/runfn.go index 5351d82f4..c757967e2 100644 --- a/kyaml/runfn/runfn.go +++ b/kyaml/runfn/runfn.go @@ -89,7 +89,7 @@ type RunFns struct { filter runtimeutil.FunctionSpec, api *yaml.RNode) (kio.Filter, error) // User username used to run the application in container, - User string + User runtimeutil.ContainerUser } // Execute runs the command @@ -280,8 +280,17 @@ func (r RunFns) getFunctionFilters(global bool, fns ...*yaml.RNode) ( // TODO(eddiezane): Provide error info about which function needs the network return fltrs, errors.Errorf("network required but not enabled with --network") } - spec.Network = r.NetworkName + spec.Container.Network.Name = r.NetworkName } + // command line username has higher priority + if r.User != "" { + spec.Container.User = r.User + } + // default user is nobody + if spec.Container.User.IsEmpty() { + spec.Container.User = runtimeutil.UserNobody + } + c, err := r.functionFilterProvider(*spec, api) if err != nil { return nil, err @@ -383,17 +392,14 @@ func (r *RunFns) ffp(spec runtimeutil.FunctionSpec, api *yaml.RNode) (kio.Filter atomic.AddUint32(&r.resultsCount, 1) } if !r.DisableContainers && spec.Container.Image != "" { - // command line username has higher priority - user := spec.Container.User - if r.User != "" { - user = r.User - } // TODO: Add a test for this behavior cf := &container.Filter{ - Image: spec.Container.Image, - Network: spec.Network, - StorageMounts: r.StorageMounts, - User: user, + ContainerSpec: runtimeutil.ContainerSpec{ + Image: spec.Container.Image, + Network: spec.Container.Network, + StorageMounts: r.StorageMounts, + User: spec.Container.User, + }, } cf.Exec.FunctionConfig = api cf.Exec.GlobalScope = r.GlobalScope diff --git a/kyaml/runfn/runfn_test.go b/kyaml/runfn/runfn_test.go index fd9237209..8599609fc 100644 --- a/kyaml/runfn/runfn_test.go +++ b/kyaml/runfn/runfn_test.go @@ -60,7 +60,7 @@ kind: return } filter, _ := instance.functionFilterProvider(spec, api) - cf := &container.Filter{Image: "example.com:version"} + cf := &container.Filter{ContainerSpec: runtimeutil.ContainerSpec{Image: "example.com:version"}} cf.Exec.FunctionConfig = api assert.Equal(t, cf, filter) } @@ -90,7 +90,7 @@ kind: return } filter, _ := instance.functionFilterProvider(spec, api) - cf := &container.Filter{Image: "example.com:version"} + cf := &container.Filter{ContainerSpec: runtimeutil.ContainerSpec{Image: "example.com:version"}} cf.Exec.FunctionConfig = api cf.Exec.GlobalScope = true assert.Equal(t, cf, filter)