✨ Pass webhook logger to handler via context

[timebertt]

admission.Webhook now passes a request logger to the handler via the context.
It can be retrieved by handlers via logf.FromContext(ctx) and is setup with commonly interesting fields about the request.
The logger and it's fields can be customized by setting Webhook.LogConstructor.

This is very similar to how controllers are passing a reconciliation logger to reconcilers and allows for greater harmonization across the entire codebase, i.e. using contextual loggers everywhere instead of manually passing around half-cooked loggers.

[k8s-ci-robot]

Hi @timebertt. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[timebertt]

Resolved the merge conflict.
@alvaroaleman could you kindly have a look?

[sbueringer]

I wonder if we should implement this the same way as we did for the controller:

	if options.LogConstructor == nil {
		log := mgr.GetLogger().WithValues(
			"controller", name,
		)
		options.LogConstructor = func(req *reconcile.Request) logr.Logger {
			log := log
			if req != nil {
				log = log.WithValues(
					"object", klog.KRef(req.Namespace, req.Name),
					"namespace", req.Namespace, "name", req.Name,
				)
			}
			return log
		}
	}

The delta is roughly:

• LogConstructor would have this signature: LogConstructor func(req *Request) logr.Logger
• The default log constructor would then use the base logger directly
• namespace & name are also added as top-level k/v pairs in addition to the nested values below object

e.g.:

// getLogger constructs a logger from the injected log and LogConstructor.
func (wh *Webhook) getLogger(req *Request) logr.Logger {
	if wh.LogConstructor == nil {
		if req != nil {
			return wh.log.WithValues("object", klog.KRef(req.Namespace, req.Name),
				"namespace", req.Namespace, "name", req.Name,
				"resource", req.Resource, "user", req.UserInfo.Username,
			)
		}
		return wh.log
	}
	return wh.LogConstructor(req)
}

[timebertt]

LogConstructor would have this signature: LogConstructor func(req *Request) logr.Logger

I added the base logr.Logger param to the LogConstructor to allow using the logger injected by the webhook server which includes the webhook path:

controller-runtime/pkg/webhook/server.go

Line 165 in 869888c

if _, err := inject.LoggerInto(baseHookLog.WithValues("webhook", path), hook); err != nil {

This is different from controllers, which use the manager's logger directly (which doesn't carry any additional information about the controller):

controller-runtime/pkg/controller/controller.go

Line 110 in 598978c

log := mgr.GetLogger().WithValues(

With this, one can add additional fields via a custom LogConstructor without adding the webhook field again.
If one wants to ignore the fields of the base logger, one can simply use any other logger.

I get that this is different from the controller loggers. If harmonization is more important here than preserving the injected fields, I will apply your suggestion. :)
@sbueringer WDYT?

[sbueringer]

Looks good to me. Thank you very much!!

[sbueringer]

In general I like the idea to make the webhook logs consistent with the controller logs. This will also allow cross-referencing between controller and webhook logs.

/ok-to-test

[sbueringer]

cc @fabriziopandini (just fyi that there is work in this area)

[timebertt]

@sbueringer I have pushed two more commits addressing parts of your suggestions. PTAL :)
/label tide/merge-method-squash

[sbueringer]

Thank you!!

/lgtm

[vincepri]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: timebertt, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vincepri]

/retest