Dynamically reload kube-aggregator certificates #88120

alenkacz · 2020-02-13T16:12:23Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Dynamically load certificates for kube-aggregator

Which issue(s) this PR fixes:

Fixes #87766

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Kube-aggregator certificates are dynamically loaded on change from disk

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

alenkacz · 2020-02-13T16:14:19Z

staging/src/k8s.io/kube-aggregator/pkg/util/dynamic_restconfig.go

+}
+
+// DynamicRestConfigProvider provides a restconfig and transport backed by dynamically loaded cert/key pair
+type DynamicRestConfigProvider struct {


this probably deserves some tests :) I'll add those after initial review validating the direction of this PR

deads2k · 2020-02-13T16:35:26Z

/assign @p0lyn0mial

alenkacz · 2020-02-13T18:18:52Z

😂 I'll check out the failures, I have run only unit tests locally :)

p0lyn0mial · 2020-02-14T08:57:37Z

What happens to the old cert? It is still valid, right?

p0lyn0mial · 2020-02-14T09:03:02Z

staging/src/k8s.io/kube-aggregator/pkg/util/dynamic_restconfig.go

+
+var _ dynamiccertificates.Listener = &DynamicRestConfigProvider{}
+
+func NewDynamicRestConfigProvider(servingContent dynamiccertificates.CertKeyContentProvider, egressSelector *egressselector.EgressSelector, proxyTransport *http.Transport, insecure bool, serverName string, caBundle []byte) (*DynamicRestConfigProvider, error) {


it has to be dynamic, serverName, caBundle are not constant, what if they have changed?

based on my understanding, these are provided when kube-aggregator starts so they cannot change without the process being restarted. But I might have missed something...

check when AddAPIService function is called - https://github.com/kubernetes/kubernetes/pull/88120/files#r379316414

p0lyn0mial · 2020-02-14T09:19:36Z

staging/src/k8s.io/kube-aggregator/pkg/util/dynamic_restconfig.go

+}
+
+func (c *DynamicRestConfigProvider) syncRestConfig() error {
+	cert, key := c.certKeyPair.CurrentCertKeyContent()


it will create a new config every second even though nothing has changed, are they any drawback?

yeah I should probably add checking whether the value changed, good point. I'll change that

p0lyn0mial · 2020-02-14T09:21:05Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

@@ -154,13 +138,26 @@ func (r *proxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	newReq, cancelFn := newRequestForProxy(location, req)
 	defer cancelFn()

-	if handlingInfo.proxyRoundTripper == nil {
+	roundTripper, err := handlingInfo.dynamicRestConfig.GetTransport()


this is a hot path, now it depends on an external component, for example, it can return an error.

any suggestions how to resolve this?

p0lyn0mial · 2020-02-14T09:25:14Z

staging/src/k8s.io/kube-aggregator/pkg/util/dynamic_restconfig.go

+}
+
+// DynamicRestConfigProvider provides a restconfig and transport backed by dynamically loaded cert/key pair
+type DynamicRestConfigProvider struct {


Thinking if we really need a controller, what would be an alternative design?

yeah I am open to other ideas, I pretty much kept this in line with how are similar problems solved in apiserver (so the dynamiccertificates package)

How about:

Adding a function to APIAggregator that would return a cert and a key

Adding a new method to APIAggregator that would call updateAPIService for all proxyHandlers and would use a func from 1.

Wire Notify method to call the new function from 2

Alternatively:

Adding a new method to APIAggregator that would call updateAPIService for all proxyHandlers and would accept a cert and a key

Wire Notify method to call the new function from 1 and providing a cert and a key

Doing this that way we don't modify the hot path and we take potential changes in services into account. It should work if the old cert is still valid. What do you think?

@sttts FYI

@p0lyn0mial that solves it for proxy_handlers, and then available_controller will have to implement the exact same thing (that's why I went the way of extracting that logic outside, so both places can reuse it).

If we go and extract it then the only difference between your proposal and current implementation is whether it uses the workqueue or not. I like reusing workqueue since it's already proven piece of code that solves the async update for me.

No hard opinion here though, just putting out here why I went the way I went. I am totally fine shifting the direction :-)

It seems that available_controller only needs a restConfig - slightly different one than proxy_handlers. I think that before it didn't use MTLS, now it does. I don't know why it was like that before but it should work with MTLS as well, right?

alright, let's try with the controller, left some comments.

it does not use mtls as I pass insecure=true here https://github.com/kubernetes/kubernetes/pull/88120/files#diff-d92af716cdd9489e3f57d61b53adad72R216 ... should I make that more obvious somehow?

I think I got the confusion around "controller" now, the Run method I think is definitely something we don't need, it was even unused. I removed it, so now it really just asynchronously processes notifications.

p0lyn0mial · 2020-02-17T09:48:11Z

staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller.go

+		if err != nil {
+			return err
+		}
+		discoveryClient := &http.Client{


creating an http client on every sync doesn't seem right, we should reuse it if restConfig hasn't changed.

p0lyn0mial · 2020-02-17T09:55:38Z

staging/src/k8s.io/kube-aggregator/pkg/util/dynamic_restconfig.go

+}
+
+// DynamicRestConfigProvider provides a restconfig and transport backed by dynamically loaded cert/key pair
+type DynamicRestConfigProvider struct {


It seems that available_controller only needs a restConfig - slightly different one than proxy_handlers. I think that before it didn't use MTLS, now it does. I don't know why it was like that before but it should work with MTLS as well, right?

p0lyn0mial · 2020-02-17T09:59:46Z

staging/src/k8s.io/kube-aggregator/pkg/util/dynamic_restconfig.go

+	// start timer that rechecks every minute, just in case.  this also serves to prime the controller quickly.
+	go wait.Until(func() {
+		c.Enqueue()
+	}, 1*time.Minute, stopCh)


do we have to call it every minute if it already supports dynamiccertificates.Notifier ?

no :) I actually think we might not need the whole Run method 🤔 removing it...

what if the underlying dynamic file read doesn't support "notification" ?:) I think we need to cover both paths.

p0lyn0mial · 2020-02-17T10:12:44Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

@@ -301,14 +304,18 @@ func (s *APIAggregator) AddAPIService(apiService *v1.APIService) error {
 		proxyPath = "/api"
 	}

+	restConfigProvider, err := util.NewDynamicRestConfigProvider(s.certKeyContentProvider, s.egressSelector, s.proxyTransport, apiService.Spec.InsecureSkipTLSVerify, apiService.Spec.Service.Name + "." + apiService.Spec.Service.Namespace + ".svc", apiService.Spec.CABundle)


we need to stop it when RemoveAPIService was called.

oh good point... but right now when I removed the Run method it's actually just implementing Notifier, so there's nothing to stop anymore, correct?

The only think that's "running" right now is the dynamic file reader and that is reused across all restconfig providers so that should stop with the whole process going away

hmm okay, but we still use up the memory we should clean up, wdyt?

k8s-ci-robot · 2020-02-18T12:13:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alenkacz
To complete the pull request process, please assign deads2k
You can assign the PR to them by writing /assign @deads2k 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:

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

fedebongio · 2020-02-18T21:05:36Z

/assign @deads2k

alenkacz · 2020-05-31T12:14:13Z

/retest

alenkacz · 2020-05-31T18:48:56Z

/retest

deads2k · 2020-06-05T14:49:32Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

@@ -177,11 +176,21 @@ func (c completedConfig) NewWithDelegate(delegationTarget genericapiserver.Deleg
 		5*time.Minute, // this is effectively used as a refresh interval right now.  Might want to do something nicer later on.
 	)

+	var certKeyContentProvider *dynamiccertificates.DynamicCertKeyPairContent
+	if len(c.ExtraConfig.ProxyClientCertFile) > 0 && len(c.ExtraConfig.ProxyClientKeyFile) > 0 {


if there isn't a client cert/key pair, then the front proxy fails and we should return an error, right?

deads2k · 2020-06-05T14:51:38Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go

@@ -234,6 +243,12 @@ func (c completedConfig) NewWithDelegate(delegationTarget genericapiserver.Deleg
 		c.GenericConfig.SharedInformerFactory.Start(context.StopCh)
 		return nil
 	})
+	if certKeyContentProvider != nil {


I wouldn't expect certKeyContentProvider to be allowed to be nil.

deads2k · 2020-06-05T15:00:18Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

@@ -154,13 +144,19 @@ func (r *proxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	newReq, cancelFn := newRequestForProxy(location, req)
 	defer cancelFn()

-	if handlingInfo.proxyRoundTripper == nil {
+	roundTripper, err := transport.New(handlingInfo.transportConfig)


previously, we re-used the same transport for all requests instead of building a fresh one. Does this carry performance penalties in terms of object creation and GC?

@liggitt

Yes, building this per request is expensive, especially when we set options (see comment on GetCert usage) that means every time updateAPIService is called we are guaranteed to no longer use the previously cached TLS config.

deads2k · 2020-06-05T15:03:38Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

+		TLS: transport.TLSConfig{
+			Insecure: apiService.Spec.InsecureSkipTLSVerify,
+			GetCert: func() (*tls.Certificate, error) {
+				cert, key := r.certKeyContentProvider.CurrentCertKeyContent()


my understanding is that we would do this for every negotiation, but we want is only to reload this if there was a change, right?

I'd be supportive of a change that adds a GetCert compatible function to the certKeyContentProvider to allow directly wiring the option here. This way we can write it to be aware of when it has changed.

deads2k · 2020-06-05T15:04:54Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

 		}
+		transportConfig.Dial = egressDialer


Why would we do this if the previous call returned an error?

deads2k · 2020-06-05T15:05:55Z

staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller.go

+		if err != nil {
+			return nil, err
+		}
+		transportConfig.Dial = egressDialer


I don't understand why we use this value if the previous was nil

liggitt · 2020-06-05T15:57:47Z

cmd/kube-apiserver/app/aggregator.go

@@ -95,28 +94,16 @@ func createAggregatorConfig(
 		return nil, err
 	}

-	var certBytes, keyBytes []byte


should we make sure the files are readable at startup? a missing file error is easier to diagnose than weird anonymous request errors because the certs couldn't load in the background

liggitt · 2020-06-05T16:06:59Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

+	transportConfig := &transport.Config{
+		TLS: transport.TLSConfig{
+			Insecure: apiService.Spec.InsecureSkipTLSVerify,
+			GetCert: func() (*tls.Certificate, error) {


every time we create a GetCert function, we create a unique config that busts the cache of reusable client configs (the cache key for this is computed based on the function pointer address, see https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/transport/cache.go#L134)

in contrast, the previous version set static options that reused the cached tls config (and the existing kept-alive backend connections)

The TLS config cache has no size limit, which means this is effectively introducing a slow memory leak if APIService conditions flutter.

Hmm does the pointer address change though? 🤔This is implemented with the idea/impression that it does not change, am I wrong?

To test this, I wrote a following test:

created a setup similar to the TestProxyCertReload

checked the address in cache keey after calling updateAPIService

wrote new certs to disk

called updateAPIService again

checked the address in cache

The address was the same.

@liggitt I've opened adcd6e1 that demonstrates the key returned from the cache is the same as long as the address of GetCert function doesn't change

@liggitt we could extend certKeyContentProvider to provide GetCert function so that the address stays the same and change the cache to take into account the content, not the address.

I've opened 8520587 to demonstrate that. It still creates a new transport for each request. Most of the time the transport will be retrieved from the cache but even that incurs a small performance cost of acquiring a lock.

We could move the code back to updateAPIService method and reload the handler on the certificate change. In that case, this approach seems to be more complicated than #92791.

@liggitt I've opened adcd6e1 that demonstrates the key returned from the cache is the same as long as the address of GetCert function doesn't change

hmm... if a function with the same pointer address can return different data to different callers, that means some of the existing ways we build the client config cache could have problems.

@liggitt we could extend certKeyContentProvider to provide GetCert function so that the address stays the same and change the cache to take into account the content, not the address.

Wouldn't that compute the cache key based on the initial cert content, which would conflate client configurations that should be kept distinct? For example, config A and config B could point to different file paths /a.ca and /b.ca which happen to contain the same content at startup... if those are being reloaded dynamically, we wouldn't want to make those share a client, since at a later point in time, those files' content could diverge.

hmm... if a function with the same pointer address can return different data to different callers, that means some of the existing ways we build the client config cache could have problems.

comparing the address of a function doesn't seem right since golang forbids comparing the addresses of functions in general.

would you accept a patch that changes this behavior? Ideally, if we could serialize getCert and compare the bytes.

comparing the address of a function doesn't seem right since golang forbids comparing the addresses of functions in general.

yeah, I'm not thrilled with the approach... the places we do that currently are for non-serializable injected implementations.

would you accept a patch that changes this behavior? Ideally, if we could serialize getCert and compare the bytes.

we'd need a way to determine the identity / compare instances of the dynamic methods (GetCert, Dial, and Proxy)

liggitt · 2020-06-05T16:09:05Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

@@ -154,13 +144,19 @@ func (r *proxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	newReq, cancelFn := newRequestForProxy(location, req)
 	defer cancelFn()

-	if handlingInfo.proxyRoundTripper == nil {
+	roundTripper, err := transport.New(handlingInfo.transportConfig)


Yes, building this per request is expensive, especially when we set options (see comment on GetCert usage) that means every time updateAPIService is called we are guaranteed to no longer use the previously cached TLS config.

p0lyn0mial · 2020-07-15T07:18:16Z

/close

I'm closing this PR in favor of #92791. Although the solution presented in this PR looked appealing we missed a few important details. Many thanks, @alenkacz for your outstanding work and uncovering a long-lasting issue in the TLS cache!

k8s-ci-robot · 2020-07-15T07:18:23Z

@alenkacz: 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.

k8s-ci-robot · 2020-07-15T07:18:30Z

@p0lyn0mial: Closed this PR.

In response to this:

/close

I'm closing this PR in favor of #92791. Although the solution presented in this PR looked appealing we missed a few important details. Many thanks, @alenkacz for your outstanding work and uncovering a long-lasting issue in the TLS cache!

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 requested review from deads2k and logicalhan February 13, 2020 16:13

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 13, 2020

alenkacz commented Feb 13, 2020

View reviewed changes

alenkacz changed the title ~~WIP: Dynamically reload proxy certs~~ WIP: Dynamically reload kube-aggregator certificates Feb 13, 2020

k8s-ci-robot assigned p0lyn0mial Feb 13, 2020

p0lyn0mial reviewed Feb 14, 2020

View reviewed changes

p0lyn0mial reviewed Feb 17, 2020

View reviewed changes

alenkacz force-pushed the av/aggregator-cert-reload branch 3 times, most recently from 7ee8963 to 7e57a28 Compare February 18, 2020 12:11

k8s-ci-robot added the area/apiserver label Feb 18, 2020

k8s-ci-robot assigned deads2k Feb 18, 2020

alenkacz force-pushed the av/aggregator-cert-reload branch 2 times, most recently from cf9c206 to 4f4c506 Compare February 19, 2020 10:34

alenkacz force-pushed the av/aggregator-cert-reload branch from 523f48c to f6163ad Compare May 23, 2020 07:57

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 23, 2020

alenkacz changed the title ~~WIP: Dynamically reload kube-aggregator certificates~~ Dynamically reload kube-aggregator certificates May 23, 2020

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 May 23, 2020

alenkacz force-pushed the av/aggregator-cert-reload branch 6 times, most recently from 393d147 to a1ef7fc Compare May 29, 2020 09:18

alenkacz force-pushed the av/aggregator-cert-reload branch from a1ef7fc to 425e744 Compare May 31, 2020 10:03

alenkacz force-pushed the av/aggregator-cert-reload branch from 425e744 to 3ca3ae2 Compare May 31, 2020 13:01

Dynamically reload proxy certs

6ed887b

alenkacz force-pushed the av/aggregator-cert-reload branch from 3ca3ae2 to 6ed887b Compare May 31, 2020 14:10

deads2k reviewed Jun 5, 2020

View reviewed changes

liggitt reviewed Jun 5, 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 Jul 15, 2020

k8s-ci-robot closed this Jul 15, 2020


		var _ dynamiccertificates.Listener = &DynamicRestConfigProvider{}

		func NewDynamicRestConfigProvider(servingContent dynamiccertificates.CertKeyContentProvider, egressSelector egressselector.EgressSelector, proxyTransport http.Transport, insecure bool, serverName string, caBundle []byte) (*DynamicRestConfigProvider, error) {

Dynamically reload kube-aggregator certificates #88120

Dynamically reload kube-aggregator certificates #88120

Conversation

alenkacz commented Feb 13, 2020

Choose a reason for hiding this comment

deads2k commented Feb 13, 2020

alenkacz commented Feb 13, 2020

p0lyn0mial commented Feb 14, 2020

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

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

alenkacz Feb 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 18, 2020

fedebongio commented Feb 18, 2020

alenkacz commented May 31, 2020

alenkacz commented May 31, 2020

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

p0lyn0mial Jul 2, 2020 • edited

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

liggitt Jul 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Jul 15, 2020

k8s-ci-robot commented Jul 15, 2020

k8s-ci-robot commented Jul 15, 2020

alenkacz Feb 17, 2020 •

edited

p0lyn0mial Jul 2, 2020 •

edited

liggitt Jul 6, 2020 •

edited