From fd9c2f7457aef557aedade632bed7b6ab0724969 Mon Sep 17 00:00:00 2001 From: Tyler Slaton Date: Mon, 4 Apr 2022 17:24:01 -0400 Subject: [PATCH] fix(crdvalidator): address code review Signed-off-by: Tyler Slaton --- cmd/crdvalidator/internal/handlers/crd.go | 19 ++++++++++++------- cmd/crdvalidator/main.go | 7 ++++++- cmd/crdvalidator/manifests/00_namespace.yaml | 4 ++++ ...e_account.yaml => 01_service_account.yaml} | 2 +- .../manifests/02_cluster_role_binding.yaml | 2 +- cmd/crdvalidator/manifests/03_service.yaml | 2 +- cmd/crdvalidator/manifests/04_webhook.yaml | 13 +++++++------ cmd/crdvalidator/manifests/05_deployment.yaml | 2 +- 8 files changed, 33 insertions(+), 18 deletions(-) create mode 100644 cmd/crdvalidator/manifests/00_namespace.yaml rename cmd/crdvalidator/manifests/{00_service_account.yaml => 01_service_account.yaml} (70%) diff --git a/cmd/crdvalidator/internal/handlers/crd.go b/cmd/crdvalidator/internal/handlers/crd.go index e3403595..f781ab19 100644 --- a/cmd/crdvalidator/internal/handlers/crd.go +++ b/cmd/crdvalidator/internal/handlers/crd.go @@ -18,17 +18,19 @@ import ( "fmt" "net/http" + "github.com/go-logr/logr" "github.com/operator-framework/rukpak/cmd/crdvalidator/internal/crd" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) -// +kubebuilder:webhook:path=/validate-crd,mutating=false,failurePolicy=fail,groups="",resources=customresourcedefinitions,verbs=create;update,versions=v1,name=crd-validation-webhook.acme.com +// +kubebuilder:webhook:path=/validate-crd,mutating=false,failurePolicy=fail,groups="",resources=customresourcedefinitions,verbs=create;update,versions=v1,name=crd-validation-webhook.io // CrdValidator houses a client, decoder and Handle function for ensuring // that a CRD create/update request is safe type CrdValidator struct { + Log logr.Logger Client client.Client decoder *admission.Decoder } @@ -36,25 +38,28 @@ type CrdValidator struct { // Handle takes an incoming CRD create/update request and confirms that it is // a safe upgrade based on the crd.Validate() function call func (v *CrdValidator) Handle(ctx context.Context, req admission.Request) admission.Response { + log := v.Log.V(1).WithName("crdhandler") // Default to non-verbose logs incomingCrd := &apiextensionsv1.CustomResourceDefinition{} err := v.decoder.Decode(req, incomingCrd) if err != nil { - return admission.Errored(http.StatusBadRequest, err) + message := fmt.Sprintf("failed to decode CRD %q", req.Name) + log.V(0).Error(err, message) + return admission.Errored(http.StatusBadRequest, fmt.Errorf("%s: %w", message, err)) } err = crd.Validate(ctx, v.Client, incomingCrd) if err != nil { - return admission.Denied(fmt.Sprintf("failed to validate safety of CRD %s: %v", req.Operation, err)) + message := fmt.Sprintf("failed to validate safety of %s for CRD %q: %s", req.Operation, req.Name, err) + log.V(0).Info(message) + return admission.Denied(message) } + log.Info("admission allowed for %s of CRD %q", req.Name, req.Operation) return admission.Allowed("") } -// CrdValidator implements admission.DecoderInjector. -// A decoder will be automatically injected. - -// InjectDecoder injects the decoder. +// InjectDecoder injects a decoder for the CrdValidator. func (v *CrdValidator) InjectDecoder(d *admission.Decoder) error { v.decoder = d return nil diff --git a/cmd/crdvalidator/main.go b/cmd/crdvalidator/main.go index fa9c9a9e..717fe2b1 100644 --- a/cmd/crdvalidator/main.go +++ b/cmd/crdvalidator/main.go @@ -56,7 +56,12 @@ func main() { // Register CRD validation handler entryLog.Info("registering webhooks to the webhook server") - hookServer.Register("/validate-crd", &webhook.Admission{Handler: &handlers.CrdValidator{Client: mgr.GetClient()}}) + hookServer.Register("/validate-crd", &webhook.Admission{ + Handler: &handlers.CrdValidator{ + Client: mgr.GetClient(), + Log: entryLog, + }, + }) entryLog.Info("starting manager") if err := mgr.Start(signals.SetupSignalHandler()); err != nil { diff --git a/cmd/crdvalidator/manifests/00_namespace.yaml b/cmd/crdvalidator/manifests/00_namespace.yaml new file mode 100644 index 00000000..8f93a42d --- /dev/null +++ b/cmd/crdvalidator/manifests/00_namespace.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +kind: Namespace +metadata: + name: crdvalidator-system diff --git a/cmd/crdvalidator/manifests/00_service_account.yaml b/cmd/crdvalidator/manifests/01_service_account.yaml similarity index 70% rename from cmd/crdvalidator/manifests/00_service_account.yaml rename to cmd/crdvalidator/manifests/01_service_account.yaml index 7f9c74b1..6e29e0c2 100644 --- a/cmd/crdvalidator/manifests/00_service_account.yaml +++ b/cmd/crdvalidator/manifests/01_service_account.yaml @@ -1,5 +1,5 @@ apiVersion: v1 kind: ServiceAccount metadata: - namespace: default + namespace: crdvalidator-system name: crd-validation-webhook diff --git a/cmd/crdvalidator/manifests/02_cluster_role_binding.yaml b/cmd/crdvalidator/manifests/02_cluster_role_binding.yaml index 37b8df60..df1d15e7 100644 --- a/cmd/crdvalidator/manifests/02_cluster_role_binding.yaml +++ b/cmd/crdvalidator/manifests/02_cluster_role_binding.yaml @@ -9,4 +9,4 @@ roleRef: subjects: - kind: ServiceAccount name: crd-validation-webhook - namespace: default + namespace: crdvalidator-system diff --git a/cmd/crdvalidator/manifests/03_service.yaml b/cmd/crdvalidator/manifests/03_service.yaml index dd989a64..23c39a24 100644 --- a/cmd/crdvalidator/manifests/03_service.yaml +++ b/cmd/crdvalidator/manifests/03_service.yaml @@ -2,7 +2,7 @@ apiVersion: v1 kind: Service metadata: name: crd-validation-webhook - namespace: default + namespace: crdvalidator-system spec: ports: - port: 9443 diff --git a/cmd/crdvalidator/manifests/04_webhook.yaml b/cmd/crdvalidator/manifests/04_webhook.yaml index bc42eb18..7728bada 100644 --- a/cmd/crdvalidator/manifests/04_webhook.yaml +++ b/cmd/crdvalidator/manifests/04_webhook.yaml @@ -1,11 +1,11 @@ apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: - name: crd-validation-webhook # TODO (tylerslaton): do we want this domain?metadata: + name: crd-validation-webhook annotations: - cert-manager.io/inject-ca-from: default/crd-validation-webhook-certificate + cert-manager.io/inject-ca-from: crdvalidator-system/crd-validation-webhook-certificate webhooks: - - name: "crd-validation-webhook.acme.com" + - name: "webhook.crdvalidator.io" rules: - apiGroups: ["apiextensions.k8s.io"] apiVersions: ["v1"] @@ -14,7 +14,7 @@ webhooks: scope: "*" clientConfig: service: - namespace: default + namespace: crdvalidator-system name: crd-validation-webhook path: /validate-crd port: 9443 @@ -25,10 +25,11 @@ apiVersion: cert-manager.io/v1 kind: Certificate metadata: name: crd-validation-webhook-certificate + namespace: crdvalidator-system spec: secretName: crd-validation-webhook-certificate dnsNames: - - crd-validation-webhook.default.svc + - crd-validation-webhook.crdvalidator-system.svc issuerRef: name: selfsigned --- @@ -36,6 +37,6 @@ apiVersion: cert-manager.io/v1 kind: Issuer metadata: name: selfsigned - namespace: default + namespace: crdvalidator-system spec: selfSigned: {} diff --git a/cmd/crdvalidator/manifests/05_deployment.yaml b/cmd/crdvalidator/manifests/05_deployment.yaml index 45e84045..28101fed 100644 --- a/cmd/crdvalidator/manifests/05_deployment.yaml +++ b/cmd/crdvalidator/manifests/05_deployment.yaml @@ -2,7 +2,7 @@ apiVersion: apps/v1 kind: Deployment metadata: name: crd-validation-webhook - namespace: default + namespace: crdvalidator-system spec: selector: matchLabels: