Skip to content

test(basic-auth): store the vault secret before the consumer references it - #13853

Merged
nic-6443 merged 1 commit into
apache:masterfrom
nic-6443:test/basic-auth-secret-ref-order
Aug 20, 2026
Merged

test(basic-auth): store the vault secret before the consumer references it#13853
nic-6443 merged 1 commit into
apache:masterfrom
nic-6443:test/basic-auth-secret-ref-order

Conversation

@nic-6443

@nic-6443 nic-6443 commented Aug 19, 2026

Copy link
Copy Markdown
Member

t/plugin/basic-auth.t TEST 22 fails in CI with:

failed to resolve secret reference: $secret://vault/test1/foo/passwd, field: password, err: no value
failed to resolve secret reference in consumer auth credential, skipping consumer: foo

It is not a timing race — it is deterministic given the Vault state the rest of the job leaves behind.

Four test files write the same Vault path with different fields, and vault kv put replaces the whole secret rather than merging:

file writes
ai-aws-content-moderation-secrets.t secret_access_key, access_key_id
authz-keycloak4.t client_secret
basic-auth.t passwd
openid-connect9.t client_secret

The first two sort before basic-auth.t in the t/plugin/[a-k]*.t job, so by the time it runs kv/apisix/foo holds a client_secret and no passwd. TEST 22 then creates a consumer whose password is $secret://vault/test1/foo/passwd, and the admin write resolves it right there: admin/consumers.lua calls check_duplicate_key()find_consumer()create_consume_cache()fetch_secrets(), synchronously inside the request — which is why the error carries the PUT /apisix/admin/consumers context. The key exists but the field does not, so it logs at error level and the default no_error_log: [error] trips.

It looks flaky only because of the rerun harness: after the job fails, rerun_flaky_tests re-runs the failed file on its own with FLUSH_ETCD=1. Alone, nothing has overwritten kv/apisix/foo, the path is absent rather than incomplete, that path does not log an error, and the file goes green — which is exactly what happened in the run that prompted this PR.

Writing the secret before the reference is configured makes the file self-sufficient regardless of what ran before it.

Reproduced deterministically, and the fix verified against it:

vault kv put kv/apisix/foo client_secret=xyz
FLUSH_ETCD=1 prove -Itest-nginx/lib -I. t/plugin/basic-auth.t

before: Failed 2/179 subtests with the two error lines above — after: All tests successful.

Sharing one Vault path across four unrelated test files is the underlying hazard; giving each file its own path would be the more thorough fix, but it touches four files and this one is enough to make basic-auth.t independent of the others.

Copilot AI lite review requested due to automatic review settings August 19, 2026 19:05
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Aug 19, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@dosubot dosubot Bot added the CI label Aug 19, 2026
…es it

Four test files write the same Vault path kv/apisix/foo with different
fields, and `vault kv put` replaces the whole secret rather than merging
it. Two of them sort before basic-auth.t in the t/plugin/[a-k]*.t job, so
by the time it runs the path holds a client_secret and no passwd.

TEST 22 then configures a consumer whose password is
$secret://vault/test1/foo/passwd, and the admin write resolves it right
there -- admin/consumers.lua check_duplicate_key() -> find_consumer() ->
create_consume_cache() -> fetch_secrets() -- so the reference is looked up
while the field is still missing. It is logged at error level and the
default no_error_log: [error] trips.

The failure only looks intermittent because rerun_flaky_tests re-runs the
failed file on its own with FLUSH_ETCD=1: alone, nothing has overwritten
kv/apisix/foo, the path is absent rather than incomplete, and that does
not log an error.

Write the secret first so the file no longer depends on what ran before
it. The following blocks still verify that the credential resolves and
authenticates.
@nic-6443
nic-6443 force-pushed the test/basic-auth-secret-ref-order branch from b42cd09 to 1d26833 Compare August 20, 2026 02:32

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nic-6443
nic-6443 merged commit 749a9bc into apache:master Aug 20, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants