pkg
pkg copied to clipboard
knative
Allow overriding webhook controller options
Creating 2 defaulting webhooks w1 and w2 inside the same pod for 2 different mutating webhook configuration manifests doesn't work since the lease name is based on the hardcoded queue name DefaultingWebhook. So, we get a single pod competing for the same lease in 2 separate reconcilers for 2 separate webhooks.
This PR allows overriding ControllerOptions for webhooks
so that callers can work around the issue while maintaining
backward compatibility.
Signed-off-by: Pierangelo Di Pilato [email protected]
Changes
- Allow overriding webhook controller options
/kind bug
Fixes #2418
Release Note
Allow overriding webhook controller options
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: pierDipi
To complete the pull request process, please assign dprotaso after the PR has been reviewed.
You can assign the PR to them by writing /assign @dprotaso in a comment when ready.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Codecov Report
Patch coverage: 78.57% and project coverage change: -0.10 :warning:
Comparison is base (
51be315) 63.52% compared to head (ecc9761) 63.43%.
Additional details and impacted files
@@ Coverage Diff @@
## main #2420 +/- ##
==========================================
- Coverage 63.52% 63.43% -0.10%
==========================================
Files 228 228
Lines 9990 9992 +2
==========================================
- Hits 6346 6338 -8
- Misses 3344 3352 +8
- Partials 300 302 +2
| Impacted Files | Coverage Δ | |
|---|---|---|
| controller/controller.go | 84.58% <0.00%> (-1.55%) |
:arrow_down: |
| webhook/context.go | 70.00% <80.00%> (+10.00%) |
:arrow_up: |
| webhook/resourcesemantics/conversion/controller.go | 85.18% <100.00%> (+1.85%) |
:arrow_up: |
| webhook/resourcesemantics/defaulting/controller.go | 94.28% <100.00%> (+0.53%) |
:arrow_up: |
| webhook/resourcesemantics/validation/controller.go | 100.00% <100.00%> (ø) |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
I don't think this the right approach - it's not obvious that setting the logger name will affect the lease name.
I think we need to talk about potential solutions/approaches in the issue beforehand.
I don't think this the right approach - it's not obvious that setting the logger name will affect the lease name.
Where do you see this behavior?
It's the queue name that affects the lease name which is the behavior that we have for every resource.
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.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: pierDipi To complete the pull request process, please ask for approval from dprotaso after the PR has been reviewed.
The full list of commands accepted by this bot can be found here.
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] avatar](https://avatars.githubusercontent.com/in/183726?v=4 "Published by a pub.dev verified publisher"){kind=small}
I think trying to shim in controller options is a bit messy - it seems like you're just looking to modify the queueName
So I'm wondering if we should refactor the webhook constructor to accept optional arguments - queueName being one of them
ie. https://github.com/knative/pkg/compare/main...dprotaso:defaulting-options?body=&expand=1&title=functional+options
yeah, that's cleaner.
can I take your branch and finish it or do you want to continue the implementation by yourself?
can I take your branch and finish it or do you want to continue the implementation by yourself?
If this is blocking you feel free to take the branch - I'm tackling some bug fixes atm.
Otherwise feel free to assign https://github.com/knative/pkg/issues/2418 to me - there are about two other POC PRs tweaking how the webhooks work so maybe it would be good to try to look at them holistically.
can I take your branch and finish it or do you want to continue the implementation by yourself?
If this is blocking you feel free to take the branch - I'm tackling some bug fixes atm.
Otherwise feel free to assign #2418 to me - there are about two other POC PRs tweaking how the webhooks work so maybe it would be good to try to look at them holistically.
I'm not blocked but we're patching vendor to get this fix, so I want to remove this hack ASAP
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.
This issue or pull request is stale because it has been open for 90 days with no activity.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle rotten - Close this issue or PR with
/close
/lifecycle stale
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.
@hectorj2f we need to find an agreement on how to move forward first, @dprotaso was proposing a slightly different approach: https://github.com/knative/pkg/compare/main...dprotaso:defaulting-options?body=&expand=1&title=functional+options which I like but we need someone to finalize it (I guess? @dprotaso )
@pierDipi We need to get this bug fixed! I'm sure we could move on with one of these solutions and ask for the required approvals.
@hectorj2f would you be willing to continue @dprotaso idea here https://github.com/knative/pkg/compare/main...dprotaso:defaulting-options?body=&expand=1&title=functional+options ?
@pierDipi I extended the previous work from @dprotaso to be used by all the webhook types https://github.com/knative/pkg/compare/main...hectorj2f:pkg:defaulting-options?expand=1.
Also I believe it makes more sense to add the ControllerOptions as an additional field to the webhook.Options. That would follow your approach without having to create a new set of With.. Get functions.
You can find my proposal here: https://github.com/knative/pkg/compare/main...hectorj2f:pkg:defaulting-options?expand=1
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.