chore(lint): scope goconst/gosec via .golangci.yml and fix gosec findings#700
Merged
Conversation
- Add .golangci.yml: goconst min-occurrences=5, min-len=5, ignore-tests, ignore-functions for logging.* and fmt.Errorf; exclude gosec on tests and on internal/events/types.go (event reason identifiers, not credentials). - cmd/: lift resource-type / MCP primitive strings to constants in mcp_types.go and reference them from check/create/events/get/list/ start/stop.go. - internal/aggregator/: add constants.go (defaultValkeyKeyPrefix, httpReadHeaderTimeout); switch namespace defaults to metav1.NamespaceDefault; replace string state/health checks with api.IsActiveState / api.Health* in event_handler.go; set ReadHeaderTimeout on http.Server constructions (G112). - internal/context/storage.go: filepath.Clean + 0o600/0o700 perms (G304/G306). - internal/oauth/manager.go: filepath.Clean on caFile path (G304). - internal/orchestrator/orchestrator.go: drop stale nolint:gosec. - internal/aggregator/server.go: drop stale nolint:gosec on initSSOForSession call. - internal/events/types.go: drop nolint:gosec from event reason consts. - Strip nolint:goconst,gosec from test files; covered by config.
This was referenced May 18, 2026
The initial .golangci.yml relied on goconst.ignore-tests, which only honours the standalone CLI flag; golangci-lint v2 requires a path-based exclusion to suppress test-file goconst hits. Add the _test.go goconst exclusion alongside the existing gosec one. Add a stop-gap ignore-string-values list covering the JSON Schema, muster response field, CLI table header, OAuth wire and stringly-typed boolean vocabulary still inlined at this point in the stack. Each follow-up PR (api.ArgType, shared resource types, JSON-key constants, OAuth wire constants) shrinks this list; the final PR drops it. Add narrow gosec/govet exclusions for findings whose source-level fixes land in later commits in the stack (G118 on the SSO bootstrap goroutine, G710 on the admin handler redirect and BDD mock, reflect.Ptr in muster_manager.go).
QuentinBisson
commented
May 19, 2026
Contributor
Author
QuentinBisson
left a comment
QuentinBisson
left a comment
There was a problem hiding this comment.
Light review (DDD / SOLID / YAGNI / DRY / don't-reinvent / tests).
0o644 → 0o600 on user config and 0o755 → 0o700 on dirs are correct G306 fixes. No new #nosec annotations in source — only ones removed — which is the right direction.
Non-blocking — scope creep beyond "lint fix"
event_handler.goswaps inline string equality (event.NewState == "running" || "connected") forapi.IsActiveState(...),api.HealthHealthy,api.StateStarting, etc. That's a behavior refactor, not a gosec fix. Behavior-equivalent only ifIsActiveStatecovers exactly{running, connected}and the typed-string conversions are loss-less. Either surface in the PR body or split into its own PR — the title says "fix gosec findings".
Non-blocking — wrong-layer constant
internal/aggregator/constants.gointroducesdefaultValkeyKeyPrefix, but the value originates ininternal/config. PR #706 then moves it. The factoring here is at the wrong layer and gets reverted next PR — net churn.
Non-blocking — .golangci.yml
- The file-level scoping (
internal/aggregator/server.go,internal/admin/handlers.go) hides any new G118/G710 in those files until #706 narrows them. Add an issue link in the YAML comment so the exclusion has a sunset. - The 152 redundant
//nolintcleanups in_test.gorely on the new path-based test exclusion — confirm in CI that goconst'signore-testssetting alone wouldn't have sufficed (one source of truth instead of two).
Resolves conflicts in three aggregator test files where main refactored RegisterPendingAuth to take a PendingAuthRegistration struct. The //nolint:gosec annotations added in those refactors are now redundant — the path-based gosec exclusion in .golangci.yml already covers _test.go.
paurosello
approved these changes
May 19, 2026
Contributor
paurosello
left a comment
paurosello
left a comment
There was a problem hiding this comment.
Conflicts resolved by merging main into the branch — verified go build ./..., full go test ./internal/aggregator/... (4.4s, all green), and make lint (0 issues). The RegisterPendingAuth refactor on main was accommodated by adopting the new PendingAuthRegistration struct signature; the //nolint:gosec annotations from main's tests are now redundant given the _test.go path exclusion this PR adds to .golangci.yml.
…st config Drop defaultValkeyKeyPrefix from internal/aggregator/constants.go and restore the inline "muster:" literal at the four call sites. The value originates in operator-facing config and #706 lifts it to the right layer; introducing the constant here at the aggregator layer is reverted in the next PR, so this is net churn. With min-occurrences=5 (already configured) four literal occurrences don't trip goconst — no //nolint:goconst needed. Annotate the sunset gosec/govet exclusions with a "tracked: #706" marker so each one has a visible follow-up reference. Spell out why goconst keeps both ignore-tests and the path-based _test.go exclusion: ignore-tests skips test files when COUNTING occurrences (so test-only string usage doesn't inflate the production count past min-occurrences); the path exclusion separately silences any goconst report on test files. Dropping ignore-tests alone produces 36 false-positive goconst hits in production files because test-file usage pushes the count past the threshold.
Resolve test-file conflicts from #716 (test constants). Drop redundant local serverURL/issuerURL aliases now that testMusterURL/testDexURL package-level constants exist.
Co-authored-by: github-actions <action@github.com> Co-authored-by: Timo Derstappen <timo@giantswarm.io> Co-authored-by: Quentin Bisson <quentin.bisson@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Unblocks #629 (Align files) and resolves the lint debt tracked in #637.
.golangci.yml(new):goconst:min-len: 5,min-occurrences: 5,ignore-tests: true, plus anignore-functionslist covering muster's logging helpers andfmt.Errorfso log/error format strings stop firing as duplicates.gosec: excluded from_test.gofiles (test fixtures contain repeated dummy tokens by design);G101excluded narrowly oninternal/events/types.gowhere event-reason identifiers contain the wordToken.Source-level fixes (no nolint substitutes):
cmd/: lifted resource-type / MCP primitive identifiers to constants incmd/mcp_types.go; the existing//nolint:goconstdirectives on those strings are now redundant.internal/aggregator/: addedconstants.gowith shareddefaultValkeyKeyPrefix/httpReadHeaderTimeout; switched the default namespace tometav1.NamespaceDefault; replaced the running/connected string comparisons withapi.IsActiveStateand the correspondingapi.HealthHealthy/api.HealthUnhealthyconstants in the event handler; setReadHeaderTimeouton every HTTP server construction (fixes G112).internal/context/storage.go:filepath.Cleanon the user-supplied config path; tightened directory/file permissions to0o700/0o600(G301/G306/G304).internal/oauth/manager.go:filepath.Cleanon the configurable CA-file path (G304).internal/orchestrator/orchestrator.go: dropped a stale//nolint:gosecon acontext.WithCancelcall that gosec no longer flags.internal/aggregator/server.go: dropped a stale//nolint:gosecon the SSO init goroutine spawn.internal/events/types.go: dropped per-line//nolint:gosecon theMCPServerToken*event reasons; now covered by the file-level exclusion above.//nolint:goconst,gosecdirectives from_test.gofiles (now covered by the config).Result:
golangci-lint run -E=gosec -E=goconst -E=govetreports a single residual category (workflow/cli/mcpserver JSON-schema vocabulary), which is addressed in the follow-up PRs in this stack (api.ArgTypeenum, shared resource-type constants, JSON-key constants).References