Defaulting Controller options for all kind of webhooks #2738

hectorj2f · 2023-05-14T10:02:07Z

Changes

In order to unify a bug fix for #2731 and #2420 reusing the initial work from @dprotaso (https://github.com/knative/pkg/compare/main...dprotaso:defaulting-options?body=&expand=1&title=functional+options) and @pierDipi.

I added support to define the controllerOptions and other configurations settings for the webhooks, so the QueueName and other values are configurable now.

/kind bug

Fixes #

Release Note

Configure controller options and other settings when generating webhook configurations.

Docs

knative-prow · 2023-05-14T10:02:17Z

Hi @hectorj2f. Thanks for your PR.

I'm waiting for a knative 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.

dprotaso · 2023-05-17T20:23:47Z

/ok-to-test

dprotaso

Curious why do you need multiple defaulting webhooks?

Since you're constructing these webhooks programmatically you could collapse all the types being defaulted into a single webhook

dprotaso · 2023-05-17T20:39:47Z

webhook/resourcesemantics/conversion/controller.go

-type GroupKindConversion struct {
-	// DefinitionName specifies the CustomResourceDefinition that should
-	// be reconciled with by the controller.
-	//
-	// The conversion webhook configuration will be updated
-	// when the CA bundle changes
-	DefinitionName string
-
-	// HubVersion specifies which version of the CustomResource supports
-	// conversions to and from all types
-	//
-	// It is expected that the Zygotes map contains an entry for the
-	// specified HubVersion
-	HubVersion string
-
-	// Zygotes contains a map of version strings (ie. v1, v2) to empty
-	// ConvertibleObject objects
-	//
-	// During a conversion request these zygotes will be deep copied
-	// and manipulated using the apis.Convertible interface
-	Zygotes map[string]ConvertibleObject
-}


Deleting these types is unfortunately a breaking change

Yes, it isn't nice. We'll need to keep this type untouched.

dprotaso · 2023-05-17T20:50:31Z

webhook/resourcesemantics/common/options.go

+func WithKinds(kinds map[schema.GroupKind]GroupKindConversion) OptionFunc {
+	return func(o *options) {
+		o.kinds = kinds
+	}
+}


I don't know if I like this approach. Essentially we're creating a common set of options but they don't work with all webhook types.

ie. WithKinds passed to a defaulting webhook is really a noop and not semantically valid. Ideally we don't want downstream users doing this.

My main goal was to simplify the code here. As an alternative, we could have a common interface with the shared options, and re-implement it on every single webhook with their specific options.

hectorj2f · 2023-05-18T06:58:59Z

Curious why do you need multiple defaulting webhooks?

Sorry, I am not sure I understand what you mean here. We have to abstract the way the controllers are initialized as each one has its own logic.

The intention of this PR comes from reusing your initial approach across all the different controllers instead of adding similar code to each one of them.

@pierDipi Based on the comments, I am not sure we want to reuse this approach #2420 to solve the problem you and myself found in #2731.

? Since you're constructing these webhooks programmatically you could collapse all the types being defaulted into a single webhook

Indeed, if we decide to do that, I'd prefer to make that change as part of a different PR to simplify these changes in a manageable way.

pierDipi · 2023-05-18T11:00:14Z

Curious why do you need multiple defaulting webhooks?

In my case, I wanted to have webhooks with different selectors, you can see it here:

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

hectorj2f · 2023-06-05T08:47:34Z

@dprotaso @pierDipi I've changed the code to be independent for each webhook type due to the potential breaking issues that other changes could cause.

webhook/resourcesemantics/validation/controller.go

pierDipi · 2023-06-05T09:00:00Z

webhook/resourcesemantics/validation/controller.go

+	opts := &options{}
+
+	for _, f := range optsFunc {
+		f(opts)
+	}


Should we validate provided options?

what happens if I don't call WithWrapContext and so no opts.wc is provided?
Same questions on other kinds of webhooks.

I'd prefer we make this change in a different PR.

Wondering if we could simply make NewController to be private newController( here as a solution until we put in place the validation ?

codecov · 2023-06-05T09:16:03Z

Codecov Report

Patch coverage: 96.57% and project coverage change: +0.21 🎉

Comparison is base (5671699) 81.80% compared to head (d2f9e8c) 82.01%.

❗ Current head d2f9e8c differs from pull request most recent head fbc32b0. Consider uploading reports for the commit fbc32b0 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2738      +/-   ##
==========================================
+ Coverage   81.80%   82.01%   +0.21%     
==========================================
  Files         163      166       +3     
  Lines       10035    10161     +126     
==========================================
+ Hits         8209     8334     +125     
- Misses       1580     1582       +2     
+ Partials      246      245       -1

Impacted Files	Coverage Δ
webhook/webhook.go	`78.66% <ø> (ø)`
webhook/resourcesemantics/validation/controller.go	`92.59% <89.47%> (-3.16%)`	⬇️
webhook/resourcesemantics/conversion/controller.go	`89.09% <95.83%> (+4.88%)`	⬆️
webhook/resourcesemantics/conversion/options.go	`100.00% <100.00%> (ø)`
webhook/resourcesemantics/defaulting/controller.go	`97.10% <100.00%> (+1.26%)`	⬆️
webhook/resourcesemantics/defaulting/options.go	`100.00% <100.00%> (ø)`
webhook/resourcesemantics/validation/options.go	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

skonto · 2023-06-07T17:51:57Z

It would be nice to get this in, I hit the same issue while merging the webhooks at the Serving side due to the lease conflict (queue name), I wish I had checked earlier as I spent some time debugging :)

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

hectorj2f · 2023-06-09T16:55:10Z

@pierDipi Could you have another look at my changes? Thanks in advance.

pierDipi

Thanks @hectorj2f !

/lgtm
/approve

knative-prow · 2023-06-12T08:36:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hectorj2f, pierDipi

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~~ [pierDipi]

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

knative-prow bot added the kind/bug Categorizes issue or PR as related to a bug. label May 14, 2023

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2023

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 14, 2023

knative-prow bot requested review from carlisia and skonto May 14, 2023 10:02

hectorj2f changed the title ~~Defaulting options~~ Defaulting Controller options for all kind of webhooks May 14, 2023

hectorj2f force-pushed the defaulting-options branch from c05e752 to 35ca933 Compare May 14, 2023 10:15

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2023

hectorj2f force-pushed the defaulting-options branch from 35ca933 to e74332d Compare May 16, 2023 15:19

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 17, 2023

dprotaso reviewed May 17, 2023

View reviewed changes

dprotaso and others added 4 commits June 4, 2023 23:25

functional options

6f1940b

move options to its own package

8f2665c

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

add controller options to the webhook options

f40aad8

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

create custom options funcs for each webhook type

d2f9e8c

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

hectorj2f force-pushed the defaulting-options branch from e74332d to d2f9e8c Compare June 4, 2023 22:33

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 4, 2023

pierDipi reviewed Jun 5, 2023

View reviewed changes

skonto mentioned this pull request Jun 7, 2023

Consolidate webhooks knative/serving#14082

Merged

address comments from reviewers

fbc32b0

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

hectorj2f requested a review from pierDipi June 9, 2023 10:45

pierDipi approved these changes Jun 12, 2023

View reviewed changes

knative-prow bot assigned pierDipi Jun 12, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 12, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2023

knative-prow bot merged commit 15605c7 into knative:main Jun 12, 2023

hectorj2f mentioned this pull request Jun 12, 2023

fix: use the controller name when creating the lease name #2731

Closed

This was referenced Jun 12, 2023

Multiple defaulting webhooks don't work #2418

Closed

Allow overriding webhook controller options #2420

Closed

creydr mentioned this pull request Jan 25, 2024

Remove unneeded pkg patch knative-extensions/eventing-kafka-broker#3634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defaulting Controller options for all kind of webhooks #2738

Defaulting Controller options for all kind of webhooks #2738

hectorj2f commented May 14, 2023 •

edited

Loading

knative-prow bot commented May 14, 2023

dprotaso commented May 17, 2023

dprotaso left a comment

dprotaso May 17, 2023

hectorj2f May 18, 2023

dprotaso May 17, 2023

hectorj2f May 18, 2023

hectorj2f commented May 18, 2023

pierDipi commented May 18, 2023

hectorj2f commented Jun 5, 2023

pierDipi Jun 5, 2023

hectorj2f Jun 8, 2023

hectorj2f Jun 8, 2023

codecov bot commented Jun 5, 2023 •

edited

Loading

skonto commented Jun 7, 2023 •

edited

Loading

hectorj2f commented Jun 9, 2023

pierDipi left a comment

knative-prow bot commented Jun 12, 2023

Defaulting Controller options for all kind of webhooks #2738

Defaulting Controller options for all kind of webhooks #2738

Conversation

hectorj2f commented May 14, 2023 • edited Loading

Changes

knative-prow bot commented May 14, 2023

dprotaso commented May 17, 2023

dprotaso left a comment

Choose a reason for hiding this comment

dprotaso May 17, 2023

Choose a reason for hiding this comment

hectorj2f May 18, 2023

Choose a reason for hiding this comment

dprotaso May 17, 2023

Choose a reason for hiding this comment

hectorj2f May 18, 2023

Choose a reason for hiding this comment

hectorj2f commented May 18, 2023

pierDipi commented May 18, 2023

hectorj2f commented Jun 5, 2023

pierDipi Jun 5, 2023

Choose a reason for hiding this comment

hectorj2f Jun 8, 2023

Choose a reason for hiding this comment

hectorj2f Jun 8, 2023

Choose a reason for hiding this comment

codecov bot commented Jun 5, 2023 • edited Loading

Codecov Report

skonto commented Jun 7, 2023 • edited Loading

hectorj2f commented Jun 9, 2023

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented Jun 12, 2023

hectorj2f commented May 14, 2023 •

edited

Loading

codecov bot commented Jun 5, 2023 •

edited

Loading

skonto commented Jun 7, 2023 •

edited

Loading