/triage accepted

labeling as a regression since a configuration that worked prior to 1.25 no longer has an equivalent that works properly

/kind regression

I see a few directions a fix could go:

1. exec auth plugin could avoid adding a GetCert hook if the plugin config indicated it would only return tokens. This would require more exec plugin config and wouldn't fix the issue for exec auth plugins that do return cert credentials
2. client-go transport config/caching could try to enabling caching transports for configs which set a GetCert callback, but we tried to figure out how to do this in the past and couldn't find a way to do it safely
3. client-go consumers that repeatedly create transports (primarily kubectl commands) could restructure to avoid doing that

All three of those changes seem likely to be invasive and inappropriate to make at the last minute for 1.25.

Option 3 seems like the proper fix, but also the one with the biggest surface area, since it would require checking all the kubectl commands, and likely some significant restructuring in how clients are constructed

1. exec auth plugin could avoid adding a GetCert hook if the plugin config indicated it would only return tokens. This would require more exec plugin config and wouldn't fix the issue for exec auth plugins that _do_ return cert credentials

I would like to avoid this approach as projects such as pinniped exclusively use client certificates for end user facing authentication.

1. exec auth plugin could avoid adding a GetCert hook if the plugin config indicated it would only return tokens. This would require more exec plugin config and wouldn't fix the issue for exec auth plugins that _do_ return cert credentials

I would like to avoid this approach as projects such as pinniped exclusively use client certificates for end user facing authentication.

me too, that doesn't really seem like a fix

it looks like the thing in kubectl that is repeatedly constructing clients is the Builder... there might be a central way to make that use the "construct http.Client once, reuse for multiple clientsets" that @aojea made last release... I'm looking into that now

/cc @cici37

it looks like the thing in kubectl that is repeatedly constructing clients is the Builder... there might be a central way to make that use the "construct http.Client once, reuse for multiple clientsets" that @aojea made last release... I'm looking into that now

/cc @soltysh @atiratree

adding them to the loop, there was also a WIP PR #108459 to fix this

it looks like the thing in kubectl that is repeatedly constructing clients is the Builder... there might be a central way to make that use the "construct http.Client once, reuse for multiple clientsets" that @aojea made last release... I'm looking into that now

/cc @soltysh @atiratree

adding them to the loop, there was also a WIP PR #108459 to fix this

oof... I didn't know this was a known issue... I'd have pushed to resolve this before forcing the migration to exec auth :-/

expanding on #111911 (comment), I see our options for 1.25 as:

1. ship 1.25 as-is with this documented as a known kubectl issue
   • removal of gcp and azure credential providers forces migration to exec auth with a known kubectl issue without a reasonable workaround
2. delay 1.25 until the kubectl issue is fixed or worked around
   • current WIP fix in share transport in kubectl #108459 is not small, and I would characterize as ~low-medium risk (it's hard to reason about the impact on all callers of some of the code paths being changed). I'd estimate the best case at O(days) for the fix to land, and I would expect another release candidate given the scope of the changes as well. this fix also relies on work @aojea did that is only in 1.23+, so it couldn't be backported to 1.22, which is still in support
   • a workaround-style fix inside the exec auth plugin could maybe avoid triggering the cert use case, which would avoid regressing the gcp and azure providers (which use tokens), but would require changing exec-auth plugin logic, which I wouldn't be confident in backporting
   • even if the fix lands in 1.25, it only fixes 1.25 kubectl unless we backport and wait for backport patch releases, so we'd still be forcing migration to exec auth with a known kubectl issue in still-active/supported patch releases without a reasonable workaround
3. (selected after discussing with @enj and @cici37) restore the removed in-tree auth plugins in 1.25, document the known kubectl issue in 1.25, and fix the kubectl issue in 1.26

As loathe as I am to pick path 3, I think it's the right call for users and the release schedule, and I think we still have a clear path to actually remove the gcp and azure credential providers in 1.26 if we:

• fix the kubectl issue early in 1.26 with a scoped-down/safer version of share transport in kubectl #108459
• backport the kubectl fix to 1.23, 1.24, 1.25 patch releases (can't backport further than 1.23 because the fix depends on client-go: share the same transport for generated clientsets #105490, but 1.22 goes out of support before 1.26.0 is released)
• force the exec auth migration in 1.26 once all supported minor versions of kubectl have a patch version available that resolves the issue

+1 to the proposed plan (3 bullet points) thanks @liggitt @aojea @enj

revert for 1.25 open at #111918 😞

Thank you for addressing this and raising the revert PR!
Agree on do it right even if it's slow!

Reverting this really kills me, but I agree that we should:

1. Not break users in ways that they cannot reasonably workaround
2. Not risk the release with large changes on a late timeline

Lets get this fixed correctly early in the 1.26 release so that we can backport the fix to 1.23+

revert PR is merged for 1.25 and in the release-1.25 branch

dropping the regression label from this issue now (since it's just an exec auth bug now, not a 1.25 regression) and adding important-soon priority for 1.26. Will work with @soltysh to get #108459 in a backport-safe shape, targeting merge first thing in 1.26

Optimizations on top of cutting down the number of new transports we make:
For plugins that do return a cert, I'd be happy to cache that cert based on its expiry date (notAfter) - perhaps with a max TTL.
For plugins that return tokens, we could provide a way for the plugin to additionally assert a freshness period and / or maximum number of uses for the token.

For some cases it could be useful for tokens to be strictly single use.

We need to update kubernetes/website#35981 to reflect the new situation.

Optimizations on top of cutting down the number of new transports we make:
For plugins that do return a cert, I'd be happy to cache that cert based on its expiry date (notAfter) - perhaps with a max TTL.
For plugins that return tokens, we could provide a way for the plugin to additionally assert a freshness period and / or maximum number of uses for the token.

The exec auth plugin can already do both of those things (can return expiration timestamps that avoid additional calls to the plugin to get new credentials). In this case, we weren't actually calling the authenticator multiple times, the same cached credentials were getting used with transports and talking to the server over multiple connections

We need to update kubernetes/website#35981 to reflect the new situation.

thanks for pointing out the docs update needed

We need to update kubernetes/website#35981 to reflect the new situation.

I have left a comment earlier in kubernetes/website#35981 (comment) and the update is done. The CHANGLOG update is covered in kubernetes/sig-release#1995. We still need a release note update. Since it was merged after rc1 cut, the release notes has to be added manually. I am working with release notes team on it: kubernetes/sig-release#1996 (comment)

I can try to improve #108459 next week and make it safer for backports

/assign

I have opened #112017 to fix this bug. I believe the change is small and safe enough to backport.