Fix updating invalid Instance parameters (#1441)

Summary:
in the past, we've allowed updating Instance with parameters that are missing in the corresponding OV (since there was no way to forbid this in the absence of an admission controller). Now that we have an instance admission controller in place, we can fix this behavior.

Fixes #1411

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

@@ -40,6 +40,10 @@ func TestValidateUpdate(t *testing.T) {
 					Name:    "bar",
 					Trigger: update,
 				},
+				{
+					Name:    "invalid",
+					Trigger: "missing",
+				},
 			},
 		},
 	}
@@ -299,15 +303,26 @@ 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 parameter 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 triggering a non-existing OV plan IS NOT allowed",
+			old:  idle,
+			new: func() *v1beta1.Instance {
+				i := idle.DeepCopy()
+				i.Spec.Parameters["invalid"] = "invalid"
+				return i
+			}(),
+			ov:      ov,
+			wantErr: true,
 		},
 		{
 			name: "parameter update together with an upgrade IS normally NOT allowed",
@@ -441,9 +456,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)