Merge pull request #1767 from awgreene/fix-conversion-webhooks

BUG 1872584: Fix conversion webhooks
operator-framework · Sep 21, 2020 · f02ac99 · f02ac99
2 parents 026fa7a + 5b7f265
commit f02ac99
Show file tree

Hide file tree

Showing 18 changed files with 269 additions and 188 deletions.
diff --git a/deploy/chart/crds/0000_50_olm_00-clusterserviceversions.crd.yaml b/deploy/chart/crds/0000_50_olm_00-clusterserviceversions.crd.yaml
@@ -8824,6 +8824,11 @@ spec:
                             type: string
                     sideEffects:
                       type: string
+                    targetPort:
+                      anyOf:
+                      - type: integer
+                      - type: string
+                      x-kubernetes-int-or-string: true
                     timeoutSeconds:
                       type: integer
                       format: int32
@@ -8834,6 +8839,7 @@ spec:
                       enum:
                       - ValidatingAdmissionWebhook
                       - MutatingAdmissionWebhook
+                      - ConversionWebhook
                     webhookPath:
                       type: string
           status:

diff --git a/deploy/ocp/manifests/0.16.1/0000_50_olm_00-clusterserviceversions.crd.yaml b/deploy/ocp/manifests/0.16.1/0000_50_olm_00-clusterserviceversions.crd.yaml
@@ -8826,6 +8826,11 @@ spec:
                             type: string
                     sideEffects:
                       type: string
+                    targetPort:
+                      anyOf:
+                      - type: integer
+                      - type: string
+                      x-kubernetes-int-or-string: true
                     timeoutSeconds:
                       type: integer
                       format: int32
@@ -8836,6 +8841,7 @@ spec:
                       enum:
                       - ValidatingAdmissionWebhook
                       - MutatingAdmissionWebhook
+                      - ConversionWebhook
                     webhookPath:
                       type: string
           status:

diff --git a/deploy/upstream/manifests/0.16.1/0000_50_olm_00-clusterserviceversions.crd.yaml b/deploy/upstream/manifests/0.16.1/0000_50_olm_00-clusterserviceversions.crd.yaml
@@ -8826,6 +8826,11 @@ spec:
                             type: string
                     sideEffects:
                       type: string
+                    targetPort:
+                      anyOf:
+                      - type: integer
+                      - type: string
+                      x-kubernetes-int-or-string: true
                     timeoutSeconds:
                       type: integer
                       format: int32
@@ -8836,6 +8841,7 @@ spec:
                       enum:
                       - ValidatingAdmissionWebhook
                       - MutatingAdmissionWebhook
+                      - ConversionWebhook
                     webhookPath:
                       type: string
           status:

diff --git a/deploy/upstream/quickstart/crds.yaml b/deploy/upstream/quickstart/crds.yaml
@@ -9025,6 +9025,11 @@ spec:
                             type: string
                     sideEffects:
                       type: string
+                    targetPort:
+                      anyOf:
+                      - type: integer
+                      - type: string
+                      x-kubernetes-int-or-string: true
                     timeoutSeconds:
                       type: integer
                       format: int32
@@ -9035,6 +9040,7 @@ spec:
                       enum:
                       - ValidatingAdmissionWebhook
                       - MutatingAdmissionWebhook
+                      - ConversionWebhook
                     webhookPath:
                       type: string
           status:

diff --git a/go.mod b/go.mod
@@ -23,7 +23,7 @@ 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.12
+	github.com/operator-framework/api v0.3.15
 	github.com/operator-framework/operator-registry v1.13.6
 	github.com/otiai10/copy v1.2.0
 	github.com/pkg/errors v0.9.1

diff --git a/go.sum b/go.sum
@@ -597,8 +597,8 @@ 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.3.7-0.20200602203552-431198de9fc2/go.mod h1:Xbje9x0SHmh0nihE21kpesB38vk3cyxnE6JdDS8Jo1Q=
-github.com/operator-framework/api v0.3.12 h1:In6bSbDr5zVOb5ats7TBMnl1h0hCIjkAxXkh+MXFJps=
-github.com/operator-framework/api v0.3.12/go.mod h1:Xbje9x0SHmh0nihE21kpesB38vk3cyxnE6JdDS8Jo1Q=
+github.com/operator-framework/api v0.3.15 h1:gVVx9y8o9RpY8+bzSCo+WLi+LZylYI8zp4Gtlei8Mac=
+github.com/operator-framework/api v0.3.15/go.mod h1:Xbje9x0SHmh0nihE21kpesB38vk3cyxnE6JdDS8Jo1Q=
 github.com/operator-framework/operator-registry v1.13.6 h1:h/dIjQQS7uneQNRifrSz7h0xg4Xyjg6C9f6XZofbMPg=
 github.com/operator-framework/operator-registry v1.13.6/go.mod h1:YhnIzOVjRU2ZwZtzt+fjcjW8ujJaSFynBEu7QVKaSdU=
 github.com/otiai10/copy v1.2.0 h1:HvG945u96iNadPoG2/Ja2+AUJeW5YuFQMixq9yirC+k=

diff --git a/manifests/0000_50_olm_00-clusterserviceversions.crd.yaml b/manifests/0000_50_olm_00-clusterserviceversions.crd.yaml
@@ -8824,6 +8824,11 @@ spec:
                             type: string
                     sideEffects:
                       type: string
+                    targetPort:
+                      anyOf:
+                      - type: integer
+                      - type: string
+                      x-kubernetes-int-or-string: true
                     timeoutSeconds:
                       type: integer
                       format: int32
@@ -8834,6 +8839,7 @@ spec:
                       enum:
                       - ValidatingAdmissionWebhook
                       - MutatingAdmissionWebhook
+                      - ConversionWebhook
                     webhookPath:
                       type: string
           status:

diff --git a/pkg/controller/install/certresources.go b/pkg/controller/install/certresources.go
@@ -125,10 +125,18 @@ func (i *webhookDescriptionWithCAPEM) getServicePort() corev1.ServicePort {
 	if i.webhookDescription.ContainerPort > 0 {
 		containerPort = int(i.webhookDescription.ContainerPort)
 	}
+
+	// Before users could set TargetPort in the CSV, OLM just set its
+	// value to the containerPort. This change keeps OLM backwards compatible
+	// if the TargetPort is not set in the CSV.
+	targetPort := intstr.FromInt(containerPort)
+	if i.webhookDescription.TargetPort != nil {
+		targetPort = *i.webhookDescription.TargetPort
+	}
 	return corev1.ServicePort{
 		Name:       strconv.Itoa(containerPort),
 		Port:       int32(containerPort),
-		TargetPort: intstr.FromInt(containerPort),
+		TargetPort: targetPort,
 	}
 }
 

diff --git a/pkg/controller/install/webhook.go b/pkg/controller/install/webhook.go
@@ -69,7 +69,8 @@ func (i *StrategyDeploymentInstaller) createOrUpdateWebhook(caPEM []byte, desc v
 		i.createOrUpdateValidatingWebhook(ogNamespacelabelSelector, caPEM, desc)
 	case v1alpha1.MutatingAdmissionWebhook:
 		i.createOrUpdateMutatingWebhook(ogNamespacelabelSelector, caPEM, desc)
-
+	case v1alpha1.ConversionWebhook:
+		i.createOrUpdateConversionWebhook(caPEM, desc)
 	}
 	return nil
 }
@@ -102,14 +103,6 @@ func (i *StrategyDeploymentInstaller) createOrUpdateMutatingWebhook(ogNamespacel
 			log.Errorf("Webhooks: Error creating MutatingWebhookConfiguration: %v", err)
 			return err
 		}
-
-		// If dealing with a Conversion Webhook, make sure that the operator only supports the AllNamespace installMode.
-		if len(desc.ConversionCRDs) != 0 && isSingletonOperator(i) {
-			if err := createOrUpdateConversionCRDs(desc, webhook.Webhooks[0].ClientConfig, i); err != nil {
-				return err
-			}
-		}
-		return nil
 	}
 	for _, webhook := range existingWebhooks.Items {
 		// Update the list of webhooks
@@ -123,13 +116,6 @@ func (i *StrategyDeploymentInstaller) createOrUpdateMutatingWebhook(ogNamespacel
 			log.Warnf("could not update MutatingWebhookConfiguration %s", webhook.GetName())
 			return err
 		}
-
-		// If dealing with a Conversion Webhook, make sure that the operator only supports the AllNamespace installMode.
-		if len(desc.ConversionCRDs) != 0 && isSingletonOperator(i) {
-			if err := createOrUpdateConversionCRDs(desc, webhook.Webhooks[0].ClientConfig, i); err != nil {
-				return err
-			}
-		}
 	}
 
 	return nil
@@ -164,13 +150,6 @@ func (i *StrategyDeploymentInstaller) createOrUpdateValidatingWebhook(ogNamespac
 			return err
 		}
 
-		// If dealing with a Conversion Webhook, make sure that the operator only supports the AllNamespace installMode.
-		if len(desc.ConversionCRDs) != 0 && isSingletonOperator(i) {
-			if err := createOrUpdateConversionCRDs(desc, webhook.Webhooks[0].ClientConfig, i); err != nil {
-				return err
-			}
-		}
-
 		return nil
 	}
 	for _, webhook := range existingWebhooks.Items {
@@ -185,25 +164,13 @@ func (i *StrategyDeploymentInstaller) createOrUpdateValidatingWebhook(ogNamespac
 			log.Warnf("could not update ValidatingWebhookConfiguration %s", webhook.GetName())
 			return err
 		}
-
-		// If dealing with a Conversion Webhook, make sure that the operator only supports the AllNamespace installMode.
-		if len(desc.ConversionCRDs) != 0 && isSingletonOperator(i) {
-			if err := createOrUpdateConversionCRDs(desc, webhook.Webhooks[0].ClientConfig, i); err != nil {
-				return err
-			}
-		}
 	}
 
 	return nil
 }
 
 // check if csv supports only AllNamespaces install mode
-func isSingletonOperator(i *StrategyDeploymentInstaller) bool {
-	// get csv
-	csv, err := i.strategyClient.GetOpLister().OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(i.owner.GetNamespace()).Get(i.owner.GetName())
-	if err != nil {
-		log.Infof("CSV not found, error: %s", err.Error())
-	}
+func isSingletonOperator(csv v1alpha1.ClusterServiceVersion) bool {
 	// check if AllNamespaces is supported and other install modes are not supported
 	for _, installMode := range csv.Spec.InstallModes {
 		if installMode.Type == v1alpha1.InstallModeTypeAllNamespaces && !installMode.Supported {
@@ -216,34 +183,39 @@ func isSingletonOperator(i *StrategyDeploymentInstaller) bool {
 	return true
 }
 
-func createOrUpdateConversionCRDs(desc v1alpha1.WebhookDescription, clientConfig admissionregistrationv1.WebhookClientConfig, i *StrategyDeploymentInstaller) error {
+func (i *StrategyDeploymentInstaller) createOrUpdateConversionWebhook(caPEM []byte, desc v1alpha1.WebhookDescription) error {
 	// get a list of owned CRDs
-	csv, err := i.strategyClient.GetOpLister().OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(i.owner.GetNamespace()).Get(i.owner.GetName())
-	if err != nil {
-		log.Infof("CSV not found, error: %s", err.Error())
+	csv, ok := i.owner.(*v1alpha1.ClusterServiceVersion)
+	if !ok {
+		return fmt.Errorf("ConversionWebhook owner must be a ClusterServiceVersion")
+	}
+
+	if !isSingletonOperator(*csv) {
+		return fmt.Errorf("CSVs with conversion webhooks must support only AllNamespaces")
+	}
+
+	if len(desc.ConversionCRDs) == 0 {
+		return fmt.Errorf("Conversion Webhook must have at least one CRD specified")
 	}
-	ownedCRDs := csv.Spec.CustomResourceDefinitions.Owned
 
 	// iterate over all the ConversionCRDs
-	for _, ConversionCRD := range desc.ConversionCRDs {
-		// check if CRD exists on cluster
-		crd, err := i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), ConversionCRD, metav1.GetOptions{})
+	for _, conversionCRD := range desc.ConversionCRDs {
+		// Get existing CRD on cluster
+		crd, err := i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{})
 		if err != nil {
-			log.Infof("CRD not found %s, error: %s", ConversionCRD, err.Error())
+			return fmt.Errorf("Unable to get CRD %s specified in Conversion Webhook: %v", conversionCRD, err)
 		}
-		log.Infof("Found conversionCRDs %s", ConversionCRD)
 
 		// check if this CRD is an owned CRD
 		foundCRD := false
-		for _, ownedCRD := range ownedCRDs {
-			if ownedCRD.Name == crd.GetName() {
-				log.Infof("CSV %q owns CRD %q", csv.GetName(), crd.GetName())
+		for _, ownedCRD := range csv.Spec.CustomResourceDefinitions.Owned {
+			if ownedCRD.Name == conversionCRD {
 				foundCRD = true
+				break
 			}
 		}
 		if !foundCRD {
-			log.Infof("CSV %q does not own CRD %q", csv.GetName(), crd.GetName())
-			return nil
+			return fmt.Errorf("CSV %s does not own CRD %s", csv.GetName(), conversionCRD)
 		}
 
 		// crd.Spec.Conversion.Strategy specifies how custom resources are converted between versions.
@@ -256,54 +228,41 @@ func createOrUpdateConversionCRDs(desc v1alpha1.WebhookDescription, clientConfig
 		// By default the strategy is none
 		// Reference:
 		// 	- https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
-		if crd.Spec.PreserveUnknownFields == true && crd.Spec.Conversion.Strategy == "Webhook" {
-			log.Infof("crd.Spec.PreserveUnknownFields must be false to let API Server call webhook to do the conversion.")
-			return nil
+		if crd.Spec.PreserveUnknownFields != false {
+			return fmt.Errorf("crd.Spec.PreserveUnknownFields must be false to let API Server call webhook to do the conversion")
 		}
 
 		// Conversion WebhookClientConfig should not be set when Strategy is None
 		// https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
 		// Conversion WebhookClientConfig needs to be set when Strategy is None
 		// https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#configure-customresourcedefinition-to-use-conversion-webhooks
-		if crd.Spec.Conversion.Strategy == "Webhook" {
-			log.Infof("Updating CRD")
-
-			// use user defined path for CRD conversion webhook, else set default value
-			conversionWebhookPath := "/"
-			if crd.Spec.Conversion.Webhook.ClientConfig.Service.Path != nil {
-				conversionWebhookPath = *crd.Spec.Conversion.Webhook.ClientConfig.Service.Path
-			}
 
-			// use user defined port, else set it to admission webhook's port
-			conversionWebhookPort := *clientConfig.Service.Port
-			if crd.Spec.Conversion.Webhook.ClientConfig.Service.Port != nil {
-				conversionWebhookPort = *crd.Spec.Conversion.Webhook.ClientConfig.Service.Port
-			}
+		// use user defined path for CRD conversion webhook, else set default value
+		conversionWebhookPath := "/"
+		if desc.WebhookPath != nil {
+			conversionWebhookPath = *desc.WebhookPath
+		}
 
-			// use user defined ConversionReviewVersions
-			conversionWebhookConversionReviewVersions := crd.Spec.Conversion.Webhook.ConversionReviewVersions
-
-			// Override Name, Namespace, and CABundle
-			crd.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{
-				Strategy: "Webhook",
-				Webhook: &apiextensionsv1.WebhookConversion{
-					ClientConfig: &apiextensionsv1.WebhookClientConfig{
-						Service: &apiextensionsv1.ServiceReference{
-							Namespace: clientConfig.Service.Namespace,
-							Name:      clientConfig.Service.Name,
-							Path:      &conversionWebhookPath,
-							Port:      &conversionWebhookPort,
-						},
-						CABundle: clientConfig.CABundle,
+		// Override Name, Namespace, and CABundle
+		crd.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{
+			Strategy: "Webhook",
+			Webhook: &apiextensionsv1.WebhookConversion{
+				ClientConfig: &apiextensionsv1.WebhookClientConfig{
+					Service: &apiextensionsv1.ServiceReference{
+						Namespace: i.owner.GetNamespace(),
+						Name:      desc.DomainName() + "-service",
+						Path:      &conversionWebhookPath,
+						Port:      &desc.ContainerPort,
 					},
-					ConversionReviewVersions: conversionWebhookConversionReviewVersions,
+					CABundle: caPEM,
 				},
-			}
+				ConversionReviewVersions: desc.AdmissionReviewVersions,
+			},
+		}
 
-			// update CRD conversion Specs
-			if _, err = i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); err != nil {
-				log.Infof("CRD %s could not be updated, error: %s", ConversionCRD, err.Error())
-			}
+		// update CRD conversion Specs
+		if _, err = i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); err != nil {
+			return fmt.Errorf("Error updating CRD with Conversion info: %v", err)
 		}
 	}
 

diff --git a/pkg/controller/operators/olm/apiservices.go b/pkg/controller/operators/olm/apiservices.go
@@ -553,9 +553,22 @@ func (a *Operator) areWebhooksAvailable(csv *v1alpha1.ClusterServiceVersion) (bo
 				return false, err
 			}
 			webhookCount = len(webhookList.Items)
+		case v1alpha1.ConversionWebhook:
+			for _, conversionCRD := range desc.ConversionCRDs {
+				// check if CRD exists on cluster
+				crd, err := a.opClient.ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{})
+				if err != nil {
+					log.Infof("CRD not found %v, error: %s", desc, err.Error())
+					return false, err
+				}
+
+				if crd.Spec.Conversion.Strategy != "Webhook" || crd.Spec.Conversion.Webhook == nil || crd.Spec.Conversion.Webhook.ClientConfig == nil && crd.Spec.Conversion.Webhook.ClientConfig.CABundle == nil {
+					return false, fmt.Errorf("ConversionWebhook not ready")
+				}
+				webhookCount++
+			}
 		}
 		if webhookCount == 0 {
-			a.logger.Info("Expected Webhook does not exist")
 			return false, nil
 		}
 	}

diff --git a/pkg/package-server/client/openapi/zz_generated.openapi.go b/pkg/package-server/client/openapi/zz_generated.openapi.go