From bf3ab293b4d2879f05b809620696aff04871d345 Mon Sep 17 00:00:00 2001 From: Yauheni Kaliuta Date: Tue, 1 Oct 2024 20:24:45 +0300 Subject: [PATCH] webhook: switch to contextual logger Use k8s contextual logger instead of own. Comparing to other parts of the operator webhook is a bit special since it serves own endpoints and has context independent from controllers. Include more info (kind and operation), just to get more details. Since controller-framework adds "admission" to the name, use own LogConstructor with own base logger for the framework's DefaultLogConstructor. Add name locally to distinguish from framework's messages. Add Name field to the structures to avoid copying string literal for both WithName() and WithValues(). The output changes and it looks like ``` {"level":"info","ts":"2024-10-11T04:49:14Z","logger":"ValidatingWebhook","msg":"Validating request","object":{"name":"default-dsci"},"namespace":"","name":"default-dsci","resource":{"group":"dscinitialization.opendatahub.io","version":"v1","resource":"dscinitializations"},"user":"kube:admin","requestID":"66fd27d2-3ed9-400b-ab79-fb8bbb7f3aa9","webhook":"ValidatingWebhook","kind":"DSCInitialization","operation":"CREATE"} ``` or for the defaulter: ``` {"level":"info","ts":"2024-10-11T04:50:48Z","logger":"DSCDefaulter","msg":"Defaulting DSC","object":{"name":"default-dsc"},"namespace":"","name":"default-dsc","resource":{"group":"datasciencecluster.opendatahub.io","version":"v1","resource":"datascienceclusters"},"user":"kube:admin","requestID":"c9213ff3-80ee-40c0-9f15-12188dece68e","webhook":"DSCDefaulter"} ``` (the messages are not from the current codebase, was added for demo only) Signed-off-by: Yauheni Kaliuta --- controllers/webhook/webhook.go | 36 +++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/controllers/webhook/webhook.go b/controllers/webhook/webhook.go index 5a539667b91..05e31aaac90 100644 --- a/controllers/webhook/webhook.go +++ b/controllers/webhook/webhook.go @@ -23,6 +23,7 @@ import ( "fmt" "net/http" + "github.com/go-logr/logr" operatorv1 "github.com/openshift/api/operator/v1" admissionv1 "k8s.io/api/admission/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -30,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -38,29 +40,44 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" ) -var log = ctrl.Log.WithName("odh-controller-webhook") - //+kubebuilder:webhook:path=/validate-opendatahub-io-v1,mutating=false,failurePolicy=fail,sideEffects=None,groups=datasciencecluster.opendatahub.io;dscinitialization.opendatahub.io,resources=datascienceclusters;dscinitializations,verbs=create;delete,versions=v1,name=operator.opendatahub.io,admissionReviewVersions=v1 //nolint:lll +// TODO: Get rid of platform in name, rename to ValidatingWebhook. type OpenDataHubValidatingWebhook struct { Client client.Client Decoder *admission.Decoder + Name string } func Init(mgr ctrl.Manager) { (&OpenDataHubValidatingWebhook{ Client: mgr.GetClient(), Decoder: admission.NewDecoder(mgr.GetScheme()), + Name: "ValidatingWebhook", + }).SetupWithManager(mgr) + + (&DSCDefaulter{ + Name: "DSCDefaulter", }).SetupWithManager(mgr) +} - (&DSCDefaulter{}).SetupWithManager(mgr) +// newLogConstructor creates a new logger constructor for a webhook. +// It is based on the root controller-runtime logger witch is set in main.go +// The purpose of it is to remove "admission" from the log name. +func newLogConstructor(name string) func(logr.Logger, *admission.Request) logr.Logger { + return func(_ logr.Logger, req *admission.Request) logr.Logger { + base := ctrl.Log + l := admission.DefaultLogConstructor(base, req) + return l.WithValues("webhook", name) + } } func (w *OpenDataHubValidatingWebhook) SetupWithManager(mgr ctrl.Manager) { hookServer := mgr.GetWebhookServer() odhWebhook := &webhook.Admission{ - Handler: w, + Handler: w, + LogConstructor: newLogConstructor(w.Name), } hookServer.Register("/validate-opendatahub-io-v1", odhWebhook) } @@ -90,6 +107,8 @@ func denyCountGtZero(ctx context.Context, cli client.Client, gvk schema.GroupVer } func (w *OpenDataHubValidatingWebhook) checkDupCreation(ctx context.Context, req admission.Request) admission.Response { + log := logf.FromContext(ctx) + switch req.Kind.Kind { case "DataScienceCluster", "DSCInitialization": default: @@ -119,6 +138,9 @@ func (w *OpenDataHubValidatingWebhook) checkDeletion(ctx context.Context, req ad } func (w *OpenDataHubValidatingWebhook) Handle(ctx context.Context, req admission.Request) admission.Response { + log := logf.FromContext(ctx).WithName(w.Name).WithValues("kind", req.Kind.Kind, "operation", req.Operation) + ctx = logf.IntoContext(ctx, log) + var resp admission.Response resp.Allowed = true // initialize Allowed to be true in case Operation falls into "default" case @@ -141,19 +163,23 @@ func (w *OpenDataHubValidatingWebhook) Handle(ctx context.Context, req admission //+kubebuilder:webhook:path=/mutate-opendatahub-io-v1,mutating=true,failurePolicy=fail,sideEffects=None,groups=datasciencecluster.opendatahub.io,resources=datascienceclusters,verbs=create;update,versions=v1,name=mutate.operator.opendatahub.io,admissionReviewVersions=v1 //nolint:lll -type DSCDefaulter struct{} +type DSCDefaulter struct { + Name string +} // just assert that DSCDefaulter implements webhook.CustomDefaulter. var _ webhook.CustomDefaulter = &DSCDefaulter{} func (m *DSCDefaulter) SetupWithManager(mgr ctrl.Manager) { mutateWebhook := admission.WithCustomDefaulter(mgr.GetScheme(), &dscv1.DataScienceCluster{}, m) + mutateWebhook.LogConstructor = newLogConstructor(m.Name) mgr.GetWebhookServer().Register("/mutate-opendatahub-io-v1", mutateWebhook) } // Implement admission.CustomDefaulter interface. // It currently only sets defaults for modelregiestry in datascienceclusters. func (m *DSCDefaulter) Default(_ context.Context, obj runtime.Object) error { + // TODO: add debug logging, log := logf.FromContext(ctx).WithName(m.Name) dsc, isDSC := obj.(*dscv1.DataScienceCluster) if !isDSC { return fmt.Errorf("expected DataScienceCluster but got a different type: %T", obj)