feat(ol-analytics-api): deliver the MITx Online OAuth2 client credentials - #5188
Open
blarghmatey wants to merge 4 commits into
Open
feat(ol-analytics-api): deliver the MITx Online OAuth2 client credentials#5188blarghmatey wants to merge 4 commits into
blarghmatey wants to merge 4 commits into
Conversation
…ials The app's org-manager check now authenticates to MITx Online with its own client-credentials token instead of forwarding the caller's identity (mitodl/ol-analytics-api#22), so the pod needs a client id and secret. Unlike SENTRY_DSN, no Pulumi stack owns these values: the OAuth2 Application record is created by hand in MITx Online's Django admin. So they are written once to secret-operations and read back here, then merged into the existing kv-v2 path the vault-secrets-operator already syncs -- which means no new mount, no new K8s Secret, and no policy change, since the app can already read that path. Temporary. Once org-manager status is visible in the Keycloak token (mitodl/hq#10594) the app stops calling MITx Online, and this and the Vault entry go away together. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AHYhdMkjAVj1suk9YwQzC
Contributor
There was a problem hiding this comment.
Pull request overview
Delivers MITx Online OAuth2 client-credentials (client ID + secret) into the existing ol-analytics-api Vault static secrets path so the ol-analytics-api pod can authenticate to MITx Online using a service token for its org-manager check.
Changes:
- Read MITx Online OAuth2 client credentials from
secret-operations/ol-analytics-api/mitxonline-oauthviavault.generic.get_secret_output. - Merge those credentials into the existing
secret-ol-analytics-api/secretskv-v2 secret alongsideSENTRY_DSNby switching toOutput.all(...).apply(...).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…is wrong The credentials at secret-operations/ol-analytics-api/mitxonline-oauth are written by an operator, not by Pulumi, so their shape is a convention rather than a guarantee. A typo'd key name produced a bare KeyError from inside an apply, which is a poor way to learn that a hand-created Vault entry is malformed. Validate up front and name the path, the missing keys, and the command to fix it. Empty values are rejected alongside missing ones -- reading a blank secret would otherwise deploy an unusable credential that only fails later, in the pod, at the token endpoint. That is why this checks explicitly rather than switching to .get(), which would swallow the problem. Raised by Sentry Seer on #5188. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AHYhdMkjAVj1suk9YwQzC
Ardiea
approved these changes
Jul 31, 2026
Reading the credential with vault.generic.get_secret_output pulled it through Pulumi state and rewrote it into a second, Pulumi-owned Vault path. Two problems: the secret ends up in the state file, and rotating it needs a `pulumi up` rather than just a new value in Vault. Have the vault-secrets-operator read secret-operations directly instead and template the values straight into the env var names the app expects, the same way applications/marimo_data handles its OIDC client pair. The credential never enters Pulumi state, and a rotation propagates on the operator's own refresh interval. Consequence worth noting: VSO now reads that path using the application's own Vault role, so the policy has to grant it -- added here. Previously the read happened with the deploying operator's credentials, which is part of why routing it through Pulumi looked deceptively simple. This also drops the deploy-time key validation added in d566c0c for Sentry's finding; templating happens in the operator at runtime, where Pulumi cannot check it. Tracked in the PR discussion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AHYhdMkjAVj1suk9YwQzC
The previous commit added a second Secret to env_from_secret_names without adding it to depends_on, so Pulumi was free to create the Deployment first and the pod would crash-loop on a missing secret until the operator caught up. static_secrets was already listed for exactly this reason; the new one just needs to match. Raised by Sentry Seer on #5188. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016AHYhdMkjAVj1suk9YwQzC
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.
What are the relevant tickets?
Related: https://github.com/mitodl/hq/issues/10594 (this wiring is deleted once that lands)
Description (What does it do?)
Delivers the MITx Online OAuth2 client-credentials pair to the
ol-analytics-apipod, so its org-manager check can authenticate with its own service token rather than forwarding the caller's identity (mitodl/ol-analytics-api#22, against the new endpoint in mitodl/mitxonline#3807).Unlike
SENTRY_DSN— which the sentry stack owns end to end — no Pulumi stack owns these values. The OAuth2Applicationrecord is created by hand in MITx Online's Django admin.So the vault-secrets-operator reads
secret-operationsdirectly at runtime and templates the values into the env var names the app expects, exactly asapplications/marimo_datadoes for its OIDC client pair:The resulting Secret is added to the Deployment's
env_from_secret_namesalongside the existing static-secrets one.The credential never passes through Pulumi state, and rotating it is just a new value in Vault — the operator picks it up on its own refresh, with no
pulumi upin the loop.One consequence worth a reviewer's attention: VSO reads that path using the application's Vault role, not the deploying operator's credentials, so
ol_analytics_api_policy.hclnow grants read on it.secret-operationsis kv-v1, so there's no/data/sub-path to grant alongside.How can this be tested?
pulumi previewon CI/QA. Expected: one newVaultStaticSecret, one new K8s Secret, an addedenvFromentry on the Deployment, and the policy change.ol-analytics-api-secrets-<env>is untouched — it still carries onlySENTRY_DSN.The Vault entry must exist before the pods can start, or VSO renders empty values and the app fails at MITx Online's token endpoint:
Pre-commit passes, including
ruffandmypy. I confirmed this change adds no new mypy findings by diffing the error set againstmain— the file has 19 pre-existing errors, unchanged.Not verified: no
pulumi previewagainst the live stack (no AWS credentials in the authoring environment), and thesecret-operationsentry doesn't exist yet, so VSO's read and template rendering are unexercised. Both want checking before deploy.Additional Context
Third of three PRs, and the ordering is load-bearing:
b2b:manager-checkscopeSuggested order: merge the mitxonline endpoint, create the Application and write the Vault entry, merge this, then merge the app PR.
An earlier revision of this PR read the secret with
vault.generic.get_secret_outputand merged it into the existing Pulumi-managed Vault path. That was wrong for the two reasons above (secret in state; rotation requires a deploy) and has been replaced with the VSO templating shown here.Checklist:
b2b:manager-checkscope) for each environmentvault kv put secret-operations/ol-analytics-api/mitxonline-oauth client_id=… client_secret=…in CI, QA, and Production Vault before applying