cleanup the code a bit

Author: Mengqi Yu

pkg/webhook/admission.go

@@ -51,9 +51,9 @@ func (w *admissionWebhook) setDefaults() {
 			// can't do defaulting, skip it.
 			return
 		}
-		if w.t == webhookTypeMutating {
+		if w.t == mutatingWebhook {
 			w.path = "/mutate-" + w.rules[0].Resources[0]
-		} else if w.t == webhookTypeValidating {
+		} else if w.t == validatingWebhook {
 			w.path = "/validate-" + w.rules[0].Resources[0]
 		}
 	}
@@ -64,16 +64,7 @@ func (w *admissionWebhook) setDefaults() {
 	}
 }
 
-// GetName returns the name of the webhook.
-func (w *admissionWebhook) GetName() string {
-	return w.name
-}
-
-// GetPath returns the path that the webhook registered.
-func (w *admissionWebhook) GetPath() string {
-	w.setDefaults()
-	return w.path
-}
+var _ webhook = &admissionWebhook{}
 
 // GetType returns the type of the webhook.
 func (w *admissionWebhook) GetType() webhookType {
@@ -88,8 +79,8 @@ func (w *admissionWebhook) Validate() error {
 	if len(w.name) == 0 {
 		return errors.New("field name should not be empty")
 	}
-	if w.t != webhookTypeMutating && w.t != webhookTypeValidating {
-		return fmt.Errorf("unsupported Type: %v, only webhookTypeMutating and webhookTypeValidating are supported", w.t)
+	if w.t != mutatingWebhook && w.t != validatingWebhook {
+		return fmt.Errorf("unsupported Type: %v, only mutatingWebhook and validatingWebhook are supported", w.t)
 	}
 	if len(w.path) == 0 {
 		return errors.New("field path should not be empty")

pkg/webhook/generator.go

@@ -34,8 +34,8 @@ import (
 )
 
 type generatorOptions struct {
-	// registry maps a path to a http.Handler.
-	registry map[string]Webhook
+	// webhooks maps a path to a webhoook.
+	webhooks map[string]webhook
 
 	// port is the port number that the server will serve.
 	// It will be defaulted to 443 if unspecified.
@@ -51,17 +51,16 @@ type generatorOptions struct {
 
 	// secret is the location for storing the certificate for the admission server.
 	// The server should have permission to create a secret in the namespace.
-	// This is optional.
-	secret *apitypes.NamespacedName // nolint: structcheck
+	secret *apitypes.NamespacedName
 
 	// service is a k8s service fronting the webhook server pod(s).
-	// This field is optional. But one and only one of service and host need to be set.
+	// One and only one of service and host can be set.
 	// This maps to field .Webhooks.ClientConfig.Service
 	// https://github.com/kubernetes/api/blob/183f3326a9353bd6d41430fc80f96259331d029c/admissionregistration/v1beta1/types.go#L260
 	service *service
 	// host is the host name of .Webhooks.ClientConfig.URL
 	// https://github.com/kubernetes/api/blob/183f3326a9353bd6d41430fc80f96259331d029c/admissionregistration/v1beta1/types.go#L250
-	// This field is optional. But one and only one of service and host need to be set.
+	// One and only one of service and host can be set.
 	// If neither service nor host is unspecified, host will be defaulted to "localhost".
 	host *string
 }
@@ -79,8 +78,8 @@ type service struct {
 
 // setDefault does defaulting for the generatorOptions.
 func (o *generatorOptions) setDefault() {
-	if o.registry == nil {
-		o.registry = map[string]Webhook{}
+	if o.webhooks == nil {
+		o.webhooks = map[string]webhook{}
 	}
 	if o.port <= 0 {
 		o.port = 443
@@ -117,71 +116,16 @@ func (o *generatorOptions) Generate() ([]runtime.Object, error) {
 	return objects, nil
 }
 
-func (o *generatorOptions) getClientConfig() (*admissionregistration.WebhookClientConfig, error) {
-	if o.host != nil && o.service != nil {
-		return nil, errors.New("URL and service can't be set at the same time")
-	}
-	cc := &admissionregistration.WebhookClientConfig{
-		// Put an non-empty and not harmful CABundle here.
-		// Not doing this will cause the field
-		CABundle: []byte(`\n`),
-	}
-	if o.host != nil {
-		u := url.URL{
-			Scheme: "https",
-			Host:   net.JoinHostPort(*o.host, strconv.Itoa(int(o.port))),
-		}
-		urlString := u.String()
-		cc.URL = &urlString
-	}
-	if o.service != nil {
-		cc.Service = &admissionregistration.ServiceReference{
-			Name:      o.service.name,
-			Namespace: o.service.namespace,
-			// Path will be set later
-		}
-	}
-	return cc, nil
-}
-
-// getClientConfigWithPath constructs a WebhookClientConfig based on the server generatorOptions.
-// It will use path to the set the path in WebhookClientConfig.
-func (o *generatorOptions) getClientConfigWithPath(path string) (*admissionregistration.WebhookClientConfig, error) {
-	cc, err := o.getClientConfig()
-	if err != nil {
-		return nil, err
-	}
-	return cc, setPath(cc, path)
-}
-
-// setPath sets the path in the WebhookClientConfig.
-func setPath(cc *admissionregistration.WebhookClientConfig, path string) error {
-	if cc.URL != nil {
-		u, err := url.Parse(*cc.URL)
-		if err != nil {
-			return err
-		}
-		u.Path = path
-		urlString := u.String()
-		cc.URL = &urlString
-	}
-	if cc.Service != nil {
-		cc.Service.Path = &path
-	}
-	return nil
-}
-
-// whConfigs creates a mutatingWebhookConfiguration and(or) a validatingWebhookConfiguration based on registry.
-// For the same type of webhook configuration, it generates a webhook entry per endpoint.
+// whConfigs creates a mutatingWebhookConfiguration and(or) a validatingWebhookConfiguration.
 func (o *generatorOptions) whConfigs() ([]runtime.Object, error) {
-	for _, webhook := range o.registry {
+	for _, webhook := range o.webhooks {
 		if err := webhook.Validate(); err != nil {
 			return nil, err
 		}
 	}
 
 	objs := []runtime.Object{}
-	mutatingWH, err := o.mutatingWHConfigs()
+	mutatingWH, err := o.mutatingWHConfig()
 	if err != nil {
 		return nil, err
 	}
@@ -198,15 +142,16 @@ func (o *generatorOptions) whConfigs() ([]runtime.Object, error) {
 	return objs, nil
 }
 
-func (o *generatorOptions) mutatingWHConfigs() (runtime.Object, error) {
+// mutatingWHConfig creates mutatingWebhookConfiguration.
+func (o *generatorOptions) mutatingWHConfig() (runtime.Object, error) {
 	mutatingWebhooks := []v1beta1.Webhook{}
-	for path, webhook := range o.registry {
-		if webhook.GetType() != webhookTypeMutating {
+	for path, webhook := range o.webhooks {
+		if webhook.GetType() != mutatingWebhook {
 			continue
 		}
 
-		admissionWebhook := webhook.(*admissionWebhook)
-		wh, err := o.admissionWebhook(path, admissionWebhook)
+		aw := webhook.(*admissionWebhook)
+		wh, err := o.admissionWebhook(path, aw)
 		if err != nil {
 			return nil, err
 		}
@@ -226,10 +171,9 @@ func (o *generatorOptions) mutatingWHConfigs() (runtime.Object, error) {
 			ObjectMeta: metav1.ObjectMeta{
 				Name: o.mutatingWebhookConfigName,
 				Annotations: map[string]string{
-					// The format is "namespace/secret-name"
 					// This annotation will be understood by cert-manager.
 					// TODO(mengqiy): point to the section in kubebuilder book when everything is ready.
-					"alpha.admissionwebhook.kubebuilder.io/ca-secret-name": o.secret.String(),
+					"alpha.admissionwebhook.cert-manager.io": "true",
 				},
 			},
 			Webhooks: mutatingWebhooks,
@@ -240,9 +184,9 @@ func (o *generatorOptions) mutatingWHConfigs() (runtime.Object, error) {
 
 func (o *generatorOptions) validatingWHConfigs() (runtime.Object, error) {
 	validatingWebhooks := []v1beta1.Webhook{}
-	for path, webhook := range o.registry {
+	for path, webhook := range o.webhooks {
 		var aw *admissionWebhook
-		if webhook.GetType() != webhookTypeValidating {
+		if webhook.GetType() != validatingWebhook {
 			continue
 		}
 
@@ -267,10 +211,9 @@ func (o *generatorOptions) validatingWHConfigs() (runtime.Object, error) {
 			ObjectMeta: metav1.ObjectMeta{
 				Name: o.validatingWebhookConfigName,
 				Annotations: map[string]string{
-					// The format is "namespace/secret-name"
 					// This annotation will be understood by cert-manager.
 					// TODO(mengqiy): point to the section in kubebuilder book when everything is ready.
-					"alpha.admissionwebhook.kubebuilder.io/ca-secret-name": o.secret.String(),
+					"alpha.admissionwebhook.cert-manager.io": "true",
 				},
 			},
 			Webhooks: validatingWebhooks,
@@ -305,6 +248,60 @@ func (o *generatorOptions) admissionWebhook(path string, wh *admissionWebhook) (
 	return webhook, nil
 }
 
+// getClientConfigWithPath constructs a WebhookClientConfig based on the server generatorOptions.
+// It will use path to the set the path in WebhookClientConfig.
+func (o *generatorOptions) getClientConfigWithPath(path string) (*admissionregistration.WebhookClientConfig, error) {
+	cc, err := o.getClientConfig()
+	if err != nil {
+		return nil, err
+	}
+	return cc, setPath(cc, path)
+}
+
+func (o *generatorOptions) getClientConfig() (*admissionregistration.WebhookClientConfig, error) {
+	if o.host != nil && o.service != nil {
+		return nil, errors.New("URL and service can't be set at the same time")
+	}
+	cc := &admissionregistration.WebhookClientConfig{
+		// Put an non-empty and not harmful CABundle here.
+		// Not doing this will cause the field
+		CABundle: []byte(`\n`),
+	}
+	if o.host != nil {
+		u := url.URL{
+			Scheme: "https",
+			Host:   net.JoinHostPort(*o.host, strconv.Itoa(int(o.port))),
+		}
+		urlString := u.String()
+		cc.URL = &urlString
+	}
+	if o.service != nil {
+		cc.Service = &admissionregistration.ServiceReference{
+			Name:      o.service.name,
+			Namespace: o.service.namespace,
+			// Path will be set later
+		}
+	}
+	return cc, nil
+}
+
+// setPath sets the path in the WebhookClientConfig.
+func setPath(cc *admissionregistration.WebhookClientConfig, path string) error {
+	if cc.URL != nil {
+		u, err := url.Parse(*cc.URL)
+		if err != nil {
+			return err
+		}
+		u.Path = path
+		urlString := u.String()
+		cc.URL = &urlString
+	}
+	if cc.Service != nil {
+		cc.Service.Path = &path
+	}
+	return nil
+}
+
 // getService creates a corev1.Service object fronting the admission server.
 func (o *generatorOptions) getService() runtime.Object {
 	if o.service == nil {
@@ -322,7 +319,7 @@ func (o *generatorOptions) getService() runtime.Object {
 				// Secret here only need name, since it will be in the same namespace as the service.
 				// This annotation will be understood by cert-manager.
 				// TODO(mengqiy): point to the section in kubebuilder book when everything is ready.
-				"alpha.service.kubebuilder.io/serving-cert-secret-name": o.secret.Name,
+				"alpha.service.cert-manager.io/serving-cert-secret-name": o.secret.Name,
 			},
 		},
 		Spec: corev1.ServiceSpec{

pkg/webhook/generator_test.go

@@ -28,7 +28,7 @@ var expected = map[string]string{
 kind: MutatingWebhookConfiguration
 metadata:
   annotations:
-    alpha.admissionwebhook.kubebuilder.io/ca-secret-name: test-system/webhook-secret
+    alpha.admissionwebhook.cert-manager.io: "true"
   creationTimestamp: null
   name: test-mutating-webhook-cfg
 webhooks:
@@ -57,7 +57,7 @@ apiVersion: admissionregistration.k8s.io/v1beta1
 kind: ValidatingWebhookConfiguration
 metadata:
   annotations:
-    alpha.admissionwebhook.kubebuilder.io/ca-secret-name: test-system/webhook-secret
+    alpha.admissionwebhook.cert-manager.io: "true"
   creationTimestamp: null
   name: test-validating-webhook-cfg
 webhooks:
@@ -87,7 +87,7 @@ apiVersion: v1
 kind: Service
 metadata:
   annotations:
-    alpha.service.kubebuilder.io/serving-cert-secret-name: webhook-secret
+    alpha.service.cert-manager.io/serving-cert-secret-name: webhook-secret
   creationTimestamp: null
   name: webhook-service
   namespace: test-system

pkg/webhook/manifests.go

@@ -26,7 +26,7 @@ import (
 	"github.com/ghodss/yaml"
 	"github.com/spf13/afero"
 
-	generateinteral "sigs.k8s.io/controller-tools/pkg/internal/general"
+	"sigs.k8s.io/controller-tools/pkg/internal/general"
 )
 
 // Options represent options for generating the webhook manifests.
@@ -44,7 +44,7 @@ func Generate(o *Options) error {
 		return err
 	}
 
-	err := generateinteral.ParseDir(o.InputDir, o.parseAnnotation)
+	err := general.ParseDir(o.InputDir, o.parseAnnotation)
 	if err != nil {
 		return fmt.Errorf("failed to parse the input dir: %v", err)
 	}
@@ -54,7 +54,7 @@ func Generate(o *Options) error {
 		return err
 	}
 
-	err = o.writeObjectsToDisk(objs...)
+	err = o.WriteObjectsToDisk(objs...)
 	if err != nil {
 		return err
 	}

pkg/webhook/manifests_test.go

@@ -30,7 +30,7 @@ func TestManifestGenerator(t *testing.T) {
 	ignoreFP := admissionregistrationv1beta1.Ignore
 	tests := []struct {
 		content string
-		exp     map[string]Webhook
+		exp     map[string]webhook
 	}{
 		{
 			content: `package foo
@@ -54,10 +54,10 @@ func TestManifestGenerator(t *testing.T) {
 	func baz() {
 		fmt.Println(time.Now())
 	}`,
-			exp: map[string]Webhook{
+			exp: map[string]webhook{
 				"/bar": &admissionWebhook{
 					name: "bar-webhook",
-					t:    webhookTypeMutating,
+					t:    mutatingWebhook,
 					path: "/bar",
 					rules: []admissionregistrationv1beta1.RuleWithOperations{
 						{
@@ -75,7 +75,7 @@ func TestManifestGenerator(t *testing.T) {
 				},
 				"/baz": &admissionWebhook{
 					name: "baz-webhook",
-					t:    webhookTypeValidating,
+					t:    validatingWebhook,
 					path: "/baz",
 					rules: []admissionregistrationv1beta1.RuleWithOperations{
 						{
@@ -106,8 +106,8 @@ func TestManifestGenerator(t *testing.T) {
 		if err != nil {
 			t.Errorf("processFile should have succeeded, but got error: %v", err)
 		}
-		if !reflect.DeepEqual(test.exp, o.registry) {
-			t.Errorf("webhooks should have matched, expected %#v and got %#v", test.exp, o.registry)
+		if !reflect.DeepEqual(test.exp, o.webhooks) {
+			t.Errorf("webhooks should have matched, expected %#v and got %#v", test.exp, o.webhooks)
 		}
 	}
 }