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

@@ -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},

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)

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

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{},

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()
+}

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 {

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
+}

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")