[WIP] Add functionality to dynamically reload the VPA certs #3462

surajnarwade · 2020-08-26T13:36:01Z

This PR adds the functionality of dynamically reloading the tls certificates for VPA admission controller.

How does it work?

cert-manager or any other mechanism updates the certs in secrets, fsnotify watcher will watch at /etc/tls-certs (where secrets are mounted) and triggers an event if certs are updated
Once, certs are updated, reload function will again reload new certs
callback hook GetCertificate in tls.Config is used to get the new certs

Reference

https://stackoverflow.com/a/40883377/7804592

k8s-ci-robot · 2020-08-26T13:36:08Z

Welcome @surajnarwade!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

arjunrn · 2020-08-28T09:33:05Z

vertical-pod-autoscaler/pkg/admission-controller/certs.go

+		}
+	}()
+
+	if err := watcher.Add("/etc/tls-certs"); err != nil {


You probably don't want this hardcoded. You should get the directories of both the cert and key and set watches on them.

+1 I think you want those as flags passed in to the KeyPairReloader

sure, I will update it 👍

arjunrn · 2020-08-28T09:36:44Z

How about a test where an KeyReloader is the configuration files are changed and then the test verifies that the key and cert have been reloaded correctly.

bskiba

Left some comments. I would also like this new behavior hidden behind the flag, so that the default behavior would still be not starting a watch for certs.

Can you describe how you tested this change?

bskiba · 2020-10-02T07:21:29Z

vertical-pod-autoscaler/pkg/admission-controller/certs.go

+		}
+	}()
+
+	if err := watcher.Add("/etc/tls-certs"); err != nil {


+1 I think you want those as flags passed in to the KeyPairReloader

bskiba · 2020-10-02T07:23:25Z

vertical-pod-autoscaler/pkg/admission-controller/certs.go

+			case event := <-watcher.Events:
+				// fsnotify.create events will tell us if there are new certs
+				if event.Op&fsnotify.Create == fsnotify.Create {
+					klog.Info("Reloading certs")


Can you add verbosity level to the log? And the log could be "New certificate found, reloading certs"

bskiba · 2020-10-02T07:23:50Z

vertical-pod-autoscaler/pkg/admission-controller/certs.go

+				if event.Op&fsnotify.Create == fsnotify.Create {
+					klog.Info("Reloading certs")
+					if err := result.reload(); err != nil {
+						klog.Infof("Could not load new certs: %v", err)


This should probably be Warning

bskiba · 2020-10-02T07:24:58Z

vertical-pod-autoscaler/pkg/admission-controller/certs.go

@@ -99,7 +98,7 @@ func NewKeypairReloader(config certsConfig) (*KeypairReloader, error) {

 				// watch for errors
 			case err := <-watcher.Errors:
-				klog.Infof("error", err)
+				klog.Infof("error: %v", err)


We need more descriptive log here. Probably also warning or error level.
Is there any action to be taken on observing errors from the watcher?

bskiba · 2020-10-02T07:26:18Z

vertical-pod-autoscaler/pkg/admission-controller/main.go

+	// load certs
+	kpr, err := NewKeypairReloader(*certsConfiguration)
+	if err != nil {
+		klog.Fatal(err)


More info in the log needed, like "Failed to load certificate on startup: %v", err

bskiba · 2020-10-02T07:27:15Z

vertical-pod-autoscaler/pkg/admission-controller/main.go

 	}
+	// this will check if there are new certs before every tls handshake
+	t := &tls.Config{GetCertificate: kpr.GetCertificateFunc()}


server.TLSConfig = &tls.Config{GetCertificate: kpr.GetCertificateFunc()}

you can also do it above:
server := &http.Server{
Addr: fmt.Sprintf(":%d", *port),
TLSConfig: &tls.Config{GetCertificate: kpr.GetCertificateFunc()},
}

One more thing is that the KeypairReloader could have a GetTLSConfig function and then we could simply have:

server := &http.Server{
Addr: fmt.Sprintf(":%d", *port),
TLSConfig: kpr.GetTLSConfig(),
}

bskiba · 2020-10-02T07:32:11Z

vertical-pod-autoscaler/pkg/admission-controller/main.go

 	}
+	// this will check if there are new certs before every tls handshake


Actually, this will return up-to-date cert before every handshake, right?

yeah, that's correct, I've updated the comment :)

bskiba · 2020-10-02T07:36:58Z

vertical-pod-autoscaler/pkg/admission-controller/certs.go

+
+// GetCertificateFunc will return function which will be used as tls.Config.GetCertificate
+func (kpr *KeypairReloader) GetCertificateFunc() func(*tls.ClientHelloInfo) (*tls.Certificate, error) {
+	return func(clientHello *tls.ClientHelloInfo) (*tls.Certificate, error) {


Since clientHello is unused, you can replace it with underscore

mwielgus · 2020-10-22T12:15:11Z

Ping.

surajnarwade · 2020-10-23T15:36:20Z

@bskiba , @mwielgus thanks for all the reviews, sorry could not get back to PR earlier. I will update the PR now

k8s-ci-robot · 2020-10-26T15:37:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: surajnarwade
To complete the pull request process, please assign krzysied after the PR has been reviewed.
You can assign the PR to them by writing /assign @krzysied in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

vertical-pod-autoscaler/OWNERS

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

fejta-bot · 2021-01-24T16:18:46Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

mwielgus · 2021-01-25T10:31:01Z

Closing due to inactivity. The PR has been WIP for 3 months.

mwielgus · 2021-01-25T10:31:14Z

Feel free to reopen if needed.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 26, 2020

k8s-ci-robot requested review from jbartosik and krzysied August 26, 2020 13:36

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 26, 2020

surajnarwade changed the title ~~Add functionality to automatically renew the VPA certs~~ Add functionality to dynamically reload the VPA certs Aug 26, 2020

surajnarwade marked this pull request as ready for review August 26, 2020 13:46

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 26, 2020

surajnarwade force-pushed the vpa-cert-renewal branch from 6d80d0c to 193fbfc Compare August 26, 2020 13:49

arjunrn reviewed Aug 28, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2020

bskiba suggested changes Oct 2, 2020

View reviewed changes

bskiba added kind/feature Categorizes issue or PR as related to a new feature. area/vertical-pod-autoscaler labels Oct 2, 2020

Suraj Narwade added 2 commits October 26, 2020 15:19

Add functionality to automatically renew the certs

cc579c6

added fsnotify as a dependency to watch files

4cc52ba

surajnarwade changed the title ~~Add functionality to dynamically reload the VPA certs~~ [WIP] Add functionality to dynamically reload the VPA certs Oct 26, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2020

removed few unnecessary caCert lines

8ad4683

surajnarwade force-pushed the vpa-cert-renewal branch from 193fbfc to 8ad4683 Compare October 26, 2020 15:37

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 24, 2021

mwielgus closed this Jan 25, 2021

L1ghtman2k mentioned this pull request Aug 17, 2022

Automatic certificate renewal for VPA #3458

Closed

bmendoza820 mentioned this pull request Nov 10, 2023

VPA admission controller certificate is not reloaded #6272

Closed

Nuckal777 mentioned this pull request Mar 25, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add functionality to dynamically reload the VPA certs #3462

[WIP] Add functionality to dynamically reload the VPA certs #3462

surajnarwade commented Aug 26, 2020 •

edited

Loading

k8s-ci-robot commented Aug 26, 2020

arjunrn Aug 28, 2020

bskiba Oct 2, 2020

surajnarwade Oct 23, 2020

arjunrn commented Aug 28, 2020

bskiba left a comment

bskiba Oct 2, 2020

bskiba Oct 2, 2020

surajnarwade Oct 23, 2020

bskiba Oct 2, 2020

bskiba Oct 2, 2020

bskiba Oct 2, 2020

bskiba Oct 2, 2020

bskiba Oct 2, 2020

surajnarwade Oct 26, 2020

bskiba Oct 2, 2020

mwielgus commented Oct 22, 2020

surajnarwade commented Oct 23, 2020

k8s-ci-robot commented Oct 26, 2020

fejta-bot commented Jan 24, 2021

mwielgus commented Jan 25, 2021

mwielgus commented Jan 25, 2021

		}
		// this will check if there are new certs before every tls handshake

[WIP] Add functionality to dynamically reload the VPA certs #3462

[WIP] Add functionality to dynamically reload the VPA certs #3462

Conversation

surajnarwade commented Aug 26, 2020 • edited Loading

k8s-ci-robot commented Aug 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjunrn commented Aug 28, 2020

bskiba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwielgus commented Oct 22, 2020

surajnarwade commented Oct 23, 2020

k8s-ci-robot commented Oct 26, 2020

fejta-bot commented Jan 24, 2021

mwielgus commented Jan 25, 2021

mwielgus commented Jan 25, 2021

surajnarwade commented Aug 26, 2020 •

edited

Loading