[test]: Use NodePool & HostedCluster read desire during clusters & node pool status reconciliation within CS as well as persist what's done by those watching controllers to Cosmos' Resources container. - #6832
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
It introduces high-impact Cosmos RBAC expansions and changes the default Cluster Service image to a personal repository, which is not safe to merge as-is.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates dev-infrastructure Cosmos DB RBAC wiring to allow the Clusters Service (CS) managed identity to read/write additional Cosmos containers (including per-management-cluster kube-applier containers and RP Cosmos containers), and switches Cluster Service image references in config/materialized fixtures to a test image for running full e2e.
Changes:
- Plumb CS managed identity principal ID through service/mgmt templates and pipelines, then grant it container-scoped Cosmos SQL RBAC roles.
- Grant CS access to Fleet/Resources containers (RP Cosmos) and to per-management-cluster kube-applier Cosmos containers.
- Update Cluster Service image defaults + rendered configs + Helm template fixtures to pull
arohcpsvcdev.azurecr.io/mchitimb/cs@<digest>.
File summaries
| File | Description |
|---|---|
| dev-infrastructure/templates/svc-cluster.bicep | Passes CS MI principal ID into RP Cosmos module for container-scoped RBAC. |
| dev-infrastructure/templates/output-svc.bicep | Exposes CS MI principalId as a pipeline-consumable output. |
| dev-infrastructure/templates/mgmt-cluster.bicep | Accepts CS MI principal ID param and passes it into kube-applier Cosmos RBAC module. |
| dev-infrastructure/modules/rp-cosmos.bicep | Adds CS role assignments (Fleet read-only, Resources read/write) scoped to containers. |
| dev-infrastructure/modules/rp-cosmos-kube-applier.bicep | Grants CS MI Data Contributor on per-MC kube-applier container. |
| dev-infrastructure/mgmt-pipeline.yaml | Wires csManagedIdentityPrincipalId from service RG outputs into mgmt pipeline inputs. |
| dev-infrastructure/configurations/mgmt-cluster.tmpl.bicepparam | Adds bicep param placeholder for csManagedIdentityPrincipalId. |
| config/config.yaml | Changes default Cluster Service image registry/repo/digest. |
| config/rendered/dev/pers/westus3.yaml | Materialized Cluster Service image override for pers env. |
| config/rendered/dev/perf/westus3.yaml | Materialized Cluster Service image override for perf env. |
| config/rendered/dev/dev/westus3.yaml | Materialized Cluster Service image override for dev env. |
| config/rendered/dev/cspr/westus3.yaml | Materialized Cluster Service image override for cspr env. |
| config/rendered/dev/ci01/centralus.yaml | Materialized Cluster Service image override for ci01 env. |
| config/rendered/dev/ci00/centralus.yaml | Materialized Cluster Service image override for ci00 env. |
| cluster-service/zz_fixture_TestHelmTemplate_dev_westus3_svc_1_cluster_service.yaml | Helm fixture updates reflecting the new CS image + ACR scope. |
| cluster-service/testdata/zz_fixture_TestHelmTemplate_cs_shared_arm_helper.yaml | Helm fixture updates reflecting the new CS image + ACR scope. |
| cluster-service/testdata/zz_fixture_TestHelmTemplate_cs_distinct_arm_helper.yaml | Helm fixture updates reflecting the new CS image + ACR scope. |
| cluster-service/testdata/zz_fixture_TestHelmTemplate_cs_containerdb.yaml | Helm fixture updates reflecting the new CS image + ACR scope. |
| cluster-service/testdata/zz_fixture_TestHelmTemplate_cs_azuredb.yaml | Helm fixture updates reflecting the new CS image + ACR scope. |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| clustersService: | ||
| image: | ||
| registry: quay.io | ||
| repository: app-sre/aro-hcp-clusters-service | ||
| digest: sha256:4bc659373dbf9b5eba2df7da2ceccec2952fa6d712a406a2a2a13e887228c912 # 5fc479961bb4bdcad6a58b5075879613a913d583 (2026-08-30 13:46) | ||
| registry: arohcpsvcdev.azurecr.io | ||
| repository: mchitimb/cs | ||
| digest: sha256:42f40e917eaa873a729ec656d99377845401642dc8a1369c54ed0c519e1622a3 |
| resource sqlRoleAssignmentResourcesReadWrite 'Microsoft.DocumentDB/databaseAccounts/sqlRoleAssignments@2021-04-15' = { | ||
| name: guid(cosmosDataContributorRoleDefinitionId, csManagedIdentityPrincipalId, resourcesContainerScope) | ||
| parent: cosmosDbAccount | ||
| properties: { | ||
| roleDefinitionId: '${cosmosDbAccount.id}/sqlRoleDefinitions/${cosmosDataContributorRoleDefinitionId}' |
|
/hold |
d8c41fc to
1362fc6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new container-scoped Cosmos role assignments need explicit deployment ordering dependencies, and the default Clusters Service image override to a personal repo is unsafe to keep in shared defaults.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
dev-infrastructure/modules/rp-cosmos.bicep:149
- Same as the Fleet role assignment: this container-scoped sqlRoleAssignment should explicitly depend on the Resources container creation to avoid nondeterministic ordering during deployment.
resource sqlRoleAssignmentResourcesReadWrite 'Microsoft.DocumentDB/databaseAccounts/sqlRoleAssignments@2021-04-15' = {
name: guid(cosmosDataContributorRoleDefinitionId, csManagedIdentityPrincipalId, resourcesContainerScope)
parent: cosmosDbAccount
properties: {
roleDefinitionId: '${cosmosDbAccount.id}/sqlRoleDefinitions/${cosmosDataContributorRoleDefinitionId}'
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Lite
| registry: arohcpsvcdev.azurecr.io | ||
| repository: mchitimb/cs | ||
| digest: sha256:394eee85c19ce289abbbd9966008276a0573a58a47e2579e0bb41bb186b4adae |
| resource sqlRoleAssignmentFleetReadOnly 'Microsoft.DocumentDB/databaseAccounts/sqlRoleAssignments@2021-04-15' = { | ||
| name: guid(cosmosReadOnlyRoleDefinitionId, csManagedIdentityPrincipalId, fleetContainerScope) | ||
| parent: cosmosDbAccount | ||
| properties: { | ||
| roleDefinitionId: '${cosmosDbAccount.id}/sqlRoleDefinitions/${cosmosReadOnlyRoleDefinitionId}' | ||
| principalId: csManagedIdentityPrincipalId | ||
| scope: fleetContainerScope | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Changes recommended
It changes the repo-wide default Cluster Service image to a personal/custom repository, which is a high-risk security/operational footgun if merged or reused outside the intended test run.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite
|
Awesome, two runs passing successfully and one is complete green https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/Azure_ARO-HCP/6832/pull-ci-Azure-ARO-HCP-main-e2e-parallel/2095996619395698688 The previous one failed on something unrelated https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/Azure_ARO-HCP/6832/pull-ci-Azure-ARO-HCP-main-e2e-parallel/2095950323251154944 Let me retrigger another retest since #6709 is merged |
… to read nodepool manifest via kube-applier read desire
…itlab.cee.redhat.com/service/aro-hcp-clusters-service/-/merge_requests/446 That image changes CS to use NodePool & HostedCluster read desire during clusters & node pool status reconciliation as well as persist what's done by those watching controllers to Cosmos' Resources container. CS takes care of deletion too.
…ding readdesire types for cluster
41b6b09 to
5fca5e6
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: machi1990 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes the default Cluster Service image to a personal repository, which is not safe/appropriate for merge into shared configuration without a test-only override mechanism and clear provenance metadata.
Review details
Suppressed comments (2)
config/config.yaml:1106
- The default Cluster Service image in config/config.yaml now points at a personal ACR repository (arohcpsvcdev.azurecr.io/mchitimb/cs). If this PR is ever merged (even accidentally), it would make non-personal environments depend on an image that likely isn’t broadly available/owned by the repo, which can break deployments and CI. Consider keeping defaults on the standard app-sre/aro-hcp-clusters-service image and applying test-only overrides via environment-specific overlays/rendered configs (then re-materialize fixtures).
clustersService:
image:
registry: arohcpsvcdev.azurecr.io
repository: mchitimb/cs
digest: sha256:d4464057af938f1defaa363d7e347e0080128f58ed0f7ca6b27622380325284c
config/config.yaml:1106
- Other image digests in this file include a trailing provenance comment (source version/commit + timestamp). This digest line lost that metadata, which makes it harder to audit what’s running.
digest: sha256:d4464057af938f1defaa363d7e347e0080128f58ed0f7ca6b27622380325284c
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Manyanda Chitimbo (@machi1990): The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
The retest also passed barring one failure https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/Azure_ARO-HCP/6832/pull-ci-Azure-ARO-HCP-main-e2e-parallel/2096113651420237824 which isn't related to the CS change itself. |
Same as #6815 and #6780 but with more changes in CS https://gitlab.cee.redhat.com/service/aro-hcp-clusters-service/-/merge_requests/446
That image changes CS to use NodePool & HostedCluster read desire during clusters & node pool status reconciliation as well as persist what's done by those watching controllers to Cosmos' Resources container.
Open a PR to run the whole e2e suite and isn't intended for merge