Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

⚠️ Allow passing a custom webhook server #2293

Merged
merged 1 commit into from May 4, 2023
Merged

⚠️ Allow passing a custom webhook server #2293

merged 1 commit into from May 4, 2023

Conversation

alvaroaleman
Copy link
Member

Currently it is impossible to pass a custom webhook server, because we reference the concrete type rather than an interface. Change this to an interface.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 1, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

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:

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 1, 2023
vincepri
vincepri reviewed May 1, 2023
View reviewed changes
pkg/manager/internal.go
Comment on lines -133 to +132
webhookServer *webhook.Server
webhookServer webhook.Server
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are some use cases that require an interface instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically anything where the default implementation isn't working. The case I have at hand is that the same binary also generates the certificates, so Start has to wait until they exist.

@alvaroaleman alvaroaleman force-pushed the webhook-interface branch 2 times, most recently from aaae15c to fec57a7 Compare May 1, 2023 15:46
@alvaroaleman alvaroaleman changed the title :warn: Allow passing a custom webhook server ⚠️ : Allow passing a custom webhook server May 1, 2023
@alvaroaleman alvaroaleman changed the title ⚠️ : Allow passing a custom webhook server ⚠️ Allow passing a custom webhook server May 1, 2023
sbueringer
sbueringer reviewed May 2, 2023
View reviewed changes
pkg/webhook/server.go Outdated Show resolved Hide resolved
pkg/webhook/server.go

// DefaultServer is the default implementation used for Server.
type DefaultServer struct {
Options Options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if Options should be private given that we also have a constructor (but no strong opinion)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I left it public is that there are a bunch of tests in the Manager that check some values in there

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2023
@alvaroaleman
:warn: Allow passing a custom webhook server
91cdd8c 
Currently it is impossible to pass a custom webhook server, because we
reference the concrete type rather than an interface. Change this to an
interface.
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2023
@sbueringer
Copy link
Member

sbueringer commented May 4, 2023 •
edited

Thx!

/lgtm

/hold
not sure if Vince wants to take another look

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 4, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2023
@alvaroaleman
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 4, 2023
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 4, 2023 •
edited

@alvaroaleman: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-controller-runtime-apidiff 91cdd8c link false /test pull-controller-runtime-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot merged commit 07a8152 into kubernetes-sigs:main May 4, 2023
8 of 9 checks passed
@JoelSpeed
Copy link
Contributor

@alvaroaleman What's the plan to be able to configure the default webhook server? The manager.Options.Port and other related ports are marked as deprecated but now that WebhookServer is an interface there is no replacement within the options to configure the Port/Host/Certs apart from the deprecated fields. Is this a known issue you are working on?

@sbueringer
Copy link
Member

Not sure if that's what you're trying to do, but I did it like this in Cluster API: https://github.com/kubernetes-sigs/cluster-api/blob/d825a40334ac768405d090ddf8eadb73eced3a6d/main.go#L277-L283

@alvaroaleman alvaroaleman deleted the webhook-interface branch May 16, 2023 15:27
@JoelSpeed
Copy link
Contributor

@sbueringer Yes! That solves the linter issue for me, I think we probably want to update the deprecation notes there though

@sbueringer sbueringer mentioned this pull request May 17, 2023
Merged
@ary1992 ary1992 mentioned this pull request Jun 23, 2023
Closed
ary1992 added a commit to ary1992/gardener that referenced this pull request Jun 28, 2023
@ary1992
Adapt webhook.Server interface.
3714477 
Earlier `webhook.Server` was a struct. All the struct fields like `Port`, `CertDir` is being moved to `Options` struct.
kubernetes-sigs/controller-runtime#2293
@RainbowMango RainbowMango mentioned this pull request Jul 25, 2023
Merged
takirala added a commit to mesosphere/kubefed that referenced this pull request Aug 3, 2023
@takirala
chore: refactor around breaking changes of ctrl runtime
c480676 
See kubernetes-sigs/controller-runtime#2122

See kubernetes-sigs/controller-runtime#2293

Signed-off-by: Tarun Gupta Akirala <takirala@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri left review comments

@sbueringer sbueringer sbueringer left review comments

@FillZpp FillZpp Awaiting requested review from FillZpp

@inteon inteon Awaiting requested review from inteon

Assignees

@sbueringer sbueringer

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@alvaroaleman @k8s-ci-robot @sbueringer @JoelSpeed @vincepri