Add conversion webhook definition to csv

This PR adds the conversion webhook definition to csv. The csv generator includes
the logic to verfiy if the {v1, v1beta1} crd has conversion clinet config defined.
If so, it scaffolds the conversion wh configuration defails in the csv

Signed-off-by: varshaprasad96 <[email protected]>

Author: varshaprasad96

internal/generate/clusterserviceversion/clusterserviceversion_test.go

@@ -252,6 +252,10 @@ var _ = Describe("Generating a ClusterServiceVersion", func() {
 				csv, err := g.generate()
 				Expect(err).ToNot(HaveOccurred())
 				Expect(csv).To(Equal(updatedCSV))
+
+				// verify if conversion webhooks are added
+				Expect(len(csv.Spec.WebhookDefinitions)).NotTo(Equal(0))
+				Expect(containsConversionWebhookDefinition(csv.Spec.WebhookDefinitions)).To(BeTrue())
 			})
 		})
 
@@ -395,3 +399,12 @@ func upgradeCSV(csv *v1alpha1.ClusterServiceVersion, name, version string) *v1al
 
 	return upgraded
 }
+
+func containsConversionWebhookDefinition(whdef []v1alpha1.WebhookDescription) bool {
+	for _, def := range whdef {
+		if def.Type == v1alpha1.ConversionWebhook {
+			return true
+		}
+	}
+	return false
+}

internal/generate/clusterserviceversion/clusterserviceversion_updaters.go

@@ -18,6 +18,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"sort"
 	"strings"
 
 	operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
@@ -28,11 +29,19 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
+	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 
 	"github.com/operator-framework/operator-sdk/internal/generate/collector"
 	"github.com/operator-framework/operator-sdk/internal/util/k8sutil"
 )
 
+// serviceportPath is refers to the group of webhook service and
+// path names and port.
+type serviceportPath struct {
+	Port *int32
+	Path string
+}
+
 // ApplyTo applies relevant manifests in c to csv, sorts the applied updates,
 // and validates the result.
 func ApplyTo(c *collector.Manifests, csv *operatorsv1alpha1.ClusterServiceVersion) error {
@@ -295,9 +304,150 @@ func applyWebhooks(c *collector.Manifests, csv *operatorsv1alpha1.ClusterService
 		}
 		webhookDescriptions = append(webhookDescriptions, mutatingToWebhookDescription(webhook, depName, svc))
 	}
+
+	for _, svc := range c.Services {
+		crdToConfigMap := getConvWebhookCRDNamesAndConfig(c, svc.GetName())
+
+		if len(crdToConfigMap) != 0 {
+			depName := findMatchingDepNameFromService(c, &svc)
+			des, err := conversionToWebhookDescription(crdToConfigMap, depName, &svc)
+			// not sure if we should exit here, is it reasonable to just log it and continue with
+			// the next step?
+			if err != nil {
+				log.Fatal(err)
+			}
+			webhookDescriptions = append(webhookDescriptions, des...)
+		}
+	}
 	csv.Spec.WebhookDefinitions = webhookDescriptions
 }
 
+// conversionToWebhookDescription takes in a map of {crdNames, apiextv.WebhookConversion} and groups
+// all the crds with same port and path. It then creates a webhook description for each unique combination of
+// port and path.
+// For example: if we have the following map: {crd1:[portX+pathX], crd2: [portX+pathX], crd3: [portY:partY]},
+// we will create 2 webhook descriptions: one with [portX+pathX]:[crd1, crd2] and the other with [portY:pathY]:[crd3]
+func conversionToWebhookDescription(crdToConfig map[string]apiextv1.WebhookConversion, depName string, ws *corev1.Service) ([]operatorsv1alpha1.WebhookDescription, error) {
+	des := make([]operatorsv1alpha1.WebhookDescription, 0)
+
+	// this is a map of serviceportAndPath configs, and the respective CRDs.
+	webhookDescriptions := crdGroups(crdToConfig)
+
+	for serviceConfig, crds := range webhookDescriptions {
+		// we need this to get the conversionReviewVersions.
+		// here, we assume all crds having same servicePortAndPath config will have
+		// same conversion review versions.
+		// this should technically never error.
+		config, ok := crdToConfig[crds[0]]
+		if !ok {
+			return nil, fmt.Errorf("webhook config for crd %q not found", crds[0])
+		}
+
+		description := operatorsv1alpha1.WebhookDescription{
+			Type:                    operatorsv1alpha1.ConversionWebhook,
+			ConversionCRDs:          crds,
+			AdmissionReviewVersions: config.ConversionReviewVersions,
+			WebhookPath:             &serviceConfig.Path,
+			DeploymentName:          depName,
+			GenerateName:            getGenerateName(crds),
+			SideEffects: func() *admissionregv1.SideEffectClass {
+				seNone := admissionregv1.SideEffectClassNone
+				return &seNone
+			}(),
+		}
+
+		if len(description.AdmissionReviewVersions) == 0 {
+			log.Infof("conversionReviewVersion not found for the deployment %q", depName)
+		}
+
+		var webhookServiceRefPort int32 = 443
+
+		if serviceConfig.Port != nil {
+			webhookServiceRefPort = *serviceConfig.Port
+		}
+
+		if ws != nil {
+			for _, port := range ws.Spec.Ports {
+				if webhookServiceRefPort == port.Port {
+					description.ContainerPort = port.Port
+					description.TargetPort = &port.TargetPort
+					break
+				}
+			}
+		}
+
+		if description.DeploymentName == "" {
+			if config.ClientConfig.Service != nil {
+				description.DeploymentName = strings.TrimSuffix(config.ClientConfig.Service.Name, "-service")
+			}
+		}
+
+		description.WebhookPath = &serviceConfig.Path
+		des = append(des, description)
+	}
+
+	return des, nil
+}
+
+// crdGroups groups the crds with similar service port and name. It returns a map of serviceportPath
+// and the corresponding crd names.
+func crdGroups(crdToConfig map[string]apiextv1.WebhookConversion) map[serviceportPath][]string {
+
+	uniqueConfig := make(map[serviceportPath][]string)
+
+	for crdName, config := range crdToConfig {
+		serviceportPath := serviceportPath{
+			Port: config.ClientConfig.Service.Port,
+			Path: *config.ClientConfig.Service.Path,
+		}
+
+		if _, ok := uniqueConfig[serviceportPath]; !ok {
+			uniqueConfig[serviceportPath] = make([]string, 0)
+		}
+
+		uniqueConfig[serviceportPath] = append(uniqueConfig[serviceportPath], crdName)
+	}
+
+	return uniqueConfig
+}
+
+func getConvWebhookCRDNamesAndConfig(c *collector.Manifests, serviceName string) map[string]apiextv1.WebhookConversion {
+	if serviceName == "" {
+		return nil
+	}
+
+	crdToConfig := make(map[string]apiextv1.WebhookConversion)
+
+	for _, crd := range c.V1CustomResourceDefinitions {
+		if crd.Spec.Conversion != nil {
+			whConv := crd.Spec.Conversion.Webhook
+			if whConv != nil && whConv.ClientConfig != nil && whConv.ClientConfig.Service != nil {
+				if whConv.ClientConfig.Service.Name == serviceName {
+					crdToConfig[crd.GetName()] = *whConv
+				}
+			}
+		}
+	}
+
+	for _, crd := range c.V1beta1CustomResourceDefinitions {
+		whConv := crd.Spec.Conversion
+		if whConv != nil && whConv.WebhookClientConfig != nil && whConv.WebhookClientConfig.Service != nil {
+			if whConv.WebhookClientConfig.Service.Name == serviceName {
+				v1whConv := apiextv1.WebhookConversion{
+					ClientConfig:             &apiextv1.WebhookClientConfig{Service: &apiextv1.ServiceReference{}},
+					ConversionReviewVersions: crd.Spec.Conversion.ConversionReviewVersions,
+				}
+				if path := whConv.WebhookClientConfig.Service.Path; path != nil {
+					v1whConv.ClientConfig.Service.Path = new(string)
+					*v1whConv.ClientConfig.Service.Path = *path
+				}
+				crdToConfig[crd.GetName()] = v1whConv
+			}
+		}
+	}
+	return crdToConfig
+}
+
 // The default AdmissionReviewVersions set in a CSV if not set in the source webhook.
 var defaultAdmissionReviewVersions = []string{"v1beta1"}
 
@@ -391,8 +541,7 @@ func mutatingToWebhookDescription(webhook admissionregv1.MutatingWebhook, depNam
 }
 
 // findMatchingDeploymentAndServiceForWebhook matches a Service to a webhook's client config (if it uses a service)
-// then matches that Service to a Deployment by comparing label selectors (if the Service uses label selectors).
-// The names of both Service and Deployment are returned if found.
+// and uses that service to find the deployment name.
 func findMatchingDeploymentAndServiceForWebhook(c *collector.Manifests, wcc admissionregv1.WebhookClientConfig) (depName string, ws *corev1.Service) {
 	// Return if a service reference is not specified, since a URL will be in that case.
 	if wcc.Service == nil {
@@ -422,15 +571,23 @@ func findMatchingDeploymentAndServiceForWebhook(c *collector.Manifests, wcc admi
 		return
 	}
 
-	// Match service against pod labels, in which the webhook server will be running.
+	depName = findMatchingDepNameFromService(c, ws)
+
+	return depName, ws
+}
+
+// findMatchingDepNameFromService matches the provided service to a deployment by comparing label selectors (if
+// Service uses label selectors).
+func findMatchingDepNameFromService(c *collector.Manifests, ws *corev1.Service) (depName string) {
+	// Match service against pod labels, in which the webhook server will be running
 	for _, dep := range c.Deployments {
 		podTemplateLabels := dep.Spec.Template.GetLabels()
 		if len(podTemplateLabels) == 0 {
 			continue
 		}
 
 		depName = dep.GetName()
-		// Check that all labels match.
+		// Check that all labels match
 		for key, serviceValue := range ws.Spec.Selector {
 			if podTemplateValue, hasKey := podTemplateLabels[key]; !hasKey || podTemplateValue != serviceValue {
 				depName = ""
@@ -441,8 +598,7 @@ func findMatchingDeploymentAndServiceForWebhook(c *collector.Manifests, wcc admi
 			break
 		}
 	}
-
-	return depName, ws
+	return depName
 }
 
 // applyCustomResources updates csv's "alm-examples" annotation with the
@@ -495,3 +651,16 @@ func validate(csv *operatorsv1alpha1.ClusterServiceVersion) error {
 
 	return nil
 }
+
+// generateName takes in a list of crds, and returns a conversion webhook generator name.
+func getGenerateName(crds []string) string {
+	sort.Strings(crds)
+	joinedResourceNames := strings.Builder{}
+
+	for _, name := range crds {
+		if name != "" {
+			joinedResourceNames.WriteString(strings.Split(name, ".")[0])
+		}
+	}
+	return fmt.Sprintf("c%s.kb.io", joinedResourceNames.String())
+}

internal/generate/clusterserviceversion/clusterserviceversion_updaters_test.go

@@ -332,6 +332,49 @@ var _ = Describe("findMatchingDeploymentAndServiceForWebhook", func() {
 			Expect(service.GetName()).To(Equal(serviceName1))
 		})
 	})
+
+	Context("match webhookconfigs with same service and path", func() {
+		path1 := "/whPath"
+		port1 := new(int32)
+		*port1 = 2311
+		crdToConfigPath := map[string]apiextv1.WebhookConversion{
+			"crd-test-1": apiextv1.WebhookConversion{
+				ClientConfig: &apiextv1.WebhookClientConfig{
+					Service: &apiextv1.ServiceReference{
+						Path: &path1,
+						Port: port1,
+					},
+				},
+			},
+
+			"crd-test-2": apiextv1.WebhookConversion{
+				ClientConfig: &apiextv1.WebhookClientConfig{
+					Service: &apiextv1.ServiceReference{
+						Path: &path1,
+						Port: port1,
+					},
+				},
+			},
+		}
+
+		val := crdGroups(crdToConfigPath)
+
+		Expect(len(val)).To(BeEquivalentTo(1))
+
+		test := serviceportPath{
+			Port: port1,
+			Path: path1,
+		}
+
+		g := val[test]
+
+		Expect(g).NotTo(BeNil())
+		Expect(len(g)).To(BeEquivalentTo(2))
+		Expect(g).To(ContainElement("crd-test-2"))
+		Expect(g).To(ContainElement("crd-test-1"))
+
+	})
+
 })
 
 func newDeployment(name string, labels map[string]string) (dep appsv1.Deployment) {

internal/generate/testdata/clusterserviceversions/output/memcached-operator.clusterserviceversion.yaml

@@ -197,3 +197,14 @@ spec:
     targetPort: 9443
     type: MutatingAdmissionWebhook
     webhookPath: /mutate-cache-my-domain-v1alpha1-memcached
+  - admissionReviewVersions:
+    - v1beta1
+    containerPort: 443
+    conversionCRDs:
+    - memcacheds.cache.example.com
+    deploymentName: memcached-operator-controller-manager
+    generateName: cmemcacheds.kb.io
+    sideEffects: None
+    targetPort: 9443
+    type: ConversionWebhook
+    webhookPath: /convert

internal/generate/testdata/clusterserviceversions/output/with-ui-metadata.clusterserviceversion.yaml

@@ -253,3 +253,14 @@ spec:
     targetPort: 9443
     type: MutatingAdmissionWebhook
     webhookPath: /mutate-cache-my-domain-v1alpha1-memcached
+  - admissionReviewVersions:
+    - v1beta1
+    containerPort: 443
+    conversionCRDs:
+    - memcacheds.cache.example.com
+    deploymentName: memcached-operator-controller-manager
+    generateName: cmemcacheds.kb.io
+    sideEffects: None
+    targetPort: 9443
+    type: ConversionWebhook
+    webhookPath: /convert

internal/generate/testdata/go/static/basic.operator.yaml

@@ -13,6 +13,16 @@ metadata:
   creationTimestamp: null
   name: memcacheds.cache.example.com
 spec:
+  conversion:
+      strategy: Webhook
+      webhookClientConfig:
+        caBundle: Cg==
+        service:
+          name: memcached-operator-webhook-service
+          namespace: memcached-operator-system
+          path: /convert
+      conversionReviewVersions:
+      - v1beta1
   group: cache.example.com
   names:
     kind: Memcached