Online image change: new webhook rules (#141)

This adds rules to the webhook for online image change: - prevent changes to upgradePolicy when imageChange is in progress - transient subcluster template: isPrimary == false, name cannot be an existing subcluster, size > 0 if name present - transient subcluster cannot be added/removed during an online image change - if multiple subclusters share a ServiceName, service specific things must be common between them (serviceType, NodePort, externalIPs, etc).
vertica · Jan 14, 2022 · e7d2b2e · e7d2b2e
1 parent 1a3e9fb
commit e7d2b2e
Show file tree

Hide file tree

Showing 3 changed files with 238 additions and 0 deletions.
diff --git a/api/v1beta1/verticadb_webhook.go b/api/v1beta1/verticadb_webhook.go
@@ -19,6 +19,7 @@ package v1beta1
 
 import (
 	"fmt"
+	"reflect"
 	"strings"
 
 	"github.com/vertica/vertica-kubernetes/pkg/paths"
@@ -118,6 +119,7 @@ func (v *VerticaDB) Default() {
 	if v.Spec.Communal.Endpoint == "" && v.IsGCloud() {
 		v.Spec.Communal.Endpoint = DefaultGCloudEndpoint
 	}
+	v.Spec.TemporarySubclusterRouting.Template.IsPrimary = false
 }
 
 //+kubebuilder:webhook:path=/validate-vertica-com-v1beta1-verticadb,mutating=false,failurePolicy=fail,sideEffects=None,groups=vertica.com,resources=verticadbs,verbs=create;update,versions=v1beta1,name=vverticadb.kb.io,admissionReviewVersions={v1,v1beta1}
@@ -242,6 +244,8 @@ func (v *VerticaDB) validateImmutableFields(old runtime.Object) field.ErrorList
 			fmt.Sprintf("subcluster %s cannot have its isPrimary type change", v.Spec.Subclusters[inx].Name))
 		allErrs = append(allErrs, err)
 	}
+	allErrs = v.checkImmutableImageChangePolicy(oldObj, allErrs)
+	allErrs = v.checkImmutableTemporarySubclusterRouting(oldObj, allErrs)
 	return allErrs
 }
 
@@ -262,6 +266,8 @@ func (v *VerticaDB) validateVerticaDBSpec() field.ErrorList {
 	allErrs = v.hasValidVolumeName(allErrs)
 	allErrs = v.hasValidVolumeMountName(allErrs)
 	allErrs = v.hasValidKerberosSetup(allErrs)
+	allErrs = v.hasValidTemporarySubclusterRouting(allErrs)
+	allErrs = v.matchingServiceNamesAreConsistent(allErrs)
 	if len(allErrs) == 0 {
 		return nil
 	}
@@ -592,6 +598,43 @@ func (v *VerticaDB) hasValidKerberosSetup(allErrs field.ErrorList) field.ErrorLi
 	return allErrs
 }
 
+// hasValidTemporarySubclusterRouting verifies the contents of
+// temporarySubclusterRouting are valid
+func (v *VerticaDB) hasValidTemporarySubclusterRouting(allErrs field.ErrorList) field.ErrorList {
+	scMap := v.GenSubclusterMap()
+	fieldPrefix := field.NewPath("spec").Child("temporarySubclusterRouting")
+	if v.Spec.TemporarySubclusterRouting.Template.Name != "" {
+		templateFieldPrefix := fieldPrefix.Child("template")
+		if v.Spec.TemporarySubclusterRouting.Template.IsPrimary {
+			err := field.Invalid(templateFieldPrefix.Child("isPrimary"),
+				v.Spec.TemporarySubclusterRouting.Template.IsPrimary,
+				"subcluster template must be a secondary subcluster")
+			allErrs = append(allErrs, err)
+		}
+		if v.Spec.TemporarySubclusterRouting.Template.Size == 0 {
+			err := field.Invalid(templateFieldPrefix.Child("size"),
+				v.Spec.TemporarySubclusterRouting.Template.Size,
+				"size of subcluster template must be greater than zero")
+			allErrs = append(allErrs, err)
+		}
+		if _, ok := scMap[v.Spec.TemporarySubclusterRouting.Template.Name]; ok {
+			err := field.Invalid(templateFieldPrefix.Child("name"),
+				v.Spec.TemporarySubclusterRouting.Template.Name,
+				"cannot choose a name of an existing subcluster")
+			allErrs = append(allErrs, err)
+		}
+	}
+	for i := range v.Spec.TemporarySubclusterRouting.Names {
+		if _, ok := scMap[v.Spec.TemporarySubclusterRouting.Names[i]]; !ok {
+			err := field.Invalid(fieldPrefix.Child("names").Index(i),
+				v.Spec.TemporarySubclusterRouting.Names[i],
+				"name must be an existing subcluster")
+			allErrs = append(allErrs, err)
+		}
+	}
+	return allErrs
+}
+
 func (v *VerticaDB) isSubclusterTypeIsChanging(oldObj *VerticaDB) (ok bool, scInx int) {
 	// Create a map of subclusterName -> isPrimary using the old object.
 	nameToPrimaryMap := map[string]bool{}
@@ -610,3 +653,86 @@ func (v *VerticaDB) isSubclusterTypeIsChanging(oldObj *VerticaDB) (ok bool, scIn
 	}
 	return false, 0
 }
+
+// matchingServiceNamesAreConsistent ensures that any subclusters that share the
+// same service name have matching values in them that pertain to the service object.
+func (v *VerticaDB) matchingServiceNamesAreConsistent(allErrs field.ErrorList) field.ErrorList {
+	processedServiceName := map[string]bool{}
+
+	for i := range v.Spec.Subclusters {
+		sc := &v.Spec.Subclusters[i]
+		if _, ok := processedServiceName[sc.ServiceName]; ok {
+			continue
+		}
+		for j := i + 1; j < len(v.Spec.Subclusters); j++ {
+			osc := &v.Spec.Subclusters[j]
+			if sc.ServiceName == osc.ServiceName {
+				fieldPrefix := field.NewPath("spec").Child("subclusters").Index(j)
+				if !reflect.DeepEqual(sc.ExternalIPs, osc.ExternalIPs) {
+					err := field.Invalid(fieldPrefix.Child("externalIPs").Index(i),
+						sc.ExternalIPs,
+						"externalIPs don't match other subcluster(s) sharing the same serviceName")
+					allErrs = append(allErrs, err)
+				}
+				if sc.NodePort != osc.NodePort {
+					err := field.Invalid(fieldPrefix.Child("nodePort").Index(i),
+						sc.NodePort,
+						"nodePort doesn't match other subcluster(s) sharing the same serviceName")
+					allErrs = append(allErrs, err)
+				}
+				if sc.ServiceType != osc.ServiceType {
+					err := field.Invalid(fieldPrefix.Child("serviceType").Index(i),
+						sc.ServiceType,
+						"serviceType doesn't match other subcluster(s) sharing the same serviceName")
+					allErrs = append(allErrs, err)
+				}
+			}
+		}
+		// Set a flag so that we don't process this service name in another subcluster
+		processedServiceName[sc.ServiceName] = true
+	}
+	return allErrs
+}
+
+func (v *VerticaDB) isImageChangeInProgress() bool {
+	return len(v.Status.Conditions) > ImageChangeInProgressIndex &&
+		v.Status.Conditions[ImageChangeInProgressIndex].Status == v1.ConditionTrue
+}
+
+// checkImmutableImageChangePolicy will see if it unsafe to change the
+// imageChangePolicy.  It will log an error if it detects a change in that field
+// when it isn't allowed.
+func (v *VerticaDB) checkImmutableImageChangePolicy(oldObj *VerticaDB, allErrs field.ErrorList) field.ErrorList {
+	if v.Spec.ImageChangePolicy == oldObj.Spec.ImageChangePolicy ||
+		!oldObj.isImageChangeInProgress() {
+		return allErrs
+	}
+	err := field.Invalid(field.NewPath("spec").Child("imageChangePolicy"),
+		v.Spec.ImageChangePolicy,
+		"imageChangePolicy cannot change because image change is in progress")
+	allErrs = append(allErrs, err)
+	return allErrs
+}
+
+// checkImmutableTemporarySubclusterRouting will check if
+// temporarySubclusterRouting is changing when it isn't allowed to.
+func (v *VerticaDB) checkImmutableTemporarySubclusterRouting(oldObj *VerticaDB, allErrs field.ErrorList) field.ErrorList {
+	// TemporarySubclusterRouting is allowed to change as long as an image
+	// change isn't in progress
+	if !oldObj.isImageChangeInProgress() {
+		return allErrs
+	}
+	if !reflect.DeepEqual(v.Spec.TemporarySubclusterRouting.Names, oldObj.Spec.TemporarySubclusterRouting.Names) {
+		err := field.Invalid(field.NewPath("spec").Child("temporarySubclusterRouting").Child("names"),
+			v.Spec.TemporarySubclusterRouting.Names,
+			"subcluster names for temporasySubclusterRouting cannot change when an image change is in progress")
+		allErrs = append(allErrs, err)
+	}
+	if !reflect.DeepEqual(v.Spec.TemporarySubclusterRouting.Template, oldObj.Spec.TemporarySubclusterRouting.Template) {
+		err := field.Invalid(field.NewPath("spec").Child("temporarySubclusterRouting").Child("template"),
+			v.Spec.TemporarySubclusterRouting.Template,
+			"template for temporasySubclusterRouting cannot change when an image change is in progress")
+		allErrs = append(allErrs, err)
+	}
+	return allErrs
+}
diff --git a/api/v1beta1/verticadb_webhook_test.go b/api/v1beta1/verticadb_webhook_test.go
@@ -322,6 +322,109 @@ var _ = Describe("verticadb_webhook", func() {
 			validateSpecValuesHaveErr(vdb, true)
 		}
 	})
+
+	It("should allow imageChangePolicy to be changed when image change is not in progress", func() {
+		vdbUpdate := createVDBHelper()
+		vdbOrig := createVDBHelper()
+		vdbOrig.Spec.ImageChangePolicy = OfflineImageChange
+		vdbUpdate.Spec.ImageChangePolicy = OnlineImageChange
+		allErrs := vdbOrig.validateImmutableFields(vdbUpdate)
+		Expect(allErrs).Should(BeNil())
+
+		vdbUpdate.Status.Conditions = make([]VerticaDBCondition, ImageChangeInProgressIndex+1)
+		vdbUpdate.Status.Conditions[ImageChangeInProgressIndex] = VerticaDBCondition{
+			Status: v1.ConditionTrue,
+		}
+		allErrs = vdbOrig.validateImmutableFields(vdbUpdate)
+		Expect(allErrs).ShouldNot(BeNil())
+	})
+
+	It("should fail for various issues with temporary subcluster routing template", func() {
+		vdb := createVDBHelper()
+		vdb.Spec.TemporarySubclusterRouting.Template.Name = vdb.Spec.Subclusters[0].Name
+		vdb.Spec.TemporarySubclusterRouting.Template.Size = 1
+		vdb.Spec.TemporarySubclusterRouting.Template.IsPrimary = false
+		validateSpecValuesHaveErr(vdb, true)
+
+		vdb.Spec.TemporarySubclusterRouting.Template.Name = "transient"
+		vdb.Spec.TemporarySubclusterRouting.Template.Size = 0
+		validateSpecValuesHaveErr(vdb, true)
+
+		vdb.Spec.TemporarySubclusterRouting.Template.Size = 1
+		vdb.Spec.TemporarySubclusterRouting.Template.IsPrimary = true
+		validateSpecValuesHaveErr(vdb, true)
+
+		vdb.Spec.TemporarySubclusterRouting.Template.IsPrimary = false
+		validateSpecValuesHaveErr(vdb, false)
+	})
+
+	It("should fail if temporary routing to a subcluster doesn't exist", func() {
+		vdb := createVDBHelper()
+		const ValidScName = "sc1"
+		const InvalidScName = "notexists"
+		vdb.Spec.Subclusters[0].Name = ValidScName
+		vdb.Spec.TemporarySubclusterRouting.Names = []string{InvalidScName}
+		validateSpecValuesHaveErr(vdb, true)
+
+		vdb.Spec.TemporarySubclusterRouting.Names = []string{ValidScName, InvalidScName}
+		validateSpecValuesHaveErr(vdb, true)
+
+		vdb.Spec.TemporarySubclusterRouting.Names = []string{ValidScName}
+		validateSpecValuesHaveErr(vdb, false)
+	})
+
+	It("should prevent change to temporarySubclusterRouting when image change in progress", func() {
+		vdbUpdate := createVDBHelper()
+		vdbOrig := createVDBHelper()
+
+		vdbUpdate.Status.Conditions = make([]VerticaDBCondition, ImageChangeInProgressIndex+1)
+		vdbUpdate.Status.Conditions[ImageChangeInProgressIndex] = VerticaDBCondition{
+			Status: v1.ConditionTrue,
+		}
+
+		vdbUpdate.Spec.TemporarySubclusterRouting.Names = []string{"sc1", "sc2"}
+		vdbOrig.Spec.TemporarySubclusterRouting.Names = []string{"sc3", "sc4"}
+		allErrs := vdbOrig.validateImmutableFields(vdbUpdate)
+		Expect(allErrs).ShouldNot(BeNil())
+
+		vdbUpdate.Spec.TemporarySubclusterRouting.Names = vdbOrig.Spec.TemporarySubclusterRouting.Names
+		vdbUpdate.Spec.TemporarySubclusterRouting.Template.Name = "transient-sc"
+		vdbOrig.Spec.TemporarySubclusterRouting.Template.Name = "another-name-transient-sc"
+		allErrs = vdbOrig.validateImmutableFields(vdbUpdate)
+		Expect(allErrs).ShouldNot(BeNil())
+	})
+
+	It("should error out if service specific fields are different in subclusters with matching serviceNames", func() {
+		vdb := createVDBHelper()
+		const ServiceName = "main"
+		vdb.Spec.Subclusters = []Subcluster{
+			{
+				Name:        "sc1",
+				Size:        2,
+				IsPrimary:   true,
+				ServiceName: ServiceName,
+				ServiceType: "NodePort",
+				NodePort:    30008,
+				ExternalIPs: []string{"8.1.2.3", "8.2.4.6"},
+			},
+			{
+				Name:        "sc2",
+				Size:        1,
+				IsPrimary:   false,
+				ServiceName: ServiceName,
+				ServiceType: "ClusterIP",
+				NodePort:    30009,
+				ExternalIPs: []string{"8.1.2.3", "7.2.4.6"},
+			},
+		}
+		validateSpecValuesHaveErr(vdb, true)
+		vdb.Spec.Subclusters[1].ServiceType = vdb.Spec.Subclusters[0].ServiceType
+		validateSpecValuesHaveErr(vdb, true)
+		vdb.Spec.Subclusters[1].NodePort = vdb.Spec.Subclusters[0].NodePort
+		validateSpecValuesHaveErr(vdb, true)
+		vdb.Spec.Subclusters[1].ExternalIPs[1] = vdb.Spec.Subclusters[0].ExternalIPs[1]
+		validateSpecValuesHaveErr(vdb, false)
+	})
 })
 
 func createVDBHelper() *VerticaDB {

diff --git a/pkg/controllers/onlineimagechange_reconciler.go b/pkg/controllers/onlineimagechange_reconciler.go
@@ -159,6 +159,9 @@ func (o *OnlineImageChangeReconciler) installTransientNodes(ctx context.Context)
 
 	actor := MakeInstallReconciler(o.VRec, o.Log, o.Vdb, o.PRunner, o.PFacts)
 	o.traceActorReconcile(actor)
+	if err := o.PFacts.Collect(ctx, o.Vdb); err != nil {
+		return ctrl.Result{}, err
+	}
 	return actor.Reconcile(ctx, &ctrl.Request{})
 }
 
@@ -170,6 +173,9 @@ func (o *OnlineImageChangeReconciler) addTransientSubcluster(ctx context.Context
 
 	actor := MakeDBAddSubclusterReconciler(o.VRec, o.Log, o.Vdb, o.PRunner, o.PFacts)
 	o.traceActorReconcile(actor)
+	if err := o.PFacts.Collect(ctx, o.Vdb); err != nil {
+		return ctrl.Result{}, err
+	}
 	d := actor.(*DBAddSubclusterReconciler)
 	return d.addMissingSubclusters(ctx, []vapi.Subcluster{*buildTransientSubcluster(o.Vdb, "")})
 }
@@ -183,6 +189,9 @@ func (o *OnlineImageChangeReconciler) addTransientNodes(ctx context.Context) (ct
 
 	actor := MakeDBAddNodeReconciler(o.VRec, o.Log, o.Vdb, o.PRunner, o.PFacts)
 	o.traceActorReconcile(actor)
+	if err := o.PFacts.Collect(ctx, o.Vdb); err != nil {
+		return ctrl.Result{}, err
+	}
 	d := actor.(*DBAddNodeReconciler)
 	return d.reconcileSubcluster(ctx, buildTransientSubcluster(o.Vdb, ""))
 }