From 904a9dea081579d64fd3099645f36f175c2dcb47 Mon Sep 17 00:00:00 2001 From: Donny Xia Date: Mon, 24 Aug 2020 13:35:18 -0700 Subject: [PATCH 1/3] explicitly specify envs to be exported in function --- kyaml/fn/runtime/container/container.go | 16 +-- kyaml/fn/runtime/container/container_test.go | 16 +-- kyaml/fn/runtime/runtimeutil/functiontypes.go | 75 +++++++++++++ .../runtime/runtimeutil/runtimeutil_test.go | 40 +++++++ kyaml/runfn/runfn.go | 23 +++- kyaml/runfn/runfn_test.go | 101 ++++++++++++++++++ 6 files changed, 242 insertions(+), 29 deletions(-) diff --git a/kyaml/fn/runtime/container/container.go b/kyaml/fn/runtime/container/container.go index 8e9bf52f8..efe15681f 100644 --- a/kyaml/fn/runtime/container/container.go +++ b/kyaml/fn/runtime/container/container.go @@ -5,8 +5,6 @@ package container import ( "fmt" - "os" - "strings" runtimeexec "sigs.k8s.io/kustomize/kyaml/fn/runtime/exec" "sigs.k8s.io/kustomize/kyaml/fn/runtime/runtimeutil" @@ -177,19 +175,7 @@ func (c *Filter) getCommand() (string, []string) { args = append(args, "--mount", storageMount.String()) } - // TODO: put these env processes into a separate function and call it in the outside of - // getCommand - os.Setenv("LOG_TO_STDERR", "true") - os.Setenv("STRUCTURED_RESULTS", "true") - - // export the local environment vars to the container - for _, pair := range os.Environ() { - items := strings.Split(pair, "=") - if items[0] == "" || items[1] == "" || items[0] == tmpDirEnvKey { - continue - } - args = append(args, "-e", items[0]) - } + args = append(args, c.Envs.GetDockerFlags()...) a := append(args, c.Image) return "docker", a } diff --git a/kyaml/fn/runtime/container/container_test.go b/kyaml/fn/runtime/container/container_test.go index c0833f5b2..754b274d9 100644 --- a/kyaml/fn/runtime/container/container_test.go +++ b/kyaml/fn/runtime/container/container_test.go @@ -7,7 +7,6 @@ import ( "bytes" "fmt" "os" - "strings" "testing" "github.com/stretchr/testify/assert" @@ -137,20 +136,13 @@ metadata: } tt.instance.Exec.FunctionConfig = cfg - os.Setenv("KYAML_TEST", "FOO") - tt.instance.setupExec() + tt.instance.Envs.AddKeyValue("KYAML_TEST", "FOO") + tt.expectedArgs = append(tt.expectedArgs, tt.instance.Envs.GetDockerFlags()...) - // configure expected env - for _, e := range os.Environ() { - // the process env - parts := strings.Split(e, "=") - if parts[0] == "" || parts[1] == "" || parts[0] == tmpDirEnvKey { - continue - } - tt.expectedArgs = append(tt.expectedArgs, "-e", parts[0]) - } tt.expectedArgs = append(tt.expectedArgs, tt.instance.Image) + tt.instance.setupExec() + if !assert.Equal(t, "docker", tt.instance.Exec.Path) { t.FailNow() } diff --git a/kyaml/fn/runtime/runtimeutil/functiontypes.go b/kyaml/fn/runtime/runtimeutil/functiontypes.go index 869d27d20..5c193dba9 100644 --- a/kyaml/fn/runtime/runtimeutil/functiontypes.go +++ b/kyaml/fn/runtime/runtimeutil/functiontypes.go @@ -6,6 +6,7 @@ package runtimeutil import ( "fmt" "os" + "sort" "strings" "sigs.k8s.io/kustomize/kyaml/yaml" @@ -40,6 +41,77 @@ const ( NetworkNameNone ContainerNetworkName = "none" NetworkNameEmpty ContainerNetworkName = "" ) +const defaultEnvValue string = "true" + +// ContainerEnvs contains the environment variables for the container +type ContainerEnvs struct { + // EnvsMap is a key-value map that will be set as env in container + EnvsMap map[string]string `json:"envsMap,omitempty" yaml:"envsMap,omitempty"` + + // ExportKeys are only env key. Value will be the value in the host system + ExportKeys []string `json:"exportKeys,omitempty" yaml:"exportKeys,omitempty"` +} + +// GetDockerFlags returns docker run style env flags +func (ce *ContainerEnvs) GetDockerFlags() []string { + envs := ce.EnvsMap + if envs == nil { + envs = make(map[string]string) + } + + // default envs + envs["LOG_TO_STDERR"] = defaultEnvValue + envs["STRUCTURED_RESULTS"] = defaultEnvValue + + flags := []string{} + // return in order to keep consistent among different runs + keys := []string{} + for k := range envs { + keys = append(keys, k) + } + sort.Strings(keys) + for _, key := range keys { + flags = append(flags, "-e", key+"="+envs[key]) + } + + for _, key := range ce.ExportKeys { + flags = append(flags, "-e", key) + } + + return flags +} + +// AddKeyValue adds a key-value pair into the envs +func (ce *ContainerEnvs) AddKeyValue(key, value string) { + if ce.EnvsMap == nil { + ce.EnvsMap = make(map[string]string) + } + ce.EnvsMap[key] = value +} + +// HasExportedKey returns true if the key is a exported key +func (ce *ContainerEnvs) HasExportedKey(key string) bool { + for _, k := range ce.ExportKeys { + if k == key { + return true + } + } + return false +} + +// AddKey adds a key into the envs +func (ce *ContainerEnvs) AddKey(key string) { + if !ce.HasExportedKey(key) { + ce.ExportKeys = append(ce.ExportKeys, key) + } +} + +// NewContainerEnvs returns an empty instance of ContainerEnvs +func NewContainerEnvs() ContainerEnvs { + var ce ContainerEnvs + ce.EnvsMap = make(map[string]string) + return ce +} // FunctionSpec defines a spec for running a function type FunctionSpec struct { @@ -75,6 +147,9 @@ type ContainerSpec struct { // User is the username/uid that application runs as in continer User ContainerUser `json:"user,omitempty" yaml:"user,omitempty"` + + // Envs are environment variables that will be exported to container + Envs ContainerEnvs `json:"envs,omitempty" yaml:"envs,omitempty"` } // ContainerNetwork diff --git a/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go b/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go index 89b728cf1..be62f8202 100644 --- a/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go +++ b/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go @@ -1428,3 +1428,43 @@ func Test_StringToStorageMount(t *testing.T) { assert.Equal(t, tc.expectedOut, (&s).String()) } } + +func TestContainerEnvs(t *testing.T) { + tests := []struct { + input ContainerEnvs + output []string + }{ + { + input: ContainerEnvs{ + EnvsMap: map[string]string{ + "foo": "bar", + }, + }, + output: []string{"-e", "LOG_TO_STDERR=true", "-e", "STRUCTURED_RESULTS=true", "-e", "foo=bar"}, + }, + { + input: ContainerEnvs{ + ExportKeys: []string{"foo"}, + }, + output: []string{"-e", "LOG_TO_STDERR=true", "-e", "STRUCTURED_RESULTS=true", "-e", "foo"}, + }, + { + input: ContainerEnvs{ + EnvsMap: map[string]string{ + "foo": "bar", + }, + ExportKeys: []string{"baz"}, + }, + output: []string{"-e", "LOG_TO_STDERR=true", "-e", "STRUCTURED_RESULTS=true", "-e", "foo=bar", "-e", "baz"}, + }, + { + input: ContainerEnvs{}, + output: []string{"-e", "LOG_TO_STDERR=true", "-e", "STRUCTURED_RESULTS=true"}, + }, + } + + for _, tc := range tests { + flags := tc.input.GetDockerFlags() + assert.Equal(t, tc.output, flags) + } +} diff --git a/kyaml/runfn/runfn.go b/kyaml/runfn/runfn.go index 4b743df2b..4030492fb 100644 --- a/kyaml/runfn/runfn.go +++ b/kyaml/runfn/runfn.go @@ -90,6 +90,9 @@ type RunFns struct { // User username used to run the application in container, User runtimeutil.ContainerUser + + // Envs are environment variables that will be exported to container + Envs runtimeutil.ContainerEnvs } // Execute runs the command @@ -269,6 +272,20 @@ func (r RunFns) getFunctionsFromFunctions() ([]kio.Filter, error) { return r.getFunctionFilters(true, r.Functions...) } +// mergeContainerEnvs will merge the envs specified by command line (imperative) and config +// file (declarative). If they have same key, the imperative value will be respected. +func (r RunFns) mergeContainerEnvs(envs runtimeutil.ContainerEnvs) runtimeutil.ContainerEnvs { + for key, value := range r.Envs.EnvsMap { + envs.AddKeyValue(key, value) + } + + for _, key := range r.Envs.ExportKeys { + envs.AddKey(key) + } + + return envs +} + func (r RunFns) getFunctionFilters(global bool, fns ...*yaml.RNode) ( []kio.Filter, error) { var fltrs []kio.Filter @@ -282,10 +299,11 @@ func (r RunFns) getFunctionFilters(global bool, fns ...*yaml.RNode) ( } spec.Container.Network.Name = runtimeutil.ContainerNetworkName(r.NetworkName) } - // command line username has higher priority - if r.User != "" { + // command line username and envs has higher priority + if !r.User.IsEmpty() { spec.Container.User = r.User } + spec.Container.Envs = r.mergeContainerEnvs(spec.Container.Envs) c, err := r.functionFilterProvider(*spec, api) if err != nil { @@ -394,6 +412,7 @@ func (r *RunFns) ffp(spec runtimeutil.FunctionSpec, api *yaml.RNode) (kio.Filter Network: spec.Container.Network, StorageMounts: r.StorageMounts, User: spec.Container.User, + Envs: spec.Container.Envs, }) cf := &c cf.Exec.FunctionConfig = api diff --git a/kyaml/runfn/runfn_test.go b/kyaml/runfn/runfn_test.go index 55cb2d429..491372097 100644 --- a/kyaml/runfn/runfn_test.go +++ b/kyaml/runfn/runfn_test.go @@ -986,3 +986,104 @@ func getFilterProvider(t *testing.T) func(runtimeutil.FunctionSpec, *yaml.RNode) }, nil } } + +func TestRunfns_mergeContainerEnvs(t *testing.T) { + testcases := []struct { + name string + instance RunFns + inputEnvs runtimeutil.ContainerEnvs + expect runtimeutil.ContainerEnvs + }{ + { + name: "all empty", + instance: RunFns{}, + inputEnvs: runtimeutil.NewContainerEnvs(), + expect: runtimeutil.NewContainerEnvs(), + }, + { + name: "empty command line envs", + instance: RunFns{}, + inputEnvs: runtimeutil.ContainerEnvs{ + EnvsMap: map[string]string{ + "foo": "bar", + }, + }, + expect: runtimeutil.ContainerEnvs{ + EnvsMap: map[string]string{ + "foo": "bar", + }, + }, + }, + { + name: "empty declarative envs", + instance: RunFns{ + Envs: runtimeutil.ContainerEnvs{ + EnvsMap: map[string]string{ + "foo": "bar", + }, + }, + }, + inputEnvs: runtimeutil.NewContainerEnvs(), + expect: runtimeutil.ContainerEnvs{ + EnvsMap: map[string]string{ + "foo": "bar", + }, + }, + }, + { + name: "same key", + instance: RunFns{ + Envs: runtimeutil.ContainerEnvs{ + EnvsMap: map[string]string{ + "foo": "bar", + }, + ExportKeys: []string{"foo"}, + }, + }, + inputEnvs: runtimeutil.ContainerEnvs{ + EnvsMap: map[string]string{ + "foo": "bar1", + }, + ExportKeys: []string{"bar"}, + }, + expect: runtimeutil.ContainerEnvs{ + EnvsMap: map[string]string{ + "foo": "bar", + }, + ExportKeys: []string{"bar", "foo"}, + }, + }, + { + name: "same exported key", + instance: RunFns{ + Envs: runtimeutil.ContainerEnvs{ + EnvsMap: map[string]string{ + "foo": "bar", + }, + ExportKeys: []string{"foo"}, + }, + }, + inputEnvs: runtimeutil.ContainerEnvs{ + EnvsMap: map[string]string{ + "foo1": "bar1", + }, + ExportKeys: []string{"foo"}, + }, + expect: runtimeutil.ContainerEnvs{ + EnvsMap: map[string]string{ + "foo": "bar", + "foo1": "bar1", + }, + ExportKeys: []string{"foo"}, + }, + }, + } + + for i := range testcases { + tc := testcases[i] + t.Run(tc.name, func(t *testing.T) { + envs := tc.instance.mergeContainerEnvs(tc.inputEnvs) + assert.Equal(t, tc.expect.GetDockerFlags(), envs.GetDockerFlags()) + }) + } +} From 46194b33855d97cc3281d50a50e127b585e51e8c Mon Sep 17 00:00:00 2001 From: Donny Xia Date: Tue, 25 Aug 2020 12:10:27 -0700 Subject: [PATCH 2/3] code review --- kyaml/fn/runtime/container/container.go | 2 +- kyaml/fn/runtime/container/container_test.go | 10 +-- kyaml/fn/runtime/runtimeutil/functiontypes.go | 72 ++++++++++------ .../runtime/runtimeutil/runtimeutil_test.go | 83 ++++++++++++++---- kyaml/runfn/runfn.go | 16 ++-- kyaml/runfn/runfn_test.go | 85 +++++-------------- 6 files changed, 143 insertions(+), 125 deletions(-) diff --git a/kyaml/fn/runtime/container/container.go b/kyaml/fn/runtime/container/container.go index efe15681f..b37263f6c 100644 --- a/kyaml/fn/runtime/container/container.go +++ b/kyaml/fn/runtime/container/container.go @@ -175,7 +175,7 @@ func (c *Filter) getCommand() (string, []string) { args = append(args, "--mount", storageMount.String()) } - args = append(args, c.Envs.GetDockerFlags()...) + args = append(args, c.Env.GetDockerFlags()...) a := append(args, c.Image) return "docker", a } diff --git a/kyaml/fn/runtime/container/container_test.go b/kyaml/fn/runtime/container/container_test.go index 754b274d9..5921ee0f9 100644 --- a/kyaml/fn/runtime/container/container_test.go +++ b/kyaml/fn/runtime/container/container_test.go @@ -135,14 +135,12 @@ metadata: t.FailNow() } tt.instance.Exec.FunctionConfig = cfg - - tt.instance.Envs.AddKeyValue("KYAML_TEST", "FOO") - tt.expectedArgs = append(tt.expectedArgs, tt.instance.Envs.GetDockerFlags()...) - - tt.expectedArgs = append(tt.expectedArgs, tt.instance.Image) - + tt.instance.Env.AddKeyValue("KYAML_TEST", "FOO") tt.instance.setupExec() + tt.expectedArgs = append(tt.expectedArgs, tt.instance.Env.GetDockerFlags()...) + tt.expectedArgs = append(tt.expectedArgs, tt.instance.Image) + if !assert.Equal(t, "docker", tt.instance.Exec.Path) { t.FailNow() } diff --git a/kyaml/fn/runtime/runtimeutil/functiontypes.go b/kyaml/fn/runtime/runtimeutil/functiontypes.go index 5c193dba9..95ccec112 100644 --- a/kyaml/fn/runtime/runtimeutil/functiontypes.go +++ b/kyaml/fn/runtime/runtimeutil/functiontypes.go @@ -43,26 +43,22 @@ const ( ) const defaultEnvValue string = "true" -// ContainerEnvs contains the environment variables for the container -type ContainerEnvs struct { - // EnvsMap is a key-value map that will be set as env in container - EnvsMap map[string]string `json:"envsMap,omitempty" yaml:"envsMap,omitempty"` +// ContainerEnv defines the environment present in a container. +type ContainerEnv struct { + // EnvVars is a key-value map that will be set as env in container + EnvVars map[string]string - // ExportKeys are only env key. Value will be the value in the host system - ExportKeys []string `json:"exportKeys,omitempty" yaml:"exportKeys,omitempty"` + // VarsToExport are only env key. Value will be the value in the host system + VarsToExport []string } // GetDockerFlags returns docker run style env flags -func (ce *ContainerEnvs) GetDockerFlags() []string { - envs := ce.EnvsMap +func (ce *ContainerEnv) GetDockerFlags() []string { + envs := ce.EnvVars if envs == nil { envs = make(map[string]string) } - // default envs - envs["LOG_TO_STDERR"] = defaultEnvValue - envs["STRUCTURED_RESULTS"] = defaultEnvValue - flags := []string{} // return in order to keep consistent among different runs keys := []string{} @@ -74,7 +70,7 @@ func (ce *ContainerEnvs) GetDockerFlags() []string { flags = append(flags, "-e", key+"="+envs[key]) } - for _, key := range ce.ExportKeys { + for _, key := range ce.VarsToExport { flags = append(flags, "-e", key) } @@ -82,16 +78,16 @@ func (ce *ContainerEnvs) GetDockerFlags() []string { } // AddKeyValue adds a key-value pair into the envs -func (ce *ContainerEnvs) AddKeyValue(key, value string) { - if ce.EnvsMap == nil { - ce.EnvsMap = make(map[string]string) +func (ce *ContainerEnv) AddKeyValue(key, value string) { + if ce.EnvVars == nil { + ce.EnvVars = make(map[string]string) } - ce.EnvsMap[key] = value + ce.EnvVars[key] = value } // HasExportedKey returns true if the key is a exported key -func (ce *ContainerEnvs) HasExportedKey(key string) bool { - for _, k := range ce.ExportKeys { +func (ce *ContainerEnv) HasExportedKey(key string) bool { + for _, k := range ce.VarsToExport { if k == key { return true } @@ -100,16 +96,34 @@ func (ce *ContainerEnvs) HasExportedKey(key string) bool { } // AddKey adds a key into the envs -func (ce *ContainerEnvs) AddKey(key string) { +func (ce *ContainerEnv) AddKey(key string) { if !ce.HasExportedKey(key) { - ce.ExportKeys = append(ce.ExportKeys, key) + ce.VarsToExport = append(ce.VarsToExport, key) } } -// NewContainerEnvs returns an empty instance of ContainerEnvs -func NewContainerEnvs() ContainerEnvs { - var ce ContainerEnvs - ce.EnvsMap = make(map[string]string) +// NewContainerEnv returns a pointer to a new ContainerEnv +func NewContainerEnv() *ContainerEnv { + var ce ContainerEnv + ce.EnvVars = make(map[string]string) + // default envs + ce.EnvVars["LOG_TO_STDERR"] = defaultEnvValue + ce.EnvVars["STRUCTURED_RESULTS"] = defaultEnvValue + return &ce +} + +// NewContainerEnvFromStringSlice returns a new ContainerEnv pointer with parsing +// input envStr. envStr example: ["foo=bar", "baz"] +func NewContainerEnvFromStringSlice(envStr []string) *ContainerEnv { + ce := NewContainerEnv() + for _, e := range envStr { + parts := strings.SplitN(e, "=", 2) + if len(parts) == 1 { + ce.AddKey(e) + } else { + ce.AddKeyValue(parts[0], parts[1]) + } + } return ce } @@ -148,8 +162,11 @@ type ContainerSpec struct { // User is the username/uid that application runs as in continer User ContainerUser `json:"user,omitempty" yaml:"user,omitempty"` - // Envs are environment variables that will be exported to container - Envs ContainerEnvs `json:"envs,omitempty" yaml:"envs,omitempty"` + // EnvRaw is a slice of env string. + EnvRaw []string `json:"envs,omitempty" yaml:"envs,omitempty"` + + // Env contains environment variables that will be exported to container + Env ContainerEnv `json:"-" yaml:"-"` } // ContainerNetwork @@ -213,6 +230,7 @@ func GetFunctionSpec(n *yaml.RNode) *FunctionSpec { if fn := getFunctionSpecFromAnnotation(n, meta); fn != nil { fn.Container.Network.Name = NetworkNameEmpty fn.StorageMounts = []StorageMount{} + fn.Container.Env = *NewContainerEnvFromStringSlice(fn.Container.EnvRaw) return fn } diff --git a/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go b/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go index be62f8202..573ea1246 100644 --- a/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go +++ b/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go @@ -1429,36 +1429,25 @@ func Test_StringToStorageMount(t *testing.T) { } } -func TestContainerEnvs(t *testing.T) { +func TestContainerEnvGetDockerFlags(t *testing.T) { tests := []struct { - input ContainerEnvs + input *ContainerEnv output []string }{ { - input: ContainerEnvs{ - EnvsMap: map[string]string{ - "foo": "bar", - }, - }, + input: NewContainerEnvFromStringSlice([]string{"foo=bar"}), output: []string{"-e", "LOG_TO_STDERR=true", "-e", "STRUCTURED_RESULTS=true", "-e", "foo=bar"}, }, { - input: ContainerEnvs{ - ExportKeys: []string{"foo"}, - }, + input: NewContainerEnvFromStringSlice([]string{"foo"}), output: []string{"-e", "LOG_TO_STDERR=true", "-e", "STRUCTURED_RESULTS=true", "-e", "foo"}, }, { - input: ContainerEnvs{ - EnvsMap: map[string]string{ - "foo": "bar", - }, - ExportKeys: []string{"baz"}, - }, + input: NewContainerEnvFromStringSlice([]string{"foo=bar", "baz"}), output: []string{"-e", "LOG_TO_STDERR=true", "-e", "STRUCTURED_RESULTS=true", "-e", "foo=bar", "-e", "baz"}, }, { - input: ContainerEnvs{}, + input: NewContainerEnv(), output: []string{"-e", "LOG_TO_STDERR=true", "-e", "STRUCTURED_RESULTS=true"}, }, } @@ -1468,3 +1457,63 @@ func TestContainerEnvs(t *testing.T) { assert.Equal(t, tc.output, flags) } } + +func TestGetContainerEnv(t *testing.T) { + tests := []struct { + input string + expected ContainerEnv + }{ + { + input: ` +apiVersion: v1 +kind: Foo +metadata: + name: foo + configFn: + container: + image: gcr.io/kustomize-functions/example-tshirt:v0.1.0 + envs: + - foo=bar +`, + expected: *NewContainerEnvFromStringSlice([]string{"foo=bar"}), + }, + { + input: ` +apiVersion: v1 +kind: Foo +metadata: + name: foo + configFn: + container: + image: gcr.io/kustomize-functions/example-tshirt:v0.1.0 + envs: + - foo=bar + - baz +`, + expected: *NewContainerEnvFromStringSlice([]string{"foo=bar", "baz"}), + }, + { + input: ` +apiVersion: v1 +kind: Foo +metadata: + name: foo + configFn: + container: + image: gcr.io/kustomize-functions/example-tshirt:v0.1.0 + envs: + - KUBECONFIG +`, + expected: *NewContainerEnvFromStringSlice([]string{"KUBECONFIG"}), + }, + } + + for _, tc := range tests { + cfg, err := yaml.Parse(tc.input) + if !assert.NoError(t, err) { + return + } + fn := GetFunctionSpec(cfg) + assert.Equal(t, tc.expected, fn.Container.Env) + } +} diff --git a/kyaml/runfn/runfn.go b/kyaml/runfn/runfn.go index 4030492fb..b3ce7281a 100644 --- a/kyaml/runfn/runfn.go +++ b/kyaml/runfn/runfn.go @@ -91,8 +91,8 @@ type RunFns struct { // User username used to run the application in container, User runtimeutil.ContainerUser - // Envs are environment variables that will be exported to container - Envs runtimeutil.ContainerEnvs + // Env contains environment variables that will be exported to container + Env runtimeutil.ContainerEnv } // Execute runs the command @@ -272,14 +272,14 @@ func (r RunFns) getFunctionsFromFunctions() ([]kio.Filter, error) { return r.getFunctionFilters(true, r.Functions...) } -// mergeContainerEnvs will merge the envs specified by command line (imperative) and config +// mergeContainerEnv will merge the envs specified by command line (imperative) and config // file (declarative). If they have same key, the imperative value will be respected. -func (r RunFns) mergeContainerEnvs(envs runtimeutil.ContainerEnvs) runtimeutil.ContainerEnvs { - for key, value := range r.Envs.EnvsMap { +func (r RunFns) mergeContainerEnv(envs runtimeutil.ContainerEnv) runtimeutil.ContainerEnv { + for key, value := range r.Env.EnvVars { envs.AddKeyValue(key, value) } - for _, key := range r.Envs.ExportKeys { + for _, key := range r.Env.VarsToExport { envs.AddKey(key) } @@ -303,7 +303,7 @@ func (r RunFns) getFunctionFilters(global bool, fns ...*yaml.RNode) ( if !r.User.IsEmpty() { spec.Container.User = r.User } - spec.Container.Envs = r.mergeContainerEnvs(spec.Container.Envs) + spec.Container.Env = r.mergeContainerEnv(spec.Container.Env) c, err := r.functionFilterProvider(*spec, api) if err != nil { @@ -412,7 +412,7 @@ func (r *RunFns) ffp(spec runtimeutil.FunctionSpec, api *yaml.RNode) (kio.Filter Network: spec.Container.Network, StorageMounts: r.StorageMounts, User: spec.Container.User, - Envs: spec.Container.Envs, + Env: spec.Container.Env, }) cf := &c cf.Exec.FunctionConfig = api diff --git a/kyaml/runfn/runfn_test.go b/kyaml/runfn/runfn_test.go index 491372097..8d10ffea3 100644 --- a/kyaml/runfn/runfn_test.go +++ b/kyaml/runfn/runfn_test.go @@ -987,102 +987,55 @@ func getFilterProvider(t *testing.T) func(runtimeutil.FunctionSpec, *yaml.RNode) } } -func TestRunfns_mergeContainerEnvs(t *testing.T) { +func TestRunfns_mergeContainerEnv(t *testing.T) { testcases := []struct { name string instance RunFns - inputEnvs runtimeutil.ContainerEnvs - expect runtimeutil.ContainerEnvs + inputEnvs runtimeutil.ContainerEnv + expect runtimeutil.ContainerEnv }{ { name: "all empty", instance: RunFns{}, - inputEnvs: runtimeutil.NewContainerEnvs(), - expect: runtimeutil.NewContainerEnvs(), + inputEnvs: *runtimeutil.NewContainerEnv(), + expect: *runtimeutil.NewContainerEnv(), }, { - name: "empty command line envs", - instance: RunFns{}, - inputEnvs: runtimeutil.ContainerEnvs{ - EnvsMap: map[string]string{ - "foo": "bar", - }, - }, - expect: runtimeutil.ContainerEnvs{ - EnvsMap: map[string]string{ - "foo": "bar", - }, - }, + name: "empty command line envs", + instance: RunFns{}, + inputEnvs: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar"}), + expect: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar"}), }, { name: "empty declarative envs", instance: RunFns{ - Envs: runtimeutil.ContainerEnvs{ - EnvsMap: map[string]string{ - "foo": "bar", - }, - }, - }, - inputEnvs: runtimeutil.NewContainerEnvs(), - expect: runtimeutil.ContainerEnvs{ - EnvsMap: map[string]string{ - "foo": "bar", - }, + Env: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar"}), }, + inputEnvs: *runtimeutil.NewContainerEnv(), + expect: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar"}), }, { name: "same key", instance: RunFns{ - Envs: runtimeutil.ContainerEnvs{ - EnvsMap: map[string]string{ - "foo": "bar", - }, - ExportKeys: []string{"foo"}, - }, - }, - inputEnvs: runtimeutil.ContainerEnvs{ - EnvsMap: map[string]string{ - "foo": "bar1", - }, - ExportKeys: []string{"bar"}, - }, - expect: runtimeutil.ContainerEnvs{ - EnvsMap: map[string]string{ - "foo": "bar", - }, - ExportKeys: []string{"bar", "foo"}, + Env: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar", "foo"}), }, + inputEnvs: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar1", "bar"}), + expect: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar", "bar", "foo"}), }, { name: "same exported key", instance: RunFns{ - Envs: runtimeutil.ContainerEnvs{ - EnvsMap: map[string]string{ - "foo": "bar", - }, - ExportKeys: []string{"foo"}, - }, - }, - inputEnvs: runtimeutil.ContainerEnvs{ - EnvsMap: map[string]string{ - "foo1": "bar1", - }, - ExportKeys: []string{"foo"}, - }, - expect: runtimeutil.ContainerEnvs{ - EnvsMap: map[string]string{ - "foo": "bar", - "foo1": "bar1", - }, - ExportKeys: []string{"foo"}, + Env: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar", "foo"}), }, + inputEnvs: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo1=bar1", "foo"}), + expect: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar", "foo1=bar1", "foo"}), }, } for i := range testcases { tc := testcases[i] t.Run(tc.name, func(t *testing.T) { - envs := tc.instance.mergeContainerEnvs(tc.inputEnvs) + envs := tc.instance.mergeContainerEnv(tc.inputEnvs) assert.Equal(t, tc.expect.GetDockerFlags(), envs.GetDockerFlags()) }) } From c202be0338a3e8ec5186244dd84153b1fe114c01 Mon Sep 17 00:00:00 2001 From: Donny Xia Date: Wed, 26 Aug 2020 12:49:42 -0700 Subject: [PATCH 3/3] Remove redundant env field --- kyaml/fn/runtime/container/container.go | 4 +-- kyaml/fn/runtime/container/container_test.go | 18 +++---------- kyaml/fn/runtime/runtimeutil/functiontypes.go | 20 +++++++++----- .../runtime/runtimeutil/runtimeutil_test.go | 2 +- kyaml/runfn/runfn.go | 16 +++++++----- kyaml/runfn/runfn_test.go | 26 +++++++++---------- 6 files changed, 40 insertions(+), 46 deletions(-) diff --git a/kyaml/fn/runtime/container/container.go b/kyaml/fn/runtime/container/container.go index b37263f6c..4b82419d5 100644 --- a/kyaml/fn/runtime/container/container.go +++ b/kyaml/fn/runtime/container/container.go @@ -152,8 +152,6 @@ func (c *Filter) setupExec() { c.Exec.Args = args } -var tmpDirEnvKey string = "TMPDIR" - // getArgs returns the command + args to run to spawn the container func (c *Filter) getCommand() (string, []string) { // run the container using docker. this is simpler than using the docker @@ -175,7 +173,7 @@ func (c *Filter) getCommand() (string, []string) { args = append(args, "--mount", storageMount.String()) } - args = append(args, c.Env.GetDockerFlags()...) + args = append(args, runtimeutil.NewContainerEnvFromStringSlice(c.Env).GetDockerFlags()...) a := append(args, c.Image) return "docker", a } diff --git a/kyaml/fn/runtime/container/container_test.go b/kyaml/fn/runtime/container/container_test.go index 5921ee0f9..23d83c58d 100644 --- a/kyaml/fn/runtime/container/container_test.go +++ b/kyaml/fn/runtime/container/container_test.go @@ -6,7 +6,6 @@ package container import ( "bytes" "fmt" - "os" "testing" "github.com/stretchr/testify/assert" @@ -135,10 +134,11 @@ metadata: t.FailNow() } tt.instance.Exec.FunctionConfig = cfg - tt.instance.Env.AddKeyValue("KYAML_TEST", "FOO") + tt.instance.Env = append(tt.instance.Env, "KYAML_TEST=FOO") tt.instance.setupExec() - tt.expectedArgs = append(tt.expectedArgs, tt.instance.Env.GetDockerFlags()...) + tt.expectedArgs = append(tt.expectedArgs, + runtimeutil.NewContainerEnvFromStringSlice(tt.instance.Env).GetDockerFlags()...) tt.expectedArgs = append(tt.expectedArgs, tt.instance.Image) if !assert.Equal(t, "docker", tt.instance.Exec.Path) { @@ -238,15 +238,3 @@ func TestFilter_ExitCode(t *testing.T) { t.FailNow() } } - -func TestIgnoreEnv(t *testing.T) { - os.Setenv(tmpDirEnvKey, "") - - fltr := Filter{ContainerSpec: runtimeutil.ContainerSpec{Image: "example.com:version"}} - _, args := fltr.getCommand() - for _, arg := range args { - if arg == tmpDirEnvKey { - t.Fatalf("%s should not be exported to container", tmpDirEnvKey) - } - } -} diff --git a/kyaml/fn/runtime/runtimeutil/functiontypes.go b/kyaml/fn/runtime/runtimeutil/functiontypes.go index 95ccec112..36546bd74 100644 --- a/kyaml/fn/runtime/runtimeutil/functiontypes.go +++ b/kyaml/fn/runtime/runtimeutil/functiontypes.go @@ -102,6 +102,18 @@ func (ce *ContainerEnv) AddKey(key string) { } } +// Raw returns a slice of string which represents the envs. +// Example: [foo=bar, baz] +func (ce *ContainerEnv) Raw() []string { + var ret []string + for k, v := range ce.EnvVars { + ret = append(ret, k+"="+v) + } + + ret = append(ret, ce.VarsToExport...) + return ret +} + // NewContainerEnv returns a pointer to a new ContainerEnv func NewContainerEnv() *ContainerEnv { var ce ContainerEnv @@ -162,11 +174,8 @@ type ContainerSpec struct { // User is the username/uid that application runs as in continer User ContainerUser `json:"user,omitempty" yaml:"user,omitempty"` - // EnvRaw is a slice of env string. - EnvRaw []string `json:"envs,omitempty" yaml:"envs,omitempty"` - - // Env contains environment variables that will be exported to container - Env ContainerEnv `json:"-" yaml:"-"` + // Env is a slice of env string that will be exposed to container + Env []string `json:"envs,omitempty" yaml:"envs,omitempty"` } // ContainerNetwork @@ -230,7 +239,6 @@ func GetFunctionSpec(n *yaml.RNode) *FunctionSpec { if fn := getFunctionSpecFromAnnotation(n, meta); fn != nil { fn.Container.Network.Name = NetworkNameEmpty fn.StorageMounts = []StorageMount{} - fn.Container.Env = *NewContainerEnvFromStringSlice(fn.Container.EnvRaw) return fn } diff --git a/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go b/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go index 573ea1246..3083851b8 100644 --- a/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go +++ b/kyaml/fn/runtime/runtimeutil/runtimeutil_test.go @@ -1514,6 +1514,6 @@ metadata: return } fn := GetFunctionSpec(cfg) - assert.Equal(t, tc.expected, fn.Container.Env) + assert.Equal(t, tc.expected, *NewContainerEnvFromStringSlice(fn.Container.Env)) } } diff --git a/kyaml/runfn/runfn.go b/kyaml/runfn/runfn.go index b3ce7281a..a2ab90dad 100644 --- a/kyaml/runfn/runfn.go +++ b/kyaml/runfn/runfn.go @@ -92,7 +92,7 @@ type RunFns struct { User runtimeutil.ContainerUser // Env contains environment variables that will be exported to container - Env runtimeutil.ContainerEnv + Env []string } // Execute runs the command @@ -274,16 +274,18 @@ func (r RunFns) getFunctionsFromFunctions() ([]kio.Filter, error) { // mergeContainerEnv will merge the envs specified by command line (imperative) and config // file (declarative). If they have same key, the imperative value will be respected. -func (r RunFns) mergeContainerEnv(envs runtimeutil.ContainerEnv) runtimeutil.ContainerEnv { - for key, value := range r.Env.EnvVars { - envs.AddKeyValue(key, value) +func (r RunFns) mergeContainerEnv(envs []string) []string { + imperative := runtimeutil.NewContainerEnvFromStringSlice(r.Env) + declarative := runtimeutil.NewContainerEnvFromStringSlice(envs) + for key, value := range imperative.EnvVars { + declarative.AddKeyValue(key, value) } - for _, key := range r.Env.VarsToExport { - envs.AddKey(key) + for _, key := range imperative.VarsToExport { + declarative.AddKey(key) } - return envs + return declarative.Raw() } func (r RunFns) getFunctionFilters(global bool, fns ...*yaml.RNode) ( diff --git a/kyaml/runfn/runfn_test.go b/kyaml/runfn/runfn_test.go index 8d10ffea3..dff30c4ef 100644 --- a/kyaml/runfn/runfn_test.go +++ b/kyaml/runfn/runfn_test.go @@ -991,43 +991,41 @@ func TestRunfns_mergeContainerEnv(t *testing.T) { testcases := []struct { name string instance RunFns - inputEnvs runtimeutil.ContainerEnv + inputEnvs []string expect runtimeutil.ContainerEnv }{ { - name: "all empty", - instance: RunFns{}, - inputEnvs: *runtimeutil.NewContainerEnv(), - expect: *runtimeutil.NewContainerEnv(), + name: "all empty", + instance: RunFns{}, + expect: *runtimeutil.NewContainerEnv(), }, { name: "empty command line envs", instance: RunFns{}, - inputEnvs: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar"}), + inputEnvs: []string{"foo=bar"}, expect: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar"}), }, { name: "empty declarative envs", instance: RunFns{ - Env: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar"}), + Env: []string{"foo=bar"}, }, - inputEnvs: *runtimeutil.NewContainerEnv(), - expect: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar"}), + expect: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar"}), }, { name: "same key", instance: RunFns{ - Env: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar", "foo"}), + Env: []string{"foo=bar", "foo"}, }, - inputEnvs: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar1", "bar"}), + inputEnvs: []string{"foo=bar1", "bar"}, expect: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar", "bar", "foo"}), }, { name: "same exported key", instance: RunFns{ - Env: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar", "foo"}), + Env: []string{"foo=bar", "foo"}, }, - inputEnvs: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo1=bar1", "foo"}), + inputEnvs: []string{"foo1=bar1", "foo"}, expect: *runtimeutil.NewContainerEnvFromStringSlice([]string{"foo=bar", "foo1=bar1", "foo"}), }, } @@ -1036,7 +1034,7 @@ func TestRunfns_mergeContainerEnv(t *testing.T) { tc := testcases[i] t.Run(tc.name, func(t *testing.T) { envs := tc.instance.mergeContainerEnv(tc.inputEnvs) - assert.Equal(t, tc.expect.GetDockerFlags(), envs.GetDockerFlags()) + assert.Equal(t, tc.expect.GetDockerFlags(), runtimeutil.NewContainerEnvFromStringSlice(envs).GetDockerFlags()) }) } }