Merge pull request #4286 from camilamacedo86/e2e-test-go-v4-conversion

🐛 (go/v4) Fix issue with scaffolding multiple webhooks for the same resource

Author: k8s-ci-robot

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())