WIP: client-go/transport: support reloading CA file #122749

enj · 2024-01-12T22:27:12Z

/kind bug
/triage accepted
/cc p0lyn0mial aojea stlaz

Fixes #119483

TODO

Signed-off-by: Monis Khan <mok@microsoft.com>

k8s-ci-robot · 2024-01-12T22:27:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enj
Once this PR has been reviewed and has the lgtm label, please assign yliaog for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

staging/src/k8s.io/client-go/OWNERS

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

Signed-off-by: Monis Khan <mok@microsoft.com>

aojea · 2024-01-12T23:02:54Z

staging/src/k8s.io/client-go/transport/cache.go

+			newRT := baseRT.Clone()
+			newRT.TLSClientConfig.RootCAs = rootCAs
+
+			transport.rt.Store(newRT)


how does this behave with existing connections?

Connection tracking happens at the round tripper so once the old rt is unused those connections would be unused (presumably with some cleanup at the OS level once the Go GC cleans up).

I think calling CloseIdleConnections on the old transport would be appropriate.

maybe we should reuse dynamicCertDialer := certRotatingDialer(tlsConfig.GetClientCertificate, dial) to close all connections ? (just in case the old rootCAs becomes invalid)

I think you would need a more targeted approach to only close connections associated with the old transport (which is hard to do since the dialer is shared).

but at the time of the reload because you have a new rootCA you'll only have connections that need to be disrupted because you only have the old transport, it kind of looks based on your explanations that certRotatingDialer watching the rootca as you are doing here seems the best approach

at the time of the reload because you have a new rootCA you'll only have connections that need to be disrupted

@aojea this is not true because the server will not close existing connections nor will the client need to verify the old connections with the new root (and servers are supposed to do graceful rotation of their roots anyway but even without that the old connections will work fine).

oh, I see you want to do seamless rotation, I was assuming that you could do the same we do in kubelet, cleanup all the connections and restart with the new certificate all the connections ...

We need the hard cut over with client certs because of how the API server handles authentication. We don't need it for root CA changes, so it seems better to gracefully rotate.

k8s-ci-robot · 2024-01-12T23:32:12Z

@enj: 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-kubernetes-unit	`34225d2`	link	true	`/test pull-kubernetes-unit`

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.

p0lyn0mial · 2024-01-15T09:31:54Z

staging/src/k8s.io/client-go/transport/cache.go

+		caFile := config.TLS.CAFile
+		caData := config.TLS.CAData
+		baseRT := transport.rt.Load().Clone()
+		go wait.UntilWithContext(wait.ContextForChannel(DialerStopCh), func(_ context.Context) {


this should be controller.

Sure, mostly meant as a POC.

p0lyn0mial · 2024-01-15T09:34:59Z

staging/src/k8s.io/client-go/transport/cache.go

@@ -127,14 +148,47 @@ func (c *tlsTransportCache) get(config *Config) (http.RoundTripper, error) {
 		proxy = config.Proxy
 	}

-	transport := utilnet.SetTransportDefaults(&http.Transport{
+	transport := buildReloadableTransport(utilnet.SetTransportDefaults(&http.Transport{


I think it can actually work!

aojea · 2024-01-16T09:05:26Z

@enj @p0lyn0mial I was yesterday trying to reproduce this behavior locally and I have several knowledge gaps to understand how this works?

When the servers reloads the root CA?

does the server closes the existing connections?
the established connections by the client starts to fail?
if the client was using certificates, it has to acquire a new client certificate too?
...

p0lyn0mial · 2024-01-16T09:51:56Z

@enj @p0lyn0mial I was yesterday trying to reproduce this behavior locally and I have several knowledge gaps to understand how this works?

When the servers reloads the root CA?

does the server closes the existing connections?
the established connections by the client starts to fail?
if the client was using certificates, it has to acquire a new client certificate too?

I assume you are referring to the CA used to validate the incoming connections (from clients in that case). In that case I think that the server won't terminate already established connections. The server wires a controller into tls. GetConfigForClient which returns a dynamic tls.Config which is used only for new connections.

@stlaz pointed me yesterday to the commit that explains certRotatingDialer but we were unsure if this could happen.

Close outbound connections when using a cert callback and certificates rotate. This means that we won't get into a situation where we have open TLS connections using expires certs, which would get unauthorized errors at the apiserver

Attempt to retrieve a new certificate if open connections near expiry, to prevent the case where the cert expires but we haven't yet opened a new TLS connection and so GetClientCertificate hasn't been called.

enj · 2024-01-16T22:54:38Z

@enj @p0lyn0mial I was yesterday trying to reproduce this behavior locally and I have several knowledge gaps to understand how this works?

When the servers reloads the root CA?
* does the server closes the existing connections?

No, the new TLS config is only used for new TLS connections.

* the established connections by the client starts to fail?

No, because they are using the old TLS config. And also, root CA rotation is expected to use approaches like cross signing to avoid disrupting clients.

* if the client was using certificates, it has to acquire a new client certificate too?
  ...

Client certs (can) have a different set of trust roots than server CA, so these operations are not linked. We do not close connections when changing the client cert roots on the server, hence stuff like #120621 can happen because authentication happens per request, not per connection (related to the comment above from @stlaz and @p0lyn0mial).

k8s-triage-robot · 2024-04-16T21:07:55Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-05-16T21:28:18Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

client-go/transport: support reloading CA file

8471619

Signed-off-by: Monis Khan <mok@microsoft.com>

k8s-ci-robot requested a review from aojea January 12, 2024 22:27

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 12, 2024

k8s-ci-robot requested a review from p0lyn0mial January 12, 2024 22:27

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 Jan 12, 2024

k8s-ci-robot requested a review from stlaz January 12, 2024 22:27

enj mentioned this pull request Jan 12, 2024

client-go doesn't properly handle reloading trust anchors during cluster CA rotation #119483

Open

fixup! client-go/transport: support reloading CA file

34225d2

Signed-off-by: Monis Khan <mok@microsoft.com>

aojea reviewed Jan 12, 2024

View reviewed changes

p0lyn0mial reviewed Jan 15, 2024

View reviewed changes

tkashem mentioned this pull request Feb 20, 2024

[WIP] client-go/transport: support dynamic reloading of CA file #123400

Open

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

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 16, 2024

enj closed this Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: client-go/transport: support reloading CA file #122749

WIP: client-go/transport: support reloading CA file #122749

enj commented Jan 12, 2024

k8s-ci-robot commented Jan 12, 2024

aojea Jan 12, 2024

enj Jan 12, 2024

enj Jan 14, 2024

p0lyn0mial Jan 15, 2024

enj Jan 16, 2024

aojea Jan 17, 2024 •

edited

enj Jan 17, 2024

aojea Jan 17, 2024

enj Jan 17, 2024

k8s-ci-robot commented Jan 12, 2024

p0lyn0mial Jan 15, 2024

enj Jan 16, 2024

p0lyn0mial Jan 15, 2024

p0lyn0mial Jan 15, 2024

aojea commented Jan 16, 2024

p0lyn0mial commented Jan 16, 2024

enj commented Jan 16, 2024

k8s-triage-robot commented Apr 16, 2024

k8s-triage-robot commented May 16, 2024

WIP: client-go/transport: support reloading CA file #122749

WIP: client-go/transport: support reloading CA file #122749

Conversation

enj commented Jan 12, 2024

k8s-ci-robot commented Jan 12, 2024

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

aojea Jan 17, 2024 • 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

k8s-ci-robot commented Jan 12, 2024

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

aojea commented Jan 16, 2024

p0lyn0mial commented Jan 16, 2024

enj commented Jan 16, 2024

k8s-triage-robot commented Apr 16, 2024

k8s-triage-robot commented May 16, 2024

aojea Jan 17, 2024 •

edited