Skip to content

Commit 470403c

Browse files
awgreeneawgreene
committed
(fix) Admission Webhook names must be unique
Problem: OLM currently creates all ValidatingWebhookConfigurations and MutatingWebhookConfigurations using the name provided in the WebhookDescription. This causes an issue when an operator that defines an admission webhook in the CSV is installed in multiple namespaces, causes the two operators to fight over the admission webhook. Solution: Generate a unique name for each webhook using the GenerateName field in the Object Metadata.
1 parent 280a2a6 commit 470403c

File tree

5 files changed

+728
-892
lines changed

5 files changed

+728
-892
lines changed

‎pkg/controller/install/webhook.go

Lines changed: 77 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99

1010
log "github.com/sirupsen/logrus"
1111
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
12-
k8serrors "k8s.io/apimachinery/pkg/api/errors"
1312
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1413
"k8s.io/apimachinery/pkg/labels"
1514
)
@@ -69,80 +68,108 @@ func (i *StrategyDeploymentInstaller) createOrUpdateWebhook(caPEM []byte, desc v
6968
}
7069

7170
func (i *StrategyDeploymentInstaller) createOrUpdateMutatingWebhook(ogNamespacelabelSelector *metav1.LabelSelector, caPEM []byte, desc v1alpha1.WebhookDescription) error {
72-
webhooks := []admissionregistrationv1.MutatingWebhook{
73-
desc.GetMutatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
74-
}
75-
existingHook, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Get(context.TODO(), desc.Name, metav1.GetOptions{})
76-
if err == nil {
77-
// Check if the only owners are this CSV or in this CSV's replacement chain
78-
if ownerutil.Adoptable(i.owner, existingHook.GetOwnerReferences()) {
79-
ownerutil.AddNonBlockingOwner(existingHook, i.owner)
80-
}
71+
webhookLabels := ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind)
72+
webhookLabels[WebhookDescKey] = desc.Name
73+
webhookSelector := labels.SelectorFromSet(webhookLabels).String()
8174

82-
// Update the list of webhooks
83-
existingHook.Webhooks = webhooks
75+
existingWebhooks, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
76+
if err != nil {
77+
return err
78+
}
8479

85-
// Attempt an update
86-
if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Update(context.TODO(), existingHook, metav1.UpdateOptions{}); err != nil {
87-
log.Warnf("could not update MutatingWebhookConfiguration %s", existingHook.GetName())
88-
return err
89-
}
90-
} else if k8serrors.IsNotFound(err) {
80+
if len(existingWebhooks.Items) == 0 {
81+
// Create a ValidatingWebhookConfiguration
9182
hook := admissionregistrationv1.MutatingWebhookConfiguration{
92-
ObjectMeta: metav1.ObjectMeta{Name: desc.Name,
93-
Namespace: i.owner.GetNamespace(),
83+
ObjectMeta: metav1.ObjectMeta{
84+
GenerateName: desc.Name + "-",
85+
Namespace: i.owner.GetNamespace(),
86+
Labels: ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind),
87+
},
88+
Webhooks: []admissionregistrationv1.MutatingWebhook{
89+
desc.GetMutatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
9490
},
95-
Webhooks: webhooks,
9691
}
97-
// Add an owner
98-
ownerutil.AddNonBlockingOwner(&hook, i.owner)
92+
addWebhookLabels(&hook, desc)
93+
9994
if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Create(context.TODO(), &hook, metav1.CreateOptions{}); err != nil {
100-
log.Errorf("Webhooks: Error creating mutating MutatingVebhookConfiguration: %v", err)
95+
log.Errorf("Webhooks: Error creating ValidationWebhookConfiguration: %v", err)
10196
return err
10297
}
10398
} else {
104-
return err
105-
}
99+
for _, webhook := range existingWebhooks.Items {
100+
// Update the list of webhooks
101+
webhook.Webhooks = []admissionregistrationv1.MutatingWebhook{
102+
desc.GetMutatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
103+
}
104+
105+
// Attempt an update
106+
if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Update(context.TODO(), &webhook, metav1.UpdateOptions{}); err != nil {
107+
log.Warnf("could not update MutatingWebhookConfiguration %s", webhook.GetName())
108+
return err
109+
}
106110

111+
}
112+
}
107113
return nil
108114
}
109115

110116
func (i *StrategyDeploymentInstaller) createOrUpdateValidatingWebhook(ogNamespacelabelSelector *metav1.LabelSelector, caPEM []byte, desc v1alpha1.WebhookDescription) error {
111-
webhooks := []admissionregistrationv1.ValidatingWebhook{
112-
desc.GetValidatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
113-
}
114-
existingHook, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(context.TODO(), desc.Name, metav1.GetOptions{})
115-
if err == nil {
116-
// Check if the only owners are this CSV or in this CSV's replacement chain
117-
if ownerutil.Adoptable(i.owner, existingHook.GetOwnerReferences()) {
118-
ownerutil.AddNonBlockingOwner(existingHook, i.owner)
119-
}
117+
webhookLabels := ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind)
118+
webhookLabels[WebhookDescKey] = desc.Name
119+
webhookSelector := labels.SelectorFromSet(webhookLabels).String()
120120

121-
// Update the list of webhooks
122-
existingHook.Webhooks = webhooks
121+
existingWebhooks, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
122+
if err != nil {
123+
return err
124+
}
123125

124-
// Attempt an update
125-
if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Update(context.TODO(), existingHook, metav1.UpdateOptions{}); err != nil {
126-
log.Warnf("could not update ValidatingWebhookConfiguration %s", existingHook.GetName())
127-
return err
128-
}
129-
} else if k8serrors.IsNotFound(err) {
126+
if len(existingWebhooks.Items) == 0 {
130127
// Create a ValidatingWebhookConfiguration
131128
hook := admissionregistrationv1.ValidatingWebhookConfiguration{
132-
ObjectMeta: metav1.ObjectMeta{Name: desc.Name,
133-
Namespace: i.owner.GetNamespace(),
129+
ObjectMeta: metav1.ObjectMeta{
130+
GenerateName: desc.Name + "-",
131+
Namespace: i.owner.GetNamespace(),
132+
Labels: ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind),
133+
},
134+
Webhooks: []admissionregistrationv1.ValidatingWebhook{
135+
desc.GetValidatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
134136
},
135-
Webhooks: webhooks,
136137
}
138+
addWebhookLabels(&hook, desc)
137139

138-
// Add an owner
139-
ownerutil.AddNonBlockingOwner(&hook, i.owner)
140140
if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Create(context.TODO(), &hook, metav1.CreateOptions{}); err != nil {
141-
log.Errorf("Webhooks: Error create creating ValidationVebhookConfiguration: %v", err)
141+
log.Errorf("Webhooks: Error creating ValidatingWebhookConfiguration: %v", err)
142142
return err
143143
}
144144
} else {
145-
return err
145+
for _, webhook := range existingWebhooks.Items {
146+
147+
// Update the list of webhooks
148+
webhook.Webhooks = []admissionregistrationv1.ValidatingWebhook{
149+
desc.GetValidatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
150+
}
151+
152+
// Attempt an update
153+
if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Update(context.TODO(), &webhook, metav1.UpdateOptions{}); err != nil {
154+
log.Warnf("could not update ValidatingWebhookConfiguration %s", webhook.GetName())
155+
return err
156+
}
157+
158+
}
146159
}
147160
return nil
148161
}
162+
163+
const WebhookDescKey = "webhookDescriptionName"
164+
165+
// addWebhookLabels adds webhook labels to an object
166+
func addWebhookLabels(object metav1.Object, webhookDesc v1alpha1.WebhookDescription) error {
167+
labels := object.GetLabels()
168+
if labels == nil {
169+
labels = map[string]string{}
170+
}
171+
labels[WebhookDescKey] = webhookDesc.Name
172+
object.SetLabels(labels)
173+
174+
return nil
175+
}

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

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,22 @@ package olm
33
import (
44
"context"
55
"fmt"
6+
"strings"
67

78
log "github.com/sirupsen/logrus"
89
appsv1 "k8s.io/api/apps/v1"
910
corev1 "k8s.io/api/core/v1"
1011
rbacv1 "k8s.io/api/rbac/v1"
1112
k8serrors "k8s.io/apimachinery/pkg/api/errors"
1213
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
14+
"k8s.io/apimachinery/pkg/labels"
1315
utilerrors "k8s.io/apimachinery/pkg/util/errors"
1416

1517
"github.com/operator-framework/api/pkg/operators/v1alpha1"
1618
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/certs"
1719
olmerrors "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/errors"
1820
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
21+
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
1922
)
2023

2124
const (
@@ -271,22 +274,33 @@ func (a *Operator) getCaBundle(csv *v1alpha1.ClusterServiceVersion) ([]byte, err
271274
}
272275

273276
for _, desc := range csv.Spec.WebhookDefinitions {
274-
webhookName := desc.Name
275-
if desc.Type == "ValidatingAdmissionWebhook" {
276-
webhook, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(context.TODO(), webhookName, metav1.GetOptions{})
277+
if desc.Type == v1alpha1.MutatingAdmissionWebhook {
278+
ownerLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
279+
ownerLabelStrings := labels.SelectorFromSet(ownerLabels).String()
280+
281+
existingHooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: ownerLabelStrings})
277282
if err != nil {
278-
return nil, fmt.Errorf("could not retrieve generated APIService: %v", err)
283+
return nil, fmt.Errorf("could not retrieve generated MutatingWebhookConfiguration: %v", err)
279284
}
280-
if len(webhook.Webhooks[0].ClientConfig.CABundle) > 0 {
281-
return webhook.Webhooks[0].ClientConfig.CABundle, nil
285+
286+
for _, item := range existingHooks.Items {
287+
if strings.HasPrefix(item.Name, desc.Name) {
288+
return item.Webhooks[0].ClientConfig.CABundle, nil
289+
}
282290
}
283-
} else {
284-
webhook, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Get(context.TODO(), webhookName, metav1.GetOptions{})
291+
} else if desc.Type == v1alpha1.ValidatingAdmissionWebhook {
292+
ownerLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
293+
ownerLabelStrings := labels.SelectorFromSet(ownerLabels).String()
294+
295+
existingHooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: ownerLabelStrings})
285296
if err != nil {
286-
return nil, fmt.Errorf("could not retrieve generated APIService: %v", err)
297+
return nil, fmt.Errorf("could not retrieve generated ValidatingWebhookConfiguration: %v", err)
287298
}
288-
if len(webhook.Webhooks[0].ClientConfig.CABundle) > 0 {
289-
return webhook.Webhooks[0].ClientConfig.CABundle, nil
299+
300+
for _, item := range existingHooks.Items {
301+
if strings.HasPrefix(item.Name, desc.Name) {
302+
return item.Webhooks[0].ClientConfig.CABundle, nil
303+
}
290304
}
291305
}
292306
}

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

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubestate"
1212
"github.com/sirupsen/logrus"
1313
log "github.com/sirupsen/logrus"
14+
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
1415
corev1 "k8s.io/api/core/v1"
1516
rbacv1 "k8s.io/api/rbac/v1"
1617
extinf "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions"
@@ -632,6 +633,44 @@ func (a *Operator) syncGCObject(obj interface{}) (syncError error) {
632633
break
633634
}
634635
logger.Debugf("Deleted cluster role binding %v due to no owning CSV", metaObj.GetName())
636+
case *admissionregistrationv1.MutatingWebhookConfiguration:
637+
if name, ns, ok := ownerutil.GetOwnerByKindLabel(metaObj, v1alpha1.ClusterServiceVersionKind); ok {
638+
_, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(ns).Get(name)
639+
if err == nil {
640+
logger.Debugf("CSV still present, must wait until it is deleted (owners=%v)", name)
641+
syncError = fmt.Errorf("cleanup must wait")
642+
return
643+
} else if !k8serrors.IsNotFound(err) {
644+
logger.Infof("error CSV retrieval error")
645+
syncError = err
646+
return
647+
}
648+
}
649+
650+
if err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Delete(context.TODO(), metaObj.GetName(), metav1.DeleteOptions{}); err != nil {
651+
logger.WithError(err).Warn("cannot delete MutatingWebhookConfiguration")
652+
break
653+
}
654+
logger.Debugf("Deleted MutatingWebhookConfiguration %v due to no owning CSV", metaObj.GetName())
655+
case *admissionregistrationv1.ValidatingWebhookConfiguration:
656+
if name, ns, ok := ownerutil.GetOwnerByKindLabel(metaObj, v1alpha1.ClusterServiceVersionKind); ok {
657+
_, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(ns).Get(name)
658+
if err == nil {
659+
logger.Debugf("CSV still present, must wait until it is deleted (owners=%v)", name)
660+
syncError = fmt.Errorf("cleanup must wait")
661+
return
662+
} else if !k8serrors.IsNotFound(err) {
663+
logger.Infof("Error CSV retrieval error")
664+
syncError = err
665+
return
666+
}
667+
}
668+
669+
if err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Delete(context.TODO(), metaObj.GetName(), metav1.DeleteOptions{}); err != nil {
670+
logger.WithError(err).Warn("cannot delete ValidatingWebhookConfiguration")
671+
break
672+
}
673+
logger.Debugf("Deleted ValidatingWebhookConfiguration %v due to no owning CSV", metaObj.GetName())
635674
}
636675

637676
return
@@ -920,6 +959,25 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) {
920959
syncError := a.objGCQueueSet.RequeueEvent("", kubestate.NewResourceEvent(kubestate.ResourceUpdated, cr))
921960
logger.Debugf("handleCSVdeletion - requeued update event for %v, res=%v", cr, syncError)
922961
}
962+
963+
webhookSelector := labels.SelectorFromSet(ownerutil.OwnerLabel(clusterServiceVersion, v1alpha1.ClusterServiceVersionKind)).String()
964+
mWebhooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
965+
if err != nil {
966+
logger.WithError(err).Warn("cannot list MutatingWebhookConfigurations")
967+
}
968+
for _, webhook := range mWebhooks.Items {
969+
syncError := a.objGCQueueSet.RequeueEvent("", kubestate.NewResourceEvent(kubestate.ResourceUpdated, &webhook))
970+
logger.Debugf("handleCSVdeletion - requeued update event for %v, res=%v", webhook, syncError)
971+
}
972+
973+
vWebhooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
974+
if err != nil {
975+
logger.WithError(err).Warn("cannot list MutatingWebhookConfigurations")
976+
}
977+
for _, webhook := range vWebhooks.Items {
978+
syncError := a.objGCQueueSet.RequeueEvent("", kubestate.NewResourceEvent(kubestate.ResourceUpdated, &webhook))
979+
logger.Debugf("handleCSVdeletion - requeued update event for %v, res=%v", webhook, syncError)
980+
}
923981
}
924982

925983
func (a *Operator) removeDanglingChildCSVs(csv *v1alpha1.ClusterServiceVersion) error {
@@ -1381,8 +1439,16 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
13811439
return
13821440
}
13831441

1384-
// Check if Webhooks have valid rules
1442+
// Create a map to track unique names
1443+
unqiueWebhookNames := map[string]struct{}{}
1444+
// Check if Webhooks have valid rules and unique names
13851445
for _, desc := range out.Spec.WebhookDefinitions {
1446+
_, present := unqiueWebhookNames[desc.Name]
1447+
if present {
1448+
out.SetPhaseWithEventIfChanged(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonUnsupportedWebhookRules, "CSV contains repeated WebhookDescription name", now, a.recorder)
1449+
return
1450+
}
1451+
unqiueWebhookNames[desc.Name] = struct{}{}
13861452
if syncError = install.ValidWebhookRules(desc.Rules); syncError != nil {
13871453
out.SetPhaseWithEventIfChanged(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonUnsupportedWebhookRules, syncError.Error(), now, a.recorder)
13881454
return

0 commit comments

Comments
 (0)