feat(backend): source mock identity client/principal id from Key Vault secret (AROSLSRE-2006) - #6811
Conversation
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
🟡 Changes recommended
The CLI help text for the mock principal-id flag references an incorrect/non-existent flag name and should be updated to match the new client-id/client-id-path options.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds an opt-in mechanism for backend to source the mock managed identity’s client ID and principal ID from Key Vault secrets mounted via the CSI driver (read once at startup), avoiding outages caused by stale statically-configured IDs after Service Principal recreation.
Changes:
- Introduces
--insecure-azure-managed-identity-mock-client-id-pathand--insecure-azure-managed-identity-mock-principal-id-pathflags, plus startup-time file resolution and validation. - Extends backend Helm chart + values to optionally mount and consume the new Key Vault secrets (falling back to existing static values when unset).
- Adds unit tests covering mutual-exclusion validation and file resolution/trim behavior.
File summaries
| File | Description |
|---|---|
| config/rendered/dev/pers/westus3.yaml | Adds empty default fields for secret-name-based mock MI ID sourcing. |
| config/rendered/dev/perf/westus3.yaml | Adds empty default fields for secret-name-based mock MI ID sourcing. |
| config/rendered/dev/dev/westus3.yaml | Adds empty default fields for secret-name-based mock MI ID sourcing. |
| config/rendered/dev/cspr/westus3.yaml | Adds empty default fields for secret-name-based mock MI ID sourcing. |
| config/rendered/dev/ci01/centralus.yaml | Adds empty default fields for secret-name-based mock MI ID sourcing. |
| config/rendered/dev/ci00/centralus.yaml | Adds empty default fields for secret-name-based mock MI ID sourcing. |
| config/config.yaml | Defines new config defaults and documents behavior. |
| config/config.schema.json | Adds schema entries/descriptions for the new config fields. |
| backend/values.yaml | Wires new config fields into chart values for optional secret mounting. |
| backend/deploy/templates/backend.service-key-vault.secretproviderclass.yaml | Optionally adds Key Vault secret objects for mock MI client/principal ID. |
| backend/deploy/templates/backend.deployment.yaml | Conditionally uses -path flags vs raw values depending on secret-name presence. |
| backend/cmd/root.go | Adds new flags, validates mutual exclusion, and resolves ID values from files at startup. |
| backend/cmd/root_test.go | Adds tests for validation and file-based resolution. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The Helm SecretProviderClass currently mounts the new miMock secrets even when MI Mock isn’t enabled (e.g., missing miMock.certName), which can break backend pod startup in misconfigured environments.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
backend/deploy/templates/backend.service-key-vault.secretproviderclass.yaml:23
- The client-id Key Vault secret is mounted whenever
miMock.clientIdSecretNameis set, even if MI Mock itself is not enabled (e.g.,miMock.certNameis empty). That can cause the CSI mount/pod startup to fail due to missing secrets despite MI Mock flags not being used. Gate this secret on the same MI Mock enablement signal as the deployment (at leastmiMock.certName).
backend/deploy/templates/backend.service-key-vault.secretproviderclass.yaml:29 - The principal-id Key Vault secret is mounted whenever
miMock.principalIdSecretNameis set, even if MI Mock is not enabled (e.g., missingmiMock.certName). This can break pod startup by requiring secrets that won't be used. Gate this secret on MI Mock enablement (at leastmiMock.certName).
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/retest-required e2e-parallel failed with 11 unrelated test failures (private KeyVault, external auth config, breakglass session, subnet/NSG reuse, control-plane z-stream upgrade, nodepool downgrade, etc.) spanning completely different feature areas than this PR's change. Checked job-history for this job: only 2/15 recent runs succeeded across the board, confirming this is pre-existing infra flakiness, not something introduced here. This PR's change is inert by default (new secretName config fields are empty, so no new code path is exercised). |
|
/retest-required Second failure was a provisioning-stage infra flake unrelated to this PR: |
|
/retest-required Third failure: provisioning succeeded this time, but 3 tests failed in the private-networking area (private KeyVault, private KAS reachability, aggregated advanced features) — unrelated to this PR's backend change. Checked job-history: this e2e-parallel job is broadly flaky right now (2/15 recent PR runs succeeded), consistent with known infra issues, not something introduced here. Retrying. |
There was a problem hiding this comment.
🟡 Changes recommended
The new Helm branch lacks a committed fixture, and lifecycle documentation overstates automatic secret refresh behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
Core behavior is covered by unit and Helm fixtures; the remaining CLI help-text mismatch is non-blocking.
Review details
Suppressed comments (1)
backend/cmd/root.go:145
- This help text says the raw principal-ID flag is required, but validation also accepts
--insecure-azure-managed-identity-mock-principal-id-path. Mention both alternatives so valid mixed raw/path configurations are not documented as invalid.
"'--insecure-azure-managed-identity-mock-principal-id' and '--insecure-azure-managed-identity-mock-tenant-id' flags. "+
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
b25447f to
e94274b
Compare
|
Rebased onto main to resolve the conflict. The conflict was in the rendered configs only: #6809 rotated the ci00/ci01/cspr/dev/perf/pers miMockClientId/miMockPrincipalId values on main (INT outage fix from the recreated mock identity Service Principal), while this branch still had the pre-rotation values from when it was cut. Rebased cleanly, kept main's current rotated IDs and this PR's new empty-by-default miMockClientIdSecretName/miMockPrincipalIdSecretName fields, and re-ran config materialize to confirm the rendered output matches. |
There was a problem hiding this comment.
🟡 Changes recommended
The migration precedence is not tested, and related CLI help describes contradictory flag requirements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Balanced
e94274b to
b8c2f78
Compare
|
/retest |
There was a problem hiding this comment.
🟢 Approval recommended
The implementation is backward-compatible, internally consistent, and adequately covered by unit and Helm fixture tests.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
/test e2e-parallel |
… secret (AROSLSRE-2006) Adds --insecure-azure-managed-identity-mock-client-id-path and --insecure-azure-managed-identity-mock-principal-id-path flags to backend, mirroring the existing certificate-bundle-path convention. When set, the value is read once from the mounted file at startup instead of being passed as a raw static value. This lets backend's Helm chart mount the mock identity's clientId/principalId as Key Vault secrets (via the CSI driver, same mechanism already used for its certificate) instead of duplicating them as static config in config.yaml. That static duplication is what caused the INT outage worked in this session: the mock identity Service Principal's principalId changed when it was recreated, and the stale config value orphaned its Key Vault RBAC grant. Wiring: - backend/values.yaml: new miMock.clientIdSecretName/principalIdSecretName, empty by default (opt-in, fully backward compatible). - backend.service-key-vault.secretproviderclass.yaml: mounts the two new secrets when their names are set, same pattern as the existing mi-mock-cert-bundle secret. - backend.deployment.yaml: uses the new -path flags when the secret names are set, falls back to the existing raw miMock.clientId/principalId otherwise. - config.yaml/config.schema.json: new empty-by-default fields. Verified backward compatibility: with the new fields left empty (the default), make materialize regenerates byte-identical Helm fixtures. Verified the new path: temporarily setting test secret names and re-running make materialize produces the expected -path flags and SecretProviderClass entries (reverted before committing). Deliberately does not touch cluster-service (its binary lives in a separate upstream repo, not this one) or the privileged e2e-subscription-rbac-grants pipeline that would need to publish these secrets to Key Vault going forward. Both are tracked as follow-ups on AROSLSRE-2006, along with the sdp-pipelines overlay cleanup once backend can source these dynamically.
…text (AROSLSRE-2006) The --insecure-azure-managed-identity-mock-principal-id help text referenced a non-existent flag (--azure-mi-mock-principal-client-id), a pre-existing typo carried forward when I copied the text for the new -path variant. Points at the actual client-id flags (raw and -path) instead.
…ection (AROSLSRE-2006) - Correct the resolveInsecureManagedIdentityMockIDsFromFiles doc comment: it resolves once at process startup only, a running pod does not pick up a later CSI secret rotation without a restart. - Gate the miMock clientIdSecretName/principalIdSecretName CSI mount blocks in the SecretProviderClass template inside the same certName check as the cert bundle, so a config with a secret name set but MI Mock not otherwise enabled doesn't break pod startup via a missing-secret CSI mount. - Add a Helm test fixture exercising both miMockClientIdSecretName and miMockPrincipalIdSecretName set together, covering the new -path flags and their SecretProviderClass entries.
…ity KV secret indirection (AROSLSRE-2006) - Update the enabling flag, certificate-bundle-path flag, raw client-id flag, and tenant-id flag help text to consistently mention the '-path' alternative wherever it references the client-id/principal-id flags, matching what validate() actually accepts. - Set non-empty sentinel static clientId/principalId values in the KV-secret helm test fixture to exercise the migration precedence: with both the static values and the secret names configured, the rendered output must still only contain the '-path' flags.
Rebased AROSLSRE-2006 branch onto latest upstream/main. Rendered configs needed regeneration via make -C config materialize to pick up unrelated upstream changes (e.g. dataPlaneHAProxyImage) plus this PR's own miMockClientIdSecretName/miMockPrincipalIdSecretName fields. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
03c5b2d to
bf43afd
Compare
|
/test images |
|
/test e2e-parallel |
|
cc Gerd Oberlechner (@geoberle) Jan-Hendrik Boll (@janboll) Miguel Soriano (@miguelsorianod) — this has been open for a few days waiting on |
|
/test e2e-parallel The actual test suite passed fully (63 passed, 0 failed) on this run. The overall job was marked FAILURE solely because the |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: raelga, roivaz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
AROSLSRE-2057, part of AROSLSRE-2006.
What
Adds backend support for reading the mock identity's
clientIdandprincipalIdfrom Key Vault secrets mounted through CSI, using the same volume as the mock identity certificate.The new
--insecure-azure-managed-identity-mock-client-id-pathand--insecure-azure-managed-identity-mock-principal-id-pathflags read the files once at startup. Helm uses these flags whenmiMockClientIdSecretNameandmiMockPrincipalIdSecretNameare configured. Both secret names default to empty, so the static-value path remains the default.Why
The mock identity IDs are duplicated in configuration. Recreating the service principal changes its
principalId, leaving those copies stale.The goal is to publish and refresh the IDs through the existing privileged identity/RBAC automation and have workloads read them from Key Vault. This keeps Graph lookups out of the backend and cluster-service deployment identities.
This PR adds the backend consumer support only. It does not yet remove the stale-config risk end to end.
Testing
Unit tests in
backend/cmd/root_test.gocover direct-value/file-path validation and file resolution. The Helm fixture covers the Key Vault path with both secret names configured, including precedence over static IDs.Validation commands:
go build ./...,go vet ./..., andgo test ./cmd/...frombackend;make materializefromconfig.Special notes for your reviewer
This PR does not publish or enable the secrets, migrate cluster-service, remove static values, or change
tenantId. Backend needs a controlled restart after the mounted IDs change because it reads the files once at startup.Follow-ups under AROSLSRE-2006:
Related: AROSLSRE-1997, AROSLSRE-1999.
PR Checklist
If E2E tests are included: