Skip to content

fix(get-vault-secrets): bound and retry the OIDC token request - #2249

Merged
NickAnge merged 4 commits into
grafana:mainfrom
pracucci:get-vault-secrets-retry-oidc-token
Aug 6, 2026
Merged

fix(get-vault-secrets): bound and retry the OIDC token request#2249
NickAnge merged 4 commits into
grafana:mainfrom
pracucci:get-vault-secrets-retry-oidc-token

Conversation

@pracucci

@pracucci pracucci commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Why

The Retrieve API KEY step intermittently fails while fetching the GitHub OIDC token, taking the whole job down with it:

❌ Failed to get OIDC token: Error message: Failed to get ID Token.
        Error Code : undefined
        Error Message: Request timeout: /222//idtoken/...?api-version=2.0&audience=vault-github-actions-grafana-ops

Two things make this worse than it needs to be:

  1. No retry. Request timeout: <path> is @actions/http-client's socket timeout. Its allowRetries only covers retryable HTTP status codes on idempotent verbs — a socket timeout rejects out of requestRaw and escapes the retry loop entirely. A single network blip is fatal and the only recovery is a human re-running the job.
  2. No usable timeout. That socket timeout defaults to three minutes, and core.getIDToken() exposes no way to shorten it. The failure we observed burned ~136s before giving up.

In grafana/deployment_tools this is routine: ~40 failed runs of one workflow over 3.5 days, every one of them this step. That workflow fans out to 9 jobs that each call this action and is triggered often enough that it ran 100 times in 53 minutes, so it hits the endpoint hard and notices quickly — but nothing about the failure is specific to that repo.

What

  • Request the token directly from ACTIONS_ID_TOKEN_REQUEST_URL with AbortSignal.timeout, bounding each attempt at 30s.
  • Retry up to 3 times with exponential backoff (1s, 2s).
  • Move the script out of action.yaml into get-id-token.js, loaded with require() from github-script the way create-github-app-token already loads its script, so the retry logic can be tested and linted.

Worst case is now ~93s and gives three chances, against ~136s and one chance today. The common case — a single stall — recovers in about a second instead of failing the job.

Racing a promise against core.getIDToken() was the smaller change, but the abandoned request keeps its socket (and the event loop) alive until the 3-minute timeout fires, so the step would still hang for minutes. Hence the direct call.

Two behaviours are deliberately preserved:

  • Proxied runners. Native fetch ignores the https_proxy/HTTPS_PROXY variables that @actions/http-client honours, so when a proxy is configured we still go through core.getIDToken() (unbounded, as today) rather than silently breaking those runners.
  • Missing id-token: write. No token endpoint exists, so retrying only delays a deterministic error. Those still fail on the first attempt with the same guidance as before.

Verification

get-id-token.test.js drives the module with a fake core, fetch and sleep:

scenario fetch calls outcome
succeeds first try 1 token published, no warnings
times out once 2 token published, 1 warning
HTTP 500 then success 2 token published, 1 warning
200 with empty body twice, then success 3 token published, 2 warnings
times out every attempt 3 setFailed + existing guidance, backs off 1s then 2s
unexpected fetch rejection 3 setFailed with the original message
proxy configured (https_proxy / HTTPS_PROXY) 0 falls back to core.getIDToken(), retried the same way
no id-token: write 0 setFailed immediately, no retries

Two more cover the request itself: the audience is appended for both instances (...&audience=vault-github-actions-grafana-ops, ...-dev), and the request carries the Bearer token and an AbortSignal. The token is masked with core.setSecret in every success path.

To check the tests have teeth I mutated the module — dropped the retry, flattened the backoff, removed the masking, inverted the proxy check, removed the timeout, ignored the HTTP status, pinned the audience — and each mutation fails at least one test.

bun test actions/get-vault-secrets runs as a new unit-test job in test-get-vault-secrets.yaml; it needs no bun install, as the test only imports bun:test. eslint, prettier --check and actionlint pass.

Notes for reviewers

  • Timeout and attempt count are hardcoded rather than exposed as inputs — happy to make either configurable.
  • 30s is deliberately generous for what is normally a sub-second call; the goal is just to stop waiting minutes. Say the word if you'd prefer it tighter.
  • env, fetch and sleep are parameters with defaults only so the tests can drive the loop; the action passes { core } and nothing else.
  • Bringing the file under eslint needed the Node globals in the shared config (globals.node for **/*.js), which is the only reason package.json and bun.lock change here. Without it module and process trip no-undef — the same reason create_github_token.js is excluded from the eslint pre-commit hook today.

The request to GitHub's OIDC endpoint intermittently times out, and
@actions/http-client does not retry socket timeouts, so a single blip
fails the whole job. Retry up to three times with exponential backoff.

Jobs missing `id-token: write` still fail on the first attempt, so a
misconfiguration surfaces as quickly as it does today.
@pracucci
pracucci requested a review from a team as a code owner July 31, 2026 18:50
core.getIDToken() allows no timeout and waits three minutes on a dead
socket, so retrying around it would still stall for minutes. Request the
token directly instead, with a 30s abortable timeout per attempt.

Native fetch does not honour the proxy variables @actions/http-client
reads, so proxied runners keep using core.getIDToken().
@pracucci pracucci changed the title fix(get-vault-secrets): retry the OIDC token request fix(get-vault-secrets): bound and retry the OIDC token request Jul 31, 2026
NickAnge
NickAnge previously approved these changes Aug 4, 2026

@NickAnge NickAnge 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.

Hey @pracucci . I like the idea. Answering your comments

Timeout and attempt count are hardcoded rather than exposed as inputs — happy to make either configurable.

I do not think these should be configurable by the client. They should be defined at the source. In my opinion, the logic is not complex enough to require different values. Let’s start with these numbers and adjust them if needed.

30s is deliberately generous for what is normally a sub-second call; the goal is just to stop waiting minutes. Say the word if you'd prefer it tighter.

No, I think 30 seconds is fine for now.

The script is embedded in action.yaml, so there's nowhere natural to land a regression test. If you'd like one, I can extract it to a .js file (which would also bring it under eslint) in a follow-up.

This one is important. I would like to see this either in this PR or in a quick follow-up. Since the logic is becoming more complex, I think it would be good to add some tests to verify that it works as expected.

…ble module

The retry loop was embedded in action.yaml, where no test could reach it. Move it
to get-id-token.js and require it from github-script, the same way
create-github-app-token already loads its script, then cover the retry, timeout,
proxy and missing-permission paths with bun tests.

Plain JS files get the Node globals in the eslint config, so the extracted module
lints instead of failing no-undef on `module` and `process`.
@pracucci

pracucci commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

The script is embedded in action.yaml, so there's nowhere natural to land a regression test. If you'd like one, I can extract it to a .js file (which would also bring it under eslint) in a follow-up.

This one is important. I would like to see this either in this PR or in a quick follow-up. Since the logic is becoming more complex, I think it would be good to add some tests to verify that it works as expected.

@NickAnge Thanks for your review. Take a look at the last commit, please. In the meanwhile I will look for a way to test it end-to-end without having to merge this PR.

@pracucci

pracucci commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@NickAnge To test it, I've pushed this commit that triggered this CI run: the two tests passed.

NickAnge
NickAnge previously approved these changes Aug 6, 2026

@NickAnge NickAnge 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.

Overall looks good. Just a small nit at the log message

Comment thread actions/get-vault-secrets/get-id-token.js Outdated
…rror

Either variable being unset triggers the error, so "and" was misleading.
@NickAnge
NickAnge added this pull request to the merge queue Aug 6, 2026
Merged via the queue into grafana:main with commit 69f2c01 Aug 6, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants