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

Add ability to the VPA admission-controller to reload it's certificate v2 #6665

Merged
merged 6 commits into from
Jun 25, 2024
Merged

Add ability to the VPA admission-controller to reload it's certificate v2 #6665

merged 6 commits into from
Jun 25, 2024

Conversation

Nuckal777
Copy link
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds the ability for the admission-controller to reload it's certificate on changes to the certificate file. This is useful when provisioning certificates with cert-manager.

Which issue(s) this PR fixes:

Fixes #6272.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added `reload-cert` flag to the VPA admission-controller, which will reload the certificates it use, when they change.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 28, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @Nuckal777. Thanks for your PR.

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

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 28, 2024
@Nuckal777 Nuckal777 mentioned this pull request Mar 28, 2024
Closed
@Nuckal777
Copy link
Contributor Author

/assign krzysied kwiesmueller

Would appreciate a review.

@kwiesmueller
Copy link
Member

/assign @laoj2 @sophieliu15

@k8s-ci-robot
Copy link
Contributor

@kwiesmueller: GitHub didn't allow me to assign the following users: laoj2.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @laoj2 @sophieliu15

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.

sophieliu15
sophieliu15 reviewed Apr 17, 2024
View reviewed changes
vertical-pod-autoscaler/pkg/admission-controller/certs.go
go func() {
defer watcher.Close()
for {
select {
Copy link

Choose a reason for hiding this comment

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

Do we need to add a default case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that no default case is required. A goroutine is forked off, which runs the blocking select. The channels are filled by the fsnotify implementation. Do you have a use case for a default case?

vertical-pod-autoscaler/pkg/admission-controller/certs_test.go Outdated Show resolved Hide resolved
@sophieliu15
Copy link

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 18, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 18, 2024
@Nuckal777
Copy link
Contributor Author

I hope, I addressed everything. Did I miss something?

kwiesmueller
kwiesmueller requested changes Apr 29, 2024
View reviewed changes
Copy link
Member

@kwiesmueller kwiesmueller left a comment

Choose a reason for hiding this comment

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

Generally lgtm.
Sorry for not bringing this up earlier. Do you think you can add an e2e/integration test for this feature?

@Nuckal777
Copy link
Contributor Author

Would generating a second pair of certificates in gencerts.sh and swapping the certificates in a test be fine?

@kwiesmueller
Copy link
Member

That sounds good to me, yes.
As long as we do it within the integration cluster so we verify the configmap reloading within the pod happens correctly when the configmap changes.

@Nuckal777
Copy link
Contributor Author

Should we run selfRegistration to update the webhook on reload as well, if enabled?

@voelzmo voelzmo mentioned this pull request May 15, 2024
Closed
@itspngu itspngu mentioned this pull request May 19, 2024
Closed
@Nuckal777
Copy link
Contributor Author

I had some thought about regarding the self registration again. I'd like to use the VPA together with cert-manager. This means the MutatingAdmissionWebhook needs to have the annotation for cert-managers ca-injection. Likely, this implies the webhook needs to deployed by something else than the admission-controller. So, the self registration needs to be disabled in that setup. cert-manager would take of the CA in the webhook and the certificates in the secret. The admission-controller would just need to reload the leaf certificates, which is implemented in the PR.

Given the self registration would not be helpful in that setup, I think we don't need to touch it here. Any opinions?

@voelzmo
Copy link
Contributor

voelzmo commented Jun 25, 2024

I'm fine with not touching the self-registration here. I also think it is unlikely that the webhook can be deployed with self-registgration if you have a mechanism which automatically renews the certificate. So this is good to go from my side.
/lgtm

@kwiesmueller do you have any additional changes you want to see in this PR?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2024
kwiesmueller
kwiesmueller approved these changes Jun 25, 2024
View reviewed changes
Copy link
Member

@kwiesmueller kwiesmueller left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@kwiesmueller
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot 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 Jun 25, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kwiesmueller, Nuckal777

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2024
@kwiesmueller
Copy link
Member

Thanks for adding the test!

@k8s-ci-robot k8s-ci-robot merged commit 22f8c84 into kubernetes:master Jun 25, 2024
6 of 7 checks passed
@Ramneek-kalra Ramneek-kalra mentioned this pull request Jun 26, 2024
Closed
@voelzmo voelzmo mentioned this pull request Jun 28, 2024
Merged
@voelzmo voelzmo mentioned this pull request Jul 22, 2024
Merged
@raywainman raywainman mentioned this pull request Aug 6, 2024
Closed
@maxcao13 maxcao13 mentioned this pull request Nov 2, 2024
Open
@wcarlsen wcarlsen mentioned this pull request Nov 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sophieliu15 sophieliu15 sophieliu15 left review comments

@kwiesmueller kwiesmueller kwiesmueller approved these changes

@krzysied krzysied Awaiting requested review from krzysied

Assignees

@voelzmo voelzmo

@kwiesmueller kwiesmueller

@krzysied krzysied

@sophieliu15 sophieliu15

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/vertical-pod-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

VPA admission controller certificate is not reloaded
6 participants
@Nuckal777 @k8s-ci-robot @kwiesmueller @sophieliu15 @voelzmo @krzysied