From 9d8933f4b67d3103ae422e0c0f1a98aedca1869a Mon Sep 17 00:00:00 2001 From: Swapnil Date: Thu, 11 Jun 2026 08:27:43 +0530 Subject: [PATCH 1/4] must-gather: add client-side keep-alive to prevent accessTokenInactivityTimeout failures Co-authored-by: Cursor --- pkg/cli/admin/mustgather/mustgather.go | 46 ++++++++++++++++++ pkg/cli/admin/mustgather/mustgather_test.go | 52 +++++++++++++++++++++ 2 files changed, 98 insertions(+) diff --git a/pkg/cli/admin/mustgather/mustgather.go b/pkg/cli/admin/mustgather/mustgather.go index 38c52b270b..a5543a1ac7 100644 --- a/pkg/cli/admin/mustgather/mustgather.go +++ b/pkg/cli/admin/mustgather/mustgather.go @@ -63,6 +63,14 @@ const ( defaultMustGatherCommand = "/usr/bin/gather" defaultVolumePercentage = 70 defaultSourceDir = "/must-gather/" + + // defaultKeepAliveInterval is the default interval for client-side + // authenticated API probes that reset the OAuth access-token inactivity + // timer. Clusters with accessTokenInactivityTimeout (common in regulated + // environments) will revoke idle tokens; periodic probes prevent that + // during long-running log-follow connections that the API server does + // not count as activity. + defaultKeepAliveInterval = 30 * time.Second ) var ( @@ -805,6 +813,38 @@ func (o *MustGatherOptions) Run() error { return kutilerrors.NewAggregate(errs) } +// startClientKeepAlive spawns a background goroutine that periodically makes +// an authenticated API call to prevent the user's OAuth access token from being +// revoked due to accessTokenInactivityTimeout. The log-follow connection held +// by getGatherContainerLogs is a single long-lived HTTP stream that the API +// server does not count as discrete activity, so without these probes the token +// can expire on clusters with short inactivity windows (e.g. 5-10 minutes in +// banking/government/PCI-DSS environments). +// +// Returns a cancel function that stops the goroutine. The caller must invoke it +// when the keep-alive is no longer needed. +func (o *MustGatherOptions) startClientKeepAlive(ctx context.Context) context.CancelFunc { + ctx, cancel := context.WithCancel(ctx) + go func() { + ticker := time.NewTicker(defaultKeepAliveInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + // A lightweight authenticated GET that resets the OAuth + // inactivity timer. Errors are expected if the cluster is + // temporarily unreachable and are not actionable here. + if _, err := o.Client.Discovery().ServerVersion(); err != nil { + klog.V(5).Infof("keep-alive probe failed (non-fatal): %v", err) + } + } + } + }() + return cancel +} + // processNextWorkItem creates & processes the must-gather pod and returns error if any func (o *MustGatherOptions) processNextWorkItem(ctx context.Context, ns string, pod *corev1.Pod) error { var err error @@ -831,6 +871,12 @@ func (o *MustGatherOptions) processNextWorkItem(ctx context.Context, ns string, log := o.newPodOutLogger(o.Out, pod.Name) + // Prevent the user's OAuth token from expiring due to + // accessTokenInactivityTimeout while we hold long-lived log-follow + // connections. The probes run until the gather output has been copied. + stopKeepAlive := o.startClientKeepAlive(ctx) + defer stopKeepAlive() + // wait for gather container to be running (gather is running) if err := o.waitForGatherContainerRunning(ctx, pod); err != nil { log("gather did not start: %s", err) diff --git a/pkg/cli/admin/mustgather/mustgather_test.go b/pkg/cli/admin/mustgather/mustgather_test.go index 8fb6fdba37..8e447be0de 100644 --- a/pkg/cli/admin/mustgather/mustgather_test.go +++ b/pkg/cli/admin/mustgather/mustgather_test.go @@ -699,3 +699,55 @@ func TestNewPod(t *testing.T) { }) } } + +func TestStartClientKeepAlive(t *testing.T) { + t.Run("makes periodic authenticated API calls", func(t *testing.T) { + fakeClient := fake.NewSimpleClientset() + o := &MustGatherOptions{ + IOStreams: genericiooptions.NewTestIOStreamsDiscard(), + Client: fakeClient, + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + stopKeepAlive := o.startClientKeepAlive(ctx) + // Let it run for enough time to fire at least 2 probes. + // defaultKeepAliveInterval is 30s, so use a shorter context for the test. + // Instead, we directly test that at least one call was made quickly by + // checking the fake client's action log after a brief sleep. + time.Sleep(35 * time.Second) + stopKeepAlive() + + actions := fakeClient.Actions() + var versionCalls int + for _, a := range actions { + if a.GetVerb() == "get" && a.GetResource().Resource == "version" { + versionCalls++ + } + } + if versionCalls == 0 { + t.Errorf("expected at least one Discovery().ServerVersion() call, got %d actions: %v", len(actions), actions) + } + }) + + t.Run("stops when cancel is called", func(t *testing.T) { + fakeClient := fake.NewSimpleClientset() + o := &MustGatherOptions{ + IOStreams: genericiooptions.NewTestIOStreamsDiscard(), + Client: fakeClient, + } + + ctx := context.Background() + stopKeepAlive := o.startClientKeepAlive(ctx) + stopKeepAlive() + + before := len(fakeClient.Actions()) + time.Sleep(100 * time.Millisecond) + after := len(fakeClient.Actions()) + + if after != before { + t.Errorf("keep-alive goroutine continued after cancel: actions before=%d, after=%d", before, after) + } + }) +} From a6c1c51456718bba7d0ea67bf19e166964485422 Mon Sep 17 00:00:00 2001 From: Swapnil Date: Thu, 11 Jun 2026 18:56:29 +0530 Subject: [PATCH 2/4] must-gather: address review feedback for keep-alive implementation - Make keep-alive interval configurable via unexported field for testability - Add context cancellation check before each probe to ensure clean shutdown - Filter context.Canceled errors from non-fatal log output - Reduce test runtime from 35s to <1s by using 50ms interval in tests Co-authored-by: Cursor --- pkg/cli/admin/mustgather/mustgather.go | 19 +++++++++----- pkg/cli/admin/mustgather/mustgather_test.go | 29 ++++++++++----------- 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/pkg/cli/admin/mustgather/mustgather.go b/pkg/cli/admin/mustgather/mustgather.go index a5543a1ac7..734bd9f585 100644 --- a/pkg/cli/admin/mustgather/mustgather.go +++ b/pkg/cli/admin/mustgather/mustgather.go @@ -453,8 +453,9 @@ type MustGatherOptions struct { Since time.Duration SinceTime string - RsyncRshCmd string - clock clock.PassiveClock + RsyncRshCmd string + keepAliveInterval time.Duration + clock clock.PassiveClock PrinterCreated printers.ResourcePrinter PrinterDeleted printers.ResourcePrinter @@ -825,18 +826,22 @@ func (o *MustGatherOptions) Run() error { // when the keep-alive is no longer needed. func (o *MustGatherOptions) startClientKeepAlive(ctx context.Context) context.CancelFunc { ctx, cancel := context.WithCancel(ctx) + interval := o.keepAliveInterval + if interval == 0 { + interval = defaultKeepAliveInterval + } go func() { - ticker := time.NewTicker(defaultKeepAliveInterval) + ticker := time.NewTicker(interval) defer ticker.Stop() for { select { case <-ctx.Done(): return case <-ticker.C: - // A lightweight authenticated GET that resets the OAuth - // inactivity timer. Errors are expected if the cluster is - // temporarily unreachable and are not actionable here. - if _, err := o.Client.Discovery().ServerVersion(); err != nil { + if ctx.Err() != nil { + return + } + if _, err := o.Client.Discovery().ServerVersion(); err != nil && !errors.Is(err, context.Canceled) { klog.V(5).Infof("keep-alive probe failed (non-fatal): %v", err) } } diff --git a/pkg/cli/admin/mustgather/mustgather_test.go b/pkg/cli/admin/mustgather/mustgather_test.go index 8e447be0de..1e35f8db9e 100644 --- a/pkg/cli/admin/mustgather/mustgather_test.go +++ b/pkg/cli/admin/mustgather/mustgather_test.go @@ -704,46 +704,45 @@ func TestStartClientKeepAlive(t *testing.T) { t.Run("makes periodic authenticated API calls", func(t *testing.T) { fakeClient := fake.NewSimpleClientset() o := &MustGatherOptions{ - IOStreams: genericiooptions.NewTestIOStreamsDiscard(), - Client: fakeClient, + IOStreams: genericiooptions.NewTestIOStreamsDiscard(), + Client: fakeClient, + keepAliveInterval: 50 * time.Millisecond, } ctx, cancel := context.WithCancel(context.Background()) defer cancel() stopKeepAlive := o.startClientKeepAlive(ctx) - // Let it run for enough time to fire at least 2 probes. - // defaultKeepAliveInterval is 30s, so use a shorter context for the test. - // Instead, we directly test that at least one call was made quickly by - // checking the fake client's action log after a brief sleep. - time.Sleep(35 * time.Second) + time.Sleep(200 * time.Millisecond) stopKeepAlive() actions := fakeClient.Actions() - var versionCalls int + var probeCalls int for _, a := range actions { - if a.GetVerb() == "get" && a.GetResource().Resource == "version" { - versionCalls++ + if a.GetVerb() == "get" { + probeCalls++ } } - if versionCalls == 0 { - t.Errorf("expected at least one Discovery().ServerVersion() call, got %d actions: %v", len(actions), actions) + if probeCalls < 2 { + t.Errorf("expected at least 2 keep-alive probes, got %d actions: %v", probeCalls, actions) } }) t.Run("stops when cancel is called", func(t *testing.T) { fakeClient := fake.NewSimpleClientset() o := &MustGatherOptions{ - IOStreams: genericiooptions.NewTestIOStreamsDiscard(), - Client: fakeClient, + IOStreams: genericiooptions.NewTestIOStreamsDiscard(), + Client: fakeClient, + keepAliveInterval: 50 * time.Millisecond, } ctx := context.Background() stopKeepAlive := o.startClientKeepAlive(ctx) + time.Sleep(150 * time.Millisecond) stopKeepAlive() before := len(fakeClient.Actions()) - time.Sleep(100 * time.Millisecond) + time.Sleep(200 * time.Millisecond) after := len(fakeClient.Actions()) if after != before { From 11351c1e89624e261a83d38901ff06425d6b42d9 Mon Sep 17 00:00:00 2001 From: Swapnil Date: Thu, 11 Jun 2026 21:44:10 +0530 Subject: [PATCH 3/4] must-gather: address tchap review feedback - Remove internal context.WithCancel; let caller control goroutine lifetime via ctx cancellation (simpler API, no redundant cancel func) - Change keep-alive probe log level from V(5) to V(2) for better visibility during troubleshooting Co-authored-by: Cursor --- pkg/cli/admin/mustgather/mustgather.go | 15 +++++---------- pkg/cli/admin/mustgather/mustgather_test.go | 13 +++++++------ 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/pkg/cli/admin/mustgather/mustgather.go b/pkg/cli/admin/mustgather/mustgather.go index 734bd9f585..cd4be58fbe 100644 --- a/pkg/cli/admin/mustgather/mustgather.go +++ b/pkg/cli/admin/mustgather/mustgather.go @@ -822,10 +822,8 @@ func (o *MustGatherOptions) Run() error { // can expire on clusters with short inactivity windows (e.g. 5-10 minutes in // banking/government/PCI-DSS environments). // -// Returns a cancel function that stops the goroutine. The caller must invoke it -// when the keep-alive is no longer needed. -func (o *MustGatherOptions) startClientKeepAlive(ctx context.Context) context.CancelFunc { - ctx, cancel := context.WithCancel(ctx) +// The goroutine runs until ctx is cancelled. +func (o *MustGatherOptions) startClientKeepAlive(ctx context.Context) { interval := o.keepAliveInterval if interval == 0 { interval = defaultKeepAliveInterval @@ -838,16 +836,12 @@ func (o *MustGatherOptions) startClientKeepAlive(ctx context.Context) context.Ca case <-ctx.Done(): return case <-ticker.C: - if ctx.Err() != nil { - return - } if _, err := o.Client.Discovery().ServerVersion(); err != nil && !errors.Is(err, context.Canceled) { - klog.V(5).Infof("keep-alive probe failed (non-fatal): %v", err) + klog.V(2).Infof("keep-alive probe failed (non-fatal): %v", err) } } } }() - return cancel } // processNextWorkItem creates & processes the must-gather pod and returns error if any @@ -879,8 +873,9 @@ func (o *MustGatherOptions) processNextWorkItem(ctx context.Context, ns string, // Prevent the user's OAuth token from expiring due to // accessTokenInactivityTimeout while we hold long-lived log-follow // connections. The probes run until the gather output has been copied. - stopKeepAlive := o.startClientKeepAlive(ctx) + keepAliveCtx, stopKeepAlive := context.WithCancel(ctx) defer stopKeepAlive() + o.startClientKeepAlive(keepAliveCtx) // wait for gather container to be running (gather is running) if err := o.waitForGatherContainerRunning(ctx, pod); err != nil { diff --git a/pkg/cli/admin/mustgather/mustgather_test.go b/pkg/cli/admin/mustgather/mustgather_test.go index 1e35f8db9e..d2414fe645 100644 --- a/pkg/cli/admin/mustgather/mustgather_test.go +++ b/pkg/cli/admin/mustgather/mustgather_test.go @@ -712,9 +712,9 @@ func TestStartClientKeepAlive(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - stopKeepAlive := o.startClientKeepAlive(ctx) + o.startClientKeepAlive(ctx) time.Sleep(200 * time.Millisecond) - stopKeepAlive() + cancel() actions := fakeClient.Actions() var probeCalls int @@ -728,7 +728,7 @@ func TestStartClientKeepAlive(t *testing.T) { } }) - t.Run("stops when cancel is called", func(t *testing.T) { + t.Run("stops when context is cancelled", func(t *testing.T) { fakeClient := fake.NewSimpleClientset() o := &MustGatherOptions{ IOStreams: genericiooptions.NewTestIOStreamsDiscard(), @@ -736,11 +736,12 @@ func TestStartClientKeepAlive(t *testing.T) { keepAliveInterval: 50 * time.Millisecond, } - ctx := context.Background() - stopKeepAlive := o.startClientKeepAlive(ctx) + ctx, cancel := context.WithCancel(context.Background()) + o.startClientKeepAlive(ctx) time.Sleep(150 * time.Millisecond) - stopKeepAlive() + cancel() + time.Sleep(50 * time.Millisecond) before := len(fakeClient.Actions()) time.Sleep(200 * time.Millisecond) after := len(fakeClient.Actions()) From 74a31135370af2be8957e18eb249fb01b261ffef Mon Sep 17 00:00:00 2001 From: Swapnil Date: Fri, 12 Jun 2026 07:38:39 +0530 Subject: [PATCH 4/4] must-gather: fix gofmt formatting Co-authored-by: Cursor --- pkg/cli/admin/mustgather/mustgather.go | 6 +++--- pkg/cli/admin/mustgather/mustgather_test.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/cli/admin/mustgather/mustgather.go b/pkg/cli/admin/mustgather/mustgather.go index cd4be58fbe..4d733b3586 100644 --- a/pkg/cli/admin/mustgather/mustgather.go +++ b/pkg/cli/admin/mustgather/mustgather.go @@ -453,9 +453,9 @@ type MustGatherOptions struct { Since time.Duration SinceTime string - RsyncRshCmd string - keepAliveInterval time.Duration - clock clock.PassiveClock + RsyncRshCmd string + keepAliveInterval time.Duration + clock clock.PassiveClock PrinterCreated printers.ResourcePrinter PrinterDeleted printers.ResourcePrinter diff --git a/pkg/cli/admin/mustgather/mustgather_test.go b/pkg/cli/admin/mustgather/mustgather_test.go index d2414fe645..99ed038d68 100644 --- a/pkg/cli/admin/mustgather/mustgather_test.go +++ b/pkg/cli/admin/mustgather/mustgather_test.go @@ -704,7 +704,7 @@ func TestStartClientKeepAlive(t *testing.T) { t.Run("makes periodic authenticated API calls", func(t *testing.T) { fakeClient := fake.NewSimpleClientset() o := &MustGatherOptions{ - IOStreams: genericiooptions.NewTestIOStreamsDiscard(), + IOStreams: genericiooptions.NewTestIOStreamsDiscard(), Client: fakeClient, keepAliveInterval: 50 * time.Millisecond, } @@ -731,7 +731,7 @@ func TestStartClientKeepAlive(t *testing.T) { t.Run("stops when context is cancelled", func(t *testing.T) { fakeClient := fake.NewSimpleClientset() o := &MustGatherOptions{ - IOStreams: genericiooptions.NewTestIOStreamsDiscard(), + IOStreams: genericiooptions.NewTestIOStreamsDiscard(), Client: fakeClient, keepAliveInterval: 50 * time.Millisecond, }