feature(service create/update): Register list of EnvVars in alphabetical order

[toVersus]

Proposed Changes

In current logic of handling user input of Environment variables, list of EnvVars are registered in random order because we convert user input string to Golang map and append them into slice of corev1.EnvVars, which has no guarantee of stable order as follows:

$ ./kn service create hello --image="gcr.io/knative-samples/helloworld-go" -e FOO=foo -e BAR=bar -e BAZ=baz -e QUX=qux -e EMPTY=
Service 'hello' successfully created in namespace 'default'.
Waiting for service 'hello' to become ready ... OK

Service URL:
http://hello-default.example.com

$ ./kn service describe hello -o yaml
(...)
    spec:
      containers:
      - env:
        - name: BAZ
          value: baz
        - name: QUX
          value: qux
        - name: EMPTY
        - name: FOO
          value: foo
        - name: BAR
          value: bar

We need to fix this for passing mock test without flakiness as well as GitOps deployment without causing any bothering diffs.

• Register list of EnvVars in alphabetical order of Env key name when creating/updating new kservice
• Add mock test for create/update service with EnvVars

After:

$ ./kn service create hello --image="gcr.io/knative-samples/helloworld-go" -e FOO=foo -e BAR=bar -e BAZ=baz -e QUX=qux -e EMPTY=
Service 'hello' successfully created in namespace 'default'.
Waiting for service 'hello' to become ready ... OK

Service URL:
http://hello-default.example.com

$ ./kn service describe hello -o yaml
(...)
    spec:
      containers:
      - env:
        - name: BAR
          value: bar
        - name: BAZ
          value: baz
        - name: EMPTY
        - name: FOO
          value: foo
        - name: QUX
          value: qux
(...)

$ ./kn service update hello -e FOO=foov2 -e BAR-
Waiting for service 'hello' to become ready ... OK
Service 'hello' updated in namespace 'default'.

$ ./kn service describe hello -o yaml
(...)
    spec:
      containers:
      - env:
        - name: BAZ
          value: baz
        - name: EMPTY
        - name: FOO
          value: foov2
        - name: QUX
          value: qux
(...)

Part of: #358
/lint

[knative-prow-robot]

@toVersus: 0 warnings.

In response to this:

Proposed Changes

In current logic of handling user input of Environment variables, list of EnvVars are registered in random order because we convert user input string to Golang map and append them into slice of corev1.EnvVars, which has no guarantee of stable order as follows:

$ ./kn service create hello --image="gcr.io/knative-samples/helloworld-go" -e FOO=foo -e BAR=bar -e BAZ=baz -e QUX=qux -e EMPTY=
Service 'hello' successfully created in namespace 'default'.
Waiting for service 'hello' to become ready ... OK

Service URL:
http://hello-default.example.com

$ ./kn service describe hello -o yaml
(...)
   spec:
     containers:
     - env:
       - name: BAZ
         value: baz
       - name: QUX
         value: qux
       - name: EMPTY
       - name: FOO
         value: foo
       - name: BAR
         value: bar

We need to fix this for passing mock test without flakiness as well as GitOps deployment without causing any bothering diffs.

• Register list of EnvVars in the same order as user input when creating new kservice
• Keep the original order of EnvVars when updating existing kservice
• Add mock test for create/update service with EnvVars

After:

$ ./kn service create hello --image="gcr.io/knative-samples/helloworld-go" -e FOO=foo -e BAR=bar -e BAZ=baz -e QUX=qux -e EMPTY=
Service 'hello' successfully created in namespace 'default'.
Waiting for service 'hello' to become ready ... OK

Service URL:
http://hello-default.example.com

$ ./kn service describe hello -o yaml
(...)
   spec:
     containers:
     - env:
       - name: FOO
         value: foo
       - name: BAR
         value: bar
       - name: BAZ
         value: baz
       - name: QUX
         value: qux
       - name: EMPTY
(...)

$ ./kn service update hello -e FOO=foov2 -e BAR-
Waiting for service 'hello' to become ready ... OK
Service 'hello' updated in namespace 'default'.

$ ./kn service describe hello -o yaml
(...)
   spec:
     containers:
     - env:
       - name: FOO
         value: foov2
       - name: BAZ
         value: baz
       - name: QUX
         value: qux
       - name: EMPTY
(...)

Part of: #358
/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[knative-prow-robot]

Hi @toVersus. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[navidshaikh]

We need to fix this for passing mock test without flakiness

Is there a flakiness in tests due to order of env?

GitOps deployment without causing any bothering diffs.

Interesting, are resource template files stored using -o yaml ?

[navidshaikh]

/ok-to-test

[toVersus]

Is there a flakiness in tests due to order of env?

Sorry, currently not in tests. However, when I try to add TestServiceCreateEnvMock mock test, it would emerge due to this assert.DeepEqual as follows:

=== RUN   TestServiceCreateEnvMock
--- FAIL: TestServiceCreateEnvMock (0.00s)
    client_mock.go:292: assertion failed: 
        --- call.args[i]
        +++ arg
            &v1alpha1.Service{
            TypeMeta:   v1.TypeMeta{},
          	ObjectMeta: v1.ObjectMeta{Name: "foo", Namespace: "default"},
          	Spec: v1alpha1.ServiceSpec{
          		... // 3 identical fields
          		DeprecatedManual:  nil,
          		DeprecatedRelease: nil,
          		ConfigurationSpec: v1alpha1.ConfigurationSpec{
          			DeprecatedGeneration:       0,
          			DeprecatedBuild:            nil,
          			DeprecatedRevisionTemplate: nil,
          			Template: &v1alpha1.RevisionTemplateSpec{
          				ObjectMeta: v1.ObjectMeta{},
          				Spec: v1alpha1.RevisionSpec{
          					RevisionSpec: v1beta1.RevisionSpec{
          						PodSpec: v1.PodSpec{
          							Volumes:        nil,
          							InitContainers: nil,
          							Containers: []v1.Container{
          								{
          									... // 5 identical fields
          									Ports:   nil,
          									EnvFrom: nil,
          									Env: []v1.EnvVar{
        - 										{Name: "a", Value: "mouse"},
          										{Name: "b", Value: "cookie"},
          										{Name: "empty"},
        + 										{Name: "a", Value: "mouse"},
          									},
          									Resources:    v1.ResourceRequirements{Limits: v1.ResourceList{}, Requests: v1.ResourceList{}},
          									VolumeMounts: nil,
          									... // 11 identical fields
          								},
          							},
          							RestartPolicy:                 "",
          							TerminationGracePeriodSeconds: nil,
          							... // 24 identical fields
          						},
          						ContainerConcurrency: 0,
          						TimeoutSeconds:       nil,
          					},
          					DeprecatedGeneration:   0,
          					DeprecatedServingState: "",
          					... // 4 identical fields
          				},
          			},
          		},
          		RouteSpec: v1alpha1.RouteSpec{},
          	},
          	Status: v1alpha1.ServiceStatus{},
          }

Interesting, are resource template files stored using -o yaml ?

Yeah, I assume to use kn in that way. Also, I guess it would become an issue when implementing GitOps mode.

[maximilien]

The test is now green, is this passing? What was the change?

[toVersus]

@maximilien, sorry for confusing you. The tests included in this PR are always green without a flakiness from the beginning.

OK, I summarize. When I first introduced the mock test for service create with env vars, I encountered the error described above. Therefore, I fixed some internal logic and made mock test green, then raised this PR.

[navidshaikh]

@toVersus : During last WG PR rodeo, its discussed that simply sort the env vars before updating the service.

[toVersus]

@navidshaikh Ah okay, but should we sort env vars in order of user input or env key name?

[sixolet]

Env key name. User input is a lot harder to get consistent

…

On Aug 31, 2019, at 1:03 AM, Tsubasa Nagasawa ***@***.***> wrote: @navidshaikh Ah okay, but should we sort env vars in order of user input or env key name? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

[toVersus]

@sixolet, Thanks for clarifying, I fixed! (Also updated both the title and description of this PR.)

[toVersus]

It's ready for review :)

[rhuss]

* Register list of EnvVars in alphabetical order of Env key name when creating/updating new kservice

I agree, that env vars should not be listed in random order, but why should the order be alphabetically and not the order as defined in the Service (env vars are registered as an array) ?

[toVersus]

In my initial proposal below, I tried to sort the env vars in the order of user input.

• Register list of EnvVars in the same order as user input when creating new kservice
• Keep the original order of EnvVars when updating existing kservice

However, the ordering logic became complicated given the consistency in the order when updating/deleting existing ones. See #389 (comment).

client/pkg/kn/commands/service/configuration_edit_flags.go

Lines 298 to 326 in 2ca3980

	func envVarsFromSlice(envFlags []string, delimiter string) ([]corev1.EnvVar, error) {
	envDict := make(map[string]string, len(envFlags))
	envFlagsUpdated := envFlags[:0]
	for _, flag := range envFlags {
	envPair := strings.SplitN(flag, delimiter, 2)
	switch len(envPair) {
	case 0:
	return nil, fmt.Errorf("argument requires a value that contains the %q character; got %q", delimiter, envPair)
	case 1:
	if _, ok := envDict[envPair[0]]; !ok {
	envFlagsUpdated = append(envFlagsUpdated, envPair[0])
	}
	envDict[envPair[0]] = ""
	case 2:
	if _, ok := envDict[envPair[0]]; !ok {
	envFlagsUpdated = append(envFlagsUpdated, envPair[0])
	}
	envDict[envPair[0]] = envPair[1]
	default:
	return nil, fmt.Errorf("argument having more than one %q delimiter; got %q", delimiter, envPair)
	}
	}
	envVars := []corev1.EnvVar{}
	for _, flag := range envFlagsUpdated {
	envVars = append(envVars, corev1.EnvVar{Name: flag, Value: envDict[flag]})
	}
	return envVars, nil
	}

client/pkg/serving/config_changes.go

Lines 238 to 275 in 2ca3980

	func mergeEnvVarsWithStableOrder(src []corev1.EnvVar, target []corev1.EnvVar, toRemove map[string]bool) []corev1.EnvVar {
	// dict records all EnvVars with fixed capacity
	dict := make(map[string]string, len(src)+len(target))
	for _, v := range target {
	dict[v.Name] = v.Value
	}
	srcUpdated := src[:0]
	for _, v := range src {
	// Register only non-duplicated EnvVars, which results in overwriting old EnvVars
	if _, ok := dict[v.Name]; ok {
	// Don't change the order of updated items
	srcUpdated = append(srcUpdated, v)
	continue
	}
	if _, ok := toRemove[v.Name]; !ok {
	dict[v.Name] = v.Value
	// Only append non-duplicated key-value pair
	srcUpdated = append(srcUpdated, v)
	}
	}
	// merged EnvVars keep the original order of equal elements
	merged := []corev1.EnvVar{}
	for _, v := range srcUpdated {
	if _, ok := dict[v.Name]; ok {
	merged = append(merged, corev1.EnvVar{Name: v.Name, Value: dict[v.Name]})
	// Delete dict item for avoiding duplicated entries
	delete(dict, v.Name)
	}
	}
	for _, v := range target {
	if _, ok := dict[v.Name]; ok {
	merged = append(merged, corev1.EnvVar{Name: v.Name, Value: dict[v.Name]})
	}
	}
	return merged
	}

I'm not sure what was discussed in the last WG PR rodeo, but they decided to register the env vars alphabetically. See #389 (comment).

[navidshaikh]

few nits on naming of functions and variables

[navidshaikh]

@toVersus lets squash the commits

[toVersus]

@navidshaikh I think Prow robot will squash the commits when a PR is merged. Should I manually squash the commits?

https://github.com/knative/test-infra/blob/816123d5e71fb88533d1564c331443387967e1dc/ci/prow/config.yaml#L95-L96

[navidshaikh]

@toVersus : There are commits in this PR for the initial approach taken, then we changed to alphabetical order, I asked explicitly to squash to reflect the approach we're taking.

[knative-metrics-robot]

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/serving/config_changes.go	80.7%	81.2%	0.5

[toVersus]

@navidshaikh I'm not sure I could take the action you asked me to do, but I removed the commit messages regarding the initial approach from this PR.

[navidshaikh]

@rhuss :

I agree, that env vars should not be listed in random order, but why should the order be alphabetically and not the order as defined in the Service (env vars are registered as an array) ?

The actual order of env doesn't impact and maintaining the alphabetically order is simpler and consistent than maintaining the user provided env order.

[rhuss]

ok, convinced. let's go with the alphabetical order (assuming that duplicate keys in the env-list is already treated properly by the env handling as the later key overrides a former key)

[navidshaikh]

/lgtm
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh, toVersus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [navidshaikh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment