Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions pkg/util/membercluster_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,11 +216,10 @@ func BuildClusterConfig(clusterName string,
return nil, fmt.Errorf("the secret for cluster %s is missing a non-empty value for %q", clusterName, clusterv1alpha1.SecretTokenKey)
}

// Initialize cluster configuration.
clusterConfig := &rest.Config{
BearerToken: string(token),
Host: apiEndpoint,
Timeout: defaultTimeout,
// BearerToken omitted: injected by the token-refreshing transport below.
Host: apiEndpoint,
Timeout: defaultTimeout,
}

// Handle TLS configuration.
Expand Down Expand Up @@ -248,6 +247,15 @@ func BuildClusterConfig(clusterName string,
}
}

// Token-refreshing transport — replaces the built-in bearerAuthRoundTripper
// position (outermost) so inner wrappers see auth already set.
clusterConfig.Wrap(NewTokenRefreshingRoundTripperWrapperConstructor(
secretGetter,
cluster.Spec.SecretRef.Namespace,
cluster.Spec.SecretRef.Name,
string(token),
))

return clusterConfig, nil
}

Expand Down
117 changes: 117 additions & 0 deletions pkg/util/membercluster_client_rotation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
/*
Copyright 2026 The Karmada Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package util

import (
"context"
"net/http"
"net/http/httptest"
"sync/atomic"
"testing"
"time"

"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"

clusterv1alpha1 "github.com/karmada-io/karmada/pkg/apis/cluster/v1alpha1"
"github.com/karmada-io/karmada/pkg/util/gclient"
)

type rotatableTokenServer struct {
*httptest.Server
accepted atomic.Value // string
lastAuth atomic.Value // string
}

func newRotatableTokenServer(initialToken string) *rotatableTokenServer {
s := &rotatableTokenServer{}
s.accepted.Store(initialToken)
s.lastAuth.Store("")
s.Server = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
auth := r.Header.Get("Authorization")
s.lastAuth.Store(auth)
if auth != "Bearer "+s.accepted.Load().(string) {
w.WriteHeader(http.StatusUnauthorized)
return
}
w.Header().Set("Content-Type", "application/json")
_, _ = w.Write([]byte(`{"apiVersion":"v1","kind":"Node","metadata":{"name":"foo"}}`))
}))
return s
}

func (s *rotatableTokenServer) accept(token string) { s.accepted.Store(token) }
func (s *rotatableTokenServer) seenAuth() string { return s.lastAuth.Load().(string) }

const (
testTokenCacheTTL = 10 * time.Millisecond
tokenRotateTimeout = 100 * time.Millisecond
tokenRotatePollRate = 5 * time.Millisecond
)

func TestBuildClusterConfig_LongLivedClientPicksUpRotatedToken(t *testing.T) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

confirmed it fails without the fix :)

--- FAIL: TestBuildClusterConfig_LongLivedClientPicksUpRotatedToken (0.04s)
  membercluster_client_rotation_test.go:112:
    Not equal:
      expected: "Bearer token-B"
      actual  : "Bearer token-A"
    Messages: the long-lived client must put the ROTATED token on the wire after rotation
  membercluster_client_rotation_test.go:116:
    Received unexpected error: the server has asked for the client to provide credentials (get nodes foo)
    Messages: after rotation the long-lived client must keep working (pick up token-B without a rebuild)
FAIL  github.com/karmada-io/karmada/pkg/util  0.409s

origTTL := tokenCacheTTL
tokenCacheTTL = testTokenCacheTTL
t.Cleanup(func() { tokenCacheTTL = origTTL })

const clusterName = "member-rotate"

srv := newRotatableTokenServer("token-A")
defer srv.Close()

hostClient := fakeclient.NewClientBuilder().WithScheme(gclient.NewSchema()).WithObjects(
&clusterv1alpha1.Cluster{
ObjectMeta: metav1.ObjectMeta{Name: clusterName},
Spec: clusterv1alpha1.ClusterSpec{
APIEndpoint: srv.URL,
SecretRef: &clusterv1alpha1.LocalSecretReference{Namespace: "ns1", Name: "secret1"},
},
},
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{Namespace: "ns1", Name: "secret1"},
Data: map[string][]byte{
clusterv1alpha1.SecretTokenKey: []byte("token-A"),
clusterv1alpha1.SecretCADataKey: getCACertFromGTestServer(t, srv.Server),
},
},
).Build()

clusterClient, err := NewClusterClientSet(clusterName, hostClient, nil)
assert.NoError(t, err)

_, err = clusterClient.KubeClient.CoreV1().Nodes().Get(context.TODO(), "foo", metav1.GetOptions{})
assert.NoError(t, err, "baseline request with token-A should succeed")

// Rotate: update Secret and reject old token on the server.
secret := &corev1.Secret{}
assert.NoError(t, hostClient.Get(context.TODO(), types.NamespacedName{Namespace: "ns1", Name: "secret1"}, secret))
secret.Data[clusterv1alpha1.SecretTokenKey] = []byte("token-B")
assert.NoError(t, hostClient.Update(context.TODO(), secret))
srv.accept("token-B")

assert.Eventually(t, func() bool {
_, err := clusterClient.KubeClient.CoreV1().Nodes().Get(context.TODO(), "foo", metav1.GetOptions{})
return err == nil
}, tokenRotateTimeout, tokenRotatePollRate,
"after rotation the long-lived client must pick up token-B without a rebuild")

assert.Equal(t, "Bearer token-B", srv.seenAuth(),
"rotated token must appear on the wire")
}
93 changes: 93 additions & 0 deletions pkg/util/round_trippers.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,14 @@ import (
"net/http"
"net/textproto"
"strings"
"sync"
"time"

corev1 "k8s.io/api/core/v1"
"k8s.io/client-go/transport"
"k8s.io/klog/v2"

clusterv1alpha1 "github.com/karmada-io/karmada/pkg/apis/cluster/v1alpha1"
)

type proxyHeaderRoundTripper struct {
Expand Down Expand Up @@ -65,3 +71,90 @@ func parseProxyHeaders(headers map[string]string) http.Header {
}
return proxyHeaders
}

// tokenCacheTTL controls how often the token is re-read from the Secret.
var tokenCacheTTL = 30 * time.Second

// tokenErrorRetryInterval is a shorter TTL used after a failed Secret read,
// so recovery happens faster than a full tokenCacheTTL cycle.
var tokenErrorRetryInterval = 5 * time.Second

// tokenRefreshingRoundTripper re-reads the member cluster token from the Karmada
// Secret on a TTL so long-lived informer clients survive token rotation.
type tokenRefreshingRoundTripper struct {
inner http.RoundTripper
secretGetter func(namespace, name string) (*corev1.Secret, error)
secretNS string
secretName string

mu sync.RWMutex
cachedToken string
cacheExpiry time.Time
}
Comment on lines +84 to +93

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

To prevent head-of-line blocking for all concurrent requests when the token expires, we should avoid holding the read/write lock (t.mu) during the network I/O call to fetch the secret. Adding a separate fetchMu mutex allows us to synchronize the token fetching process independently of token reading.

Suggested change
type tokenRefreshingRoundTripper struct {
inner http.RoundTripper
secretGetter func(namespace, name string) (*corev1.Secret, error)
secretNS string
secretName string
mu sync.RWMutex
cachedToken string
cacheExpiry time.Time
}
type tokenRefreshingRoundTripper struct {
inner http.RoundTripper
secretGetter func(namespace, name string) (*corev1.Secret, error)
secretNS string
secretName string
mu sync.RWMutex
cachedToken string
cacheExpiry time.Time
fetchMu sync.Mutex
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a good point! But I feel secretGetter reads from the controller-runtime in-memory informer cache, which is a map lookup and already fast enough(~100ns). And the write lock is held for that duration only, so concurrent requests are not meaningfully blocked I feel


var _ http.RoundTripper = &tokenRefreshingRoundTripper{}

// WrappedRoundTripper implements utilnet.RoundTripperWrapper.
func (t *tokenRefreshingRoundTripper) WrappedRoundTripper() http.RoundTripper { return t.inner }

// NewTokenRefreshingRoundTripperWrapperConstructor returns a WrapperFunc that injects a TTL-refreshed bearer token.
// secretGetter is expected to be informer-backed (in-memory read, not a live API call).
func NewTokenRefreshingRoundTripperWrapperConstructor(
secretGetter func(string, string) (*corev1.Secret, error),
secretNS, secretName, initialToken string,
) transport.WrapperFunc {
Comment on lines +102 to +105

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

According to the repository style guide, function signatures should preferably be written on a single line, including the parameter list and return types.

func NewTokenRefreshingRoundTripperWrapperConstructor(secretGetter func(string, string) (*corev1.Secret, error), secretNS, secretName, initialToken string) transport.WrapperFunc {
References
  1. Function signatures should preferably be written on a single line, including the parameter list and return types. (link)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I should apply this... the existing NewProxyHeaderRoundTripperWrapperConstructor in the same file uses the same multi-line format. And no linting error with multi-line format..

return func(rt http.RoundTripper) http.RoundTripper {
return &tokenRefreshingRoundTripper{
inner: rt,
secretGetter: secretGetter,
secretNS: secretNS,
secretName: secretName,
cachedToken: initialToken,
cacheExpiry: time.Now().Add(tokenCacheTTL),
}
}
}

// RoundTrip implements the http.RoundTripper interface.
func (t *tokenRefreshingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
req = req.Clone(req.Context())
req.Header.Set("Authorization", "Bearer "+t.getToken())
return t.inner.RoundTrip(req)
}

func (t *tokenRefreshingRoundTripper) getToken() string {
t.mu.RLock()
if time.Now().Before(t.cacheExpiry) {
token := t.cachedToken
t.mu.RUnlock()
return token
}
t.mu.RUnlock()
return t.forceRefresh()
}

func (t *tokenRefreshingRoundTripper) forceRefresh() string {
t.mu.Lock()
defer t.mu.Unlock()

if time.Now().Before(t.cacheExpiry) {
return t.cachedToken
}

secret, err := t.secretGetter(t.secretNS, t.secretName)
if err != nil {
klog.Warningf("tokenRefreshingRoundTripper: failed to refresh token from secret %s/%s, keeping last known token: %v",
t.secretNS, t.secretName, err)
t.cacheExpiry = time.Now().Add(tokenErrorRetryInterval)
return t.cachedToken
}

if token := string(secret.Data[clusterv1alpha1.SecretTokenKey]); token != "" {
t.cachedToken = token
} else {
klog.Warningf("tokenRefreshingRoundTripper: secret %s/%s has empty token key, keeping last known token",
t.secretNS, t.secretName)
}
t.cacheExpiry = time.Now().Add(tokenCacheTTL)
return t.cachedToken
}
Comment on lines +136 to +160

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Currently, forceRefresh holds the write lock t.mu during the entire network I/O call (t.secretGetter). This blocks all concurrent requests (even those just reading the token) for the duration of the API call.

By using t.fetchMu.TryLock(), we can ensure that only one goroutine performs the network I/O while all other concurrent requests immediately fall back to using the cached token instead of blocking. Additionally, we should add defensive checks to ensure secret and secret.Data are not nil before accessing them to prevent potential nil pointer dereference panics. Also, please add a doc comment to this unexported function to improve maintainability.

// forceRefresh handles the token refresh logic by fetching the secret and updating the cached token.
func (t *tokenRefreshingRoundTripper) forceRefresh() string {
	if !t.fetchMu.TryLock() {
		t.mu.RLock()
		token := t.cachedToken
		t.mu.RUnlock()
		return token
	}
	defer t.fetchMu.Unlock()

	t.mu.RLock()
	if time.Now().Before(t.cacheExpiry) {
		token := t.cachedToken
		t.mu.RUnlock()
		return token
	}
	t.mu.RUnlock()

	secret, err := t.secretGetter(t.secretNS, t.secretName)

	t.mu.Lock()
	defer t.mu.Unlock()

	if err != nil {
		klog.Warningf("tokenRefreshingRoundTripper: failed to refresh token from secret %s/%s, keeping last known token: %v",
			t.secretNS, t.secretName, err)
		t.cacheExpiry = time.Now().Add(tokenCacheTTL)
		return t.cachedToken
	}

	if secret != nil && secret.Data != nil {
		if token := string(secret.Data[clusterv1alpha1.SecretTokenKey]); token != "" {
			t.cachedToken = token
		}
	}
	t.cacheExpiry = time.Now().Add(tokenCacheTTL)
	return t.cachedToken
}
References
  1. Add doc comments to unexported functions that contain key logic to improve maintainability.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

same reason as above I feel

Loading