Fix updating invalid Instance parameters

pkg/apis/kudo/v1beta1/instance_types_helpers.go

@@ -220,8 +220,8 @@ func wasRunAfter(p1 PlanStatus, p2 PlanStatus) bool {
 	return p1.LastUpdatedTimestamp.Time.After(p2.LastUpdatedTimestamp.Time)
 }
 
-// GetParamDefinitions retrieves parameter metadata from OperatorVersion CRD
-func GetParamDefinitions(params map[string]string, ov *OperatorVersion) []Parameter {
+// GetExistingParamDefinitions retrieves parameter metadata from OperatorVersion
+func GetExistingParamDefinitions(params map[string]string, ov *OperatorVersion) []Parameter {
 	defs := []Parameter{}
 	for p1 := range params {
 		for _, p2 := range ov.Spec.Parameters {
@@ -233,6 +233,24 @@ func GetParamDefinitions(params map[string]string, ov *OperatorVersion) []Parame
 	return defs
 }
 
+// GetParamDefinitions retrieves parameter metadata from OperatorVersion but returns an error if any parameter is missing
+func GetParamDefinitions(params map[string]string, ov *OperatorVersion) ([]Parameter, error) {
+	defs := []Parameter{}
+	for p1 := range params {
+		p1 := p1
+		p2 := funk.Find(ov.Spec.Parameters, func(e Parameter) bool {
+			return e.Name == p1
+		})
+
+		if p2 == nil {
+			return nil, fmt.Errorf("failed to find parameter %q in the OperatorVersion", p1)
+		}
+
+		defs = append(defs, p2.(Parameter))
+	}
+	return defs, nil
+}
+
 // ParameterDiff returns map containing all parameters that were removed or changed between old and new
 func ParameterDiff(old, new map[string]string) map[string]string {
 	diff := make(map[string]string)
@@ -254,6 +272,27 @@ func ParameterDiff(old, new map[string]string) map[string]string {
 	return diff
 }
 
+// RichParameterDiff compares new and old map and returns two maps: first containing all changed/added
+// and second all removed parameters.
+func RichParameterDiff(old, new map[string]string) (changed, removed map[string]string) {
+	changed, removed = make(map[string]string), make(map[string]string)
+
+	for key, val := range old {
+		// If a parameter was removed in the new spec
+		if _, ok := new[key]; !ok {
+			removed[key] = val
+		}
+	}
+
+	for key, val := range new {
+		// If new spec parameter was added or changed
+		if v, ok := old[key]; !ok || v != val {
+			changed[key] = val
+		}
+	}
+	return
+}
+
 func CleanupPlanExists(ov *OperatorVersion) bool { return PlanExists(CleanupPlanName, ov) }
 
 func PlanExists(plan string, ov *OperatorVersion) bool {

pkg/apis/kudo/v1beta1/instance_types_helpers_test.go

@@ -161,3 +161,56 @@ func TestInstance_ResetPlanStatus(t *testing.T) {
 		},
 	}, instance.Status)
 }
+
+func TestGetParamDefinitions(t *testing.T) {
+	ov := &OperatorVersion{
+		ObjectMeta: metav1.ObjectMeta{Name: "foo-operator", Namespace: "default"},
+		TypeMeta:   metav1.TypeMeta{Kind: "OperatorVersion", APIVersion: "kudo.dev/v1beta1"},
+		Spec: OperatorVersionSpec{
+			Parameters: []Parameter{
+				{Name: "foo"},
+				{Name: "other-foo"},
+				{Name: "bar"},
+			},
+		},
+	}
+
+	tests := []struct {
+		name    string
+		params  map[string]string
+		ov      *OperatorVersion
+		want    []Parameter
+		wantErr bool
+	}{
+		{
+			name:    "all parameters exists",
+			params:  map[string]string{"foo": "1", "bar": "2"},
+			ov:      ov,
+			want:    []Parameter{{Name: "foo"}, {Name: "bar"}},
+			wantErr: false,
+		},
+		{
+			name:    "one parameter is missing",
+			params:  map[string]string{"foo": "1", "fake-one": "2"},
+			ov:      ov,
+			want:    nil,
+			wantErr: true,
+		},
+		{
+			name:    "both parameters are missing",
+			params:  map[string]string{"fake-one": "1", "fake-two": "2"},
+			ov:      ov,
+			want:    nil,
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := GetParamDefinitions(tt.params, tt.ov)
+
+			assert.True(t, (err != nil) == tt.wantErr, "GetParamDefinitions() error = %v, wantErr %v", err, tt.wantErr)
+			assert.ElementsMatch(t, got, tt.want, "GetParamDefinitions() got = %v, want %v", got, tt.want)
+		})
+	}
+}

pkg/controller/instance/instance_controller.go

@@ -682,7 +682,7 @@ func inferNewExecutionPlan(i *v1beta1.Instance, ov *v1beta1.OperatorVersion) (*s
 		// instance updated
 		log.Printf("Instance: instance %s/%s has updated parameters from %v to %v", i.Namespace, i.Name, instanceSnapshot.Parameters, i.Spec.Parameters)
 		paramDiff := kudov1beta1.ParameterDiff(instanceSnapshot.Parameters, i.Spec.Parameters)
-		paramDefinitions := kudov1beta1.GetParamDefinitions(paramDiff, ov)
+		paramDefinitions := kudov1beta1.GetExistingParamDefinitions(paramDiff, ov)
 		plan, err := planNameFromParameters(paramDefinitions, ov)
 		if err != nil {
 			return nil, &v1beta1.InstanceError{Err: fmt.Errorf("supposed to execute plan because instance %s/%s was updated but no valid plan found: %v", i.Namespace, i.Name, err), EventName: convert.StringPtr("PlanNotFound")}

pkg/controller/instance/instance_controller_test.go

@@ -340,25 +340,58 @@ func Test_makePipes(t *testing.T) {
 	}
 }
 
-func TestSpecParameterDifference(t *testing.T) {
-	var testParams = []struct {
-		name string
-		new  map[string]string
-		diff map[string]string
-	}{
-		{"update one value", map[string]string{"one": "11", "two": "2"}, map[string]string{"one": "11"}},
-		{"update multiple values", map[string]string{"one": "11", "two": "22"}, map[string]string{"one": "11", "two": "22"}},
-		{"add new value", map[string]string{"one": "1", "two": "2", "three": "3"}, map[string]string{"three": "3"}},
-		{"remove one value", map[string]string{"one": "1"}, map[string]string{"two": "2"}},
-		{"no difference", map[string]string{"one": "1", "two": "2"}, map[string]string{}},
-		{"empty new map", map[string]string{}, map[string]string{"one": "1", "two": "2"}},
+func TestParameterDiff(t *testing.T) {
+	var (
+		tests = []struct {
+			name string
+			new  map[string]string
+			diff map[string]string
+		}{
+			{name: "update one value", new: map[string]string{"one": "11", "two": "2"}, diff: map[string]string{"one": "11"}},
+			{name: "update multiple values", new: map[string]string{"one": "11", "two": "22"}, diff: map[string]string{"one": "11", "two": "22"}},
+			{name: "add new value", new: map[string]string{"one": "1", "two": "2", "three": "3"}, diff: map[string]string{"three": "3"}},
+			{name: "remove one value", new: map[string]string{"one": "1"}, diff: map[string]string{"two": "2"}},
+			{name: "no difference", new: map[string]string{"one": "1", "two": "2"}, diff: map[string]string{}},
+			{name: "empty new map", new: map[string]string{}, diff: map[string]string{"one": "1", "two": "2"}},
+		}
+	)
+
+	var old = map[string]string{"one": "1", "two": "2"}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			diff := v1beta1.ParameterDiff(old, tt.new)
+			assert.Equal(t, tt.diff, diff)
+		})
 	}
+}
 
+func TestRichParameterDiff(t *testing.T) {
+	var empty = map[string]string{}
 	var old = map[string]string{"one": "1", "two": "2"}
 
-	for _, test := range testParams {
-		diff := v1beta1.ParameterDiff(old, test.new)
-		assert.Equal(t, test.diff, diff)
+	var tests = []struct {
+		name    string
+		new     map[string]string
+		changed map[string]string
+		removed map[string]string
+	}{
+		{name: "update one value", new: map[string]string{"one": "11", "two": "2"}, changed: map[string]string{"one": "11"}, removed: empty},
+		{name: "update multiple values", new: map[string]string{"one": "11", "two": "22"}, changed: map[string]string{"one": "11", "two": "22"}, removed: empty},
+		{name: "add new value", new: map[string]string{"one": "1", "two": "2", "three": "3"}, changed: map[string]string{"three": "3"}, removed: empty},
+		{name: "remove one value", new: map[string]string{"one": "1"}, changed: empty, removed: map[string]string{"two": "2"}},
+		{name: "no difference", new: map[string]string{"one": "1", "two": "2"}, changed: empty, removed: empty},
+		{name: "empty new map", new: empty, changed: empty, removed: map[string]string{"one": "1", "two": "2"}},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			changed, removed := v1beta1.RichParameterDiff(old, tt.new)
+			assert.Equal(t, tt.changed, changed, "unexpected difference in changed parameters")
+			assert.Equal(t, tt.removed, removed, "unexpected difference in removed parameters")
+		})
 	}
 }
 

pkg/webhook/instance_admission.go

@@ -175,16 +175,8 @@ func admitUpdate(old, new *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion
 	newUID := new.Spec.PlanExecution.UID
 	oldUID := old.Spec.PlanExecution.UID
 
-	paramDiff := kudov1beta1.ParameterDiff(old.Spec.Parameters, new.Spec.Parameters)
-	paramDefs := kudov1beta1.GetParamDefinitions(paramDiff, ov)
-	triggeredPlan, err := triggeredPlan(paramDefs, ov)
-	if err != nil {
-		return nil, fmt.Errorf("failed to update Instance %s/%s: %v", old.Namespace, old.Name, err)
-	}
-
 	// update and upgrade helpers
 	hadPlan := oldPlan != ""
-	isParameterUpdate := triggeredPlan != nil
 	isUpgrade := newOvRef != oldOvRef
 	isNovelPlan := !hadPlan && newPlan != ""
 	isPlanOverride := hadPlan && newPlan != "" && newPlan != oldPlan
@@ -193,6 +185,18 @@ func admitUpdate(old, new *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion
 	isDeleting := new.IsDeleting() // a non-empty meta.deletionTimestamp is a signal to switch to the uninstalling life-cycle phase
 	isPlanTerminal := isTerminal(new, newPlan, new.Spec.PlanExecution.UID)
 
+	parameterDefs, err := changedParameterDefinitions(old.Spec.Parameters, new.Spec.Parameters, ov)
+	if err != nil {
+		return nil, fmt.Errorf("failed to update Instance %s/%s: %v", old.Namespace, old.Name, err)
+	}
+
+	triggeredPlan, err := triggeredByParameterUpdate(parameterDefs, ov)
+	if err != nil {
+		return nil, fmt.Errorf("failed to update Instance %s/%s: %v", old.Namespace, old.Name, err)
+	}
+
+	isParameterUpdate := triggeredPlan != nil
+
 	// --------------------------------------------------------------------------------------------------------------------------------
 	// ---- Instance can have two major life-cycle phases: normal and cleanup (uninstall) phase. Different rule sets apply in both. ---
 	// --------------------------------------------------------------------------------------------------------------------------------
@@ -286,8 +290,8 @@ func isTerminal(i *kudov1beta1.Instance, plan string, uid types.UID) bool {
 	return status != nil && status.UID == uid && status.Status.IsTerminal()
 }
 
-// triggeredPlan determines what plan to run based on parameters that changed and the corresponding parameter trigger.
-func triggeredPlan(params []kudov1beta1.Parameter, ov *kudov1beta1.OperatorVersion) (*string, error) {
+// triggeredByParameterUpdate determines what plan to run based on parameters that changed and the corresponding parameter trigger.
+func triggeredByParameterUpdate(params []kudov1beta1.Parameter, ov *kudov1beta1.OperatorVersion) (*string, error) {
 	// If no parameters were changed, we return an empty string so no plan would be triggered
 	if len(params) == 0 {
 		return nil, nil
@@ -320,6 +324,22 @@ func triggeredPlan(params []kudov1beta1.Parameter, ov *kudov1beta1.OperatorVersi
 	}
 }
 
+// merge method merges two maps and returns the result. Note, that left map is being modified in process.
+func changedParameterDefinitions(old map[string]string, new map[string]string, ov *kudov1beta1.OperatorVersion) ([]kudov1beta1.Parameter, error) {
+	c, r := kudov1beta1.RichParameterDiff(old, new)
+	cpd, err := kudov1beta1.GetParamDefinitions(c, ov)
+	if err != nil {
+		return nil, err
+	}
+
+	// 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.
+	rpd, _ := kudov1beta1.GetParamDefinitions(r, ov)
+
+	return append(cpd, rpd...), nil
+}
+
 // InstanceAdmission implements inject.Client.
 // A client will be automatically injected.
 

pkg/webhook/instance_admission_test.go

@@ -299,15 +299,15 @@ func TestValidateUpdate(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "parameter update triggering a non-existing OV plan IS allowed but will NOT trigger a plan",
+			name: "parameter update triggering a non-existing OV plan IS NOT allowed",
 			old:  idle,
 			new: func() *v1beta1.Instance {
 				i := idle.DeepCopy()
 				i.Spec.Parameters["bazz"] = "newBazz"
 				return i
 			}(),
-			ov:   ov,
-			want: nil,
+			ov:      ov,
+			wantErr: true,
 		},
 		{
 			name: "parameter update together with an upgrade IS normally NOT allowed",
@@ -441,9 +441,9 @@ func Test_triggeredPlan(t *testing.T) {
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := triggeredPlan(tt.params, tt.ov)
+			got, err := triggeredByParameterUpdate(tt.params, tt.ov)
 			if (err != nil) != tt.wantErr {
-				t.Errorf("triggeredPlan() error = %v, wantErr %v, got = %s", err, tt.wantErr, stringPtrToString(got))
+				t.Errorf("triggeredByParameterUpdate() error = %v, wantErr %v, got = %s", err, tt.wantErr, stringPtrToString(got))
 				return
 			}
 			assert.Equal(t, tt.want, got)