From 31785efdc5d6f41062ab8480e66e42cdd819bb59 Mon Sep 17 00:00:00 2001 From: Jonathan Knight Date: Wed, 21 Sep 2022 12:07:52 +0300 Subject: [PATCH] Reject changes that would create an invalid StatefulSet patch (#552) --- api/v1/coherence_webhook.go | 32 +++++++++++++++++++ api/v1/coherenceresourcespec_types.go | 21 ++++++++---- api/v1/common_test.go | 2 +- .../statefulset/statefulset_controller.go | 24 +++++++++----- pkg/runner/runner_test.go | 2 +- 5 files changed, 64 insertions(+), 17 deletions(-) diff --git a/api/v1/coherence_webhook.go b/api/v1/coherence_webhook.go index 131574347..e69ed67f3 100644 --- a/api/v1/coherence_webhook.go +++ b/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 +} diff --git a/api/v1/coherenceresourcespec_types.go b/api/v1/coherenceresourcespec_types.go index 469d558c4..eef520268 100644 --- a/api/v1/coherenceresourcespec_types.go +++ b/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 { diff --git a/api/v1/common_test.go b/api/v1/common_test.go index 2656611dd..c3064c8ae 100644 --- a/api/v1/common_test.go +++ b/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) } diff --git a/controllers/statefulset/statefulset_controller.go b/controllers/statefulset/statefulset_controller.go index bf8f2e14f..d9bc0c943 100644 --- a/controllers/statefulset/statefulset_controller.go +++ b/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] diff --git a/pkg/runner/runner_test.go b/pkg/runner/runner_test.go index aeb88235f..587ecc75b 100644 --- a/pkg/runner/runner_test.go +++ b/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 {