Do not trigger an update when a parameter is added by an OV upgrade. (#…

…1780)

* Test adding a required no-default parameter on upgrade.

Currently, an operator upgrade that brings in a new required parameter
without a default, is impossible if the operator defines both
"update" and "upgrade" plans.

An upgrade without setting this parameter fails (which is a correct
behaviour); however, an attempt to upgrade while setting the previously
nonexistent parameter also fails due to both plans being triggered,
regardless of the fact that no existing parameter is changed.

This test verifies that an upgrade of such an operator is possible
together with setting a value for the new required parameter,
and that the plan chosen is "upgrade" and not "update".

Signed-off-by: Andrei Sekretenko <[email protected]>

* Do not trigger an update when a parameter is added by an OV upgrade.

Now, parameters added by a new operator version (for example, when
a new OV brings in a new required parameter with no default, so that
it is necessary to set a value together with upgrade) no longer trigger
an update plan.

This fixes #1776

Signed-off-by: Andrei Sekretenko <[email protected]>

pkg/webhook/instance_admission.go

@@ -217,7 +217,7 @@ func admitUpdate(old, new *kudoapi.Instance, ov, oldOv *kudoapi.OperatorVersion)
 		return nil, fmt.Errorf("plan %s does not exist", newPlan)
 	}
 
-	changedDefs, removedDefs, err := changedParameters(old.Spec.Parameters, new.Spec.Parameters, ov)
+	changedDefs, removedDefs, err := changedParameters(old.Spec.Parameters, new.Spec.Parameters, oldOv, ov)
 	if err != nil {
 		return nil, fmt.Errorf("failed to update Instance %s/%s: %v", old.Namespace, old.Name, err)
 	}
@@ -229,8 +229,8 @@ func admitUpdate(old, new *kudoapi.Instance, ov, oldOv *kudoapi.OperatorVersion)
 		return nil, fmt.Errorf("failed to validate parameters for Instance %s/%s: %v", new.Namespace, new.Name, err)
 	}
 
-	parameterDefs := append(changedDefs, removedDefs...)
-	triggeredPlan, err := triggeredByParameterUpdate(parameterDefs, ov)
+	updatedParameterDefs := append(changedDefs, removedDefs...)
+	triggeredPlan, err := triggeredByParameterUpdate(updatedParameterDefs, ov)
 	if err != nil {
 		return nil, fmt.Errorf("failed to update Instance %s/%s: %v", old.Namespace, old.Name, err)
 	}
@@ -365,14 +365,25 @@ func triggeredByParameterUpdate(params []kudoapi.Parameter, ov *kudoapi.Operator
 }
 
 // changedParameters returns a list of parameter definitions for params which value changed or that were added from old to new
-// This does *not* include parameters which *definition* has changed in an OV upgrade but where the value has not changed!
-func changedParameters(old, new map[string]string, newOv *kudoapi.OperatorVersion) ([]kudoapi.Parameter, []kudoapi.Parameter, error) {
-	changed, removed := kudoapi.RichParameterDiff(old, new)
-	changedDefs, err := kudoapi.GetParamDefinitions(changed, newOv)
+// This does *not* include:
+// - parameters which *definition* has changed in an OV upgrade but where the value has not changed
+// - parameters that have been added in an OV upgrade
+func changedParameters(old, new map[string]string, oldOv, newOv *kudoapi.OperatorVersion) ([]kudoapi.Parameter, []kudoapi.Parameter, error) {
+	changedOrAdded, removed := kudoapi.RichParameterDiff(old, new)
+	changedOrAddedDefs, err := kudoapi.GetParamDefinitions(changedOrAdded, newOv)
 	if err != nil {
 		return nil, nil, err
 	}
 
+	// Valid parameters not present in the old OV are not treated as changed.
+	changedDefs := []kudoapi.Parameter{}
+	for _, param := range changedOrAddedDefs {
+		param := param
+		if funk.Find(oldOv.Spec.Parameters, func(p kudoapi.Parameter) bool { return param.Name == p.Name }) != nil {
+			changedDefs = append(changedDefs, param)
+		}
+	}
+
 	// we ignore the error for missing OV parameter definitions for removed parameters. For once, this is a valid use-case when
 	// upgrading an Instance (new OV might remove parameters), but the user can also manually edit current OV and remove parameters.
 	// while discouraged, this is still possible since OV is not immutable.

test/e2e/update-with-upgrade/00-assert.yaml

@@ -0,0 +1,7 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  origin: deploy
+  foo: bar

test/e2e/update-with-upgrade/00-install.yaml

@@ -0,0 +1,5 @@
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - command: kubectl kudo install --instance update-with-upgrade ./update-with-upgrade-v1
+    namespaced: true

test/e2e/update-with-upgrade/01-assert.yaml

@@ -0,0 +1,8 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  origin: upgrade
+  foo: bar
+  bar: baz

test/e2e/update-with-upgrade/01-upgrade-operator-version.yaml

@@ -0,0 +1,5 @@
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+commands:
+  - command: kubectl kudo upgrade --instance update-with-upgrade ./update-with-upgrade-v2 -p bar=baz
+    namespaced: true

test/e2e/update-with-upgrade/update-with-upgrade-v1/operator.yaml

@@ -0,0 +1,50 @@
+apiVersion: kudo.dev/v1beta1
+name: "update-with-upgrade"
+operatorVersion: "0.1.0"
+kubernetesVersion: 1.13.0
+tasks:
+  - name: deploy-configmap
+    kind: Apply
+    spec:
+      resources:
+        - configmap-deployed.yaml
+  - name: update-configmap
+    kind: Apply
+    spec:
+      resources:
+        - configmap-updated.yaml
+  - name: upgrade-configmap
+    kind: Apply
+    spec:
+      resources:
+        - configmap-upgraded.yaml
+plans:
+  deploy:
+    strategy: serial
+    phases:
+      - name: deploy-configmap
+        strategy: serial
+        steps:
+          - name: deploy-configmap
+            tasks:
+              - deploy-configmap
+  # This should not occur when upgrading the operator:
+  update:
+    strategy: serial
+    phases:
+      - name: update-configmap
+        strategy: serial
+        steps:
+          - name: update-configmap
+            tasks:
+              - update-configmap
+  # Instead, this should occur when upgrading the operator:
+  upgrade:
+    strategy: serial
+    phases:
+      - name: upgrade-configmap
+        strategy: serial
+        steps:
+          - name: upgrade-configmap
+            tasks:
+              - upgrade-configmap

test/e2e/update-with-upgrade/update-with-upgrade-v1/params.yaml

@@ -0,0 +1,5 @@
+apiVersion: kudo.dev/v1beta1
+parameters:
+  - name: foo
+    required: true
+    default: "bar"

test/e2e/update-with-upgrade/update-with-upgrade-v1/templates/configmap-deployed.yaml

@@ -0,0 +1,9 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  # This field is used to determine the plan which created the config map
+  origin: deploy
+  # This field introduces a dependency on a parameter
+  foo: "{{ .Params.foo }}"

test/e2e/update-with-upgrade/update-with-upgrade-v1/templates/configmap-updated.yaml

@@ -0,0 +1,9 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  # This field is used to determine the plan which created the config map
+  origin: update
+  # This field introduces a dependency on a parameter
+  foo: "{{ .Params.foo }}"

test/e2e/update-with-upgrade/update-with-upgrade-v1/templates/configmap-upgraded.yaml

@@ -0,0 +1,9 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  # This field is used to determine the plan which created the config map
+  origin: upgrade
+  # This field introduces a dependency on a parameter
+  foo: "{{ .Params.foo }}"

test/e2e/update-with-upgrade/update-with-upgrade-v2/operator.yaml

@@ -0,0 +1,50 @@
+apiVersion: kudo.dev/v1beta1
+name: "update-with-upgrade"
+operatorVersion: "0.2.0"
+kubernetesVersion: 1.13.0
+tasks:
+  - name: deploy-configmap
+    kind: Apply
+    spec:
+      resources:
+        - configmap-deployed.yaml
+  - name: update-configmap
+    kind: Apply
+    spec:
+      resources:
+        - configmap-updated.yaml
+  - name: upgrade-configmap
+    kind: Apply
+    spec:
+      resources:
+        - configmap-upgraded.yaml
+plans:
+  deploy:
+    strategy: serial
+    phases:
+      - name: deploy-configmap
+        strategy: serial
+        steps:
+          - name: deploy-configmap
+            tasks:
+              - deploy-configmap
+  # This should not occur when upgrading the operator:
+  update:
+    strategy: serial
+    phases:
+      - name: update-configmap
+        strategy: serial
+        steps:
+          - name: update-configmap
+            tasks:
+              - update-configmap
+  # Instead, this should occur when upgrading the operator:
+  upgrade:
+    strategy: serial
+    phases:
+      - name: upgrade-configmap
+        strategy: serial
+        steps:
+          - name: upgrade-configmap
+            tasks:
+              - upgrade-configmap

test/e2e/update-with-upgrade/update-with-upgrade-v2/params.yaml

@@ -0,0 +1,9 @@
+apiVersion: kudo.dev/v1beta1
+parameters:
+  - name: foo
+    required: true
+    default: "bar"
+  # NOTE: "bar" intentionally specifies no default,
+  # so that upgrade is impossible without setting the value.
+  - name: bar
+    required: true

test/e2e/update-with-upgrade/update-with-upgrade-v2/templates/configmap-deployed.yaml

@@ -0,0 +1,10 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  # This field is used to determine the plan which created the config map
+  origin: deploy
+  # This field introduces a dependency on a parameter
+  foo: "{{ .Params.foo }}"
+  bar: "{{ .Params.bar }}"

test/e2e/update-with-upgrade/update-with-upgrade-v2/templates/configmap-updated.yaml

@@ -0,0 +1,10 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  # This field is used to determine the plan which created the config map
+  origin: update
+  # This field introduces a dependency on a parameter
+  foo: "{{ .Params.foo }}"
+  bar: "{{ .Params.bar }}"

test/e2e/update-with-upgrade/update-with-upgrade-v2/templates/configmap-upgraded.yaml

@@ -0,0 +1,10 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test
+data:
+  # This field is used to determine the plan which created the config map
+  origin: upgrade
+  # This field introduces a dependency on a parameter
+  foo: "{{ .Params.foo }}"
+  bar: "{{ .Params.bar }}"