From ad7bde093a358067e7fd347131a00ce360740792 Mon Sep 17 00:00:00 2001 From: hihilla Date: Wed, 7 Apr 2021 09:55:28 +0200 Subject: [PATCH] Webhook validates deletion This adds Delete action to validating webhook. This is necessary for when there's a rollout block and a user is not aware. In this case, the user might delete the contender, but shipper will not reactivate the incumbent. Shipper will also populate override annotations from releases to owning applications, to prevent a case where an overriding release is deleted, but the application is not overriding the block so Shipper will not reactivated incumbent release. --- cmd/shipperctl/configurator/cluster.go | 1 + cmd/shipperctl/configurator/cluster_test.go | 2 + pkg/controller/release/release_controller.go | 32 ++++++- .../release/release_controller_test.go | 7 +- pkg/util/rolloutblock/blocks.go | 8 ++ pkg/util/rolloutblock/object_name_list.go | 6 ++ pkg/webhook/webhook.go | 72 +++++++++++--- test/e2e/e2e_test.go | 96 +++++++++++++++++++ 8 files changed, 207 insertions(+), 17 deletions(-) diff --git a/cmd/shipperctl/configurator/cluster.go b/cmd/shipperctl/configurator/cluster.go index d50ae7e08..4625827c8 100644 --- a/cmd/shipperctl/configurator/cluster.go +++ b/cmd/shipperctl/configurator/cluster.go @@ -428,6 +428,7 @@ func (c *Cluster) CreateOrUpdateValidatingWebhookConfiguration(caBundle []byte, Operations: []admissionregistrationv1beta1.OperationType{ admissionregistrationv1beta1.Create, admissionregistrationv1beta1.Update, + admissionregistrationv1beta1.Delete, }, Rule: admissionregistrationv1beta1.Rule{ APIGroups: []string{shipper.SchemeGroupVersion.Group}, diff --git a/cmd/shipperctl/configurator/cluster_test.go b/cmd/shipperctl/configurator/cluster_test.go index 0c5a0c34e..84f9c6d70 100644 --- a/cmd/shipperctl/configurator/cluster_test.go +++ b/cmd/shipperctl/configurator/cluster_test.go @@ -28,6 +28,7 @@ func TestCreateValidatingWebhookConfiguration(t *testing.T) { operations := []admissionregistrationv1beta1.OperationType{ admissionregistrationv1beta1.Create, admissionregistrationv1beta1.Update, + admissionregistrationv1beta1.Delete, } expectedConfiguration := f.newValidatingWebhookConfiguration(caBundle, shipperSystemNamespace, operations) gvr := admissionregistrationv1beta1.SchemeGroupVersion.WithResource("validatingwebhookconfigurations") @@ -65,6 +66,7 @@ func TestUpdateValidatingWebhookConfiguration(t *testing.T) { operations = []admissionregistrationv1beta1.OperationType{ admissionregistrationv1beta1.Create, admissionregistrationv1beta1.Update, + admissionregistrationv1beta1.Delete, } expectedConfiguration := f.newValidatingWebhookConfiguration(caBundle, shipperSystemNamespace, operations) getAction := kubetesting.NewGetAction(gvr, "", expectedConfiguration.Name) diff --git a/pkg/controller/release/release_controller.go b/pkg/controller/release/release_controller.go index 90edb1f19..3a501d65f 100644 --- a/pkg/controller/release/release_controller.go +++ b/pkg/controller/release/release_controller.go @@ -1,7 +1,9 @@ package release import ( + "encoding/json" "fmt" + "strings" "time" "k8s.io/apimachinery/pkg/api/equality" @@ -301,6 +303,10 @@ func (c *Controller) syncOneReleaseHandler(key string) error { c.recorder, ) + if err := c.updateApplicationOverrides(rel); err != nil { + klog.Errorf("failed to update application overrides: %v", err) + } + rolloutBlocked, events, err := rolloutblock.BlocksRollout(c.rolloutBlockLister, rel) for _, ev := range events { c.recorder.Event(rel, ev.Type, ev.Reason, ev.Message) @@ -388,6 +394,31 @@ ApplyChanges: return err } +func (c *Controller) updateApplicationOverrides(rel *shipper.Release) error { + namespace := rel.GetNamespace() + // update application with same override annotations as release + appName, err := releaseutil.ApplicationNameForRelease(rel) + if err != nil { + return err + } + application, err := c.applicationLister.Applications(namespace).Get(appName) + if err != nil { + return err + } + applicationOverrides := application.GetAnnotations()[shipper.RolloutBlocksOverrideAnnotation] + newOverrides := rolloutblock.ApplicationOverrides(applicationOverrides, rel.GetAnnotations()[shipper.RolloutBlocksOverrideAnnotation]) + if strings.EqualFold(applicationOverrides, newOverrides) { + return nil + } + patch := make(map[string]interface{}) + patch["annotations"] = newOverrides + b, _ := json.Marshal(patch) + if _, err = c.clientset.ShipperV1alpha1().Applications(namespace).Patch(appName, types.MergePatchType, b); err != nil { + return err + } + return nil +} + func (c *Controller) applicationReleases(rel *shipper.Release) ([]*shipper.Release, error) { appName, err := releaseutil.ApplicationNameForRelease(rel) if err != nil { @@ -444,7 +475,6 @@ func updateConditions(rel *shipper.Release, diff *diffutil.MultiDiff, targetStep isLastStep := int(targetStep) == len(strategy.Steps)-1 prevStep := rel.Status.AchievedStep - var achievedStep int32 if complete { var achievedStepName string diff --git a/pkg/controller/release/release_controller_test.go b/pkg/controller/release/release_controller_test.go index 0f63281ae..3178896c8 100644 --- a/pkg/controller/release/release_controller_test.go +++ b/pkg/controller/release/release_controller_test.go @@ -249,9 +249,10 @@ func newRolloutBlock(name string, namespace string) *shipper.RolloutBlock { func buildApplication(namespace string, appName string) *shipper.Application { return &shipper.Application{ ObjectMeta: metav1.ObjectMeta{ - Name: appName, - Namespace: namespace, - UID: "foobarbaz", + Name: appName, + Namespace: namespace, + UID: "foobarbaz", + Annotations: make(map[string]string), }, Status: shipper.ApplicationStatus{ History: []string{}, diff --git a/pkg/util/rolloutblock/blocks.go b/pkg/util/rolloutblock/blocks.go index 70cae1c13..8858d728f 100644 --- a/pkg/util/rolloutblock/blocks.go +++ b/pkg/util/rolloutblock/blocks.go @@ -90,3 +90,11 @@ func ValidateAnnotations(existing, overrides ObjectNameList) error { } return nil } + +func ApplicationOverrides(applicationOverrides, releaseOverrides string) string { + appOverrides := NewObjectNameList(applicationOverrides) + relOverrides := NewObjectNameList(releaseOverrides) + diff := relOverrides.Diff(appOverrides) + appOverrides.AddMultiple(diff) + return appOverrides.String() +} diff --git a/pkg/util/rolloutblock/object_name_list.go b/pkg/util/rolloutblock/object_name_list.go index ebe3e61ed..e5e07f9be 100644 --- a/pkg/util/rolloutblock/object_name_list.go +++ b/pkg/util/rolloutblock/object_name_list.go @@ -61,6 +61,12 @@ func (o ObjectNameList) Add(statement string) { o[statement] = struct{}{} } +func (o ObjectNameList) AddMultiple(o2 ObjectNameList) { + for s := range o2 { + o.Add(s) + } +} + func (o ObjectNameList) Diff(o2 ObjectNameList) ObjectNameList { res := make(ObjectNameList) for s := range o { diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index a23089c12..db16def13 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -220,18 +220,39 @@ func (c *Webhook) validateHandlerFunc(review *admission.AdmissionReview) *admiss request := review.Request var err error + if request.Operation == kubeclient.Delete { + err = c.validateDelete(request) + } else { + err = c.validateCreateUpdate(request) + } + + if err != nil { + return &admission.AdmissionResponse{ + Result: &metav1.Status{ + Message: err.Error(), + }, + } + } + + return &admission.AdmissionResponse{ + Allowed: true, + } +} + +func (c *Webhook) validateCreateUpdate(request *kubeclient.AdmissionRequest) error { + var err error switch request.Kind.Kind { case "Application": var application shipper.Application err = json.Unmarshal(request.Object.Raw, &application) if err == nil { - err = c.validateApplication(request, application) + err = c.validateBlocksForApplication(request, application) } case "Release": var release shipper.Release err = json.Unmarshal(request.Object.Raw, &release) if err == nil { - err = c.validateRelease(request, release) + err = c.validateBlocksForRelease(request, release) } case "Cluster": var cluster shipper.Cluster @@ -249,21 +270,31 @@ func (c *Webhook) validateHandlerFunc(review *admission.AdmissionReview) *admiss var rolloutBlock shipper.RolloutBlock err = json.Unmarshal(request.Object.Raw, &rolloutBlock) } + return err +} - if err != nil { - return &admission.AdmissionResponse{ - Result: &metav1.Status{ - Message: err.Error(), - }, - } +func (c *Webhook) validateDelete(request *kubeclient.AdmissionRequest) error { + var err error + var obj metav1.Object + switch request.Kind.Kind { + case "Application": + var application shipper.Application + err = json.Unmarshal(request.OldObject.Raw, &application) + obj = &application + case "Release": + var release shipper.Release + err = json.Unmarshal(request.OldObject.Raw, &release) + obj = &release + default: + return nil } - - return &admission.AdmissionResponse{ - Allowed: true, + if err != nil { + return err } + return c.validateBlocksForObject(obj) } -func (c *Webhook) validateRelease(request *admission.AdmissionRequest, release shipper.Release) error { +func (c *Webhook) validateBlocksForRelease(request *admission.AdmissionRequest, release shipper.Release) error { var err error overrides, existingBlocks, err := rolloutblock.GetAllBlocks(c.rolloutBlocksLister, &release) if err != nil { @@ -296,7 +327,7 @@ func (c *Webhook) validateRelease(request *admission.AdmissionRequest, release s return err } -func (c *Webhook) validateApplication(request *admission.AdmissionRequest, application shipper.Application) error { +func (c *Webhook) validateBlocksForApplication(request *admission.AdmissionRequest, application shipper.Application) error { var err error overrides, existingBlocks, err := rolloutblock.GetAllBlocks(c.rolloutBlocksLister, &application) if err != nil { @@ -322,3 +353,18 @@ func (c *Webhook) validateApplication(request *admission.AdmissionRequest, appli return err } + +func (c *Webhook) validateBlocksForObject(obj metav1.Object) error { + overrides, existingBlocks, err := rolloutblock.GetAllBlocks(c.rolloutBlocksLister, obj) + if err != nil { + return err + } + if err := rolloutblock.ValidateAnnotations(existingBlocks, overrides); err != nil { + return err + } + if err := rolloutblock.ValidateBlocks(existingBlocks, overrides); err != nil { + return err + } + + return nil +} diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index fe6a08a5c..a62dfcca5 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -198,6 +198,102 @@ func TestRejectModificationToReleaseEnv(t *testing.T) { t.Logf("successfully failed to update release environment %q", relName) } +func TestRejectDeleteReleaseRolloutBlock(t *testing.T) { + if !*runEndToEnd { + t.Skip("skipping end-to-end tests: --e2e is false") + } + t.Parallel() + + targetReplicas := 4 + ns, err := setupNamespace(t.Name()) + if err != nil { + t.Fatalf("could not create namespace %s: %q", t.Name(), err) + } + f := newFixture(ns.GetName(), t) + defer func() { + if *inspectFailed && t.Failed() { + return + } + teardownNamespace(ns.GetName()) + }() + + newApp := newApplication(ns.GetName(), appName, &allIn) + newApp.Spec.Template.Values = &shipper.ChartValues{"replicaCount": targetReplicas} + newApp.Spec.Template.Chart.Name = "test-nginx" + newApp.Spec.Template.Chart.Version = "0.0.1" + + _, err = shipperClient.ShipperV1alpha1().Applications(ns.GetName()).Create(newApp) + if err != nil { + t.Fatalf("could not create application %q: %q", appName, err) + } + + t.Logf("waiting for a new release for new application %q", appName) + rel := f.waitForRelease(appName, 0) + relName := rel.GetName() + t.Logf("waiting for release %q to complete", relName) + f.waitForComplete(rel.GetName()) + t.Logf("checking that release %q has %d pods (strategy step 0 -- finished)", relName, targetReplicas) + f.checkReadyPods(relName, targetReplicas) + + _, err = createRolloutBlock(ns.GetName(), rolloutBlockName) + if err != nil { + t.Fatalf("could not create rollout block %q: %q", rolloutBlockName, err) + } + + if err = shipperClient.ShipperV1alpha1().Releases(ns.GetName()).Delete(relName, &metav1.DeleteOptions{}); err == nil { + t.Fatalf("deleted release %q: %q", relName, err) + } + t.Logf("successfully failed to delete release %q", relName) +} + +func TestRejectDeleteApplicationRolloutBlock(t *testing.T) { + if !*runEndToEnd { + t.Skip("skipping end-to-end tests: --e2e is false") + } + t.Parallel() + + targetReplicas := 4 + ns, err := setupNamespace(t.Name()) + if err != nil { + t.Fatalf("could not create namespace %s: %q", t.Name(), err) + } + f := newFixture(ns.GetName(), t) + defer func() { + if *inspectFailed && t.Failed() { + return + } + teardownNamespace(ns.GetName()) + }() + + newApp := newApplication(ns.GetName(), appName, &allIn) + newApp.Spec.Template.Values = &shipper.ChartValues{"replicaCount": targetReplicas} + newApp.Spec.Template.Chart.Name = "test-nginx" + newApp.Spec.Template.Chart.Version = "0.0.1" + + _, err = shipperClient.ShipperV1alpha1().Applications(ns.GetName()).Create(newApp) + if err != nil { + t.Fatalf("could not create application %q: %q", appName, err) + } + + t.Logf("waiting for a new release for new application %q", appName) + rel := f.waitForRelease(appName, 0) + relName := rel.GetName() + t.Logf("waiting for release %q to complete", relName) + f.waitForComplete(rel.GetName()) + t.Logf("checking that release %q has %d pods (strategy step 0 -- finished)", relName, targetReplicas) + f.checkReadyPods(relName, targetReplicas) + + _, err = createRolloutBlock(ns.GetName(), rolloutBlockName) + if err != nil { + t.Fatalf("could not create rollout block %q: %q", rolloutBlockName, err) + } + + if err = shipperClient.ShipperV1alpha1().Applications(ns.GetName()).Delete(appName, &metav1.DeleteOptions{}); err == nil { + t.Fatalf("deleted application %q: %q", appName, err) + } + t.Logf("successfully failed to delete application %q", relName) +} + func TestNewAppAllInWithRolloutBlockOverride(t *testing.T) { if !*runEndToEnd { t.Skip("skipping end-to-end tests: --e2e is false")