Allow simultaneous upgrade and parameter update (#1495)

if `deploy` plan is triggered by the update. This seems to be a reasonable exception, since `deploy` plan is used for upgrades in many cases anyway.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

pkg/webhook/instance_admission.go

@@ -197,24 +197,28 @@ func admitUpdate(old, new *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion
 	}
 
 	isParameterUpdate := triggeredPlan != nil
+	updateIncompatibleWithUpgrade := isParameterUpdate && *triggeredPlan != kudov1beta1.DeployPlanName
 
-	// --------------------------------------------------------------------------------------------------------------------------------
-	// ---- Instance can have two major life-cycle phases: normal and cleanup (uninstall) phase. Different rule sets apply in both. ---
-	// --------------------------------------------------------------------------------------------------------------------------------
+	// --------------------------------------------------------------------------------------------
+	// --- Instance can have two major life-cycle phases: normal and cleanup (uninstall) phase. ---
+	// --- Different rule sets apply in both.                               				    ---
+	// ---------------------------------------------------------------------------------------------
 
+	// ----------------------------------
 	// --- Instance uninstall/cleanup ---
+	// ----------------------------------
 	// Following rules apply:
 	// - only 'cleanup' plan (if exists) can be scheduled in this phase
 	// - only the instance controller (and not the webhook or the user) can schedule 'cleanup'
 	// - 'cleanup' overrides any existing update/upgrade plan
 	// - 'cleanup' itself can *NOT* be cancelled or overridden by any other plan since the instance is being deleted
 	if isDeleting {
 		Cleanup := kudov1beta1.CleanupPlanName
-		cleanupOverride := oldPlan == Cleanup && newPlan != oldPlan
+		isCleanupOverride := oldPlan == Cleanup && newPlan != oldPlan
 		notCleanupScheduled := newPlan != "" && newPlan != Cleanup
 
 		switch {
-		case cleanupOverride:
+		case isCleanupOverride:
 			return nil, fmt.Errorf("failed to update Instance %s/%s: '%s' plan can not be cancelled or overridden by another plan since the instance is being deleted", old.Namespace, old.Name, oldPlan)
 		case isParameterUpdate || isUpgrade:
 			return nil, fmt.Errorf("failed to update Instance %s/%s: parameter update and/or upgrade is not allowed when an instance is being deleted", old.Namespace, old.Name)
@@ -225,20 +229,22 @@ func admitUpdate(old, new *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion
 		return nil, nil
 	}
 
+	// ----------------------------
 	// ---- Normal life-cycle -----
+	// ----------------------------
 	switch {
 	case hadPlan && isParameterUpdate && *triggeredPlan != oldPlan:
 		return nil, fmt.Errorf("failed to update Instance %s/%s: plan '%s' is scheduled (or running) and an update would trigger a different plan '%s'", old.Namespace, old.Name, oldPlan, *triggeredPlan)
 	case isUpgrade && hadPlan:
 		return nil, fmt.Errorf("failed to update Instance %s/%s: upgrade to new OperatorVersion %s while a plan '%s' is scheduled (or running) is not allowed", old.Namespace, old.Name, newOvRef, oldPlan)
 	case isUpgrade && isNovelPlan:
 		return nil, fmt.Errorf("failed to update Instance %s/%s: upgrade to new OperatorVersion %s and triggering new plan '%s' is not allowed", old.Namespace, old.Name, newOvRef, newPlan)
+	case isUpgrade && updateIncompatibleWithUpgrade:
+		return nil, fmt.Errorf("failed to update Instance %s/%s: upgrade to new OperatorVersion %s together with a parameter update triggering '%s' is not allowed", old.Namespace, old.Name, newOvRef, *triggeredPlan)
 	case isPlanOverride:
 		return nil, fmt.Errorf("failed to update Instance %s/%s: overriding currently scheduled (or running) plan '%s' with '%s' is not supported", old.Namespace, old.Name, oldPlan, newPlan)
 	case isPlanCancellation:
 		return nil, fmt.Errorf("failed to update Instance %s/%s: cancelling currently scheduled (or running) plan '%s' is not supported", old.Namespace, old.Name, oldPlan)
-	case isParameterUpdate && isUpgrade:
-		return nil, fmt.Errorf("failed to update Instance %s/%s: upgrade to new OperatorVersion %s together with a parameter update triggering '%s' is not allowed", old.Namespace, old.Name, newOvRef, *triggeredPlan)
 	case isParameterUpdate && isNovelPlan:
 		return nil, fmt.Errorf("failed to update Instance %s/%s: triggering one plan '%s' directly and through parameter update '%s' is not allowed", old.Namespace, old.Name, oldPlan, newPlan)
 	// this case is effectively a noop because isPlanOverride is disallowed for now. However, once plan overrides are implemented, this will be needed so don't remove.

pkg/webhook/instance_admission_test.go

@@ -18,6 +18,7 @@ func TestValidateUpdate(t *testing.T) {
 	deploy := v1beta1.DeployPlanName
 	update := v1beta1.UpdatePlanName
 	cleanup := v1beta1.CleanupPlanName
+	backup := "backup"
 	empty := ""
 
 	testUUID := uuid.NewUUID()
@@ -26,7 +27,7 @@ func TestValidateUpdate(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{Name: "foo-operator", Namespace: "default"},
 		TypeMeta:   metav1.TypeMeta{Kind: "OperatorVersion", APIVersion: "kudo.dev/v1beta1"},
 		Spec: v1beta1.OperatorVersionSpec{
-			Plans: map[string]v1beta1.Plan{deploy: {}, update: {}, cleanup: {}},
+			Plans: map[string]v1beta1.Plan{deploy: {}, update: {}, cleanup: {}, backup: {}},
 			Parameters: []v1beta1.Parameter{
 				{
 					Name:    "foo",
@@ -40,6 +41,10 @@ func TestValidateUpdate(t *testing.T) {
 					Name:    "bar",
 					Trigger: update,
 				},
+				{
+					Name:    "backup",
+					Trigger: backup,
+				},
 				{
 					Name:    "invalid",
 					Trigger: "missing",
@@ -323,27 +328,39 @@ func TestValidateUpdate(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "parameter update together with an upgrade IS normally NOT allowed",
+			name: "parameter update together with an upgrade IS NOT allowed if a plan other than deploy is triggered",
 			old:  idle,
 			new: func() *v1beta1.Instance {
 				i := upgraded.DeepCopy()
-				i.Spec.Parameters = map[string]string{"foo": "newFoo"}
+				i.Spec.Parameters = map[string]string{"backup": "back"}
 				return i
 			}(),
 			ov:      ov,
 			wantErr: true,
 		},
+		{
+			name: "parameter update together with an upgrade IS allowed if deploy is triggered",
+			old:  idle,
+			new: func() *v1beta1.Instance {
+				i := upgraded.DeepCopy()
+				i.Spec.Parameters = map[string]string{"foo": "newFoo"}
+				return i
+			}(),
+			ov:      ov,
+			wantErr: false,
+			want:    &update,
+		},
 		{
 			name: "parameter update together with an upgrade IS allowed if update removes a parameter that doesn't exist in the new OV",
 			old:  idle,
 			new: func() *v1beta1.Instance {
 				i := upgraded.DeepCopy()
-				delete(i.Spec.Parameters, "foo") // removing from instance parameters
+				delete(i.Spec.Parameters, "backup") // removing from instance parameters
 				return i
 			}(),
 			ov: func() *v1beta1.OperatorVersion {
 				o := ov.DeepCopy()
-				o.Spec.Parameters = o.Spec.Parameters[1:len(o.Spec.Parameters)] // "foo" is the first parameter in the array
+				o.Spec.Parameters = o.Spec.Parameters[:len(o.Spec.Parameters)-1] // "backup" is the last parameter in the array
 				return o
 			}(),
 			want: &update,