backend: oidc-callback: Stop logging unrelated outer-scope error on missing state

[govindup63]

Summary

The /oidc-callback handler in backend/cmd/headlamp.go logged a stray err variable when the OIDC state query parameter was missing. The variable was not declared in the handler closure: it resolved to the outer-scope err returned by the kubeconfig load that runs once at server startup inside createHeadlampHandler.

The practical effect is that every malformed callback request surfaced whatever happened at boot, attached to an unrelated 400 response. For example, a typical in-cluster pod without a local kubeconfig logs this on every empty-state callback:

error loading kubeconfig files: error reading kubeconfig file:
open /home/headlamp/.config/Headlamp/kubeconfigs/config: no such file or directory

That is the exact misleading line reported in the issue.

Related Issue

Fixes #4839

Changes

• backend/cmd/headlamp.go: pass nil to logger.Log on the missing-state branch (instead of the captured outer-scope err). Added a short comment explaining the scoping trap so the capture is not reintroduced.
• backend/cmd/headlamp_test.go: new regression test TestOidcCallbackEmptyStateDoesNotLogStaleError. Hooks the logger via logger.SetLogFunc, runs createHeadlampHandler with a deliberately broken KubeConfigPath so the kubeconfig load returns a known error into the captured outer-scope err, fires GET /oidc-callback with no state, then asserts:
  • The response is 400 Bad Request (unchanged behavior).
  • The captured log entry for invalid request state is empty carries a nil error value (not the stale kubeconfig load error).

Steps to Test

1. cd backend && go test ./cmd/ -run TestOidcCallbackEmptyStateDoesNotLogStaleError -v. Passes.
2. Reverting only headlamp.go (keeping the test) reproduces the bug: the test fails with Expected nil, but got: &errors.errorString{s:"error loading kubeconfig files: error reading kubeconfig file: open .../kubeconfigs/config: no such file or directory"}. That is exactly the misleading log the reporter described.
3. cd backend && go test ./cmd/... -race. All existing cmd/ tests still pass.
4. cd backend && go build ./... && go vet ./cmd/.... Clean.

Notes for the Reviewer

• This is a minimal correctness fix. The behavior of the endpoint (400 Bad Request on missing state) is unchanged. Only the log line argument changes.
• There is a sibling logger misuse at the type-assertion branch a few lines below (logger.Log(... , nil, err, ...) when the oauth2 token has no expected token field). At that point the closure's local err is always nil, so the call is misleading-but-harmless rather than a real outer-scope leak. I left it untouched to keep this PR scoped to the issue. Happy to clean it up in a follow-up.
• The regression test uses logger.SetLogFunc, which the logger package already exposes for tests (see backend/pkg/logger/logger_test.go). No new test helpers were added.

[Copilot]

Pull request overview

Fixes misleading /oidc-callback logging by ensuring the “missing state” branch does not log a captured outer-scope kubeconfig-load error, and adds a regression test to prevent reintroduction.

Changes:

• Update /oidc-callback missing-state logging to pass nil error (avoids outer-scope err capture) and document the scoping pitfall.
• Add a regression test that hooks logger.SetLogFunc and asserts the missing-state log entry carries a nil error.

CI/check status and PR commit history (e.g., merge commits) aren’t available in this review context—please confirm CI is green and the PR history is linear.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
backend/cmd/headlamp.go	Fixes the missing-state log call to avoid logging an unrelated outer-scope error; adds an explanatory comment.
backend/cmd/headlamp_test.go	Adds regression test capturing logger calls to ensure no stale error is logged on empty state.

Comments suppressed due to low confidence (3)

backend/cmd/headlamp_test.go:1448

• In this test, calling createHeadlampHandler(context.Background(), cfg) with an empty PluginDir triggers the background plugin watcher paths in createHeadlampHandler (because !UseInCluster), and plugins.Watch treats an empty watchPath as the current directory, which can walk/watch a large tree and consume many fsnotify resources during tests. Consider setting PluginDir to t.TempDir() (and optionally UserPluginDir/StaticPluginDir) and using a cancellable context (context.WithCancel + t.Cleanup(cancel)) so the watcher goroutines are stopped at the end of the test.

	cfg := &HeadlampConfig{
		HeadlampConfig: &headlampconfig.HeadlampConfig{
			HeadlampCFG: &headlampconfig.HeadlampCFG{
				UseInCluster:    false,
				KubeConfigPath:  "/tmp/" + staleErrMarker,
				KubeConfigStore: kubeconfig.NewContextStore(),
			},
			Cache:            cache.New[interface{}](),
			TelemetryConfig:  GetDefaultTestTelemetryConfig(),
			TelemetryHandler: &telemetry.RequestHandler{},
		},
	}

backend/cmd/headlamp_test.go:1463

• logMu is locked and held via defer until the end of the test. Since the logger hook acquires the same mutex on every log call, any background goroutine that logs after this point will block until the test returns, which can create unnecessary contention or even hangs in the presence of cleanup that logs. Prefer copying logCalls under the mutex and unlocking immediately before searching/asserting.

	logMu.Lock()
	defer logMu.Unlock()

backend/cmd/headlamp_test.go:1417

• Using a hard-coded "/tmp/" + marker for KubeConfigPath can collide if that path happens to exist on the test machine (making the startup kubeconfig load succeed and weakening the regression). Consider using filepath.Join(t.TempDir(), "missing") (or similar) to guarantee a non-existent kubeconfig path.

	// Bad path so the startup kubeconfig load returns a distinctive err.
	staleErrMarker := "no-such-kubeconfig-for-issue-4839"

[illume]

Thanks for working on this.

Would you mind addressing the open Copilot review comments? Please mark each comment as resolved after addressing it.

[govindup63]

Thanks @illume, marked them as resolved.

[govindup63]

golangci-lint was failing on the new test (funlen + wsl_v5). Pushed a fix in b7d6729bb.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[govindup63]

Thanks @copilot, addressed in b829fbefe (use t.Setenv to pin the static-plugin dir, dropped the no-op field assignment). Marked resolved.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[govindup63]

Thanks @copilot, applied in e0e34ef78 — hoisted var err error to the top of the closure so the shadow is enforced by the language. Marked resolved.

[govindup63]

Right, fixed in 47bc03b6e — passing nil directly. Marked resolved.

[illume]

🎉 thanks!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: govindup63, illume

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:

• OWNERS [illume]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.