Reject changes that would create an invalid StatefulSet patch (#552)

Author: thegridman

api/v1/coherence_webhook.go

@@ -10,8 +10,11 @@ import (
 	"fmt"
 	"github.com/go-test/deep"
 	"github.com/oracle/coherence-operator/pkg/operator"
+	appsv1 "k8s.io/api/apps/v1"
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -131,6 +134,7 @@ func (in *Coherence) ValidateUpdate(previous runtime.Object) error {
 		return err
 	}
 	prev := previous.(*Coherence)
+
 	if err := in.validatePersistence(prev); err != nil {
 		return err
 	}
@@ -140,6 +144,14 @@ func (in *Coherence) ValidateUpdate(previous runtime.Object) error {
 	if err := in.validateNodePorts(); err != nil {
 		return err
 	}
+
+	sts := in.Spec.CreateStatefulSet(in)
+	stsOld := prev.Spec.CreateStatefulSet(prev)
+	errorList := ValidateStatefulSetUpdate(&sts, &stsOld)
+	if len(errorList) > 0 {
+		return fmt.Errorf("rejecting update as it would have resulted in an invalid statefuleset: %v", errorList)
+	}
+
 	return nil
 }
 
@@ -212,3 +224,23 @@ func (in *Coherence) validateNodePorts() error {
 
 	return nil
 }
+
+// ValidateStatefulSetUpdate tests if required fields in the StatefulSet are set.
+func ValidateStatefulSetUpdate(statefulSet, oldStatefulSet *appsv1.StatefulSet) field.ErrorList {
+	var allErrs field.ErrorList
+
+	// statefulset updates aren't super common and general updates are likely to be touching spec, so we'll do this
+	// deep copy right away.  This avoids mutating our inputs
+	newStatefulSetClone := statefulSet.DeepCopy()
+	newStatefulSetClone.Spec.Replicas = oldStatefulSet.Spec.Replicas               // +k8s:verify-mutation:reason=clone
+	newStatefulSetClone.Spec.Template = oldStatefulSet.Spec.Template               // +k8s:verify-mutation:reason=clone
+	newStatefulSetClone.Spec.UpdateStrategy = oldStatefulSet.Spec.UpdateStrategy   // +k8s:verify-mutation:reason=clone
+	newStatefulSetClone.Spec.MinReadySeconds = oldStatefulSet.Spec.MinReadySeconds // +k8s:verify-mutation:reason=clone
+
+	newStatefulSetClone.Spec.PersistentVolumeClaimRetentionPolicy = oldStatefulSet.Spec.PersistentVolumeClaimRetentionPolicy // +k8s:verify-mutation:reason=clone
+	if !apiequality.Semantic.DeepEqual(newStatefulSetClone.Spec, oldStatefulSet.Spec) {
+		allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "updates to statefulset spec for fields other than 'replicas', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden"))
+	}
+
+	return allErrs
+}

api/v1/coherenceresourcespec_types.go

@@ -564,7 +564,7 @@ func (in *CoherenceResourceSpec) CreateKubernetesResources(d *Coherence) (Resour
 	res = append(res, in.CreateHeadlessService(d))
 
 	// Create the StatefulSet
-	res = append(res, in.CreateStatefulSet(d))
+	res = append(res, in.CreateStatefulSetResource(d))
 
 	// Create the Services for each port (and optionally ServiceMonitors)
 	res = append(res, in.CreateServicesForPort(d)...)
@@ -722,8 +722,19 @@ func (in *CoherenceResourceSpec) CreateHeadlessService(deployment *Coherence) Re
 	}
 }
 
+// CreateStatefulSetResource creates the deployment's StatefulSet resource.
+func (in *CoherenceResourceSpec) CreateStatefulSetResource(deployment *Coherence) Resource {
+	sts := in.CreateStatefulSet(deployment)
+
+	return Resource{
+		Kind: ResourceTypeStatefulSet,
+		Name: sts.GetName(),
+		Spec: &sts,
+	}
+}
+
 // CreateStatefulSet creates the deployment's StatefulSet.
-func (in *CoherenceResourceSpec) CreateStatefulSet(deployment *Coherence) Resource {
+func (in *CoherenceResourceSpec) CreateStatefulSet(deployment *Coherence) appsv1.StatefulSet {
 	sts := appsv1.StatefulSet{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: deployment.GetNamespace(),
@@ -830,11 +841,7 @@ func (in *CoherenceResourceSpec) CreateStatefulSet(deployment *Coherence) Resour
 		sts.Spec.VolumeClaimTemplates = append(sts.Spec.VolumeClaimTemplates, v.ToPVC())
 	}
 
-	return Resource{
-		Kind: ResourceTypeStatefulSet,
-		Name: sts.GetName(),
-		Spec: &sts,
-	}
+	return sts
 }
 
 func (in *CoherenceResourceSpec) GetImagePullSecrets() []corev1.LocalObjectReference {

api/v1/common_test.go

@@ -473,6 +473,6 @@ func assertStatefulSetCreation(t *testing.T, deployment *coh.Coherence, stsExpec
 	viper.Set(operator.FlagCoherenceImage, testCoherenceImage)
 	viper.Set(operator.FlagOperatorImage, testOperatorImage)
 
-	res := deployment.Spec.CreateStatefulSet(deployment)
+	res := deployment.Spec.CreateStatefulSetResource(deployment)
 	assertStatefulSet(t, res, stsExpected)
 }

controllers/statefulset/statefulset_controller.go

@@ -45,7 +45,7 @@ const (
 )
 
 // blank assignment to verify that ReconcileStatefulSet implements reconcile.Reconciler.
-// If the reconcile.Reconciler API was to change then we'd get a compile error here.
+// If the `reconcile.Reconciler` API was to change then we'd get a compile error here.
 var _ reconcile.Reconciler = &ReconcileStatefulSet{}
 
 var log = logf.Log.WithName(controllerName)
@@ -397,12 +397,12 @@ func (in *ReconcileStatefulSet) updateStatefulSet(ctx context.Context, deploymen
 	switch {
 	case currentReplicas < desiredReplicas:
 		// scale up - if also updating we do the rolling upgrade first followed by the
-		// scale up so we do not do a rolling upgrade of the bigger scaled up cluster
+		// scale up so that we do not do a rolling upgrade of the bigger scaled up cluster
 
 		// try the patch first
 		result, err = in.patchStatefulSet(ctx, deployment, current, desired, storage, logger)
 		if err == nil && !result.Requeue {
-			// there was nothing else to patch so we can do the scale up
+			// there was nothing else to patch, so we can do the scale up
 			result, err = in.scale(ctx, deployment, current, currentReplicas, desiredReplicas)
 		}
 	case currentReplicas > desiredReplicas:
@@ -411,7 +411,7 @@ func (in *ReconcileStatefulSet) updateStatefulSet(ctx context.Context, deploymen
 
 		// do the scale down
 		_, err = in.scale(ctx, deployment, current, currentReplicas, desiredReplicas)
-		// requeue the request so we do any upgrade next time around
+		// requeue the request so that we do any upgrade next time around
 		result.Requeue = true
 	default:
 		// just an update
@@ -441,8 +441,16 @@ func (in *ReconcileStatefulSet) patchStatefulSet(ctx context.Context, deployment
 		original = desired
 	}
 
+	errorList := coh.ValidateStatefulSetUpdate(desired, original)
+	if len(errorList) > 0 {
+		msg := fmt.Sprintf("upddates to the statefuleset would have been invalid, the update will not be re-queued: %v", errorList)
+		events := in.GetEventRecorder()
+		events.Event(deployment, corev1.EventTypeWarning, reconciler.EventReasonUpdated, msg)
+		return reconcile.Result{Requeue: false}, fmt.Errorf(msg)
+	}
+
 	// We NEVER change the replicas or Status in an update.
-	// Replicas is handled by scaling so we always set the desired replicas to match the current replicas
+	// Replicas is handled by scaling, so we always set the desired replicas to match the current replicas
 	desired.Spec.Replicas = current.Spec.Replicas
 	original.Spec.Replicas = current.Spec.Replicas
 
@@ -451,7 +459,7 @@ func (in *ReconcileStatefulSet) patchStatefulSet(ctx context.Context, deployment
 	desired.ObjectMeta.Finalizers = current.ObjectMeta.Finalizers
 
 	// We need to ensure we do not create a patch due to differences in
-	// StatefulSet Status so we blank out the status fields
+	// StatefulSet Status, so we blank out the status fields
 	desired.Status = appsv1.StatefulSetStatus{}
 	current.Status = appsv1.StatefulSetStatus{}
 	original.Status = appsv1.StatefulSetStatus{}
@@ -465,7 +473,7 @@ func (in *ReconcileStatefulSet) patchStatefulSet(ctx context.Context, deployment
 
 	// ensure we do not patch any fields that may be set by a previous version of the Operator
 	// as this will cause a rolling update of the Pods, typically these are fields where
-	// the Operator sets defaults and we changed the default behaviour
+	// the Operator sets defaults, and we changed the default behaviour
 	in.blankContainerFields(deployment, desired)
 	in.blankContainerFields(deployment, current)
 	in.blankContainerFields(deployment, original)
@@ -663,7 +671,7 @@ func (in *ReconcileStatefulSet) blankOperatorInitContainerFields(sts *appsv1.Sta
 
 // Blanks out any fields that may have been set by a previous Operator version.
 // DO NOT blank out anything that the user has control over as they may have
-// updated them so we need to include them in the patch
+// updated them, so we need to include them in the patch
 func (in *ReconcileStatefulSet) blankCoherenceContainerFields(sts *appsv1.StatefulSet) {
 	for i := range sts.Spec.Template.Spec.Containers {
 		c := sts.Spec.Template.Spec.Containers[i]

pkg/runner/runner_test.go

@@ -173,7 +173,7 @@ func EnvVarsFromDeployment(d *coh.Coherence) map[string]string {
 		d.Spec.JVM = &coh.JVMSpec{}
 	}
 
-	res := d.Spec.CreateStatefulSet(d)
+	res := d.Spec.CreateStatefulSetResource(d)
 	sts := res.Spec.(*appsv1.StatefulSet)
 	c := coh.FindContainer(coh.ContainerNameCoherence, sts)
 	for _, ev := range c.Env {