Collapse redundant plugin-configuration error wrapping - #751
CodeBuildder wants to merge 1 commit into
Conversation
Configure()'s plugin loop wraps every plugin constructor's error a second time, right on top of a wrap the constructor already added. NewAuthenticator, NewAuthorizer, and NewCRDManager each return a generic "couldn't configure X" style error, then the loop wraps that again with "cannot configure X plugin." Neither message adds anything new, it's just noise, and it's exactly what the issue describes (Cannot configure auth plugin: Couldn't configure Auth:). This isn't just an Authenticator/Keycloak thing either, NewAgentsDB, NewCRDManager, and NewAuthorizer all have the same double-wrap shape. NewAgentsDB is the one exception since its wrap has real info in it (driver name, file path), so that one's left alone. Partially addresses spiffe#395 Signed-off-by: Kaushik Kumaran <47471121+CodeBuildder@users.noreply.github.com>
24c4b1e to
a70e7ef
Compare
mrsabath
left a comment
mrsabath
left a comment
There was a problem hiding this comment.
Correctly collapses the double-wrap and upgrades to %w (making the chain unwrappable) while preserving which-plugin context via pluginType. New config_test.go covers all four failure paths plus a success control.
Verified the collapsed shared if err != nil is safe: err is freshly redeclared each loop iteration via pluginType, err := stringFromToken(...), so an unrecognized plugin type (no matching case) leaves err == nil and cannot trip a stale-error false positive.
One out-of-diff note: NewAgentsDB still self-wraps with %v, so it is the odd one out in the new "constructor owns its wrap, loop adds one generic layer" model — a follow-up could migrate the remaining constructor errors to %w.
Assessment only — not an approval.
| } | ||
| } | ||
| if err != nil { | ||
| return fmt.Errorf("failed to configure %s plugin: %w", pluginType, err) |
There was a problem hiding this comment.
[suggestion] This changes the user-facing message text — from "Cannot configure <role> plugin: ..." to "failed to configure <pluginType> plugin: ..." using the raw config key (e.g. DataStore, SPIRECRDManager). Behaviorally fine and arguably clearer, but any log-scraping/alerting keyed on the old strings will break — worth a changelog note.
2 participants
Fixes (partially) #395
Configure()'s plugin loop wraps every plugin constructor's error a second time, right on top of a wrap the constructor already added. NewAuthenticator, NewAuthorizer, and NewCRDManager each return a generic "couldn't configure X" style error, and then the loop wraps that again with "cannot configure X plugin." Neither message adds anything new, it's just noise, and it's exactly what the issue is describing (
Cannot configure auth plugin: Couldn't configure Auth:).This isn't just an Authenticator/Keycloak thing either. NewAgentsDB, NewCRDManager, and NewAuthorizer all have this same double-wrap shape. NewAgentsDB is the one exception, since its wrap actually has real info in it (driver name, file path), so I left that one alone.
I reproduced this against a real unreachable OIDC issuer, nothing listening on the port, running the actual CLI:
The fix: the loop's four separate wraps collapse into one shared check after the switch, reusing the plugin type it already extracted, using %w so it stays wrappable (same style as the invalid plugin type key check a few lines up). Then I dropped the three generic constructor wraps in NewAuthenticator, NewAuthorizer, and NewCRDManager since that framing now comes from the loop instead.
Worth calling out, #398 (adding the issuer URL to the discovery error) is untouched, you can see it's still there in the reproduction above.
Verification: go build, go vet (only pre-existing findings, all in files I didn't touch), gofmt clean, and go test ./api/... ./pkg/... all passing.
Added tests in api/agent/config_test.go, one real failure per plugin type plus a success case: a bad sqlite path, an unreachable OIDC issuer, an RBAC config pointing at a role that doesn't exist, and a CRD manager failing because we're not actually in a k8s cluster. Plus one control test that a normal config still configures fine. Each failure test checks that the new wrap shows up, the constructor's actual detail survives, and the old redundant text is gone.
This covers the nesting/redundancy part of the issue. It doesn't touch the "maybe suggest a fix" idea from the thread, happy to open that separately if it's wanted.