Skip to content

Commit 438d44d

Browse files
awgreeneawgreene
committed
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.
1 parent fa4270b commit 438d44d

File tree

4 files changed

+150
-13
lines changed

4 files changed

+150
-13
lines changed

‎pkg/controller/install/webhook.go

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,17 @@ package install
33
import (
44
"context"
55
"fmt"
6+
"hash/fnv"
67

78
"github.com/operator-framework/api/pkg/operators/v1alpha1"
9+
hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash"
810
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
911

1012
log "github.com/sirupsen/logrus"
1113
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
1214
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1315
"k8s.io/apimachinery/pkg/labels"
16+
"k8s.io/apimachinery/pkg/util/rand"
1417
)
1518

1619
func ValidWebhookRules(rules []admissionregistrationv1.RuleWithOperations) error {
@@ -105,6 +108,7 @@ func (i *StrategyDeploymentInstaller) createOrUpdateMutatingWebhook(ogNamespacel
105108
webhook.Webhooks = []admissionregistrationv1.MutatingWebhook{
106109
desc.GetMutatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
107110
}
111+
addWebhookLabels(&webhook, desc)
108112

109113
// Attempt an update
110114
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
151155
webhook.Webhooks = []admissionregistrationv1.ValidatingWebhook{
152156
desc.GetValidatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
153157
}
158+
addWebhookLabels(&webhook, desc)
154159

155160
// Attempt an update
156161
if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Update(context.TODO(), &webhook, metav1.UpdateOptions{}); err != nil {
@@ -162,7 +167,8 @@ func (i *StrategyDeploymentInstaller) createOrUpdateValidatingWebhook(ogNamespac
162167
return nil
163168
}
164169

165-
const WebhookDescKey = "webhookDescriptionGenerateName"
170+
const WebhookDescKey = "olm.webhook-description-generate-name"
171+
const WebhookHashKey = "olm.webhook-description-hash"
166172

167173
// addWebhookLabels adds webhook labels to an object
168174
func addWebhookLabels(object metav1.Object, webhookDesc v1alpha1.WebhookDescription) error {
@@ -171,7 +177,15 @@ func addWebhookLabels(object metav1.Object, webhookDesc v1alpha1.WebhookDescript
171177
labels = map[string]string{}
172178
}
173179
labels[WebhookDescKey] = webhookDesc.GenerateName
180+
labels[WebhookHashKey] = HashWebhookDesc(webhookDesc)
174181
object.SetLabels(labels)
175182

176183
return nil
177184
}
185+
186+
// HashWebhookDesc calculates a hash given a webhookDescription
187+
func HashWebhookDesc(webhookDesc v1alpha1.WebhookDescription) string {
188+
hasher := fnv.New32a()
189+
hashutil.DeepHashObject(hasher, &webhookDesc)
190+
return rand.SafeEncodeString(fmt.Sprint(hasher.Sum32()))
191+
}

‎pkg/controller/operators/olm/apiservices.go

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -486,3 +486,87 @@ func (a *Operator) updateDeploymentSpecsWithApiServiceData(csv *v1alpha1.Cluster
486486
}
487487
return strategyDetailsDeployment, nil
488488
}
489+
490+
func (a *Operator) cleanUpRemovedWebhooks(csv *v1alpha1.ClusterServiceVersion) error {
491+
webhookLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
492+
webhookLabels = ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
493+
webhookSelector := labels.SelectorFromSet(webhookLabels).String()
494+
495+
csvWebhookGenerateNames := make(map[string]struct{}, len(csv.Spec.WebhookDefinitions))
496+
for _, webhook := range csv.Spec.WebhookDefinitions {
497+
csvWebhookGenerateNames[webhook.GenerateName] = struct{}{}
498+
}
499+
500+
// Delete Validating Webhooks who generateNames have changed
501+
validatingWebhookConfigurationList, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
502+
if err != nil {
503+
return err
504+
}
505+
for _, webhook := range validatingWebhookConfigurationList.Items {
506+
webhookGenerateNameLabel, ok := webhook.GetLabels()[install.WebhookDescKey]
507+
if !ok {
508+
return fmt.Errorf("ValidatingWebhookConfiguration %s does not have WebhookDesc key", webhook.Name)
509+
}
510+
if _, ok := csvWebhookGenerateNames[webhookGenerateNameLabel]; !ok {
511+
err = a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Delete(context.TODO(), webhook.Name, metav1.DeleteOptions{})
512+
if err != nil && k8serrors.IsNotFound(err) {
513+
return err
514+
}
515+
}
516+
}
517+
518+
// Delete Mutating Webhooks who generateNames have changed
519+
mutatingWebhookConfigurationList, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
520+
if err != nil {
521+
return err
522+
}
523+
for _, webhook := range mutatingWebhookConfigurationList.Items {
524+
webhookGenerateNameLabel, ok := webhook.GetLabels()[install.WebhookDescKey]
525+
if !ok {
526+
return fmt.Errorf("MutatingWebhookConfiguration %s does not have WebhookDesc key", webhook.Name)
527+
}
528+
if _, ok := csvWebhookGenerateNames[webhookGenerateNameLabel]; !ok {
529+
err = a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Delete(context.TODO(), webhook.Name, metav1.DeleteOptions{})
530+
if err != nil && k8serrors.IsNotFound(err) {
531+
return err
532+
}
533+
}
534+
}
535+
536+
return nil
537+
}
538+
539+
func (a *Operator) areWebhooksAvailable(csv *v1alpha1.ClusterServiceVersion) (bool, error) {
540+
err := a.cleanUpRemovedWebhooks(csv)
541+
if err != nil {
542+
return false, err
543+
}
544+
for _, desc := range csv.Spec.WebhookDefinitions {
545+
// Create Webhook Label Selector
546+
webhookLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
547+
webhookLabels[install.WebhookDescKey] = desc.GenerateName
548+
webhookLabels[install.WebhookHashKey] = install.HashWebhookDesc(desc)
549+
webhookSelector := labels.SelectorFromSet(webhookLabels).String()
550+
551+
webhookCount := 0
552+
switch desc.Type {
553+
case v1alpha1.ValidatingAdmissionWebhook:
554+
webhookList, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
555+
if err != nil {
556+
return false, err
557+
}
558+
webhookCount = len(webhookList.Items)
559+
case v1alpha1.MutatingAdmissionWebhook:
560+
webhookList, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
561+
if err != nil {
562+
return false, err
563+
}
564+
webhookCount = len(webhookList.Items)
565+
}
566+
if webhookCount == 0 {
567+
a.logger.Info("Expected Webhook does not exist")
568+
return false, nil
569+
}
570+
}
571+
return true, nil
572+
}

‎pkg/controller/operators/olm/operator.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1721,13 +1721,14 @@ func (a *Operator) checkReplacementsAndUpdateStatus(csv *v1alpha1.ClusterService
17211721
func (a *Operator) updateInstallStatus(csv *v1alpha1.ClusterServiceVersion, installer install.StrategyInstaller, strategy install.Strategy, requeuePhase v1alpha1.ClusterServiceVersionPhase, requeueConditionReason v1alpha1.ConditionReason) error {
17221722
apiServicesInstalled, apiServiceErr := a.areAPIServicesAvailable(csv)
17231723
strategyInstalled, strategyErr := installer.CheckInstalled(strategy)
1724+
webhooksInstalled, webhookErr := a.areWebhooksAvailable(csv)
17241725
now := a.now()
17251726

17261727
if strategyErr != nil {
17271728
a.logger.WithError(strategyErr).Debug("operator not installed")
17281729
}
17291730

1730-
if strategyInstalled && apiServicesInstalled {
1731+
if strategyInstalled && apiServicesInstalled && webhooksInstalled {
17311732
// if there's no error, we're successfully running
17321733
csv.SetPhaseWithEventIfChanged(v1alpha1.CSVPhaseSucceeded, v1alpha1.CSVReasonInstallSuccessful, "install strategy completed with no errors", now, a.recorder)
17331734
return nil
@@ -1753,6 +1754,16 @@ func (a *Operator) updateInstallStatus(csv *v1alpha1.ClusterServiceVersion, inst
17531754
return fmt.Errorf("APIServices not installed")
17541755
}
17551756

1757+
if !webhooksInstalled || webhookErr != nil {
1758+
msg := "Webhooks not installed"
1759+
csv.SetPhaseWithEventIfChanged(requeuePhase, requeueConditionReason, fmt.Sprintf(msg), now, a.recorder)
1760+
if err := a.csvQueueSet.Requeue(csv.GetNamespace(), csv.GetName()); err != nil {
1761+
a.logger.Warn(err.Error())
1762+
}
1763+
1764+
return fmt.Errorf(msg)
1765+
}
1766+
17561767
if strategyErr != nil {
17571768
csv.SetPhaseWithEventIfChanged(requeuePhase, requeueConditionReason, fmt.Sprintf("installing: %s", strategyErr), now, a.recorder)
17581769
if err := a.csvQueueSet.Requeue(csv.GetNamespace(), csv.GetName()); err != nil {

‎test/e2e/webhook_e2e_test.go

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ const (
2626
webhookName = "webhook.test.com"
2727
)
2828

29-
var _ = Describe("CSVs with a Webhook", func() {
29+
var _ = FDescribe("CSVs with a Webhook", func() {
3030
var c operatorclient.ClientInterface
3131
var crc versioned.Interface
3232
var namespace *corev1.Namespace
@@ -97,7 +97,7 @@ var _ = Describe("CSVs with a Webhook", func() {
9797
_, err = fetchCSV(GinkgoT(), crc, csv.Name, namespace.Name, csvSucceededChecker)
9898
Expect(err).Should(BeNil())
9999

100-
actualWebhook, err := getWebhookWithGenName(c, webhook)
100+
actualWebhook, err := getWebhookWithGenerateName(c, webhook.GenerateName)
101101
Expect(err).Should(BeNil())
102102

103103
Expect(actualWebhook.Webhooks[0].NamespaceSelector).Should(Equal(ogSelector))
@@ -136,7 +136,7 @@ var _ = Describe("CSVs with a Webhook", func() {
136136
_, err = fetchCSV(GinkgoT(), crc, csv.Name, namespace.Name, csvSucceededChecker)
137137
Expect(err).Should(BeNil())
138138

139-
actualWebhook, err := getWebhookWithGenName(c, webhook)
139+
actualWebhook, err := getWebhookWithGenerateName(c, webhook.GenerateName)
140140
Expect(err).Should(BeNil())
141141

142142
ogLabel, err := getOGLabelKey(og)
@@ -147,6 +147,34 @@ var _ = Describe("CSVs with a Webhook", func() {
147147
MatchExpressions: []metav1.LabelSelectorRequirement(nil),
148148
}
149149
Expect(actualWebhook.Webhooks[0].NamespaceSelector).Should(Equal(expected))
150+
151+
// Ensure that changes to the WebhookDescription within the CSV trigger an update to on cluster resources
152+
changedGenerateName := webhookName + "-changed"
153+
Eventually(func() error {
154+
existingCSV, err := crc.OperatorsV1alpha1().ClusterServiceVersions(namespace.Name).Get(context.TODO(), csv.GetName(), metav1.GetOptions{})
155+
if err != nil {
156+
return err
157+
}
158+
existingCSV.Spec.WebhookDefinitions[0].GenerateName = changedGenerateName
159+
160+
existingCSV, err = crc.OperatorsV1alpha1().ClusterServiceVersions(namespace.Name).Update(context.TODO(), existingCSV, metav1.UpdateOptions{})
161+
return err
162+
}, time.Minute, 5*time.Second).Should(Succeed())
163+
Eventually(func() bool {
164+
// Previous Webhook should be deleted
165+
_, err = getWebhookWithGenerateName(c, webhookName)
166+
if err != nil && err.Error() != "NotFound" {
167+
return false
168+
}
169+
170+
// Current Webhook should exist
171+
_, err = getWebhookWithGenerateName(c, changedGenerateName)
172+
if err != nil {
173+
return false
174+
}
175+
176+
return true
177+
}, time.Minute, 5*time.Second).Should(BeTrue())
150178
})
151179
It("Fails to install a CSV if multiple Webhooks share the same name", func() {
152180
sideEffect := admissionregistrationv1.SideEffectClassNone
@@ -325,7 +353,7 @@ var _ = Describe("CSVs with a Webhook", func() {
325353
_, err = fetchCSV(GinkgoT(), crc, csv.Name, namespace.Name, csvSucceededChecker)
326354
Expect(err).Should(BeNil())
327355

328-
_, err = getWebhookWithGenName(c, webhook)
356+
_, err = getWebhookWithGenerateName(c, webhook.GenerateName)
329357
Expect(err).Should(BeNil())
330358

331359
// Update the CSV so it it replaces the existing CSV
@@ -340,7 +368,7 @@ var _ = Describe("CSVs with a Webhook", func() {
340368
_, err = fetchCSV(GinkgoT(), crc, csv.GetName(), namespace.Name, csvSucceededChecker)
341369
Expect(err).Should(BeNil())
342370

343-
_, err = getWebhookWithGenName(c, webhook)
371+
_, err = getWebhookWithGenerateName(c, webhook.GenerateName)
344372
Expect(err).Should(BeNil())
345373

346374
// Make sure old resources are cleaned up.
@@ -373,7 +401,7 @@ var _ = Describe("CSVs with a Webhook", func() {
373401
fetchedCSV, err := fetchCSV(GinkgoT(), crc, csv.Name, namespace.Name, csvSucceededChecker)
374402
Expect(err).Should(BeNil())
375403

376-
actualWebhook, err := getWebhookWithGenName(c, webhook)
404+
actualWebhook, err := getWebhookWithGenerateName(c, webhook.GenerateName)
377405
Expect(err).Should(BeNil())
378406

379407
oldWebhookCABundle := actualWebhook.Webhooks[0].ClientConfig.CABundle
@@ -411,7 +439,7 @@ var _ = Describe("CSVs with a Webhook", func() {
411439
Expect(err).Should(BeNil())
412440

413441
// get new webhook
414-
actualWebhook, err = getWebhookWithGenName(c, webhook)
442+
actualWebhook, err = getWebhookWithGenerateName(c, webhook.GenerateName)
415443
Expect(err).Should(BeNil())
416444

417445
newWebhookCABundle := actualWebhook.Webhooks[0].ClientConfig.CABundle
@@ -449,7 +477,7 @@ var _ = Describe("CSVs with a Webhook", func() {
449477

450478
_, err = fetchCSV(GinkgoT(), crc, csv.Name, namespace.Name, csvSucceededChecker)
451479
Expect(err).Should(BeNil())
452-
actualWebhook, err := getWebhookWithGenName(c, webhook)
480+
actualWebhook, err := getWebhookWithGenerateName(c, webhook.GenerateName)
453481
Expect(err).Should(BeNil())
454482

455483
expected := &metav1.LabelSelector{
@@ -515,8 +543,8 @@ var _ = Describe("CSVs with a Webhook", func() {
515543
})
516544
})
517545

518-
func getWebhookWithGenName(c operatorclient.ClientInterface, desc v1alpha1.WebhookDescription) (*admissionregistrationv1.ValidatingWebhookConfiguration, error) {
519-
webhookSelector := labels.SelectorFromSet(map[string]string{install.WebhookDescKey: desc.GenerateName}).String()
546+
func getWebhookWithGenerateName(c operatorclient.ClientInterface, generateName string) (*admissionregistrationv1.ValidatingWebhookConfiguration, error) {
547+
webhookSelector := labels.SelectorFromSet(map[string]string{install.WebhookDescKey: generateName}).String()
520548
existingWebhooks, err := c.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
521549
if err != nil {
522550
return nil, err
@@ -525,7 +553,7 @@ func getWebhookWithGenName(c operatorclient.ClientInterface, desc v1alpha1.Webho
525553
if len(existingWebhooks.Items) > 0 {
526554
return &existingWebhooks.Items[0], nil
527555
}
528-
return nil, fmt.Errorf("Could not find Webhook")
556+
return nil, fmt.Errorf("NotFound")
529557
}
530558

531559
func createCSVWithWebhook(namespace string, webhookDesc v1alpha1.WebhookDescription) v1alpha1.ClusterServiceVersion {

0 commit comments

Comments
 (0)