Skip to content

Add client-go credential plugin allowlist to kuberc#134870

Merged
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
pmengelbert:pmengelbert/kuberc/4
Nov 10, 2025
Merged

Add client-go credential plugin allowlist to kuberc#134870
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
pmengelbert:pmengelbert/kuberc/4

Conversation

@pmengelbert
Copy link
Copy Markdown
Contributor

@pmengelbert pmengelbert commented Oct 24, 2025

What type of PR is this?

/kind feature

What this PR does / why we need it:

It permits the user to specify a client-go credential plugin allowlist as a security knob. The user may specify it in kuberc. See design details in KEP-3104

Which issue(s) this PR is related to:

KEP: kubernetes/enhancements#3104

Special notes for your reviewer:

Does this PR introduce a user-facing change?

yes.

Changed kuberc configuration schema. Two new optional fields added to kuberc configuration, `credPluginPolicy` and `credPluginAllowlist`. This is documented in [KEP-3104](https://github.com/kubernetes/enhancements/blob/master/keps/sig-cli/3104-introduce-kuberc/README.md#allowlist-design-details) and documentation is added to the website by [kubernetes/website#52877](https://github.com/kubernetes/website/pull/52877)

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

- [KEP]: https://github.com/kubernetes/enhancements/issues/3104
- [Usage]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-cli/3104-introduce-kuberc/README.md#allowlist-design-details
- [Other doc]: https://github.com/kubernetes/website/pull/52877

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 24, 2025
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

Hi @pmengelbert. Thanks for your PR.

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

Details

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-sigs/prow 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. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 24, 2025
@k8s-ci-robot k8s-ci-robot added area/code-generation area/kubectl kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 24, 2025
@github-project-automation github-project-automation Bot moved this to Needs Triage in SIG CLI Oct 24, 2025
@enj enj moved this to Needs Triage in SIG Auth Oct 25, 2025
@pmengelbert
Copy link
Copy Markdown
Contributor Author

/sig auth
/sig cli
/sig api-machinery

@enj
Copy link
Copy Markdown
Member

enj commented Oct 25, 2025

/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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 25, 2025
@enj
Copy link
Copy Markdown
Member

enj commented Nov 6, 2025

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2025
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: bee30272d37f8c9f44904d85752b91c789574cbd

@enj
Copy link
Copy Markdown
Member

enj commented Nov 6, 2025

/remove-label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot removed the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 6, 2025
@liggitt liggitt added the api-review Categorizes an issue or PR as actively needing an API review. label Nov 6, 2025
@liggitt liggitt self-assigned this Nov 6, 2025
Comment thread staging/src/k8s.io/kubectl/pkg/cmd/cmd.go
Comment thread staging/src/k8s.io/kubectl/pkg/cmd/cmd.go
Comment thread staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go
}
}

func (a *Authenticator) checkAllowlist() error {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if the allowlist already contains the exact command, I think we can return early/fast (since LookPath would resolve them the same way)

  • cmd="baz", allowlist=["foo","bar","baz"] can return true fast without needing to call LookPath at all
  • cmd="baz", allowlist=["/usr/bin/foo","/usr/bin/bar","/usr/bin/baz"] has to call LookPath to resolve cmd, then can see that exists verbatim in the allowlist already, and doesn't need to call LookPath at all
  • cmd="baz", allowlist=["../foo","../bar","../baz"] has to call LookPath to resolve cmd, and then LookPath on allowlist items to see if they match (but should probably check the one whose basename matches the cmd first to avoid calling LookPath a ton of times

Comment thread staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go
// will be called on both, and the resulting strings must be equal. If
// either call to `exec.LookPath` results in an error, the `Name` check
// will be considered a failure.
Name string `json:"name,omitempty"`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

make sure the internal types are non-serializable with json:"-"

import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

serializable v1beta1/types.go should only be referencing serializable versioned types, not internal types. I think it will be clearer to define these locally in staging/src/k8s.io/kubectl/pkg/config/types.go and staging/src/k8s.io/kubectl/pkg/config/v1beta1/types.go and translate them into internal clientcmd policy types in one spot in applyPluginPolicy

Comment thread staging/src/k8s.io/kubectl/pkg/config/types.go Outdated
Comment thread staging/src/k8s.io/client-go/tools/clientcmd/api/v1/types.go Outdated
// at the explicit path `/usr/local/bin/my-plugin`.
// +optional
// +listType=atomic
CredentialPluginAllowlist []clientcmdapi.AllowlistEntry `json:"credentialPluginAllowlist,omitempty"`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is currently referencing types defined in internal packages and counting on them having json tags that make them serializable... that's pretty confusing.

Keep the kuberc types all defined under kubectl/pkg/config/types.go and kubectl/pkg/config/v1beta1/types.go where they can follow normal behavior for json tags

Copy link
Copy Markdown
Member

@enj enj left a comment

Choose a reason for hiding this comment

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

Mostly minor comments.

Comment thread staging/src/k8s.io/client-go/tools/clientcmd/validation_test.go
Comment thread staging/src/k8s.io/kubectl/pkg/config/types.go
Comment thread staging/src/k8s.io/kubectl/pkg/config/v1alpha1/conversion.go
Comment thread staging/src/k8s.io/kubectl/pkg/config/v1beta1/types.go
Comment thread staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go Outdated
Comment thread staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go Outdated
Comment thread staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go Outdated
Comment thread staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go
@pmengelbert
Copy link
Copy Markdown
Contributor Author

/retest

2 similar comments
@pmengelbert
Copy link
Copy Markdown
Contributor Author

/retest

@pmengelbert
Copy link
Copy Markdown
Contributor Author

/retest

)

// compile-time check that these two types are aligned
var _ = clientcmdapi.AllowlistEntry(config.AllowlistEntry{})
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm surprised this works… did you make sure this flags struct differences if they occur?

Comment thread staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go Outdated
@liggitt
Copy link
Copy Markdown
Member

liggitt commented Nov 9, 2025

/lgtm
/approve

this lgtm, will add milestone once exception request is approved

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: 44f560a44870fb672691d164a7dd6ef00b82aeb0

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, pmengelbert, soltysh

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

The pull request process is described here

Details 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

Remove reference to internal types in kuberc types

* Remove unserialized types from public APIs

Also remove defaulting

* Don't do conversion gen for plugin policy types

Because the plugin policy types are explicitly allowed to be empty, they
should not affect conversion. The autogenerated conversion functions for
the `Preference` type will leave those fields empty.

* Remove defaulting tests

Comments and simplifications (h/t jordan liggitt)

Signed-off-by: Peter Engelbert <[email protected]>
@liggitt
Copy link
Copy Markdown
Member

liggitt commented Nov 9, 2025

/lgtm

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: 7ef9367f4abe1ed346e105255bd50b818102ce0e

@enj
Copy link
Copy Markdown
Member

enj commented Nov 10, 2025

/milestone v1.35

@enj
Copy link
Copy Markdown
Member

enj commented Nov 10, 2025

(exception was approved)

// will be called on both, and the resulting strings must be equal. If
// either call to `exec.LookPath` results in an error, the `Name` check
// will be considered a failure.
Name string
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sigh... reviewing the docs PR, I just noticed this doesn't match the field name in kubeconfigs for exec plugins ... those use command

I think I got confused with the name field of kubelet image credential providers.

I think it's too late to rework this for 1.35, but let's make a note to rename this field to command in the v1 promotion to match kubeconfig

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Opened #135424 to track this work.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Based on my understanding, I think there should also be a ticket to update the documentation, right? @soltysh

}
entryResolvedPath, err := exec.LookPath(entry.Name)
if err != nil {
klog.V(5).ErrorS(err, "resolving credential plugin allowlist", "name", entry.Name)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this an error that someone has to see and react to or is this a debug message?

A helper method in client-go should not use the global klog. You don't know in which context it gets called and whether the caller wants this log output.

If a helper method uses logging, make the caller aware by having klog.Logger or context.Context in the API of the helper.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Asking more broadly: what is the user experience if this happens? Can it happen?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, should resolving the certificate in roundTripper.RoundTrip (= r.a.getCreds()) honor the cancellation of the context of the http.Request?

It seems so, because getCreds under the hood does some blocking and potentially long-running operations, like invoking a command, right?

Unfortunately that ripples up into the public client-go API 😢

Here's an incomplete patch (note the context.TODO when setting tlsConfig.GetClientCertificate):

diff --git a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go
index f21ca89bb16..e074569f905 100644
--- a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go
+++ b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go
@@ -18,6 +18,7 @@ package exec
 
 import (
 	"bytes"
+	"context"
 	"crypto/tls"
 	"crypto/x509"
 	"errors"
@@ -364,7 +365,7 @@ func (r *roundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
 	}
 
 	ctx := req.Context()
-	creds, err := r.a.getCreds()
+	creds, err := r.a.getCreds(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("getting credentials: %v", err)
 	}
@@ -377,7 +378,7 @@ func (r *roundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
 		return nil, err
 	}
 	if res.StatusCode == http.StatusUnauthorized {
-		if err := r.a.maybeRefreshCreds(creds); err != nil {
+		if err := r.a.maybeRefreshCreds(ctx, creds); err != nil {
 			klog.FromContext(ctx).Error(err, "Refreshing credentials failed")
 		}
 	}
@@ -391,15 +392,15 @@ func (a *Authenticator) credsExpired() bool {
 	return a.now().After(a.exp)
 }
 
-func (a *Authenticator) cert() (*tls.Certificate, error) {
-	creds, err := a.getCreds()
+func (a *Authenticator) cert(ctx context.Context) (*tls.Certificate, error) {
+	creds, err := a.getCreds(ctx)
 	if err != nil {
 		return nil, err
 	}
 	return creds.cert, nil
 }
 
-func (a *Authenticator) getCreds() (*credentials, error) {
+func (a *Authenticator) getCreds(ctx context.Context) (*credentials, error) {
 	a.mu.Lock()
 	defer a.mu.Unlock()
 
@@ -407,7 +408,7 @@ func (a *Authenticator) getCreds() (*credentials, error) {
 		return a.cachedCreds, nil
 	}
 
-	if err := a.refreshCredsLocked(); err != nil {
+	if err := a.refreshCredsLocked(ctx); err != nil {
 		return nil, err
 	}
 
@@ -416,7 +417,7 @@ func (a *Authenticator) getCreds() (*credentials, error) {
 
 // maybeRefreshCreds executes the plugin to force a rotation of the
 // credentials, unless they were rotated already.
-func (a *Authenticator) maybeRefreshCreds(creds *credentials) error {
+func (a *Authenticator) maybeRefreshCreds(ctx context.Context, creds *credentials) error {
 	a.mu.Lock()
 	defer a.mu.Unlock()
 
@@ -427,12 +428,12 @@ func (a *Authenticator) maybeRefreshCreds(creds *credentials) error {
 		return nil
 	}
 
-	return a.refreshCredsLocked()
+	return a.refreshCredsLocked(ctx)
 }
 
 // refreshCredsLocked executes the plugin and reads the credentials from
 // stdout. It must be called while holding the Authenticator's mutex.
-func (a *Authenticator) refreshCredsLocked() error {
+func (a *Authenticator) refreshCredsLocked(ctx context.Context) error {
 	interactive, err := a.interactiveFunc()
 	if err != nil {
 		return fmt.Errorf("exec plugin cannot support interactive mode: %w", err)
@@ -455,7 +456,7 @@ func (a *Authenticator) refreshCredsLocked() error {
 	env = append(env, fmt.Sprintf("%s=%s", execInfoEnv, data))
 
 	stdout := &bytes.Buffer{}
-	cmd := exec.Command(a.cmd, a.args...)
+	cmd := exec.CommandContext(ctx, a.cmd, a.args...)
 	cmd.Env = env
 	cmd.Stderr = a.stderr
 	cmd.Stdout = stdout
@@ -463,7 +464,7 @@ func (a *Authenticator) refreshCredsLocked() error {
 		cmd.Stdin = a.stdin
 	}
 
-	err = a.updateCommandAndCheckAllowlistLocked(cmd)
+	err = a.updateCommandAndCheckAllowlistLocked(ctx, cmd)
 	incrementPolicyMetric(err)
 	if err != nil {
 		return err
@@ -578,14 +579,14 @@ func (a *Authenticator) wrapCmdRunErrorLocked(err error) error {
 // according to the credential plugin policy. If the plugin is allowed, `nil`
 // is returned. If the plugin is not allowed, an error must be returned
 // explaining why.
-func (a *Authenticator) updateCommandAndCheckAllowlistLocked(cmd *exec.Cmd) error {
+func (a *Authenticator) updateCommandAndCheckAllowlistLocked(ctx context.Context, cmd *exec.Cmd) error {
 	switch a.execPluginPolicy.PolicyType {
 	case "", api.PluginPolicyAllowAll:
 		return nil
 	case api.PluginPolicyDenyAll:
 		return fmt.Errorf("plugin %q not allowed: policy set to %q", a.cmd, api.PluginPolicyDenyAll)
 	case api.PluginPolicyAllowlist:
-		return a.checkAllowlistLocked(cmd)
+		return a.checkAllowlistLocked(ctx, cmd)
 	default:
 		return fmt.Errorf("unknown plugin policy %q", a.execPluginPolicy.PolicyType)
 	}
@@ -593,7 +594,7 @@ func (a *Authenticator) updateCommandAndCheckAllowlistLocked(cmd *exec.Cmd) erro
 
 // `checkAllowlistLocked` checks the specified plugin against the allowlist,
 // and may update the Authenticator's allowlistLookup set.
-func (a *Authenticator) checkAllowlistLocked(cmd *exec.Cmd) error {
+func (a *Authenticator) checkAllowlistLocked(ctx context.Context, cmd *exec.Cmd) error {
 	// a.cmd is the original command as specified in the configuration, then filepath.Clean().
 	// cmd.Path is the possibly-resolved command.
 	// If either are an exact match in the allowlist, return success.
@@ -626,7 +627,7 @@ func (a *Authenticator) checkAllowlistLocked(cmd *exec.Cmd) error {
 	}
 
 	// There is no verbatim match
-	a.resolveAllowListEntriesLocked(cmd.Path)
+	a.resolveAllowListEntriesLocked(ctx, cmd.Path)
 
 	// allowlistLookup may have changed, recheck
 	if a.allowlistLookup.Has(cmdResolvedPath) {
@@ -639,7 +640,7 @@ func (a *Authenticator) checkAllowlistLocked(cmd *exec.Cmd) error {
 // resolveAllowListEntriesLocked tries to resolve allowlist entries with LookPath,
 // and adds successfully resolved entries to allowlistLookup.
 // The optional commandHint can be used to limit which entries are resolved to ones which match the hint basename.
-func (a *Authenticator) resolveAllowListEntriesLocked(commandHint string) {
+func (a *Authenticator) resolveAllowListEntriesLocked(ctx context.Context, commandHint string) {
 	hintName := filepath.Base(commandHint)
 	for _, entry := range a.execPluginPolicy.Allowlist {
 		entryBasename := filepath.Base(entry.Name)
@@ -649,7 +650,7 @@ func (a *Authenticator) resolveAllowListEntriesLocked(commandHint string) {
 		}
 		entryResolvedPath, err := exec.LookPath(entry.Name)
 		if err != nil {
-			klog.V(5).ErrorS(err, "resolving credential plugin allowlist", "name", entry.Name)
+			klog.FromContext(ctx).V(5).Info("Resolving credential plugin allowlist", "name", entry.Name, "err", err)
 			continue
 		}
 		if entryResolvedPath != "" {
diff --git a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec_test.go b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec_test.go
index ada512c66a6..c39399104bd 100644
--- a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec_test.go
+++ b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec_test.go
@@ -18,6 +18,7 @@ package exec
 
 import (
 	"bytes"
+	"context"
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rand"
@@ -46,6 +47,7 @@ import (
 	"k8s.io/client-go/pkg/apis/clientauthentication"
 	"k8s.io/client-go/tools/clientcmd/api"
 	"k8s.io/client-go/transport"
+	"k8s.io/klog/v2/ktesting"
 	testingclock "k8s.io/utils/clock/testing"
 )
 
@@ -794,6 +796,7 @@ func TestRefreshCreds(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
+			_, ctx := ktesting.NewTestContext(t)
 			c := test.config
 
 			if c.Command == "" {
@@ -817,7 +820,7 @@ func TestRefreshCreds(t *testing.T) {
 			a.stderr = stderr
 			a.environ = func() []string { return nil }
 
-			if err := a.refreshCredsLocked(); err != nil {
+			if err := a.refreshCredsLocked(ctx); err != nil {
 				if !test.wantErr {
 					t.Errorf("get token %v", err)
 				} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
@@ -966,6 +969,7 @@ func TestPluginPolicy(t *testing.T) {
 	tests := matrix.makeTests(t, testdataDir, path)
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
+			_, ctx := ktesting.NewTestContext(t)
 			c := test.config
 			a, err := newAuthenticator(newCache(), func(_ int) bool { return false }, c, &clientauthentication.Cluster{})
 			if err != nil {
@@ -982,7 +986,7 @@ func TestPluginPolicy(t *testing.T) {
 			a.stderr = stderr
 			a.environ = func() []string { return nil }
 
-			if err := a.refreshCredsLocked(); err != nil {
+			if err := a.refreshCredsLocked(ctx); err != nil {
 				if !test.wantErr {
 					t.Fatalf("unexpected allows plugin error: %v", err)
 				} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
@@ -1354,7 +1358,7 @@ func TestAuthorizationHeaderPresentCancelsExecAction(t *testing.T) {
 
 			// UpdateTransportConfig returns error on existing TLS certificate callback, unless a bearer token is present in the
 			// transport config, in which case it takes precedence
-			cert := func() (*tls.Certificate, error) {
+			cert := func(context.Context) (*tls.Certificate, error) {
 				return nil, nil
 			}
 			tc := &transport.Config{TLS: transport.TLSConfig{Insecure: true, GetCertHolder: &transport.GetCertHolder{GetCert: cert}}}
@@ -1542,6 +1546,7 @@ func TestInstallHintRateLimit(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
+			_, ctx := ktesting.NewTestContext(t)
 			c := api.ExecConfig{
 				Command:         "does not exist",
 				APIVersion:      "client.authentication.k8s.io/v1beta1",
@@ -1561,7 +1566,7 @@ func TestInstallHintRateLimit(t *testing.T) {
 
 			count := 0
 			for i := 0; i < test.calls; i++ {
-				err := a.refreshCredsLocked()
+				err := a.refreshCredsLocked(ctx)
 				if strings.Contains(err.Error(), c.InstallHint) {
 					count++
 				}
diff --git a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics_test.go b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics_test.go
index b50ac89b668..cbd5a7568d0 100644
--- a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics_test.go
+++ b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics_test.go
@@ -25,6 +25,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"k8s.io/client-go/tools/clientcmd/api"
 	"k8s.io/client-go/tools/metrics"
+	"k8s.io/klog/v2/ktesting"
 	"k8s.io/utils/ptr"
 )
 
@@ -112,6 +113,7 @@ func (f *mockCallsMetricCounter) Increment(exitCode int, errorType string) {
 }
 
 func TestCallsMetric(t *testing.T) {
+	_, ctx := ktesting.NewTestContext(t)
 	const (
 		goodOutput = `{
 			"kind": "ExecCredential",
@@ -149,7 +151,7 @@ func TestCallsMetric(t *testing.T) {
 		// Run refresh creds twice so that our test validates that the metrics are set correctly twice
 		// in a row with the same authenticator.
 		refreshCreds := func() {
-			if err := a.refreshCredsLocked(); (err == nil) != (exitCode == 0) {
+			if err := a.refreshCredsLocked(ctx); (err == nil) != (exitCode == 0) {
 				if err != nil {
 					t.Fatalf("wanted no error, but got %q", err.Error())
 				} else {
@@ -179,7 +181,7 @@ func TestCallsMetric(t *testing.T) {
 			t.Fatal(err)
 		}
 		a.stderr = io.Discard
-		if err := a.refreshCredsLocked(); err == nil {
+		if err := a.refreshCredsLocked(ctx); err == nil {
 			t.Fatal("expected the authenticator to fail because the plugin does not exist")
 		}
 		wantCallsMetrics = append(wantCallsMetrics, mockCallsMetric{exitCode: 1, errorType: "plugin_not_found_error"})
@@ -210,6 +212,7 @@ func (f *mockPolicyCallsMetricCounter) Increment(status string) {
 }
 
 func TestPolicyCallsMetric(t *testing.T) {
+	_, ctx := ktesting.NewTestContext(t)
 	const (
 		goodOutput = `{
 			"kind": "ExecCredential",
@@ -282,7 +285,7 @@ func TestPolicyCallsMetric(t *testing.T) {
 		}
 		a.stderr = io.Discard
 
-		err = a.refreshCredsLocked()
+		err = a.refreshCredsLocked(ctx)
 		if err != nil && !test.wantDenied {
 			t.Fatalf("wanted no error, but got %q", err.Error())
 		}
diff --git a/staging/src/k8s.io/client-go/transport/cache_test.go b/staging/src/k8s.io/client-go/transport/cache_test.go
index 54705276d03..36040cf0428 100644
--- a/staging/src/k8s.io/client-go/transport/cache_test.go
+++ b/staging/src/k8s.io/client-go/transport/cache_test.go
@@ -69,7 +69,7 @@ func TestTLSConfigKey(t *testing.T) {
 
 	// Make sure config fields that affect the tls config affect the cache key
 	dialer := net.Dialer{}
-	getCert := &GetCertHolder{GetCert: func() (*tls.Certificate, error) { return nil, nil }}
+	getCert := &GetCertHolder{GetCert: func(ctx context.Context) (*tls.Certificate, error) { return nil, nil }}
 	uniqueConfigurations := map[string]*Config{
 		"proxy":    {Proxy: func(request *http.Request) (*url.URL, error) { return nil, nil }},
 		"no tls":   {},
diff --git a/staging/src/k8s.io/client-go/transport/config.go b/staging/src/k8s.io/client-go/transport/config.go
index d8a3d64b309..37528f73c1f 100644
--- a/staging/src/k8s.io/client-go/transport/config.go
+++ b/staging/src/k8s.io/client-go/transport/config.go
@@ -156,5 +156,5 @@ type TLSConfig struct {
 
 // GetCertHolder is used to make the wrapped function comparable so that it can be used as a map key.
 type GetCertHolder struct {
-	GetCert func() (*tls.Certificate, error)
+	GetCert func(ctx context.Context) (*tls.Certificate, error)
 }
diff --git a/staging/src/k8s.io/client-go/transport/transport.go b/staging/src/k8s.io/client-go/transport/transport.go
index 8fdcc5700b5..62950aeedd5 100644
--- a/staging/src/k8s.io/client-go/transport/transport.go
+++ b/staging/src/k8s.io/client-go/transport/transport.go
@@ -187,7 +187,7 @@ func TLSConfigFor(c *Config) (*tls.Config, error) {
 				return dynamicCertLoader()
 			}
 			if c.HasCertCallback() {
-				cert, err := c.TLS.GetCertHolder.GetCert()
+				cert, err := c.TLS.GetCertHolder.GetCert(context.TODO())
 				if err != nil {
 					return nil, err
 				}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We chose against using exec.CommandContext because many interactive authentication schemes can take longer than the request timeout, and we don't want users to get stuck if the plugin gets terminated before it has a chance to cache credentials after the initial slow login.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We chose against using exec.CommandContext because many interactive authentication schemes can take longer than the request timeout, and we don't want users to get stuck if the plugin gets terminated before it has a chance to cache credentials after the initial slow login.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's continue in #129336.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We chose against using exec.CommandContext because many interactive authentication schemes can take longer than the request timeout, and we don't want users to get stuck if the plugin gets terminated before it has a chance to cache credentials after the initial slow login.

So you keep a goroutine in the client running which waits for the slow command after the request has already timed out? Or does RoundTrip keep running beyond its deadline?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-generation area/kubectl area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Projects

Status: API review completed, 1.35
Archived in project
Archived in project

Development

Successfully merging this pull request may close these issues.

10 participants