Repository navigation
cmd: --issuer and --workload-issuer accumulate when repeated - #743
Conversation
Both flags were plain strings, so a second --workload-issuer replaced the first and every member of the platform behind the dropped issuer was refused with "unknown issuer", with nothing in the logs about the flag. Seen on Cloud Run, whose --args splits on commas, which is why the flag was repeated in the first place. The two flags move to a shared IssuerFlags helper registered by both binaries with pflag's StringSliceVar: repeated or comma-separated, every value counts, and the joined result feeds the same Options fields and the same parser as before. Help text and reference docs say "repeatable or comma-separated"; the Cloud Run guide shows both ways to pass several issuers through --args. pflag becomes a direct dependency at the version cobra already pins.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors OIDC issuer flag handling by introducing a reusable IssuerFlags struct in internal/controlplane to support repeatable and comma-separated --issuer and --workload-issuer flags across both agentmesh-control-plane and agentmesh-one. It also updates the corresponding documentation and adds tests. Feedback is provided to improve the robustness of the Empty() check in IssuerFlags to handle cases where an empty string is explicitly passed as a flag value, which would otherwise bypass the validation.
| func (f *IssuerFlags) WorkloadIssuer() string { return strings.Join(f.Workload, ",") } | ||
|
|
||
| // Empty reports whether no issuer of either kind was given. | ||
| func (f *IssuerFlags) Empty() bool { return len(f.Login) == 0 && len(f.Workload) == 0 } |
There was a problem hiding this comment.
Correctness / Robustness
The current implementation of Empty() checks the lengths of the slices:
func (f *IssuerFlags) Empty() bool { return len(f.Login) == 0 && len(f.Workload) == 0 }However, if a user explicitly passes an empty string (e.g., --issuer="" or --workload-issuer=""), pflag.StringSliceVar parses this as a slice containing a single empty string ([]string{""}). In this case, len(f.Login) or len(f.Workload) will be 1, causing Empty() to return false. This bypasses the validation check in agentmesh-control-plane and allows the server to start with empty issuers.
To make this check more robust, we should verify if the resolved (joined) issuer strings are empty.
| func (f *IssuerFlags) Empty() bool { return len(f.Login) == 0 && len(f.Workload) == 0 } | |
| func (f *IssuerFlags) Empty() bool { return f.OIDCIssuer() == "" && f.WorkloadIssuer() == "" } |
1 participant
Problem
--issuerand--workload-issueronagentmesh-oneandagentmesh-control-planewereStringVars. Given twice, the second value replaced the first, silently. Observed on Cloud Run, where--argssplits on commas so the comma form cannot be passed naively and the flag was repeated instead:Only the second issuer was configured; every OpenShell sandbox behind the first was refused with
JWT validation failed: unknown issuer: http://35.253.119.53and nothing in the logs pointed at the flag.Change
A small
controlplane.IssuerFlagshelper registers both flags with pflag'sStringSliceVar(repeatable and comma-separated, both accumulate) and both binaries use it, so they cannot drift. The joined value feeds the sameOptionsfields andparseWorkloadIssuersas before;<issuer>=<suffix>entries are unaffected. Help text and reference docs now say "repeatable or comma-separated"; the Cloud Run guide shows how to pass several issuers through--args(repeat the flag, or--args="^;^…").go.mod:github.com/spf13/pflagmoves from indirect to direct at the version cobra already pins; no new module.Verified
TestIssuerFlagsAccumulate: repeated, comma-separated, both forms with a suffix entry, and empty; the joined value is fed toparseWorkloadIssuersand the expected issuers come out.agentmesh-one --workload-issuer=http://127.0.0.1:1 --workload-issuer=http://127.0.0.1:2logsWorkload issuer not resolved … "issuer": "http://127.0.0.1:1"and… "http://127.0.0.1:2"(both configured; controlplane: resolve workload issuers in the background instead of failing to start #736 lets it start with neither reachable).go test ./internal/controlplane/ ./cmd/...,./hack/lint.sh0 issues.