Fix issue with scaffolding multiple webhooks for the same resource

Previously, attempting to create multiple webhooks for the same resource with different configurations (e.g., `--defaulting` and `--programmatic-validation`) resulted in an error indicating that the webhook already exists.

This update allows multiple webhook types to be scaffolded for a single resource without conflict, providing flexibility for users be able to scaffold different types of webhooks for the same GKV afterwords and without the need to be all by once.

Author: camilamacedo86

pkg/plugins/golang/v4/scaffolds/internal/templates/webhooks/webhook.go

@@ -81,7 +81,7 @@ func (f *Webhook) SetTemplateDefaults() error {
 	if f.Force {
 		f.IfExistsAction = machinery.OverwriteFile
 	} else {
-		f.IfExistsAction = machinery.Error
+		f.IfExistsAction = machinery.SkipFile
 	}
 
 	f.AdmissionReviewVersions = "v1"

pkg/plugins/golang/v4/webhook.go

@@ -119,9 +119,12 @@ func (p *createWebhookSubcommand) InjectResource(res *resource.Resource) error {
 		return err
 	}
 
-	if !p.resource.HasDefaultingWebhook() && !p.resource.HasValidationWebhook() && !p.resource.HasConversionWebhook() {
-		return fmt.Errorf("%s create webhook requires at least one of --defaulting,"+
-			" --programmatic-validation and --conversion to be true", p.commandName)
+	// Ensure at least one webhook type is specified
+	if !p.resource.HasDefaultingWebhook() &&
+		!p.resource.HasValidationWebhook() &&
+		!p.resource.HasConversionWebhook() {
+		return fmt.Errorf("%s create webhook requires at least one of --defaulting, --programmatic-validation, "+
+			"and --conversion to be true", p.commandName)
 	}
 
 	// check if resource exist to create webhook

test/e2e/v4/generate_test.go

@@ -18,6 +18,7 @@ package v4
 
 import (
 	"fmt"
+	"os"
 	"path/filepath"
 	"strings"
 
@@ -55,6 +56,8 @@ func GenerateV4(kbc *utils.TestContext) {
 	err = utils.ImplementWebhooks(webhookFilePath, strings.ToLower(kbc.Kind))
 	ExpectWithOffset(1, err).NotTo(HaveOccurred())
 
+	scaffoldConversionWebhook(kbc)
+
 	ExpectWithOffset(1, pluginutil.UncommentCode(
 		filepath.Join(kbc.Dir, "config", "default", "kustomization.yaml"),
 		"#- ../certmanager", "#")).To(Succeed())
@@ -92,6 +95,8 @@ func GenerateV4WithoutMetrics(kbc *utils.TestContext) {
 	err = utils.ImplementWebhooks(webhookFilePath, strings.ToLower(kbc.Kind))
 	ExpectWithOffset(1, err).NotTo(HaveOccurred())
 
+	scaffoldConversionWebhook(kbc)
+
 	ExpectWithOffset(1, pluginutil.UncommentCode(
 		filepath.Join(kbc.Dir, "config", "default", "kustomization.yaml"),
 		"#- ../certmanager", "#")).To(Succeed())
@@ -153,6 +158,8 @@ func GenerateV4WithNetworkPolicies(kbc *utils.TestContext) {
 	err = utils.ImplementWebhooks(webhookFilePath, strings.ToLower(kbc.Kind))
 	ExpectWithOffset(1, err).NotTo(HaveOccurred())
 
+	scaffoldConversionWebhook(kbc)
+
 	ExpectWithOffset(1, pluginutil.UncommentCode(
 		filepath.Join(kbc.Dir, "config", "default", "kustomization.yaml"),
 		"#- ../certmanager", "#")).To(Succeed())
@@ -368,3 +375,114 @@ func uncommentPodStandards(kbc *utils.TestContext) {
 		ExpectWithOffset(1, err).NotTo(HaveOccurred())
 	}
 }
+
+// scaffoldConversionWebhook sets up conversion webhooks for testing the ConversionTest API
+func scaffoldConversionWebhook(kbc *utils.TestContext) {
+	By("scaffolding conversion webhooks for testing ConversionTest v1 to v2 conversion")
+
+	// Create API for v1 (hub) with conversion enabled
+	err := kbc.CreateAPI(
+		"--group", kbc.Group,
+		"--version", "v1",
+		"--kind", "ConversionTest",
+		"--controller=true",
+		"--resource=true",
+		"--make=false",
+	)
+	ExpectWithOffset(1, err).NotTo(HaveOccurred(), "failed to create v1 API for conversion testing")
+
+	// Create API for v2 (spoke) without a controller
+	err = kbc.CreateAPI(
+		"--group", kbc.Group,
+		"--version", "v2",
+		"--kind", "ConversionTest",
+		"--controller=false",
+		"--resource=true",
+		"--make=false",
+	)
+	ExpectWithOffset(1, err).NotTo(HaveOccurred(), "failed to create v2 API for conversion testing")
+
+	// Create the conversion webhook for v1
+	By("setting up the conversion webhook for v1")
+	err = kbc.CreateWebhook(
+		"--group", kbc.Group,
+		"--version", "v1",
+		"--kind", "ConversionTest",
+		"--conversion",
+		"--make=false",
+	)
+	ExpectWithOffset(1, err).NotTo(HaveOccurred(), "failed to create conversion webhook for v1")
+
+	// Insert Size field in v1
+	By("implementing the size spec in v1")
+	ExpectWithOffset(1, pluginutil.InsertCode(
+		filepath.Join(kbc.Dir, "api", "v1", "conversiontest_types.go"),
+		"Foo string `json:\"foo,omitempty\"`",
+		"\n\tSize int `json:\"size,omitempty\"` // Number of desired instances",
+	)).NotTo(HaveOccurred(), "failed to add size spec to conversiontest_types v1")
+
+	// Insert Replicas field in v2
+	By("implementing the replicas spec in v2")
+	ExpectWithOffset(1, pluginutil.InsertCode(
+		filepath.Join(kbc.Dir, "api", "v2", "conversiontest_types.go"),
+		"Foo string `json:\"foo,omitempty\"`",
+		"\n\tReplicas int `json:\"replicas,omitempty\"` // Number of replicas",
+	)).NotTo(HaveOccurred(), "failed to add replicas spec to conversiontest_types v2")
+
+	// TODO: Remove the code bellow when we have hub and spoke scaffolded by
+	// Kubebuilder. Intead of create the file we will replace the TODO(user)
+	// with the code implementation.
+	By("implementing markers")
+	ExpectWithOffset(1, pluginutil.InsertCode(
+		filepath.Join(kbc.Dir, "api", "v1", "conversiontest_types.go"),
+		"// +kubebuilder:object:root=true\n// +kubebuilder:subresource:status",
+		"\n// +kubebuilder:storageversion\n// +kubebuilder:conversion:hub\n",
+	)).NotTo(HaveOccurred(), "failed to add markers to conversiontest_types v1")
+
+	// Create the hub conversion file in v1
+	By("creating the conversion implementation in v1 as hub")
+	err = os.WriteFile(filepath.Join(kbc.Dir, "api", "v1", "conversiontest_conversion.go"), []byte(`
+package v1
+
+// ConversionTest defines the hub conversion logic.
+// Implement the Hub interface to signal that v1 is the hub version.
+func (*ConversionTest) Hub() {}
+`), 0644)
+	ExpectWithOffset(1, err).NotTo(HaveOccurred(), "failed to create hub conversion file in v1")
+
+	// Create the conversion file in v2
+	By("creating the conversion implementation in v2")
+	err = os.WriteFile(filepath.Join(kbc.Dir, "api", "v2", "conversiontest_conversion.go"), []byte(`
+package v2
+
+import (
+	"log"
+
+	"sigs.k8s.io/controller-runtime/pkg/conversion"
+	v1 "sigs.k8s.io/kubebuilder/v4/api/v1"
+)
+
+// ConvertTo converts this ConversionTest to the Hub version (v1).
+func (src *ConversionTest) ConvertTo(dstRaw conversion.Hub) error {
+	dst := dstRaw.(*v1.ConversionTest)
+	log.Printf("Converting from %T to %T", src.APIVersion, dst.APIVersion)
+
+	// Implement conversion logic from v2 to v1
+	dst.Spec.Size = src.Spec.Replicas // Convert replicas in v2 to size in v1
+
+	return nil
+}
+
+// ConvertFrom converts the Hub version (v1) to this ConversionTest (v2).
+func (dst *ConversionTest) ConvertFrom(srcRaw conversion.Hub) error {
+	src := srcRaw.(*v1.ConversionTest)
+	log.Printf("Converting from %T to %T", src.APIVersion, dst.APIVersion)
+
+	// Implement conversion logic from v1 to v2
+	dst.Spec.Replicas = src.Spec.Size // Convert size in v1 to replicas in v2
+
+	return nil
+}
+`), 0644)
+	ExpectWithOffset(1, err).NotTo(HaveOccurred(), "failed to create conversion file in v2")
+}

test/e2e/v4/plugin_cluster_test.go

@@ -262,6 +262,27 @@ func Run(kbc *utils.TestContext, hasWebhook, isToUseInstaller, hasMetrics bool,
 			return nil
 		}
 		EventuallyWithOffset(1, verifyCAInjection, time.Minute, time.Second).Should(Succeed())
+
+		By("validating that the CA injection is applied for CRD conversion")
+		crdKind := "ConversionTest"
+		verifyCAInjection = func() error {
+			crdOutput, err := kbc.Kubectl.Get(
+				false,
+				"customresourcedefinition.apiextensions.k8s.io",
+				"-o", fmt.Sprintf(
+					"jsonpath={.items[?(@.spec.names.kind=='%s')].spec.conversion.webhook.clientConfig.caBundle}",
+					crdKind),
+			)
+			ExpectWithOffset(1, err).NotTo(HaveOccurred(),
+				"failed to get CRD conversion webhook configuration")
+
+			// Check if the CA bundle is populated (length > 10 to avoid placeholder values)
+			ExpectWithOffset(1, len(crdOutput)).To(BeNumerically(">", 10),
+				"CA bundle should be injected into the CRD")
+			return nil
+		}
+		EventuallyWithOffset(1, verifyCAInjection, time.Minute, time.Second).Should(Succeed(),
+			"CA injection validation failed")
 	}
 
 	By("creating an instance of the CR")
@@ -341,6 +362,41 @@ func Run(kbc *utils.TestContext, hasWebhook, isToUseInstaller, hasMetrics bool,
 		By("removing the namespace")
 		_, err = kbc.Kubectl.Command("delete", "namespace", namespace)
 		Expect(err).NotTo(HaveOccurred(), "namespace should be removed successfully")
+
+		By("validating the conversion")
+
+		// Update the ConversionTest CR sample in v1 to set a specific `size`
+		By("modifying the ConversionTest CR sample to set `size` for conversion testing")
+		conversionCRFile := filepath.Join("config", "samples",
+			fmt.Sprintf("%s_v1_conversiontest.yaml", kbc.Group))
+		conversionCRPath, err := filepath.Abs(filepath.Join(fmt.Sprintf("e2e-%s", kbc.TestSuffix), conversionCRFile))
+		Expect(err).To(Not(HaveOccurred()))
+
+		// Edit the file to include `size` in the spec field for v1
+		f, err := os.OpenFile(conversionCRPath, os.O_APPEND|os.O_WRONLY, 0o644)
+		Expect(err).To(Not(HaveOccurred()))
+		defer func() {
+			err = f.Close()
+			Expect(err).To(Not(HaveOccurred()))
+		}()
+		_, err = f.WriteString("\nspec:\n  size: 3")
+		Expect(err).To(Not(HaveOccurred()))
+
+		// Apply the ConversionTest Custom Resource in v1
+		By("applying the modified ConversionTest CR in v1 for conversion")
+		_, err = kbc.Kubectl.Apply(true, "-f", conversionCRPath)
+		ExpectWithOffset(1, err).NotTo(HaveOccurred(), "failed to apply modified ConversionTest CR")
+
+		// TODO: Add validation to check the conversion
+		// the v2 should have spec.replicas == 3
+
+		if hasMetrics {
+			By("validating conversion metrics to confirm conversion operations")
+			metricsOutput := getMetricsOutput(kbc)
+			conversionMetric := `controller_runtime_reconcile_total{controller="conversiontest",result="success"} 1`
+			ExpectWithOffset(1, metricsOutput).To(ContainSubstring(conversionMetric),
+				"Expected metric for successful ConversionTest reconciliation")
+		}
 	}
 }
 
@@ -384,10 +440,19 @@ func getControllerName(kbc *utils.TestContext) string {
 // getMetricsOutput return the metrics output from curl pod
 func getMetricsOutput(kbc *utils.TestContext) string {
 	_, err := kbc.Kubectl.Command(
-		"create", "clusterrolebinding", fmt.Sprintf("metrics-%s", kbc.TestSuffix),
-		fmt.Sprintf("--clusterrole=e2e-%s-metrics-reader", kbc.TestSuffix),
-		fmt.Sprintf("--serviceaccount=%s:%s", kbc.Kubectl.Namespace, kbc.Kubectl.ServiceAccount))
-	ExpectWithOffset(1, err).NotTo(HaveOccurred())
+		"get", "clusterrolebinding", fmt.Sprintf("metrics-%s", kbc.TestSuffix),
+	)
+	if err != nil && strings.Contains(err.Error(), "NotFound") {
+		// Create the clusterrolebinding only if it doesn't exist
+		_, err = kbc.Kubectl.Command(
+			"create", "clusterrolebinding", fmt.Sprintf("metrics-%s", kbc.TestSuffix),
+			fmt.Sprintf("--clusterrole=e2e-%s-metrics-reader", kbc.TestSuffix),
+			fmt.Sprintf("--serviceaccount=%s:%s", kbc.Kubectl.Namespace, kbc.Kubectl.ServiceAccount),
+		)
+		ExpectWithOffset(1, err).NotTo(HaveOccurred())
+	} else {
+		ExpectWithOffset(1, err).NotTo(HaveOccurred(), "Failed to check clusterrolebinding existence")
+	}
 
 	token, err := serviceAccountToken(kbc)
 	ExpectWithOffset(2, err).NotTo(HaveOccurred())