Cluster install migration - #6082
Jakob Gray (JakobGray) wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request continues the “cluster install migration” by shifting Cluster Service (CS) provisioning for cluster create out of the frontend and into backend async controllers, while backfilling key CS synchronous validations into frontend validation/admission so invalid requests still fail fast. It also updates integration-test tooling/fixtures to work with the new async CS ID behavior and updates etcd encryption expectations to require CustomerManaged.
Changes:
- Stop frontend cluster-create from calling CS
PostCluster; persist clusters with an emptyClusterServiceIDand rely on backend provisioning. - Add/extend frontend validation & admission checks (e.g., VNet-integrated subnet same-VNet, managed identity uniqueness, NSG subscription/MRG constraints, etcd encryption mode constraints).
- Update integration test utilities/artifacts to deterministically derive/stamp
ClusterServiceIDand remove now-obsolete CS mock comparison steps.
Note: The PR title does not follow Conventional Commits format as required by CONTRIBUTING.md “Pull Request Standards” (§2, lines 79–83).
Reviewed changes
Copilot reviewed 46 out of 48 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
test-integration/utils/integrationutils/utils.go |
Update frontend constructor usage for integration tests. |
test-integration/utils/integrationutils/set_cluster_service_id.go |
Make CS IDs derivable deterministically; add lazy parent stamping helper. |
test-integration/utils/integrationutils/cluster_service_mock.go |
Remove HREF lookup/merged-cluster helpers no longer needed by tests. |
test-integration/utils/databasemutationhelpers/step_set_cluster_service_id.go |
Update step to use deterministic CS ID derivation. |
test-integration/utils/databasemutationhelpers/step_httpcreate.go |
Ensure parent cluster has CSID before creating child resources. |
test-integration/utils/databasemutationhelpers/step_cluster_service_compare.go |
Remove CS compare step implementation (deleted). |
test-integration/utils/databasemutationhelpers/resource_crud_test_util.go |
Remove clusterServiceCompare step wiring. |
test-integration/frontend/version_compliance_test.go |
Ensure parent cluster CSID is present for child-resource scenarios. |
test-integration/frontend/cross_version_roundtrip_test.go |
Update payloads/flows for customer-managed etcd encryption + CSID stamping behavior. |
test-integration/frontend/artifacts/VersionCompliance/NodePool/basic-nodepool/cluster.json |
Update fixture for customer-managed etcd encryption. |
test-integration/frontend/artifacts/VersionCompliance/Cluster/basic-cluster/request.json |
Update fixture for customer-managed etcd encryption. |
test-integration/frontend/artifacts/VersionCompliance/Cluster/basic-cluster/expected/list/2026-06-30-preview.json |
Update expected output for customer-managed etcd encryption. |
test-integration/frontend/artifacts/VersionCompliance/Cluster/basic-cluster/expected/list/2025-12-23-preview.json |
Update expected output for customer-managed etcd encryption. |
test-integration/frontend/artifacts/VersionCompliance/Cluster/basic-cluster/expected/list/2024-06-10-preview.json |
Update expected output for customer-managed etcd encryption. |
test-integration/frontend/artifacts/VersionCompliance/Cluster/basic-cluster/expected/get/2026-06-30-preview.json |
Update expected output for customer-managed etcd encryption. |
test-integration/frontend/artifacts/VersionCompliance/Cluster/basic-cluster/expected/get/2025-12-23-preview.json |
Update expected output for customer-managed etcd encryption. |
test-integration/frontend/artifacts/VersionCompliance/Cluster/basic-cluster/expected/get/2024-06-10-preview.json |
Update expected output for customer-managed etcd encryption. |
test-integration/frontend/artifacts/FrontendCRUD/NodePool/create-current/06-cosmosCompare-confirm-content/cluster-create-with-tags.json |
Bump expected Cosmos document instance version. |
test-integration/frontend/artifacts/FrontendCRUD/NodePool/create-current/05-listActiveOperations-cluster-create/operation-nodepool-create-nodepool-02.json |
Update expected operation fields in fixture. |
test-integration/frontend/artifacts/FrontendCRUD/NodePool/create-current/05-listActiveOperations-cluster-create/operation-nodepool-create-basic-nodepool.json |
Update expected operation fields in fixture. |
test-integration/frontend/artifacts/FrontendCRUD/ExternalAuth/create-current/06-cosmosCompare-ending-content/cluster-create-with-tags.json |
Bump expected Cosmos document instance version. |
test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features/06-clusterServiceCompare-update-cleared-flag/expected-content.json |
Remove CS compare expected content fixture. |
test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features/03-loadCosmos-serviceProviderCluster/serviceProviderCluster.json |
Add ServiceProviderCluster fixture for deterministic versioning. |
test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features/03-clusterServiceCompare-create-propagated-flag/expected-content.json |
Remove CS compare expected content fixture. |
test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features-no-afec/05-httpCreate-cluster-with-4.19/expected-error.txt |
Add expected error fixture for invalid version scenario. |
test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features-no-afec/05-httpCreate-cluster-with-4.19/create-with-4.19.json |
Adjust test resource IDs to avoid new uniqueness/admission constraints. |
test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features-no-afec/05-httpCreate-cluster-with-4.19/00-key.json |
Add key fixture for scenario. |
test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features-no-afec/04-setClusterServiceID-cluster/00-key.json |
Add explicit setClusterServiceID step key fixture. |
test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features-no-afec/03-loadCosmos-serviceProviderCluster/serviceProviderCluster.json |
Add ServiceProviderCluster fixture for scenario. |
test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features-no-afec/03-clusterServiceCompare-tags-ignored/expected-content.json |
Remove CS compare expected content fixture. |
test-integration/frontend/artifacts/FrontendCRUD/Cluster/delete-with-pending-nodepool-operation/06-cosmosCompare-final-state/cluster-test-cluster.json |
Bump expected Cosmos document instance version. |
test-integration/claude.md |
Document the new setClusterServiceID step and lazy parent stamping behavior. |
internal/validation/validators.go |
Add SameVirtualNetwork validator for subnet/VNet parity checks. |
internal/validation/validate_cluster.go |
Add NSG subscription/MRG checks; add vnetIntegrationSubnet same-VNet; add managed identity uniqueness enforcement. |
internal/validation/validate_cluster_comprehensive_test.go |
Expand comprehensive validation tests for new rules/constraints. |
internal/validation/hcpopenshiftcluster_test.go |
Update unit tests for etcd encryption mode enforcement & identity uniqueness. |
internal/ocm/convert.go |
Require SPC desired version to build CS cluster version ID (no fallback). |
internal/ocm/convert_test.go |
Update CS cluster conversion tests for new version-ID behavior and etcd encryption fields. |
internal/api/types_cluster.go |
Clarify etcd encryption default-vs-supported behavior in comments. |
internal/api/testhelpers.go |
Make minimum valid cluster test case include customer-managed etcd encryption. |
internal/api/enums.go |
Exclude PlatformManaged from valid etcd key-management modes (temporarily). |
internal/admission/admit_cluster.go |
Add subscription-wide uniqueness admission checks for MRG/subnet/NSG on CREATE. |
internal/admission/admit_cluster_test.go |
Add tests for new platform resource uniqueness admission checks. |
frontend/pkg/frontend/testhelpers.go |
Update test frontend constructor signature usage. |
frontend/pkg/frontend/frontend.go |
Remove unused CS provision/noop flags from Frontend and constructor. |
frontend/pkg/frontend/frontend_test.go |
Update expected preflight errors/fixtures for new validation rules and constructor signature. |
frontend/pkg/frontend/cluster.go |
Prefetch subscription clusters/node pools for CREATE admission; remove synchronous CS create on cluster create. |
frontend/cmd/cmd.go |
Remove frontend CLI flags for CS provision/noop behaviors; update frontend wiring. |
| subnetPath := fldPath.Child("subnetId") | ||
| if newObj.SubnetID == nil { | ||
| return field.ErrorList{field.Required(subnetPath, "")} | ||
| } |
| nsgPath := fldPath.Child("networkSecurityGroupId") | ||
| if newObj.NetworkSecurityGroupID == nil { | ||
| return field.ErrorList{field.Required(nsgPath, "")} | ||
| } |
| if op.Type == operation.Create { | ||
| subscriptionID := originalCluster.ID.SubscriptionID | ||
| clusterIterator, err := f.resourcesDBClient.HCPClusters(subscriptionID, "").List(ctx, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot list clusters for cluster admission: %w", err) | ||
| } | ||
| for _, cluster := range clusterIterator.Items(ctx) { | ||
| admissionContext.SubscriptionClusters = append(admissionContext.SubscriptionClusters, cluster) | ||
|
|
||
| nodePoolIterator, err := f.resourcesDBClient.HCPClusters(subscriptionID, cluster.ID.ResourceGroupName).NodePools(cluster.ID.Name).List(ctx, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot list node pools for cluster admission: %w", err) | ||
| } | ||
| for _, nodePool := range nodePoolIterator.Items(ctx) { | ||
| admissionContext.SubscriptionNodePools = append(admissionContext.SubscriptionNodePools, nodePool) | ||
| } | ||
| if err := nodePoolIterator.GetError(); err != nil { | ||
| return nil, fmt.Errorf("cannot list node pools for cluster admission: %w", err) | ||
| } |
| mrgPath := fldPath.Child("managedResourceGroup") | ||
| if len(newObj.ManagedResourceGroup) == 0 { | ||
| return field.ErrorList{field.Required(mrgPath, "")} | ||
| } |
| return errs | ||
| } | ||
|
|
||
| // validateManagedIdentitiesUniqueWithinCluster ensures that each managed identity |
There was a problem hiding this comment.
Why do we care?
There was a problem hiding this comment.
This moves the validation from CS to RP, which was already enforcing it. See Why for some additional context
There was a problem hiding this comment.
No, but why do these need to be unique? What's the point of the check?
There was a problem hiding this comment.
This was discussed on a thread a while ago with details that I do not recall, for now we are simply moving what CS is enforcing,
There was a problem hiding this comment.
At minimum, could we please add a GoDoc comment for why unique identities are needed? I actually remember that thread going exactly the other way, and would appreciate it if someone suggesting to merge this actually justifies their need.
There was a problem hiding this comment.
As discussed through other channels:
Jakob Gray (@JakobGray) let's add a code comment mentioning that we are going to revisit this after GA, linking the https://redhat.atlassian.net/browse/ARO-21615 jira.
We can put a summary of a conversation that happened around it: the last discussions were around the will to remove the restriction, but we needed to perform a set of verifications and decisions to ensure that would be feasible and work with our requirements. Work started on it but priorities shifted and we need to retake that
| } | ||
|
|
||
| func (l *httpCreateStep) RunTest(ctx context.Context, t *testing.T, stepInput StepInput) { | ||
| err := integrationutils.EnsureParentClusterServiceID( |
There was a problem hiding this comment.
Why is this needed? I am unsure about the correctness of this. This is to interact with HTTP Frontend, and it shouldn't imply setting always the parent cluster service id. If a cluster needs the CSID, use the appropriate artifact and step
There was a problem hiding this comment.
It exists because the frontend still gates node pool/external auth create when parent.ClusterServiceID == nil, while cluster create no longer sets that ID synchronously. We can remove it and add more artifacts though
| | `cosmosCompare` | Assert entire cosmos state matches expected JSON documents | | ||
| | `kubernetesCompare` | Assert K8s resource state matches expected JSON files (uses `ResourceInstanceEquals`) | | ||
| | `completeOperation` | Mark an async operation as succeeded | | ||
| | `setClusterServiceID` | Stamp a Cluster Service internal ID onto a cluster/node pool/external auth doc. Derives deterministically from the resource name (clusters) or the parent's CS ID (node pools/external auths) unless an explicit `cluster-service-id.json` is provided. Child `httpCreate` steps auto-provision the parent cluster's CS ID via the same derive-and-set helper (`EnsureParentClusterServiceID`), so most tests never need this step explicitly for the parent cluster. | |
| // EnsureParentClusterServiceID stamps a random Cluster Service internal ID onto | ||
| // a child resource's parent cluster when it doesn't already have one. This is used | ||
| // when a child resource create requires the parent cluster to have a ClusterServiceID. | ||
| func EnsureParentClusterServiceID( |
| if op.Type == operation.Create { | ||
| subscriptionID := originalCluster.ID.SubscriptionID | ||
| clusterIterator, err := f.resourcesDBClient.HCPClusters(subscriptionID, "").List(ctx, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot list clusters for cluster admission: %w", err) |
| for _, cluster := range clusterIterator.Items(ctx) { | ||
| admissionContext.SubscriptionClusters = append(admissionContext.SubscriptionClusters, cluster) | ||
|
|
||
| nodePoolIterator, err := f.resourcesDBClient.HCPClusters(subscriptionID, cluster.ID.ResourceGroupName).NodePools(cluster.ID.Name).List(ctx, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot list node pools for cluster admission: %w", err) | ||
| } | ||
| for _, nodePool := range nodePoolIterator.Items(ctx) { | ||
| admissionContext.SubscriptionNodePools = append(admissionContext.SubscriptionNodePools, nodePool) | ||
| } |
34ec73a to
199b685
Compare
199b685 to
6eb3aac
Compare
|
As a note just in case: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/Azure_ARO-HCP/6082/pull-ci-Azure-ARO-HCP-main-e2e-parallel/2077606785388122112 we had an e2e passing before further changes in the integration-tests. |
follow-up for Azure#6082, adding new negative E2E test case reusing MIs within a cluster.
Cluster Service no longer returns synchronous 400s for platform uniqueness checks after async CS create migration. Move those checks to the frontend so invalid cluster PUTs fail at admission time with the same error messages CS used in performSpecValidation. These checks are best-effort and may not catch concurrent creates. - Prefetch subscription clusters and node pools in newClusterAdmissionContext - Add admitClusterManagedResourceGroupName, admitClusterSubnetResourceID, and admitClusterNetworkSecurityGroupResourceID (CREATE only) - Add NSG same-subscription and not-in-MRG rules to static validation (parity with validateAroHcpClusterNetworkSecurityGroupResourceId) Co-authored-by: Cursor <cursoragent@cursor.com>
Mirror CS validateAroHcpSwiftSubnetSameVnet: vnetIntegrationSubnetId must use the same VNet as subnetId. Co-authored-by: Cursor <cursoragent@cursor.com>
Mirror CS validateAzureOperatorsAuthenticationManagedIdentitiesUniqueWithinCluster across service, control-plane, and data-plane operator identities.
Align with CS validateAroHcpEtcdEncryptionDataEncryptionKeyManagementMode: only CustomerManaged is accepted until platform-managed etcd encryption is supported.
Stop calling PostCluster during HCP cluster create in the frontend. Persist the cluster and enqueue the ARM operation with an empty ClusterServiceID; the backend controller now owns CS provisioning. Remove frontend-only provision-shard and noop provision/deprovision flags.
…rontendCRUD Add an explicit setClusterServiceID step so frontend integration tests mirror deferred CS create (backend stamps the ID) without auto-mutating Cosmos in httpCreate. Parent CS IDs are stamped in artifact steps and Go helpers (StampRandomClusterServiceID); child IDs derive from the parent. - Random cluster HREFs by default, or fixed IDs via cluster-service-id.json - Seed the CS mock (loadClusterService) before cluster updates so GetCluster/UpdateCluster paths are exercised - Name setClusterServiceID step suffixes after the target resource - Stamp CS ID before completeOperation to match production ordering; update mid-suite cosmosCompare expectations (e.g. create-current instanceVersion) - BuildCSCluster/clusterCSVersionID now require ControlPlaneVersion .DesiredVersion on the ServiceProviderCluster, removing the old fallback to the customer-facing version on the HCP cluster document - Document child-create vs cluster-update patterns in claude.md Co-authored-by: Cursor <cursoragent@cursor.com>
47daaca to
5a0577b
Compare
|
New changes are detected. LGTM label has been removed. |
|
PR needs rebase. 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. |
|
Work merged in #6121 |
|
The same code has been merged in a unified PR with other changes, in #6121. Closing this. |
follow-up for Azure#6082, adding new negative E2E test case reusing MIs within a cluster.
follow-up for Azure#6082, adding new negative E2E test case reusing MIs within a cluster.
follow-up for Azure#6082, adding new negative E2E test case reusing MIs within a cluster.
follow-up for Azure#6082, adding new negative E2E test case reusing MIs within a cluster.
follow-up for Azure#6082, adding new negative E2E test case reusing MIs within a cluster.
follow-up for Azure#6082, adding new negative E2E test case reusing MIs within a cluster.
https://redhat.atlassian.net/browse/ARO-24823
What
This continues migrating HCP cluster installation so Cluster Service (CS) provisioning happens asynchronously in the backend instead of synchronously in the frontend, and backfills the validation CS used to do synchronously so users still get fast, equivalent errors.
Defer CS provisioning to the backend
Backfill validation CS used to enforce synchronously
Test tooling
Why
Cluster Service no longer returns synchronous 400s for these checks once cluster creation became async, since the frontend no longer waits on a PostCluster response. Moving the checks into frontend admission/validation preserves today's fail-fast behavior (same error messages CS used to return) instead of silently deferring rejection to a later, harder-to-surface backend failure. These are best-effort checks; races between concurrently-admitted clusters are still possible, same as before.
Testing
Special notes for your reviewer
See #5709 for review of frontend migration in greater detail
See #6004 for review of validation changes in greater detail
See #4821 for backend changes. These should be rolled out to production before frontend changes are added.
PR Checklist
If E2E tests are included:
demonstrate that the test is able to detect a defect/error and fail with
proper error message and logs which communicates nature of the problem.