fix(CI): read Vault secrets by field-agnostic import in Windows smoke job - #3446
Open
Cynthia Qin (cqin-confluent) wants to merge 4 commits into
Open
fix(CI): read Vault secrets by field-agnostic import in Windows smoke job#3446Cynthia Qin (cqin-confluent) wants to merge 4 commits into
Cynthia Qin (cqin-confluent) wants to merge 4 commits into
Conversation
… job The Smoke windows/amd64 job has failed on every run since it was added in #3342 on 2026-05-01, always at the same line: Field "CONFLUENT_CLOUD_EMAIL" not present in secret The job authenticates fine (`vault login` exits 0) and the read itself succeeds, so this is neither a permissions nor a path problem. The linux/arm64 job reads the same secret and passes, which proves the secret exists and holds valid credentials. The difference is how each job reads it. Linux uses `vault-sem-get-secret`, which exports every field the secret contains, so it is agnostic to how those fields are named. Windows cannot use that helper (it is Linux-only), so the read was hand-rolled with `vault kv get -field=<NAME>` using the *exported* variable names. Those are not the names the fields are stored under -- the helper is what maps them -- so every lookup missed. Mirror the helper instead of hardcoding names: read each secret whole and export every field as an uppercased variable. This removes the coupling to field naming, so it also will not break the next time these secrets are rotated or renamed. Two supporting changes: - Fail immediately if an expected variable is still unset after the import, rather than 20 minutes later inside `go test` as "required environment variable ... is not set". - Log the field names that were imported. Values are never echoed. If the stored names turn out to differ by more than case, this makes the very next run report exactly what they are. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
Copilot started reviewing on behalf of
Cynthia Qin (cqin-confluent)
August 14, 2026 19:40
View session
There was a problem hiding this comment.
Pull request overview
This PR updates the Semaphore smoke-test pipeline to make the Windows/amd64 job read Vault secrets in a field-agnostic way (mirroring vault-sem-get-secret used on Linux), eliminating the current hardcoded -field=... lookups that have caused the Windows smoke job to fail consistently.
Changes:
- Replace per-field Vault reads on Windows with a PowerShell helper that imports all secret fields and exports them as uppercased environment variables.
- Add a fail-fast check to error immediately if required variables still aren’t present after import.
- Add logging of imported field names only (no secret values) to aid debugging.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+74
to
+85
| function Import-VaultSecret($Path) { | ||
| $secret = vault kv get -format=json $Path | ConvertFrom-Json | ||
| if ($LASTEXITCODE -ne 0) { throw "failed to read Vault secret $Path" } | ||
|
|
||
| # KV v2 nests fields under .data.data; KV v1 puts them directly on .data. | ||
| $fields = if ($secret.data.PSObject.Properties.Name -contains "data") { $secret.data.data } else { $secret.data } | ||
|
|
||
| foreach ($field in $fields.PSObject.Properties) { | ||
| [Environment]::SetEnvironmentVariable($field.Name.ToUpper(), $field.Value) | ||
| } | ||
| Write-Host "Imported $Path fields: $(($fields.PSObject.Properties.Name | Sort-Object) -join ', ')" | ||
| } |
Two hardening changes from a review of the previous commit. Check vault's exit code before piping to ConvertFrom-Json. On a partial or malformed response the parse would run first, and PowerShell's JSON errors can echo the offending input, which may contain secret values. Export only the three variables the job needs, matched case-insensitively, instead of exporting every field in both secrets. The previous approach mirrored `vault-sem-get-secret`, but it also placed every unrelated field in those secrets into the environment inherited by `go test`, the CLI under test, and the metric emitter -- a wider disclosure surface than the job requires. Matching case-insensitively keeps the fix independent of how the fields are cased, which was the point of reading them by hand. Note this does not change what is read from Vault: `vault kv get -field=` has no server-side projection, so the whole secret was already fetched either way. Only what reaches the environment changes. The explicit post-import guard is now redundant and has been dropped, since a missing or empty field throws at the point of export. On a mismatch the error lists the available field names, so a single run still reveals them. Values are never echoed, and the success path prints nothing at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous two commits diagnosed this wrong. They assumed the secret stored each credential in its own field under a different casing, and matched fields case-insensitively. That would not have fixed the job. Checking how the linux/arm64 job's helper actually behaves settles it. It requests one specific field, named "script", and sources its contents as a shell script. It performs no mapping or renaming of variable names, and the vault helper it calls beforehand only handles OIDC auth. So these secrets keep everything in that single "script" field, whose value is a shell script of `export NAME=value` lines. No CONFLUENT_CLOUD_EMAIL field exists in any casing, which is precisely what Vault reported. Read the "script" field and parse the assignments out of it, since PowerShell cannot source a shell script. The parser tolerates `export` or bare assignments, surrounding single or double quotes, values containing `=`, and skips comments and blank lines. The properties from the previous commit are kept: the exit code is checked before the output is used, only the variables the job needs are exported, no values are ever echoed, and a missing or empty variable throws immediately while listing the names the script does define. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two cleanups from re-reviewing the whole change; no behavior change intended. The parse loop tested `-notmatch` and then read `$Matches` after the negated test. PowerShell does populate `$Matches` whenever the underlying regex matches, regardless of which operator was used, so this worked -- but it depends on non-obvious behavior that cannot be verified from a machine without PowerShell. Use a positive `-match` and nest the body instead, which needs no such assumption and reads more directly. Rename `$script` to `$exports`. `$script` is legal but sits one colon away from PowerShell's `$script:` scope modifier, which is needlessly confusing here. Also note in a comment that the hashtable lookups are case-insensitive, since PowerShell hashtables are by default. That means the import still succeeds if the script declares these names under a different case -- worth stating, as an earlier revision of this branch wrongly believed case was the whole problem. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
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.
Release Notes
No user-facing changes. This PR touches CI pipeline configuration only.
Checklist
Whatsection below whether this PR applies to Confluent Cloud, Confluent Platform, or both.Test & Reviewsection below — including an explicit statement of what could not be verified.Blast Radiussection below.What
Applies to neither Cloud nor Platform — this changes
.semaphore/smoke-tests.ymlonly, no shipped CLI code.The
Smoke windows/amd64job has failed on every run since it was introduced in #3342 on 2026-05-01 — roughly 3.5 months at a 3-hourly cadence, each failure firing a Slack notification. It always dies on the same line:Root cause
These CI secrets do not store one field per credential. They keep everything in a single field named
script, whose value is a shell script ofexport NAME=valuelines. The helper thelinux/arm64job uses requests that one field and sources it; it performs no mapping or renaming of variable names, and the vault helper it calls beforehand only handles OIDC auth.So there is no
CONFLUENT_CLOUD_EMAILfield at all — which is exactly what Vault reported. The Windows job hand-rolled the read withvault kv get -field=<VARIABLE NAME>because the helper is Linux-only, and every lookup missed.Fix
Read the
scriptfield and parse the assignments out of it, since PowerShell cannot source a shell script. The parser toleratesexportor bare assignments, surrounding single or double quotes, values containing=, and skips comments and blank lines.Only the variables the job needs are exported. Deliberately not "export everything the script defines": that would put unrelated values into the environment inherited by
go test, the CLI under test, and the metric emitter, for no benefit.This is a CLI-side bug, not a Vault or DevOps problem
Worth stating, since "Vault error" reads like someone else's issue:
vault loginexits 0 on the Windows agentField ... not present in secretpermission denied; bad paths sayNo value found at ...The job was merged having never passed once. Nothing needs to change in Vault or in DevOps tooling to fix it.
Blast Radius
None for customers. No shipped CLI code paths are modified.
CI risk is close to zero because the job fails 100% of the time today — there is no working behavior to regress. The worst case is that it keeps failing, which is the status quo. The
linux/arm64block is byte-for-byte untouched, so existing smoke coverage is unaffected either way.The one CI-level risk worth naming is a malformed pipeline definition, which could take the passing Linux job down with it. Mitigated: the YAML parses, and the
- |multi-line PowerShell construct is already used twice in this file, including in this same Windows job.No false-green risk: a missing or empty variable throws at import, and
requiredEnvin the live tests fatals on an empty value. The job cannot pass without real credentials.Security notes
References
windows/amd64build/test job, now green on main): fix(test): isolateconfluent updatetests from the shared test binary #3445Test & Review
Verified:
source-able lines —export NAME=value, double- and single-quoted values, a quoted value containing=, a bare assignment with noexport, extra whitespace, and comment/blank/garbage lines. All parsed or skipped as intended, and an empty value is caught by the guard. To be precise about the limitation: this was executed through an equivalent port, not in PowerShell, because no PowerShell is available on the authoring machine.#lines inside it are preserved as PowerShell comments rather than stripped as YAML comments, and braces and parentheses balance.linux/arm64block is byte-for-byte untouched.$LASTEXITCODEreflectsvaultas a direct native-command assignment (no intervening pipeline), and[Environment]::SetEnvironmentVariablewith two arguments uses Process scope — the same scope the existing$Env:...lines in this job already rely on to persist across commands.-matchrather than testing-notmatchand then reading$Matches. The latter does work, but it relies on non-obvious operator behavior that cannot be checked without PowerShell, so the dependency was removed.\r?\nis correct for both that and the single-string case, and.Trim()removes any stray\r.Not verified: there is no PowerShell, no Windows host, and no Vault access on the machine this was written on, so it has not been executed. The smoke pipeline is also scheduled against
mainonly, so PR CI will not exercise it — the first real signal is the next scheduled run after merge.The remaining unknown is narrow: the exact shell syntax inside the
scriptfield. The parser is deliberately tolerant of the plausible forms, and if it still cannot find a variable it fails immediately and lists the names the script does define, so one run would pin it down.Revision history of this PR
The first two commits diagnosed this incorrectly — they assumed one field per credential stored under a different casing, and matched case-insensitively. That would not have fixed the job. The third commit corrects the diagnosis after checking how the Linux helper actually behaves; the fourth is review cleanup with no intended behavior change. Reviewers should read the final state rather than the intermediate commits.
🤖 Generated with Claude Code