Bug 1826446: (fix) Admission Webhook names must be unique

deploy/chart/crds/0000_50_olm_00-catalogsources.yaml

@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.2.8
+    controller-gen.kubebuilder.io/version: v0.3.0
   creationTimestamp: null
   name: catalogsources.operators.coreos.com
 spec:

deploy/chart/crds/0000_50_olm_00-installplans.yaml

@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.2.8
+    controller-gen.kubebuilder.io/version: v0.3.0
   creationTimestamp: null
   name: installplans.operators.coreos.com
 spec:

deploy/chart/crds/0000_50_olm_00-operatorgroups.yaml

@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.2.8
+    controller-gen.kubebuilder.io/version: v0.3.0
   creationTimestamp: null
   name: operatorgroups.operators.coreos.com
 spec:

deploy/chart/crds/0000_50_olm_00-subscriptions.yaml

@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.2.8
+    controller-gen.kubebuilder.io/version: v0.3.0
   creationTimestamp: null
   name: subscriptions.operators.coreos.com
 spec:

go.mod

@@ -22,8 +22,8 @@ require (
 	github.com/onsi/gomega v1.9.0
 	github.com/openshift/api v0.0.0-20200331152225-585af27e34fd
 	github.com/openshift/client-go v0.0.0-20200326155132-2a6cd50aedd0
-	github.com/operator-framework/api v0.3.2
-	github.com/operator-framework/operator-registry v1.12.1
+	github.com/operator-framework/api v0.3.5
+	github.com/operator-framework/operator-registry v1.12.2
 	github.com/otiai10/copy v1.0.2
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.2.1

go.sum

@@ -584,11 +584,14 @@ github.com/openshift/client-go v0.0.0-20200326155132-2a6cd50aedd0 h1:kMiuiZXH1Gd
 github.com/openshift/client-go v0.0.0-20200326155132-2a6cd50aedd0/go.mod h1:uUQ4LClRO+fg5MF/P6QxjMCb1C9f7Oh4RKepftDnEJE=
 github.com/openzipkin/zipkin-go v0.1.6/go.mod h1:QgAqvLzwWbR/WpD4A3cGpPtJrZXNIiJc5AZX7/PBEpw=
 github.com/operator-framework/api v0.1.1/go.mod h1:yzNYR7qyJqRGOOp+bT6Z/iYSbSPNxeh3Si93Gx/3OBY=
-github.com/operator-framework/api v0.3.2 h1:FvM+tWBIEDNfE0IxT8nr3t2HymcbItC4rngPJC467y4=
-github.com/operator-framework/api v0.3.2/go.mod h1:TmRmw+8XOUaDPq6SP9gA8cIexNf/Pq8LMFY7YaKQFTs=
+github.com/operator-framework/api v0.3.4/go.mod h1:TmRmw+8XOUaDPq6SP9gA8cIexNf/Pq8LMFY7YaKQFTs=
+github.com/operator-framework/api v0.3.5 h1:eUFzNOACJLsufdLiwRvj7/7O1yDgsu/6d1xM4idgE3I=
+github.com/operator-framework/api v0.3.5/go.mod h1:TmRmw+8XOUaDPq6SP9gA8cIexNf/Pq8LMFY7YaKQFTs=
 github.com/operator-framework/operator-registry v1.5.3/go.mod h1:agrQlkWOo1q8U1SAaLSS2WQ+Z9vswNT2M2HFib9iuLY=
 github.com/operator-framework/operator-registry v1.12.1 h1:JWk8s6LPwyJ16qQzOhALS0hycmmzUGteGQSCbfsPeBY=
 github.com/operator-framework/operator-registry v1.12.1/go.mod h1:rf4b/h77GUv1+geiej2KzGRQr8iBLF4dXNwr5AuGkrQ=
+github.com/operator-framework/operator-registry v1.12.2 h1:rQayebXFwetB9HrodU+fjbH5zFZ52uKIpDGhEzDDjZs=
+github.com/operator-framework/operator-registry v1.12.2/go.mod h1:8rsa504LH5BqEQJZsK+/5+5PmEFFphhQVsyDkvD+us4=
 github.com/otiai10/copy v1.0.1/go.mod h1:8bMCJrAqOtN/d9oyh5HR7HhLQMvcGMpGdwRDYsfOCHc=
 github.com/otiai10/copy v1.0.2 h1:DDNipYy6RkIkjMwy+AWzgKiNTyj2RUI9yEMeETEpVyc=
 github.com/otiai10/copy v1.0.2/go.mod h1:c7RpqBkwMom4bYTSkLSym4VSJz/XtncWRAj/J4PEIMY=

manifests/0000_50_olm_00-catalogsources.yaml

@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.2.8
+    controller-gen.kubebuilder.io/version: v0.3.0
   creationTimestamp: null
   name: catalogsources.operators.coreos.com
 spec:

manifests/0000_50_olm_00-installplans.yaml

@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.2.8
+    controller-gen.kubebuilder.io/version: v0.3.0
   creationTimestamp: null
   name: installplans.operators.coreos.com
 spec:

manifests/0000_50_olm_00-operatorgroups.yaml

@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.2.8
+    controller-gen.kubebuilder.io/version: v0.3.0
   creationTimestamp: null
   name: operatorgroups.operators.coreos.com
 spec:

manifests/0000_50_olm_00-subscriptions.yaml

@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.2.8
+    controller-gen.kubebuilder.io/version: v0.3.0
   creationTimestamp: null
   name: subscriptions.operators.coreos.com
 spec:

pkg/controller/install/certresources.go

@@ -103,7 +103,7 @@ type webhookDescriptionWithCAPEM struct {
 }
 
 func (i *webhookDescriptionWithCAPEM) getName() string {
-	return i.webhookDescription.Name
+	return i.webhookDescription.GenerateName
 }
 
 func (i *webhookDescriptionWithCAPEM) setCAPEM(caPEM []byte) {

pkg/controller/install/webhook.go

@@ -9,7 +9,6 @@ import (
 
 	log "github.com/sirupsen/logrus"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
-	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 )
@@ -72,80 +71,107 @@ func (i *StrategyDeploymentInstaller) createOrUpdateWebhook(caPEM []byte, desc v
 }
 
 func (i *StrategyDeploymentInstaller) createOrUpdateMutatingWebhook(ogNamespacelabelSelector *metav1.LabelSelector, caPEM []byte, desc v1alpha1.WebhookDescription) error {
-	webhooks := []admissionregistrationv1.MutatingWebhook{
-		desc.GetMutatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
+	webhookLabels := ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind)
+	webhookLabels[WebhookDescKey] = desc.GenerateName
+	webhookSelector := labels.SelectorFromSet(webhookLabels).String()
+
+	existingWebhooks, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
+	if err != nil {
+		return err
 	}
-	existingHook, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Get(context.TODO(), desc.Name, metav1.GetOptions{})
-	if err == nil {
-		// Check if the only owners are this CSV or in this CSV's replacement chain
-		if ownerutil.Adoptable(i.owner, existingHook.GetOwnerReferences()) {
-			ownerutil.AddNonBlockingOwner(existingHook, i.owner)
-		}
 
-		// Update the list of webhooks
-		existingHook.Webhooks = webhooks
+	if len(existingWebhooks.Items) == 0 {
+		// Create a MutatingWebhookConfiguration
+		webhook := admissionregistrationv1.MutatingWebhookConfiguration{
+			ObjectMeta: metav1.ObjectMeta{
+				GenerateName: desc.GenerateName + "-",
+				Namespace:    i.owner.GetNamespace(),
+				Labels:       ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind),
+			},
+			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(), existingHook, metav1.UpdateOptions{}); err != nil {
-			log.Warnf("could not update MutatingWebhookConfiguration %s", existingHook.GetName())
+		if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Create(context.TODO(), &webhook, metav1.CreateOptions{}); err != nil {
+			log.Errorf("Webhooks: Error creating MutatingWebhookConfiguration: %v", err)
 			return err
 		}
-	} else if k8serrors.IsNotFound(err) {
-		hook := admissionregistrationv1.MutatingWebhookConfiguration{
-			ObjectMeta: metav1.ObjectMeta{Name: desc.Name,
-				Namespace: i.owner.GetNamespace(),
-			},
-			Webhooks: webhooks,
+		return nil
+	}
+	for _, webhook := range existingWebhooks.Items {
+		// Update the list of webhooks
+		webhook.Webhooks = []admissionregistrationv1.MutatingWebhook{
+			desc.GetMutatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
 		}
-		// Add an owner
-		ownerutil.AddNonBlockingOwner(&hook, i.owner)
-		if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Create(context.TODO(), &hook, metav1.CreateOptions{}); err != nil {
-			log.Errorf("Webhooks: Error creating mutating MutatingVebhookConfiguration: %v", err)
+
+		// Attempt an update
+		if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Update(context.TODO(), &webhook, metav1.UpdateOptions{}); err != nil {
+			log.Warnf("could not update MutatingWebhookConfiguration %s", webhook.GetName())
 			return err
 		}
-	} else {
-		return err
 	}
 
 	return nil
 }
 
 func (i *StrategyDeploymentInstaller) createOrUpdateValidatingWebhook(ogNamespacelabelSelector *metav1.LabelSelector, caPEM []byte, desc v1alpha1.WebhookDescription) error {
-	webhooks := []admissionregistrationv1.ValidatingWebhook{
-		desc.GetValidatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
+	webhookLabels := ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind)
+	webhookLabels[WebhookDescKey] = desc.GenerateName
+	webhookSelector := labels.SelectorFromSet(webhookLabels).String()
+
+	existingWebhooks, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
+	if err != nil {
+		return err
 	}
-	existingHook, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(context.TODO(), desc.Name, metav1.GetOptions{})
-	if err == nil {
-		// Check if the only owners are this CSV or in this CSV's replacement chain
-		if ownerutil.Adoptable(i.owner, existingHook.GetOwnerReferences()) {
-			ownerutil.AddNonBlockingOwner(existingHook, i.owner)
-		}
 
-		// Update the list of webhooks
-		existingHook.Webhooks = webhooks
+	if len(existingWebhooks.Items) == 0 {
+		// Create a ValidatingWebhookConfiguration
+		webhook := admissionregistrationv1.ValidatingWebhookConfiguration{
+			ObjectMeta: metav1.ObjectMeta{
+				GenerateName: desc.GenerateName + "-",
+				Namespace:    i.owner.GetNamespace(),
+				Labels:       ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind),
+			},
+			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(), existingHook, metav1.UpdateOptions{}); err != nil {
-			log.Warnf("could not update ValidatingWebhookConfiguration %s", existingHook.GetName())
+		if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Create(context.TODO(), &webhook, metav1.CreateOptions{}); err != nil {
+			log.Errorf("Webhooks: Error creating ValidatingWebhookConfiguration: %v", err)
 			return err
 		}
-	} else if k8serrors.IsNotFound(err) {
-		// Create a ValidatingWebhookConfiguration
-		hook := admissionregistrationv1.ValidatingWebhookConfiguration{
-			ObjectMeta: metav1.ObjectMeta{Name: desc.Name,
-				Namespace: i.owner.GetNamespace(),
-			},
-			Webhooks: webhooks,
+		return nil
+	}
+	for _, webhook := range existingWebhooks.Items {
+		// Update the list of webhooks
+		webhook.Webhooks = []admissionregistrationv1.ValidatingWebhook{
+			desc.GetValidatingWebhook(i.owner.GetNamespace(), ogNamespacelabelSelector, caPEM),
 		}
 
-		// Add an owner
-		ownerutil.AddNonBlockingOwner(&hook, i.owner)
-		if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Create(context.TODO(), &hook, metav1.CreateOptions{}); err != nil {
-			log.Errorf("Webhooks: Error create creating ValidationVebhookConfiguration: %v", err)
+		// Attempt an update
+		if _, err := i.strategyClient.GetOpClient().KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Update(context.TODO(), &webhook, metav1.UpdateOptions{}); err != nil {
+			log.Warnf("could not update ValidatingWebhookConfiguration %s", webhook.GetName())
 			return err
 		}
-	} else {
-		return err
 	}
+
+	return nil
+}
+
+const WebhookDescKey = "webhookDescriptionGenerateName"
+
+// addWebhookLabels adds webhook labels to an object
+func addWebhookLabels(object metav1.Object, webhookDesc v1alpha1.WebhookDescription) error {
+	labels := object.GetLabels()
+	if labels == nil {
+		labels = map[string]string{}
+	}
+	labels[WebhookDescKey] = webhookDesc.GenerateName
+	object.SetLabels(labels)
+
 	return nil
 }

pkg/controller/operators/olm/apiservices.go

@@ -10,12 +10,14 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/certs"
 	olmerrors "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/errors"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 )
 
 const (
@@ -255,10 +257,8 @@ func (a *Operator) areAPIServicesAvailable(csv *v1alpha1.ClusterServiceVersion)
 	return true, nil
 }
 
-// updateDeploymentSpecsWithApiServiceData transforms an install strategy to include information about apiservices
-// it is used in generating hashes for deployment specs to know when something in the spec has changed,
-// but duplicates a lot of installAPIServiceRequirements and should be refactored.
-func (a *Operator) getCaBundle(csv *v1alpha1.ClusterServiceVersion) ([]byte, error) {
+// getCABundle returns the CA associated with a deployment
+func (a *Operator) getCABundle(csv *v1alpha1.ClusterServiceVersion) ([]byte, error) {
 	for _, desc := range csv.GetOwnedAPIServiceDescriptions() {
 		apiServiceName := desc.GetName()
 		apiService, err := a.lister.APIRegistrationV1().APIServiceLister().Get(apiServiceName)
@@ -271,22 +271,27 @@ func (a *Operator) getCaBundle(csv *v1alpha1.ClusterServiceVersion) ([]byte, err
 	}
 
 	for _, desc := range csv.Spec.WebhookDefinitions {
-		webhookName := desc.Name
-		if desc.Type == "ValidatingAdmissionWebhook" {
-			webhook, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(context.TODO(), webhookName, metav1.GetOptions{})
+		webhookLabels := ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind)
+		webhookLabels[install.WebhookDescKey] = desc.GenerateName
+		webhookSelector := labels.SelectorFromSet(webhookLabels).String()
+		if desc.Type == v1alpha1.MutatingAdmissionWebhook {
+			existingWebhooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
 			if err != nil {
-				return nil, fmt.Errorf("could not retrieve generated APIService: %v", err)
+				return nil, fmt.Errorf("could not retrieve generated MutatingWebhookConfiguration: %v", err)
 			}
-			if len(webhook.Webhooks[0].ClientConfig.CABundle) > 0 {
-				return webhook.Webhooks[0].ClientConfig.CABundle, nil
+
+			if len(existingWebhooks.Items) > 0 {
+				return existingWebhooks.Items[0].Webhooks[0].ClientConfig.CABundle, nil
 			}
-		} else {
-			webhook, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Get(context.TODO(), webhookName, metav1.GetOptions{})
+
+		} else if desc.Type == v1alpha1.ValidatingAdmissionWebhook {
+			existingWebhooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
 			if err != nil {
-				return nil, fmt.Errorf("could not retrieve generated APIService: %v", err)
+				return nil, fmt.Errorf("could not retrieve generated ValidatingWebhookConfiguration: %v", err)
 			}
-			if len(webhook.Webhooks[0].ClientConfig.CABundle) > 0 {
-				return webhook.Webhooks[0].ClientConfig.CABundle, nil
+
+			if len(existingWebhooks.Items) > 0 {
+				return existingWebhooks.Items[0].Webhooks[0].ClientConfig.CABundle, nil
 			}
 		}
 	}
@@ -313,7 +318,7 @@ func (a *Operator) updateDeploymentSpecsWithApiServiceData(csv *v1alpha1.Cluster
 		depSpecs[sddSpec.Name] = sddSpec.Spec
 	}
 
-	caBundle, err := a.getCaBundle(csv)
+	caBundle, err := a.getCABundle(csv)
 	if err != nil {
 		return nil, fmt.Errorf("could not retrieve caBundle: %v", err)
 	}
@@ -400,7 +405,7 @@ func (a *Operator) updateDeploymentSpecsWithApiServiceData(csv *v1alpha1.Cluster
 
 		depSpec, ok := depSpecs[desc.DeploymentName]
 		if !ok {
-			return nil, fmt.Errorf("StrategyDetailsDeployment missing deployment %s for owned APIServices %s", desc.DeploymentName, desc.Name)
+			return nil, fmt.Errorf("StrategyDetailsDeployment missing deployment %s for WebhookDescription %s", desc.DeploymentName, desc.GenerateName)
 		}
 
 		if depSpec.Template.Spec.ServiceAccountName == "" {