Merge pull request #1002 from davidxia/webhook-name

✨ Allow customizing generated webhook configuration's name

Author: k8s-ci-robot

pkg/webhook/parser.go

@@ -31,6 +31,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-tools/pkg/genall"
 	"sigs.k8s.io/controller-tools/pkg/markers"
 )
@@ -42,16 +43,31 @@ const (
 )
 
 var (
-	// ConfigDefinition s a marker for defining Webhook manifests.
+	// ConfigDefinition is a marker for defining Webhook manifests.
 	// Call ToWebhook on the value to get a Kubernetes Webhook.
 	ConfigDefinition = markers.Must(markers.MakeDefinition("kubebuilder:webhook", markers.DescribesPackage, Config{}))
+	// WebhookConfigDefinition is a marker for defining MutatingWebhookConfiguration or ValidatingWebhookConfiguration manifests.
+	WebhookConfigDefinition = markers.Must(markers.MakeDefinition("kubebuilder:webhookconfiguration", markers.DescribesPackage, WebhookConfig{}))
 )
 
 // supportedWebhookVersions returns currently supported API version of {Mutating,Validating}WebhookConfiguration.
 func supportedWebhookVersions() []string {
 	return []string{defaultWebhookVersion}
 }
 
+// +controllertools:marker:generateHelp
+
+type WebhookConfig struct {
+	// Mutating marks this as a mutating webhook (it's validating only if false)
+	//
+	// Mutating webhooks are allowed to change the object in their response,
+	// and are called *before* all validating webhooks.  Mutating webhooks may
+	// choose to reject an object, similarly to a validating webhook.
+	Mutating bool
+	// Name indicates the name of the K8s MutatingWebhookConfiguration or ValidatingWebhookConfiguration object.
+	Name string `marker:"name,optional"`
+}
+
 // +controllertools:marker:generateHelp:category=Webhook
 
 // Config specifies how a webhook should be served.
@@ -154,6 +170,32 @@ func verbToAPIVariant(verbRaw string) admissionregv1.OperationType {
 	}
 }
 
+// ToMutatingWebhookConfiguration converts this WebhookConfig to its Kubernetes API form.
+func (c WebhookConfig) ToMutatingWebhookConfiguration() (admissionregv1.MutatingWebhookConfiguration, error) {
+	if !c.Mutating {
+		return admissionregv1.MutatingWebhookConfiguration{}, fmt.Errorf("%s is a validating webhook", c.Name)
+	}
+
+	return admissionregv1.MutatingWebhookConfiguration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: c.Name,
+		},
+	}, nil
+}
+
+// ToValidatingWebhookConfiguration converts this WebhookConfig to its Kubernetes API form.
+func (c WebhookConfig) ToValidatingWebhookConfiguration() (admissionregv1.ValidatingWebhookConfiguration, error) {
+	if c.Mutating {
+		return admissionregv1.ValidatingWebhookConfiguration{}, fmt.Errorf("%s is a mutating webhook", c.Name)
+	}
+
+	return admissionregv1.ValidatingWebhookConfiguration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: c.Name,
+		},
+	}, nil
+}
+
 // ToMutatingWebhook converts this rule to its Kubernetes API form.
 func (c Config) ToMutatingWebhook() (admissionregv1.MutatingWebhook, error) {
 	if !c.Mutating {
@@ -362,20 +404,55 @@ func (Generator) RegisterMarkers(into *markers.Registry) error {
 	if err := into.Register(ConfigDefinition); err != nil {
 		return err
 	}
+	if err := into.Register(WebhookConfigDefinition); err != nil {
+		return err
+	}
 	into.AddHelp(ConfigDefinition, Config{}.Help())
+	into.AddHelp(WebhookConfigDefinition, Config{}.Help())
 	return nil
 }
 
 func (g Generator) Generate(ctx *genall.GenerationContext) error {
 	supportedWebhookVersions := supportedWebhookVersions()
 	mutatingCfgs := make(map[string][]admissionregv1.MutatingWebhook, len(supportedWebhookVersions))
 	validatingCfgs := make(map[string][]admissionregv1.ValidatingWebhook, len(supportedWebhookVersions))
+	var mutatingWebhookCfgs admissionregv1.MutatingWebhookConfiguration
+	var validatingWebhookCfgs admissionregv1.ValidatingWebhookConfiguration
+
 	for _, root := range ctx.Roots {
 		markerSet, err := markers.PackageMarkers(ctx.Collector, root)
 		if err != nil {
 			root.AddError(err)
 		}
 
+		webhookCfgs := markerSet[WebhookConfigDefinition.Name]
+		var hasValidatingWebhookConfig, hasMutatingWebhookConfig bool = false, false
+		for _, webhookCfg := range webhookCfgs {
+			webhookCfg := webhookCfg.(WebhookConfig)
+
+			if webhookCfg.Mutating {
+				if hasMutatingWebhookConfig {
+					return fmt.Errorf("duplicate mutating %s with name %s", WebhookConfigDefinition.Name, webhookCfg.Name)
+				}
+
+				if mutatingWebhookCfgs, err = webhookCfg.ToMutatingWebhookConfiguration(); err != nil {
+					return err
+				}
+
+				hasMutatingWebhookConfig = true
+			} else {
+				if hasValidatingWebhookConfig {
+					return fmt.Errorf("duplicate validating %s with name %s", WebhookConfigDefinition.Name, webhookCfg.Name)
+				}
+
+				if validatingWebhookCfgs, err = webhookCfg.ToValidatingWebhookConfiguration(); err != nil {
+					return err
+				}
+
+				hasValidatingWebhookConfig = true
+			}
+		}
+
 		cfgs := markerSet[ConfigDefinition.Name]
 		sort.SliceStable(cfgs, func(i, j int) bool {
 			return cfgs[i].(Config).Name < cfgs[j].(Config).Name
@@ -410,16 +487,22 @@ func (g Generator) Generate(ctx *genall.GenerationContext) error {
 	versionedWebhooks := make(map[string][]interface{}, len(supportedWebhookVersions))
 	for _, version := range supportedWebhookVersions {
 		if cfgs, ok := mutatingCfgs[version]; ok {
-			// The only possible version in supportedWebhookVersions is v1,
-			// so use it for all versioned types in this context.
-			objRaw := &admissionregv1.MutatingWebhookConfiguration{}
+			var objRaw *admissionregv1.MutatingWebhookConfiguration
+			if mutatingWebhookCfgs.Name != "" {
+				objRaw = &mutatingWebhookCfgs
+			} else {
+				// The only possible version in supportedWebhookVersions is v1,
+				// so use it for all versioned types in this context.
+				objRaw = &admissionregv1.MutatingWebhookConfiguration{}
+				objRaw.SetName("mutating-webhook-configuration")
+			}
 			objRaw.SetGroupVersionKind(schema.GroupVersionKind{
 				Group:   admissionregv1.SchemeGroupVersion.Group,
 				Version: version,
 				Kind:    "MutatingWebhookConfiguration",
 			})
-			objRaw.SetName("mutating-webhook-configuration")
 			objRaw.Webhooks = cfgs
+
 			for i := range objRaw.Webhooks {
 				// SideEffects is required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
 				// return an error
@@ -441,16 +524,22 @@ func (g Generator) Generate(ctx *genall.GenerationContext) error {
 		}
 
 		if cfgs, ok := validatingCfgs[version]; ok {
-			// The only possible version in supportedWebhookVersions is v1,
-			// so use it for all versioned types in this context.
-			objRaw := &admissionregv1.ValidatingWebhookConfiguration{}
+			var objRaw *admissionregv1.ValidatingWebhookConfiguration
+			if validatingWebhookCfgs.Name != "" {
+				objRaw = &validatingWebhookCfgs
+			} else {
+				// The only possible version in supportedWebhookVersions is v1,
+				// so use it for all versioned types in this context.
+				objRaw = &admissionregv1.ValidatingWebhookConfiguration{}
+				objRaw.SetName("validating-webhook-configuration")
+			}
 			objRaw.SetGroupVersionKind(schema.GroupVersionKind{
 				Group:   admissionregv1.SchemeGroupVersion.Group,
 				Version: version,
 				Kind:    "ValidatingWebhookConfiguration",
 			})
-			objRaw.SetName("validating-webhook-configuration")
 			objRaw.Webhooks = cfgs
+
 			for i := range objRaw.Webhooks {
 				// SideEffects is required in admissionregistration/v1, if this is not set or set to `Some` or `Known`,
 				// return an error

pkg/webhook/parser_integration_test.go

@@ -56,6 +56,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		By("setting up the parser")
 		reg := &markers.Registry{}
 		Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
+		Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())
 
 		By("requesting that the manifest be generated")
 		outputDir, err := os.MkdirTemp("", "webhook-integration-test")
@@ -85,6 +86,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		By("setting up the parser")
 		reg := &markers.Registry{}
 		Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
+		Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())
 
 		By("requesting that the manifest be generated")
 		outputDir, err := os.MkdirTemp("", "webhook-integration-test")
@@ -116,6 +118,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		By("setting up the parser")
 		reg := &markers.Registry{}
 		Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
+		Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())
 
 		By("requesting that the manifest be generated")
 		outputDir, err := os.MkdirTemp("", "webhook-integration-test")
@@ -145,6 +148,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		By("setting up the parser")
 		reg := &markers.Registry{}
 		Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
+		Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())
 
 		By("requesting that the manifest be generated")
 		outputDir, err := os.MkdirTemp("", "webhook-integration-test")
@@ -174,6 +178,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		By("setting up the parser")
 		reg := &markers.Registry{}
 		Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
+		Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())
 
 		By("requesting that the manifest be generated")
 		outputDir, err := os.MkdirTemp("", "webhook-integration-test")
@@ -219,6 +224,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		By("setting up the parser")
 		reg := &markers.Registry{}
 		Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
+		Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())
 
 		By("requesting that the manifest be generated")
 		outputDir, err := os.MkdirTemp("", "webhook-integration-test")
@@ -268,6 +274,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		By("setting up the parser")
 		reg := &markers.Registry{}
 		Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
+		Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())
 
 		By("requesting that the manifest be generated")
 		outputDir, err := os.MkdirTemp("", "webhook-integration-test")
@@ -313,6 +320,7 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		By("setting up the parser")
 		reg := &markers.Registry{}
 		Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
+		Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())
 
 		By("requesting that the manifest be generated")
 		outputDir, err := os.MkdirTemp("", "webhook-integration-test")
@@ -326,6 +334,83 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
 		err = webhook.Generator{}.Generate(genCtx)
 		Expect(err).To(HaveOccurred())
 	})
+
+	It("should properly generate the webhook definition when a name is specified with the `kubebuilder:webhookconfiguration` marker", func() {
+		By("switching into testdata to appease go modules")
+		cwd, err := os.Getwd()
+		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Chdir("./testdata/valid-custom-name")).To(Succeed()) // go modules are directory-sensitive
+		defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }()
+
+		By("loading the roots")
+		pkgs, err := loader.LoadRoots(".")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(pkgs).To(HaveLen(1))
+
+		By("setting up the parser")
+		reg := &markers.Registry{}
+		Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
+		Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())
+
+		By("requesting that the manifest be generated")
+		outputDir, err := os.MkdirTemp("", "webhook-integration-test")
+		Expect(err).NotTo(HaveOccurred())
+		defer os.RemoveAll(outputDir)
+		genCtx := &genall.GenerationContext{
+			Collector:  &markers.Collector{Registry: reg},
+			Roots:      pkgs,
+			OutputRule: genall.OutputToDirectory(outputDir),
+		}
+		Expect(webhook.Generator{}.Generate(genCtx)).To(Succeed())
+		for _, r := range genCtx.Roots {
+			Expect(r.Errors).To(HaveLen(0))
+		}
+
+		By("loading the generated v1 YAML")
+		actualFile, err := os.ReadFile(path.Join(outputDir, "manifests.yaml"))
+		Expect(err).NotTo(HaveOccurred())
+		actualMutating, actualValidating := unmarshalBothV1(actualFile)
+
+		By("loading the desired v1 YAML")
+		expectedFile, err := os.ReadFile("manifests.yaml")
+		Expect(err).NotTo(HaveOccurred())
+		expectedMutating, expectedValidating := unmarshalBothV1(expectedFile)
+
+		By("comparing the two")
+		assertSame(actualMutating, expectedMutating)
+		assertSame(actualValidating, expectedValidating)
+	})
+
+	It("should fail to generate when there are multiple `kubebuilder:webhookconfiguration` markers of the same mutation type", func() {
+		By("switching into testdata to appease go modules")
+		cwd, err := os.Getwd()
+		Expect(err).NotTo(HaveOccurred())
+		Expect(os.Chdir("./testdata/invalid-multiple-webhookconfigurations")).To(Succeed()) // go modules are directory-sensitive
+		defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }()
+
+		By("loading the roots")
+		pkgs, err := loader.LoadRoots(".")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(pkgs).To(HaveLen(1))
+
+		By("setting up the parser")
+		reg := &markers.Registry{}
+		Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
+		Expect(reg.Register(webhook.WebhookConfigDefinition)).To(Succeed())
+
+		By("requesting that the manifest be generated")
+		outputDir, err := os.MkdirTemp("", "webhook-integration-test")
+		Expect(err).NotTo(HaveOccurred())
+		defer os.RemoveAll(outputDir)
+		genCtx := &genall.GenerationContext{
+			Collector:  &markers.Collector{Registry: reg},
+			Roots:      pkgs,
+			OutputRule: genall.OutputToDirectory(outputDir),
+		}
+		err = webhook.Generator{}.Generate(genCtx)
+		Expect(err).To(HaveOccurred())
+	})
+
 })
 
 func unmarshalBothV1(in []byte) (mutating admissionregv1.MutatingWebhookConfiguration, validating admissionregv1.ValidatingWebhookConfiguration) {