Skip to content

Commit ad4bd9e

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 ad4bd9e

File tree

5 files changed

+709
-891
lines changed

5 files changed

+709
-891
lines changed

‎pkg/controller/install/webhook.go

Lines changed: 87 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +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"
12-
k8serrors "k8s.io/apimachinery/pkg/api/errors"
1314
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1415
"k8s.io/apimachinery/pkg/labels"
16+
"k8s.io/apimachinery/pkg/util/rand"
1517
)
1618

1719
func ValidWebhookRules(rules []admissionregistrationv1.RuleWithOperations) error {
@@ -69,80 +71,115 @@ func (i *StrategyDeploymentInstaller) createOrUpdateWebhook(caPEM []byte, desc v
6971
}
7072

7173
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-
}
74+
webhookLabels := ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind)
75+
webhookLabels[WebhookLabelKey] = HashWebhookDesc(desc)
76+
webhookSelector := labels.SelectorFromSet(webhookLabels).String()
8177

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

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) {
83+
if len(existingWebhooks.Items) == 0 {
84+
// Create a ValidatingWebhookConfiguration
9185
hook := admissionregistrationv1.MutatingWebhookConfiguration{
92-
ObjectMeta: metav1.ObjectMeta{Name: desc.Name,
93-
Namespace: i.owner.GetNamespace(),
86+
ObjectMeta: metav1.ObjectMeta{
87+
GenerateName: desc.Name + "-",
88+
Namespace: i.owner.GetNamespace(),
89+
Labels: ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind),
90+
},
91+
Webhooks: []admissionregistrationv1.MutatingWebhook{
92+
desc.GetMutatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
9493
},
95-
Webhooks: webhooks,
9694
}
97-
// Add an owner
98-
ownerutil.AddNonBlockingOwner(&hook, i.owner)
95+
addWebhookLabels(&hook, desc)
96+
9997
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)
98+
log.Errorf("Webhooks: Error creating ValidationWebhookConfiguration: %v", err)
10199
return err
102100
}
103101
} else {
104-
return err
105-
}
102+
for _, webhook := range existingWebhooks.Items {
103+
// Update the list of webhooks
104+
webhook.Webhooks = []admissionregistrationv1.MutatingWebhook{
105+
desc.GetMutatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
106+
}
106107

108+
// Attempt an update
109+
if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Update(context.TODO(), &webhook, metav1.UpdateOptions{}); err != nil {
110+
log.Warnf("could not update MutatingWebhookConfiguration %s", webhook.GetName())
111+
return err
112+
}
113+
114+
}
115+
}
107116
return nil
108117
}
109118

110119
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-
}
120+
webhookLabels := ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind)
121+
webhookLabels[WebhookLabelKey] = HashWebhookDesc(desc)
122+
webhookSelector := labels.SelectorFromSet(webhookLabels).String()
120123

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

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) {
129+
if len(existingWebhooks.Items) == 0 {
130130
// Create a ValidatingWebhookConfiguration
131131
hook := admissionregistrationv1.ValidatingWebhookConfiguration{
132-
ObjectMeta: metav1.ObjectMeta{Name: desc.Name,
133-
Namespace: i.owner.GetNamespace(),
132+
ObjectMeta: metav1.ObjectMeta{
133+
GenerateName: desc.Name + "-",
134+
Namespace: i.owner.GetNamespace(),
135+
Labels: ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind),
136+
},
137+
Webhooks: []admissionregistrationv1.ValidatingWebhook{
138+
desc.GetValidatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
134139
},
135-
Webhooks: webhooks,
136140
}
141+
addWebhookLabels(&hook, desc)
137142

138-
// Add an owner
139-
ownerutil.AddNonBlockingOwner(&hook, i.owner)
140143
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)
144+
log.Errorf("Webhooks: Error creating ValidatingWebhookConfiguration: %v", err)
142145
return err
143146
}
144147
} else {
145-
return err
148+
for _, webhook := range existingWebhooks.Items {
149+
150+
// Update the list of webhooks
151+
webhook.Webhooks = []admissionregistrationv1.ValidatingWebhook{
152+
desc.GetValidatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
153+
}
154+
155+
// Attempt an update
156+
if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Update(context.TODO(), &webhook, metav1.UpdateOptions{}); err != nil {
157+
log.Warnf("could not update ValidatingWebhookConfiguration %s", webhook.GetName())
158+
return err
159+
}
160+
161+
}
146162
}
147163
return nil
148164
}
165+
166+
const WebhookLabelKey = "webhookDescriptionSHA"
167+
168+
// addWebhookLabels adds webhook labels to an object
169+
func addWebhookLabels(object metav1.Object, webhookDesc v1alpha1.WebhookDescription) error {
170+
labels := object.GetLabels()
171+
if labels == nil {
172+
labels = map[string]string{}
173+
}
174+
labels[WebhookLabelKey] = HashWebhookDesc(webhookDesc)
175+
object.SetLabels(labels)
176+
177+
return nil
178+
}
179+
180+
// HashWebhookDesc calculates a hash given a webhookDescription
181+
func HashWebhookDesc(webhookDesc v1alpha1.WebhookDescription) string {
182+
hasher := fnv.New32a()
183+
hashutil.DeepHashObject(hasher, &webhookDesc)
184+
return rand.SafeEncodeString(fmt.Sprint(hasher.Sum32()))
185+
}

‎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: 58 additions & 0 deletions
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 {

0 commit comments

Comments
 (0)