Detect WebhookDescription changes in CSVs

Problem:
Once a CSV has reached the Succeeded phase, updating
WebhookDescriptions in the CSV on cluster does not cause OLM to update
existing ValidatingWebhookConfigurations or
MutatingWebhookConfigurations on cluster.

Solution:
Update OLM to calculate if a change to the WebhookDescription has
occurred when a CSV is updated and if so update the on cluster
resources.

Author: awgreene

pkg/controller/install/webhook.go

@@ -3,14 +3,17 @@ package install
 import (
 	"context"
 	"fmt"
+	"hash/fnv"
 
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
+	hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 
 	log "github.com/sirupsen/logrus"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/util/rand"
 )
 
 func ValidWebhookRules(rules []admissionregistrationv1.RuleWithOperations) error {
@@ -105,6 +108,7 @@ func (i *StrategyDeploymentInstaller) createOrUpdateMutatingWebhook(ogNamespacel
 		webhook.Webhooks = []admissionregistrationv1.MutatingWebhook{
 			desc.GetMutatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
 		}
+		addWebhookLabels(&webhook, desc)
 
 		// Attempt an update
 		if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Update(context.TODO(), &webhook, metav1.UpdateOptions{}); err != nil {
@@ -151,6 +155,7 @@ func (i *StrategyDeploymentInstaller) createOrUpdateValidatingWebhook(ogNamespac
 		webhook.Webhooks = []admissionregistrationv1.ValidatingWebhook{
 			desc.GetValidatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
 		}
+		addWebhookLabels(&webhook, desc)
 
 		// Attempt an update
 		if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Update(context.TODO(), &webhook, metav1.UpdateOptions{}); err != nil {
@@ -163,6 +168,7 @@ func (i *StrategyDeploymentInstaller) createOrUpdateValidatingWebhook(ogNamespac
 }
 
 const WebhookDescKey = "webhookDescriptionGenerateName"
+const WebhookHashKey = "webhookDescriptionSHA"
 
 // addWebhookLabels adds webhook labels to an object
 func addWebhookLabels(object metav1.Object, webhookDesc v1alpha1.WebhookDescription) error {
@@ -171,7 +177,15 @@ func addWebhookLabels(object metav1.Object, webhookDesc v1alpha1.WebhookDescript
 		labels = map[string]string{}
 	}
 	labels[WebhookDescKey] = webhookDesc.GenerateName
+	labels[WebhookHashKey] = HashWebhookDesc(webhookDesc)
 	object.SetLabels(labels)
 
 	return nil
 }
+
+// HashWebhookDesc calculates a hash given a webhookDescription
+func HashWebhookDesc(webhookDesc v1alpha1.WebhookDescription) string {
+	hasher := fnv.New32a()
+	hashutil.DeepHashObject(hasher, &webhookDesc)
+	return rand.SafeEncodeString(fmt.Sprint(hasher.Sum32()))
+}

pkg/controller/operators/olm/apiservices.go

@@ -486,3 +486,34 @@ func (a *Operator) updateDeploymentSpecsWithApiServiceData(csv *v1alpha1.Cluster
 	}
 	return strategyDetailsDeployment, nil
 }
+
+func (a *Operator) areWebhooksAvailable(csv *v1alpha1.ClusterServiceVersion) (bool, error) {
+	for _, desc := range csv.Spec.WebhookDefinitions {
+		// Create Webhook Label Selector
+		webhookLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
+		webhookLabels[install.WebhookDescKey] = desc.GenerateName
+		webhookLabels[install.WebhookHashKey] = install.HashWebhookDesc(desc)
+		webhookSelector := labels.SelectorFromSet(webhookLabels).String()
+
+		webhookCount := 0
+		switch desc.Type {
+		case v1alpha1.ValidatingAdmissionWebhook:
+			webhookList, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
+			if err != nil {
+				return false, err
+			}
+			webhookCount = len(webhookList.Items)
+		case v1alpha1.MutatingAdmissionWebhook:
+			webhookList, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
+			if err != nil {
+				return false, err
+			}
+			webhookCount = len(webhookList.Items)
+		}
+		if webhookCount == 0 {
+			a.logger.Info("Expected Webhook does not exist")
+			return false, nil
+		}
+	}
+	return true, nil
+}

pkg/controller/operators/olm/operator.go

@@ -1721,13 +1721,14 @@ func (a *Operator) checkReplacementsAndUpdateStatus(csv *v1alpha1.ClusterService
 func (a *Operator) updateInstallStatus(csv *v1alpha1.ClusterServiceVersion, installer install.StrategyInstaller, strategy install.Strategy, requeuePhase v1alpha1.ClusterServiceVersionPhase, requeueConditionReason v1alpha1.ConditionReason) error {
 	apiServicesInstalled, apiServiceErr := a.areAPIServicesAvailable(csv)
 	strategyInstalled, strategyErr := installer.CheckInstalled(strategy)
+	webhooksInstalled, webhookErr := a.areWebhooksAvailable(csv)
 	now := a.now()
 
 	if strategyErr != nil {
 		a.logger.WithError(strategyErr).Debug("operator not installed")
 	}
 
-	if strategyInstalled && apiServicesInstalled {
+	if strategyInstalled && apiServicesInstalled && webhooksInstalled {
 		// if there's no error, we're successfully running
 		csv.SetPhaseWithEventIfChanged(v1alpha1.CSVPhaseSucceeded, v1alpha1.CSVReasonInstallSuccessful, "install strategy completed with no errors", now, a.recorder)
 		return nil
@@ -1753,6 +1754,16 @@ func (a *Operator) updateInstallStatus(csv *v1alpha1.ClusterServiceVersion, inst
 		return fmt.Errorf("APIServices not installed")
 	}
 
+	if !webhooksInstalled || webhookErr != nil {
+		msg := "Webhooks not installed"
+		csv.SetPhaseWithEventIfChanged(requeuePhase, requeueConditionReason, fmt.Sprintf(msg), now, a.recorder)
+		if err := a.csvQueueSet.Requeue(csv.GetNamespace(), csv.GetName()); err != nil {
+			a.logger.Warn(err.Error())
+		}
+
+		return fmt.Errorf(msg)
+	}
+
 	if strategyErr != nil {
 		csv.SetPhaseWithEventIfChanged(requeuePhase, requeueConditionReason, fmt.Sprintf("installing: %s", strategyErr), now, a.recorder)
 		if err := a.csvQueueSet.Requeue(csv.GetNamespace(), csv.GetName()); err != nil {

test/e2e/webhook_e2e_test.go

@@ -119,13 +119,15 @@ var _ = Describe("CSVs with a Webhook", func() {
 		})
 		It("Creates Webhooks scoped to a single namespace", func() {
 			sideEffect := admissionregistrationv1.SideEffectClassNone
+			timeout := int32(30)
 			webhook := v1alpha1.WebhookDescription{
 				GenerateName:            webhookName,
 				Type:                    v1alpha1.ValidatingAdmissionWebhook,
 				DeploymentName:          genName("webhook-dep-"),
 				ContainerPort:           443,
 				AdmissionReviewVersions: []string{"v1beta1", "v1"},
 				SideEffects:             &sideEffect,
+				TimeoutSeconds:          &timeout,
 			}
 
 			csv := createCSVWithWebhook(namespace.GetName(), webhook)
@@ -147,6 +149,34 @@ var _ = Describe("CSVs with a Webhook", func() {
 				MatchExpressions: []metav1.LabelSelectorRequirement(nil),
 			}
 			Expect(actualWebhook.Webhooks[0].NamespaceSelector).Should(Equal(expected))
+
+			// Ensure that changes to the WebhookDescription within the CSV trigger an update to on cluster resources
+			newTimeout := int32(5)
+			Eventually(func() (bool, error) {
+				existingCSV, err := crc.OperatorsV1alpha1().ClusterServiceVersions(namespace.Name).Get(context.TODO(), csv.GetName(), metav1.GetOptions{})
+				if err != nil {
+					return false, err
+				}
+				existingCSV.Spec.WebhookDefinitions[0].TimeoutSeconds = &newTimeout
+
+				existingCSV, err = crc.OperatorsV1alpha1().ClusterServiceVersions(namespace.Name).Update(context.TODO(), existingCSV, metav1.UpdateOptions{})
+				if err != nil {
+					return false, nil
+				}
+				return true, nil
+			}, time.Minute, 5*time.Second).Should(BeTrue())
+			Eventually(func() bool {
+				actualWebhook, err = getWebhookWithGenName(c, webhook)
+				if err != nil {
+					return false
+				}
+
+				if *actualWebhook.Webhooks[0].TimeoutSeconds != newTimeout {
+					return false
+				}
+
+				return true
+			}, time.Minute, 5*time.Second).Should(BeTrue())
 		})
 		It("Fails to install a CSV if multiple Webhooks share the same name", func() {
 			sideEffect := admissionregistrationv1.SideEffectClassNone