diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index b81e64aff9..31b6993ef8 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -139,15 +139,15 @@ func (p *ConfigurationEditFlags) Apply( if err != nil { return errors.Wrap(err, "Invalid --env") } - envMapToRemove := make(map[string]bool) + envKeyToRemove := []string{} for key := range envMap { - if strings.HasSuffix(key, "-") { - envMapToRemove[key[:len(key)-1]] = true - delete(envMap, key) + if !strings.HasSuffix(key, "-") { continue } + envKeyToRemove = append(envKeyToRemove, key[:len(key)-1]) + delete(envMap, key) } - err = servinglib.UpdateEnvVars(template, envMap, envMapToRemove) + err = servinglib.UpdateEnvVars(template, envMap, envKeyToRemove) if err != nil { return err } diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 16dc284ab9..d7c977eb67 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -33,12 +33,19 @@ var UserImageAnnotationKey = "client.knative.dev/user-image" // UpdateEnvVars gives the configuration all the env var values listed in the given map of // vars. Does not touch any environment variables not mentioned, but it can add // new env vars and change the values of existing ones, then sort by environment key name. -func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, toUpdate map[string]string, toRemove map[string]bool) error { +func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error { container, err := ContainerOfRevisionTemplate(template) if err != nil { return err } - container.Env = mergeEnvVarsWithStableOrder(container.Env, toUpdate, toRemove) + updated := updateEnvVars(container.Env, toUpdate) + removed := removeEnvVars(updated, toRemove) + // Sort by Env key name + sort.SliceStable(removed, func(i, j int) bool { + return removed[i].Name < removed[j].Name + }) + container.Env = removed + return nil } @@ -235,27 +242,37 @@ func UpdateLabels(service *servingv1alpha1.Service, template *servingv1alpha1.Re // ======================================================================================= -// mergeEnvVarsWithStableOrder merges source EnvVars into target and sorts by Env key name. -func mergeEnvVarsWithStableOrder(src []corev1.EnvVar, target map[string]string, toRemove map[string]bool) []corev1.EnvVar { - for _, v := range src { - if _, ok := toRemove[v.Name]; ok { - continue +func updateEnvVars(env []corev1.EnvVar, toUpdate map[string]string) []corev1.EnvVar { + set := make(map[string]bool) + for i := range env { + envVar := &env[i] + value, present := toUpdate[envVar.Name] + if present { + envVar.Value = value + set[envVar.Name] = true } - // Register only non-duplicate EnvVars, which results in overwriting old EnvVars - if _, ok := target[v.Name]; !ok { - target[v.Name] = v.Value + } + for name, value := range toUpdate { + if !set[name] { + env = append( + env, + corev1.EnvVar{ + Name: name, + Value: value, + }) } } + return env +} - merged := []corev1.EnvVar{} - for key, val := range target { - merged = append(merged, corev1.EnvVar{Name: key, Value: val}) +func removeEnvVars(env []corev1.EnvVar, toRemove []string) []corev1.EnvVar { + for _, name := range toRemove { + for i, envVar := range env { + if envVar.Name == name { + env = append(env[:i], env[i+1:]...) + break + } + } } - - // Sort by Env key name - sort.SliceStable(merged, func(i, j int) bool { - return merged[i].Name < merged[j].Name - }) - - return merged + return env } diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index 7598cb640b..a36385435b 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -84,7 +84,7 @@ func testUpdateEnvVarsNew(t *testing.T, template *servingv1alpha1.RevisionTempla } found, err := EnvToMap(env) assert.NilError(t, err) - err = UpdateEnvVars(template, found, map[string]bool{}) + err = UpdateEnvVars(template, found, []string{}) assert.NilError(t, err) assert.DeepEqual(t, env, container.Env) } @@ -153,7 +153,7 @@ func testUpdateEnvVarsAppendOld(t *testing.T, template *servingv1alpha1.Revision env := map[string]string{ "b": "bar", } - err := UpdateEnvVars(template, env, map[string]bool{}) + err := UpdateEnvVars(template, env, []string{}) assert.NilError(t, err) expected := []corev1.EnvVar{ @@ -180,7 +180,7 @@ func testUpdateEnvVarsModify(t *testing.T, revision *servingv1alpha1.RevisionTem env := map[string]string{ "a": "fancy", } - err := UpdateEnvVars(revision, env, map[string]bool{}) + err := UpdateEnvVars(revision, env, []string{}) assert.NilError(t, err) expected := map[string]string{ @@ -207,7 +207,7 @@ func testUpdateEnvVarsRemove(t *testing.T, revision *servingv1alpha1.RevisionTem {Name: "a", Value: "foo"}, {Name: "b", Value: "bar"}, } - remove := map[string]bool{"b": true} + remove := []string{"b"} err := UpdateEnvVars(revision, map[string]string{}, remove) assert.NilError(t, err) @@ -329,7 +329,7 @@ func testUpdateEnvVarsAll(t *testing.T, template *servingv1alpha1.RevisionTempla "a": "fancy", "b": "boo", } - remove := map[string]bool{"d": true} + remove := []string{"d"} err := UpdateEnvVars(template, env, remove) assert.NilError(t, err) @@ -408,124 +408,6 @@ func TestUpdateLabelsRemoveExisting(t *testing.T) { assert.DeepEqual(t, expected, actual) } -func TestMergeEnvVarsWithStableOrder(t *testing.T) { - tests := []struct { - name string - src []corev1.EnvVar - target map[string]string - toRemove map[string]bool - want []corev1.EnvVar - }{ - { - name: "add new EnvVar from nothing in there", - src: []corev1.EnvVar{}, - target: map[string]string{ - "FOO": "foo", - }, - toRemove: map[string]bool{}, - want: []corev1.EnvVar{ - {Name: "FOO", Value: "foo"}, - }, - }, - { - name: "add no new EnvVar", - src: []corev1.EnvVar{ - {Name: "FOO", Value: "foo"}, - }, - target: map[string]string{}, - toRemove: map[string]bool{}, - want: []corev1.EnvVar{ - {Name: "FOO", Value: "foo"}, - }, - }, - { - name: "add non-duplicate EnvVars", - src: []corev1.EnvVar{ - {Name: "FOO", Value: "foo"}, - }, - target: map[string]string{ - "BAR": "bar", - }, - toRemove: map[string]bool{}, - want: []corev1.EnvVar{ - {Name: "BAR", Value: "bar"}, - {Name: "FOO", Value: "foo"}, - }, - }, - { - name: "add duplicated EnvVars", - src: []corev1.EnvVar{ - {Name: "FOO", Value: "foo"}, - {Name: "BAR", Value: "bar"}, - {Name: "BAZ", Value: "baz"}, - }, - target: map[string]string{ - "FOO": "foov2", - "QUX": "qux", - }, - toRemove: map[string]bool{}, - want: []corev1.EnvVar{ - {Name: "BAR", Value: "bar"}, - {Name: "BAZ", Value: "baz"}, - {Name: "FOO", Value: "foov2"}, - {Name: "QUX", Value: "qux"}, - }, - }, - { - name: "remove more than one EnvVars", - src: []corev1.EnvVar{ - {Name: "FOO", Value: "foo"}, - {Name: "BAR", Value: "bar"}, - {Name: "BAZ", Value: "baz"}, - }, - target: map[string]string{}, - toRemove: map[string]bool{"FOO": true, "BAR": true}, - want: []corev1.EnvVar{ - {Name: "BAZ", Value: "baz"}, - }, - }, - { - name: "remove non-existing EnvVars", - src: []corev1.EnvVar{ - {Name: "FOO", Value: "foo"}, - {Name: "BAR", Value: "bar"}, - {Name: "BAZ", Value: "baz"}, - }, - target: map[string]string{}, - toRemove: map[string]bool{"WHAT": true, "BAR": true}, - want: []corev1.EnvVar{ - {Name: "BAZ", Value: "baz"}, - {Name: "FOO", Value: "foo"}, - }, - }, - { - name: "add duplicated EnvVars and remove more than one EnvVars", - src: []corev1.EnvVar{ - {Name: "FOO", Value: "foo"}, - {Name: "BAR", Value: "bar"}, - {Name: "BAZ", Value: "baz"}, - }, - target: map[string]string{ - "FOO": "foov2", - "QUX": "qux", - }, - toRemove: map[string]bool{"WHAT": true, "BAR": true}, - want: []corev1.EnvVar{ - {Name: "BAZ", Value: "baz"}, - {Name: "FOO", Value: "foov2"}, - {Name: "QUX", Value: "qux"}, - }, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - got := mergeEnvVarsWithStableOrder(test.src, test.target, test.toRemove) - assert.DeepEqual(t, got, test.want) - }) - } -} - // ========================================================================================================= func getV1alpha1RevisionTemplateWithOldFields() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Container) {