From 7c215f64516560e1b14cbac8e57daf3c2133089a Mon Sep 17 00:00:00 2001 From: yxxhero Date: Tue, 3 Mar 2026 08:20:12 +0800 Subject: [PATCH] fix: address PR review comments Addresses all review comments from PR #2446: 1. Add validation for QPS and Burst values - QPS must be > 0 - Burst must be >= 1 - Returns clear error messages for invalid values 2. Fix client config loading - Use clientcmd.NewDefaultClientConfigLoadingRules() for proper fallback - Only override ExplicitPath when kubeconfig is provided - Ensures standard env var handling and ~/.kube/config fallback 3. Improve cache locking - Use double-check locking pattern to avoid holding mutex during client creation - Reduces contention when multiple goroutines create clients concurrently 4. Add comprehensive tests - Test validation of invalid QPS values - Test validation of invalid Burst values - Test successful creation with valid values - Ensure error messages are descriptive 5. Fix documentation link - Updated Kubernetes client-go rate limiting link to point to correct rest.Config documentation - Previous link pointed to API server event rate limiting All tests pass and code quality checks succeed. Signed-off-by: yxxhero --- KUBEDOG_CONFIG.md | 2 +- pkg/kubedog/tracker.go | 25 +++++++++---- pkg/kubedog/tracker_test.go | 73 +++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 8 deletions(-) diff --git a/KUBEDOG_CONFIG.md b/KUBEDOG_CONFIG.md index fd87d117..1f095f8f 100644 --- a/KUBEDOG_CONFIG.md +++ b/KUBEDOG_CONFIG.md @@ -205,4 +205,4 @@ The default values (QPS=100, Burst=200) were chosen to: - [Issue #2445](https://github.com/helmfile/helmfile/issues/2445) - Original issue that led to configurable QPS/Burst - [Kubedog Documentation](https://github.com/werf/kubedog) -- [Kubernetes Client Go Rate Limiting](https://kubernetes.io/docs/reference/config-api/apiserver-eventratelimit.v1alpha1/) +- [Kubernetes client-go `rest.Config` QPS/Burst](https://pkg.go.dev/k8s.io/client-go/rest#Config) diff --git a/pkg/kubedog/tracker.go b/pkg/kubedog/tracker.go index 666ced83..7d2b040f 100644 --- a/pkg/kubedog/tracker.go +++ b/pkg/kubedog/tracker.go @@ -73,6 +73,13 @@ func NewTracker(config *TrackerConfig) (*Tracker, error) { burst = *config.KubedogBurst } + if qps <= 0 { + return nil, fmt.Errorf("invalid kubedog QPS %v: must be > 0", qps) + } + if burst < 1 { + return nil, fmt.Errorf("invalid kubedog burst %v: must be >= 1", burst) + } + clientSet, err := getOrCreateClient(config.KubeContext, kubeconfig, qps, burst) if err != nil { return nil, fmt.Errorf("failed to initialize kubernetes client: %w", err) @@ -101,18 +108,15 @@ func getOrCreateClient(kubeContext, kubeconfig string, qps float32, burst int) ( } kubeInitMu.Lock() - defer kubeInitMu.Unlock() - if client, ok := clientCache[key]; ok { + kubeInitMu.Unlock() return client, nil } + kubeInitMu.Unlock() - var explicitPath string + loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() if kubeconfig != "" { - explicitPath = kubeconfig - } - loadingRules := &clientcmd.ClientConfigLoadingRules{ - ExplicitPath: explicitPath, + loadingRules.ExplicitPath = kubeconfig } overrides := &clientcmd.ConfigOverrides{} @@ -134,6 +138,13 @@ func getOrCreateClient(kubeContext, kubeconfig string, qps float32, burst int) ( return nil, fmt.Errorf("failed to create kubernetes client: %w", err) } + kubeInitMu.Lock() + defer kubeInitMu.Unlock() + + if existingClient, ok := clientCache[key]; ok { + return existingClient, nil + } + clientCache[key] = client return client, nil diff --git a/pkg/kubedog/tracker_test.go b/pkg/kubedog/tracker_test.go index 6bc3eb2f..049adb08 100644 --- a/pkg/kubedog/tracker_test.go +++ b/pkg/kubedog/tracker_test.go @@ -128,3 +128,76 @@ func TestTrackerConfig_WithQPSBurst(t *testing.T) { assert.Equal(t, float32(50.0), *config.KubedogQPS) assert.Equal(t, 100, *config.KubedogBurst) } + +func TestNewTracker_InvalidQPS(t *testing.T) { + invalidQPS := float32(-1.0) + burst := 100 + + cfg := &TrackerConfig{ + Logger: nil, + Namespace: "test-ns", + KubeContext: "test-ctx", + Kubeconfig: "/nonexistent/kubeconfig", + TrackOptions: NewTrackOptions(), + KubedogQPS: &invalidQPS, + KubedogBurst: &burst, + } + + tr, err := NewTracker(cfg) + + assert.Error(t, err) + assert.Nil(t, tr) + assert.Contains(t, err.Error(), "invalid kubedog QPS") + assert.Contains(t, err.Error(), "must be > 0") +} + +func TestNewTracker_InvalidBurst(t *testing.T) { + qps := float32(50.0) + invalidBurst := 0 + + cfg := &TrackerConfig{ + Logger: nil, + Namespace: "test-ns", + KubeContext: "test-ctx", + Kubeconfig: "/nonexistent/kubeconfig", + TrackOptions: NewTrackOptions(), + KubedogQPS: &qps, + KubedogBurst: &invalidBurst, + } + + tr, err := NewTracker(cfg) + + assert.Error(t, err) + assert.Nil(t, tr) + assert.Contains(t, err.Error(), "invalid kubedog burst") + assert.Contains(t, err.Error(), "must be >= 1") +} + +func TestNewTracker_ValidQPSBurst(t *testing.T) { + qps := float32(50.0) + burst := 100 + + cfg := &TrackerConfig{ + Logger: nil, + Namespace: "test-ns", + KubeContext: "", + Kubeconfig: "", // Will use default kubeconfig + TrackOptions: NewTrackOptions(), + KubedogQPS: &qps, + KubedogBurst: &burst, + } + + // This test may fail if no kubeconfig is available, which is expected + // in CI environments. The important part is that validation passes. + tr, err := NewTracker(cfg) + + // If kubeconfig doesn't exist, we expect an error about loading kubeconfig, + // NOT an error about invalid QPS/Burst + if err != nil { + assert.NotContains(t, err.Error(), "invalid kubedog QPS") + assert.NotContains(t, err.Error(), "invalid kubedog burst") + assert.Nil(t, tr) + } else { + assert.NotNil(t, tr) + } +}