Abstract how our webhook implementations target things.

[mattmoor]

trafficstars

This change introduces a targeter.Interface abstraction that we've been using downstream to abstract the core logic of webhooks from the details of how delivery to them is handled.

The most immediate benefit of this refactoring is that each of our webhooks no longer needs direct knowledge of caBundle and the secret logic, which is consolidated in the provided implementations. So for examples, folks could now provide an implementation of targeter.Interface that receives its caBundle information in a wholly different way (e.g. mounted from the filesystem).

This encapsulation of how the WebhookClientConfig is produced also enables folks to produce these configurations in more imaginative ways. For example, I have included an implementation that allows folks to host a webhook with a BasePath(), produce configs for "names" under this BasePath() and then extract the "name" being handled when receiving a webhook request.

This encapsulation also lets folks produce WebhookClientConfig blocks that don't even need caBundle and leverage URL blocks, if they are hosting things with public-CA TLS-terminated endpoints (e.g. as a ksvc).

Changes

• :broom: Update or clean up current behavior /kind cleanup

Release Note

Docs

[knative-prow[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor

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:

• ~~webhook/OWNERS~~ [mattmoor]

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

[codecov[bot]]

Codecov Report

Merging #2518 (fd91cdc) into main (2d8305b) will decrease coverage by 0.10%. The diff coverage is 90.62%.

@@            Coverage Diff             @@
##             main    #2518      +/-   ##
==========================================
- Coverage   81.62%   81.51%   -0.11%     
==========================================
  Files         163      166       +3     
  Lines        9737     9788      +51     
==========================================
+ Hits         7948     7979      +31     
- Misses       1551     1567      +16     
- Partials      238      242       +4     

Impacted Files	Coverage Δ
webhook/configmaps/configmaps.go	82.35% <50.00%> (-5.34%)	:arrow_down:
webhook/resourcesemantics/defaulting/defaulting.go	75.76% <50.00%> (-2.53%)	:arrow_down:
...k/resourcesemantics/validation/reconcile_config.go	80.48% <50.00%> (-7.69%)	:arrow_down:
webhook/resourcesemantics/conversion/reconciler.go	77.50% <62.50%> (-10.00%)	:arrow_down:
webhook/configmaps/controller.go	100.00% <100.00%> (ø)
webhook/resourcesemantics/conversion/controller.go	81.81% <100.00%> (-2.40%)	:arrow_down:
webhook/resourcesemantics/defaulting/controller.go	95.23% <100.00%> (-0.60%)	:arrow_down:
webhook/resourcesemantics/validation/controller.go	95.00% <100.00%> (-5.00%)	:arrow_down:
webhook/targeter/dynamic.go	100.00% <100.00%> (ø)
webhook/targeter/fixed.go	100.00% <100.00%> (ø)
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2d8305b...fd91cdc. Read the comment docs.

[mattmoor]

/cc @n3wscott

[mattmoor]

Do you have an example usage of the DispatchingInterface? My guess is you're using it to support multiple webhooks within a single MutatingWebhookConfiguration. If that's the case why not just have the GetConfig include the name of the webhook?

Nothing public yet, but yes. It isn't about multiple webhooks within a single MutatingWebhookConfiguration, it's sort of the opposite. We have N webhook configurations pointed at /base/{name} and want to be able to dispatch to an appropriate configuration based on the request path. This is a simple case, it gets more fun from there.

The problem with including name directly in the interface for fetching the client config is that it assumes one particular implementation, to spitball a little bit:

1. For NewFixed the name parameter would be meaningless (it's the same config for all names).
2. (Devil's advocate a bit) Where name is sufficient for reading named configuration with a particular type, it is insufficient for say a polymorphic namespaced resource where I might want (kind, namespace, name) to determine the config to process.

I don't believe we're leveraging NewDynamic downstream as-is, since I think I wanted a few changes not easily upstreamed, so I am happy to remove it if it's confusing things, but I thought it might help illustrate a second implementation. The main thing I'm after is a good targeter.Interface (or w/e we call it) to encapsulate how we slot in different logic for producing these configs.

I'm not in any particular rush here, so marinate on it a bit, and maybe I can set up some time next week to hop on a call (I can check if the WG call slot works, if that's best).

[dprotaso]

I guess the name I was referring to was some combination of the webhook metadata.name and the webhooks[*].name and for conversion webhooks that would be just the metadata.name

That's the final destination for these webhook configs. So for me it's easy to reason our webhooks scan the current webhook spec and delegate to the helper with some additional context.

(Devil's advocate a bit) Where name is sufficient for reading named configuration with a particular type, it is insufficient for say a polymorphic namespaced resource where I might want (kind, namespace, name) to determine the config to process.

Given this - Get probably needs the entire context of the target webhook - ie. type & object meta, webhook name for *WebhookConfiguration and the CRD definition for conversion webhooks

We have N webhook configurations pointed at /base/{name} and want to be able to dispatch to an appropriate configuration based on the request path. This is a simple case, it gets more fun from there.

But what's putting the Name on the context? I want to avoid these hidden APIs semantics on context objects. Thus I hope improvements don't continue this pattern.

For NewFixed the name parameter would be meaningless (it's the same config for all names).

Yup - I think that's fine - ie. serving's webhook uses the same config for validation, defaulting and configmaps etc.

[knative-prow-robot]

@mattmoor: PR needs rebase.

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.

[github-actions[bot]]

This Pull Request is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen with /reopen. Mark as fresh by adding the comment /remove-lifecycle stale.