Cosmos: Update key-based live tests to fixed self-owned accounts. - #5049
Cosmos: Update key-based live tests to fixed self-owned accounts.#5049Arooshi Avasthy (aavasthy) wants to merge 14 commits into
Conversation
|
Just reading the description, but isn't this the opposite of what we want? We need all key based auth to be in the ephemeral tenant, but can use MSI based EntraID auth in the normal tenant? |
There was a problem hiding this comment.
Pull request overview
Moves key-based Cosmos live tests to permanent accounts while preserving ARM deployment for thin-client and AAD tests.
Changes:
- Adds fixed-account provisioning, configuration, and secret resolution.
- Splits test matrices by resource strategy.
- Extends pipeline templates to support tests without ARM deployment.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
sdk/cosmos/pipeline/resolve-test-account-steps.yml |
Invokes account resolution. |
sdk/cosmos/pipeline/resolve-cosmos-test-account.tests.ps1 |
Tests resolver validation. |
sdk/cosmos/pipeline/resolve-cosmos-test-account.ps1 |
Exports selected account settings. |
sdk/cosmos/pipeline/README.md |
Documents fixed-account workflow. |
sdk/cosmos/pipeline/live-test-accounts.schema.json |
Defines secret schema. |
sdk/cosmos/pipeline/live-test-accounts.sample.json |
Provides sample account data. |
sdk/cosmos/pipeline/account-provisioning/README.md |
Documents provisioning and rotation. |
sdk/cosmos/pipeline/account-provisioning/New-CosmosLiveTestAccounts.ps1 |
Provisions permanent accounts. |
sdk/cosmos/pipeline/account-provisioning/cosmos-live-test-accounts.definition.json |
Defines account topology. |
sdk/cosmos/live-thinclient-matrix.json |
Isolates ARM-backed thin-client tests. |
sdk/cosmos/live-platform-matrix.json |
Maps key-based legs to fixed accounts. |
sdk/cosmos/ci.yml |
Wires the new test paths. |
eng/pipelines/templates/stages/archetype-sdk-client.yml |
Generates fixed-account jobs. |
eng/pipelines/templates/jobs/live.tests.yml |
Supports skipping resource deployment. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| DefaultConsistencyLevel = $acct.defaultConsistencyLevel | ||
| EnableAutomaticFailover = $autoFailover | ||
| EnableMultipleWriteLocations = $multiWrite | ||
| ApiKind = 'GlobalDocumentDB' |
There was a problem hiding this comment.
Fixed — added both capabilities to the script to match the bicep. No live tests currently use vector or full-text search, so the already-provisioned accounts aren't blocking anything
| } | ||
| } | ||
| else { | ||
| Write-Info "Cosmos account '$accountName' already exists (selector=$selector); leaving configuration as-is" |
There was a problem hiding this comment.
Intentional — reruns shouldn't silently mutate live accounts. Config changes go through a manual delete + rerun, per the runbook.
| FixedAccountMatrixConfigs: | ||
| - Name: Cosmos_live_test | ||
| Path: sdk/cosmos/live-platform-matrix.json | ||
| Selection: sparse | ||
| GenerateVMJobs: true |
There was a problem hiding this comment.
The scope of this PR is the move to fixed accounts; adding a sweeper is a separate concern with its own design questions (schedule, age threshold, safety against a concurrent run, which identity runs it).
There was a problem hiding this comment.
I think it is not completely orthogonal - for example one step to take now might be to not just use UUID based names - but prefix them with some tiemstamp - so, that any async sweeper could delete let's say db/collections that were created >12 hours ago? Not necessarily blocking for this PR - but worth thinking about this because it will become a hot topic as soon as this PR is merged.
| The script does **not** write to any ADO variable group itself - copy the | ||
| JSON it prints (or the contents of `-OutputPath`) into the | ||
| `rust-ci` secret variable in the | ||
| `Test Secrets for Cosmos Live Tests - user administered` variable group (or | ||
| whichever group `sdk/cosmos/pipeline/resolve-test-account-steps.yml` is | ||
| pointed at). Treat the JSON as a secret at every step - it contains account | ||
| keys. |
There was a problem hiding this comment.
Fixed — updated the runbook in sdk/cosmos/pipeline/account-provisioning/README.md to point rotation at the rust-ci Key Vault secret
| accounts in the `sdk-ci` resource group and prints the JSON to store in | ||
| the ADO secret. See its README for the full runbook. | ||
| 4. That JSON is stored as the `rust-ci` secret | ||
| variable in the `Test Secrets for Cosmos Live Tests - user administered` | ||
| ADO variable group (already wired into `sdk/cosmos/ci.yml`). |
There was a problem hiding this comment.
Fixed — updated sdk/cosmos/pipeline/README.md to describe rust-ci as a Key Vault secret linked (read-through) into the variable group, and clarified that rotation happens on the KV secret itself since the variable-group value is not writable.
| @@ -0,0 +1,252 @@ | |||
| <# | |||
There was a problem hiding this comment.
Fixed — added the standard copyright/license header at the top of New-CosmosLiveTestAccounts.ps1 . Verified Get-Help still returns the synopsis, so the comment-based help block still parses correctly.
| $endpoint = [string]$account.endpoint | ||
| $key = [string]$account.key | ||
| $secondaryKey = if ($account.PSObject.Properties.Name -contains 'secondaryKey') { [string]$account.secondaryKey } else { '' } | ||
| $database = if ($account.PSObject.Properties.Name -contains 'database' -and $account.database) { [string]$account.database } else { 'shared-test-db' } | ||
| $consistency = [string]$account.consistency | ||
| $testCategory = [string]$account.testCategory |
There was a problem hiding this comment.
fixed. Added a Get-OptionalString helper and now read endpoint , key , secondaryKey , database , consistency , testCategory conditionally, so the targeted missing required '' messages surface instead of a StrictMode throw. Verified against payloads missing endpoint and consistency — both output the intended error.
| if (-not ($config.PSObject.Properties.Name -contains 'version') -or $config.version -ne 1) { | ||
| Fail "Unsupported or missing schema version '$($config.version)' (parser supports: 1)." | ||
| } |
There was a problem hiding this comment.
fixed. Now reading version into a sentinel first, so the message formats cleanly instead of throwing under StrictMode. Verified with a payload missing version — outputs Unsupported or missing schema version '' as intended.
Tests are still part of ephemeral tenant. In the existing process we would spin up a new account every time and run tests on it. With this new process, we have fixed accounts. We store the account + key in a secret vault and systems engineering team helped us create a new service connection for it just once. Now every time a tenant comes through we will just need to create new account and replace the account+key in vault but do not have to create a new service connection. |
|
Ok, so the part of the description where it says:
is just wrong, and should be updated? |
|
/azp run rust - cosmos - weekly |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run rust - cosmos - weekly |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run rust - cosmos - weekly |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Don't modify this file. Modify sdk/cosmos/.cspell.json. That's what it's there fore and doesn't require maintainer sign-off.
There was a problem hiding this comment.
reverted the changes in sdk/cosmos/.cspell.json
There was a problem hiding this comment.
Daniel Jurek (@danieljurek) or Ben Broderick Phillips (@benbp) can you review this please? I think you helped them originally so I don't have as much context.
Fabian Meiswinkel (FabianMeiswinkel)
left a comment
There was a problem hiding this comment.
LGTM except few NITs and Heath's blocking comment
a5aea65 to
13df811
Compare
|
/azp run rust - cosmos - weekly |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
The build.rs comment referencing the emulator command-line switch `/enableaadauthentication` triggered a cspell 'Unknown word' failure on the Build Analyze step. Adding an inline `cspell:ignore` directive keeps the comment intact without touching the shared root cspell config, matching the existing pattern used in tests/emulator_tests/cosmos_aad.rs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run rust - cosmos - weekly |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
1e1ba4a to
f1de010
Compare
|
/azp run rust - cosmos - weekly |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
PR Description
Our key-based Cosmos live tests currently deploy a brand-new Cosmos account into an ephemeral Microsoft test tenant on every CI run. That tenant is rotated frequently, and each rotation requires a new ADO service connection (which we can't create ourselves - it needs engineering systems). This makes the tests fragile and painful to keep alive.
Run the key-based live tests against fixed, self-owned Cosmos accounts in the subscription (
sdk-ciresource group), with the endpoints and keys stored in a single Key Vault secret.sdk-civia newrust-ciKV secret. Changed..rs)How it works
sdk-ciresource group. Theywere created once (idempotent script under
sdk/cosmos/pipeline/ account-provisioning/) and match the 6 previous per-run configurations(Eventual/Session/Strong SingleWrite, Session MultiWrite, Session Split,
Session MR Failover).
secret (
rust-ci, JSON blob keyed by logical account name), linkedinto the existing
Test Secrets for Cosmos Live Tests - user administeredvariable group.Cosmos_live_testmatrix leg carries anAccountSelector(e.g.session-multiwrite). The newresolve-test-account-steps.ymlpre-teststep runs
resolve-cosmos-test-account.ps1, which picks the right sliceof the secret and exports the same env vars the ARM template used to
produce (
AZURE_COSMOS_CONNECTION_STRING,ACCOUNT_HOST,DATABASE_NAME,AZURE_COSMOS_DEFAULT_CONSISTENCY,COSMOS_RUSTFLAGS).entirely - it needs no tenant login.