Skip to content

Commit 5466275

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 5466275

File tree

5 files changed

+731
-901
lines changed

5 files changed

+731
-901
lines changed

‎pkg/controller/install/webhook.go

Lines changed: 81 additions & 54 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) {
91-
hook := admissionregistrationv1.MutatingWebhookConfiguration{
92-
ObjectMeta: metav1.ObjectMeta{Name: desc.Name,
93-
Namespace: i.owner.GetNamespace(),
80+
if len(existingWebhooks.Items) == 0 {
81+
// Create a MutatingWebhookConfiguration
82+
webhook := admissionregistrationv1.MutatingWebhookConfiguration{
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)
99-
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)
92+
addWebhookLabels(&webhook, desc)
93+
94+
if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Create(context.TODO(), &webhook, metav1.CreateOptions{}); err != nil {
95+
log.Errorf("Webhooks: Error creating MutatingWebhookConfiguration: %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+
}
106104

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+
}
110+
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
131-
hook := admissionregistrationv1.ValidatingWebhookConfiguration{
132-
ObjectMeta: metav1.ObjectMeta{Name: desc.Name,
133-
Namespace: i.owner.GetNamespace(),
128+
webhook := admissionregistrationv1.ValidatingWebhookConfiguration{
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(&webhook, desc)
137139

138-
// Add an owner
139-
ownerutil.AddNonBlockingOwner(&hook, i.owner)
140-
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)
140+
if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Create(context.TODO(), &webhook, metav1.CreateOptions{}); err != nil {
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+
}
159+
}
160+
return nil
161+
}
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{}
146170
}
171+
labels[WebhookDescKey] = webhookDesc.Name
172+
object.SetLabels(labels)
173+
147174
return nil
148175
}

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

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,14 @@ import (
1010
rbacv1 "k8s.io/api/rbac/v1"
1111
k8serrors "k8s.io/apimachinery/pkg/api/errors"
1212
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
13+
"k8s.io/apimachinery/pkg/labels"
1314
utilerrors "k8s.io/apimachinery/pkg/util/errors"
1415

1516
"github.com/operator-framework/api/pkg/operators/v1alpha1"
1617
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/certs"
1718
olmerrors "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/errors"
1819
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
20+
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
1921
)
2022

2123
const (
@@ -255,10 +257,8 @@ func (a *Operator) areAPIServicesAvailable(csv *v1alpha1.ClusterServiceVersion)
255257
return true, nil
256258
}
257259

258-
// updateDeploymentSpecsWithApiServiceData transforms an install strategy to include information about apiservices
259-
// it is used in generating hashes for deployment specs to know when something in the spec has changed,
260-
// but duplicates a lot of installAPIServiceRequirements and should be refactored.
261-
func (a *Operator) getCaBundle(csv *v1alpha1.ClusterServiceVersion) ([]byte, error) {
260+
// getCABundle returns the CA associated with a deployment
261+
func (a *Operator) getCABundle(csv *v1alpha1.ClusterServiceVersion) ([]byte, error) {
262262
for _, desc := range csv.GetOwnedAPIServiceDescriptions() {
263263
apiServiceName := desc.GetName()
264264
apiService, err := a.lister.APIRegistrationV1().APIServiceLister().Get(apiServiceName)
@@ -271,22 +271,27 @@ func (a *Operator) getCaBundle(csv *v1alpha1.ClusterServiceVersion) ([]byte, err
271271
}
272272

273273
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{})
274+
webhookLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
275+
webhookLabels[install.WebhookDescKey] = desc.Name
276+
webhookSelector := labels.SelectorFromSet(webhookLabels).String()
277+
if desc.Type == v1alpha1.MutatingAdmissionWebhook {
278+
existingWebhooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
277279
if err != nil {
278-
return nil, fmt.Errorf("could not retrieve generated APIService: %v", err)
280+
return nil, fmt.Errorf("could not retrieve generated MutatingWebhookConfiguration: %v", err)
279281
}
280-
if len(webhook.Webhooks[0].ClientConfig.CABundle) > 0 {
281-
return webhook.Webhooks[0].ClientConfig.CABundle, nil
282+
283+
if len(existingWebhooks.Items) > 0 {
284+
return existingWebhooks.Items[0].Webhooks[0].ClientConfig.CABundle, nil
282285
}
283-
} else {
284-
webhook, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Get(context.TODO(), webhookName, metav1.GetOptions{})
286+
287+
} else if desc.Type == v1alpha1.ValidatingAdmissionWebhook {
288+
existingWebhooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
285289
if err != nil {
286-
return nil, fmt.Errorf("could not retrieve generated APIService: %v", err)
290+
return nil, fmt.Errorf("could not retrieve generated ValidatingWebhookConfiguration: %v", err)
287291
}
288-
if len(webhook.Webhooks[0].ClientConfig.CABundle) > 0 {
289-
return webhook.Webhooks[0].ClientConfig.CABundle, nil
292+
293+
if len(existingWebhooks.Items) > 0 {
294+
return existingWebhooks.Items[0].Webhooks[0].ClientConfig.CABundle, nil
290295
}
291296
}
292297
}
@@ -313,7 +318,7 @@ func (a *Operator) updateDeploymentSpecsWithApiServiceData(csv *v1alpha1.Cluster
313318
depSpecs[sddSpec.Name] = sddSpec.Spec
314319
}
315320

316-
caBundle, err := a.getCaBundle(csv)
321+
caBundle, err := a.getCABundle(csv)
317322
if err != nil {
318323
return nil, fmt.Errorf("could not retrieve caBundle: %v", err)
319324
}

‎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 ValidatingWebhookConfigurations")
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+
webhookNames := map[string]struct{}{}
1444+
// Check if Webhooks have valid rules and unique names
13851445
for _, desc := range out.Spec.WebhookDefinitions {
1446+
_, present := webhookNames[desc.Name]
1447+
if present {
1448+
out.SetPhaseWithEventIfChanged(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonUnsupportedWebhookRules, "CSV contains repeated WebhookDescription name", now, a.recorder)
1449+
return
1450+
}
1451+
webhookNames[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)