feat: add containerRegistry to ARM API for ACR pull via managed identity (ARO-24037) - #5562
Conversation
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for configuring Azure Container Registry (ACR) authentication on HCP OpenShift clusters (2025-12-23-preview), wiring the new containerRegistry field through API schemas, internal types, conversions, and validation.
Changes:
- Introduces
containerRegistryinto the platform profile schema (TypeSpec/OpenAPI) and updates example payloads. - Extends internal API types/enums plus external<->internal conversion logic to carry the new profile across versions.
- Updates validation to allow
platform.containerRegistryupdates while keeping the rest ofplatformimmutable.
Reviewed changes
Copilot reviewed 18 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/validation/validate_cluster.go | Allows mutability for platform.containerRegistry and adds validation for the new profile. |
| internal/api/v20251223preview/hcpopenshiftclusters_methods.go | Maps containerRegistry in model conversions and normalizes incoming payloads. |
| internal/api/v20240610preview/hcpopenshiftclusters_methods.go | Preserves unknown containerRegistry field across older API version conversions. |
| internal/api/v20240610preview/conversion_fuzz_test.go | Updates fuzz round-trip customizations for version-skewed fields, including containerRegistry. |
| internal/api/types_cluster.go | Adds internal API structs for ContainerRegistryProfile and related types. |
| internal/api/enums.go | Adds ContainerRegistryCredentialType enum and allowed set. |
| api/.../openapi.json | Defines containerRegistry schemas and embeds it into PlatformProfile (and update variants). |
| api/.../examples/*.json | Adds containerRegistry to generated example payloads for the 2025-12-23-preview API. |
| api/.../hcpCluster-models.tsp | Adds TypeSpec models for containerRegistry and supporting shapes/enums. |
Files not reviewed (1)
- internal/api/zz_generated.deepcopy.go: Language not supported
73dab29 to
2d82b53
Compare
Manyanda Chitimbo (machi1990)
left a comment
Manyanda Chitimbo (machi1990)
left a comment
There was a problem hiding this comment.
lgtm
Not adding the command as I'd defer that to SMEs
|
/hold
|
Matthew Barnes (mbarnes)
left a comment
Matthew Barnes (mbarnes)
left a comment
There was a problem hiding this comment.
Suggested simplification for consistency with other places in the API that take a user-assigned managed identity resource ID.
2d82b53 to
70d4923
Compare
70d4923 to
2bee5be
Compare
2bee5be to
0bb66e9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 42 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- internal/api/zz_generated.deepcopy.go: Generated file
Comments suppressed due to low confidence (2)
internal/ocm/convert.go:1
- On update, this only sends
containerRegistrywhen it’s non-nil, which prevents users from clearing/removing an existing container registry config (nil becomes “no change”). If the CS API supports clearing, explicitly set the field to nil on updates whenhcpCluster.CustomerProperties.Platform.ContainerRegistryis nil; otherwise document/validate that removal isn’t supported.
internal/validation/validate_cluster.go:1 - The required-field message here doesn’t match the conventions used elsewhere in this repo’s validation tests (which often assert
"Required"). Consider using the same capitalization/wording as otherfield.Requiredusages (or centralizing the message) to keep error outputs consistent and reduce brittle test expectations.
0bb66e9 to
b2094f1
Compare
77397f5 to
1a47c94
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: twolff-gh 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.
🟡 Changes recommended
It includes a duplicated go:generate directive and currently treats managedIdentity: "" as a silent clear instead of rejecting it as invalid input.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- Files reviewed: 69/73 changed files
- Comments generated: 2
- Review effort level: Lite
|
/retest |
There was a problem hiding this comment.
🟡 Changes recommended
The docs currently contradict the implemented keyed-validation storage/compare behavior, and admission can return an internal error unnecessarily when ActiveVersions are temporarily unavailable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
docs/cosmos-data-flow.md:1064
- These table rows still reference
condition.Messageas the change-detection / storage mechanism for keyed validations. Since the code now writes the input key toStatus.ValidationInputKeys, the read/write field list should reference that map (and avoid implying thatMessagecontains machine-readable keys).
| Read | `ServiceProviderCluster` | <ul><li>`Status.Validations[<name>]` (shouldProcess: condition must not be True; keyed validations also re-run when `InputKey(cluster) != condition.Message`)</li></ul> |
| Read | `ServiceProviderNodePool` | <ul><li>`Status.Validations[<name>]` (shouldProcess: condition must not be True)</li></ul> |
| Read | `HCPOpenShiftCluster` | <ul><li>`ServiceProviderProperties.DeletionTimestamp` (SyncOnce: must be nil)</li><li>`CustomerProperties.Platform.ContainerRegistry.PullManagedIdentity` (keyed-validation input for `ContainerRegistryPullCredentialsPermissionValidation`; also the validated resource — CAPZ `assign/action` permission is checked against it via CheckAccess V2)</li><li>`CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators` (CAPZ identity)</li><li>`CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity`</li></ul> |
| Read | `HCPOpenShiftClusterNodePool` | <ul><li>`ServiceProviderProperties.DeletionTimestamp` (SyncOnce: must be nil)</li></ul> |
| **Write** | **`ServiceProviderCluster`** | <ul><li>**`Status.Validations[<name>]`** = condition (True/False). For keyed validations the success condition `Message` holds the input key (e.g. the pull MI resource ID), not a human-readable string.</li></ul> |
- Files reviewed: 69/73 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are at least two correctness issues to address (ACR name casing in the new E2E and inconsistent cross-subscription handling between request validation and backend validation).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/e2e/cluster_container_registry.go:159
- Azure Container Registry names must be lowercase; generating the name from the resource group without lowercasing can cause ACR creation to fail if the RG contains any uppercase characters.
- Files reviewed: 70/74 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new unit test for invalid containerRegistry managedIdentity input does not currently assert an error, which can allow regressions in normalization behavior to go undetected.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/azureapi/v20260901preview/hcpopenshiftclusters_methods_test.go:68
- The "invalid resource ID" test case doesn't currently assert that
normalizeContainerRegistryreturns an error: withwantError: "", the test takes the early-return path (line 88) and will still pass even if the invalid ID is incorrectly accepted. This makes the test ineffective for the intended behavior.
- Files reviewed: 90/94 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new E2E test uses printf-style format verbs in Gomega assertion messages, which won’t render as intended and should be corrected for debuggable failures.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (8)
Previously missed (2) — in code that hasn't changed since the last review.
test/e2e/cluster_container_registry.go:172
- Gomega's optional description arguments are concatenated with fmt.Sprint; they are not treated as printf-style format strings. As written, the "%s" placeholder will be printed literally, making failures harder to debug. Use fmt.Sprintf (or build the string without format verbs).
This issue also appears in the following locations of the same file:
- line 191
- line 284
- line 293
- line 327
- line 336
test/e2e/cluster_container_registry.go:260
- This failure message uses %q placeholders but Gomega does not treat optional description arguments as a format string; it will print the placeholders literally. Use fmt.Sprintf to format the message (or compare normalized strings with Equal to avoid BeTrue + custom message).
This issue also appears on line 387 of the same file.
test/e2e/cluster_container_registry.go:191
- Same issue here: Gomega doesn't printf-format the optional description args, so the "%s" placeholder will be printed literally. Wrap the message in fmt.Sprintf (or avoid format verbs).
test/e2e/cluster_container_registry.go:284 - Gomega doesn't printf-format the optional description args; with the current call the "%s" placeholder will be printed literally. Wrap with fmt.Sprintf (or build a plain string) so the namespace name shows up correctly on failure.
test/e2e/cluster_container_registry.go:293 - Gomega doesn't printf-format the optional description args; the "%s" placeholder will be printed literally. Use fmt.Sprintf so the namespace name is included in the failure output.
test/e2e/cluster_container_registry.go:327 - Gomega optional description args are not printf-formatted; the pod name won't be substituted into the "%s" placeholder. Use fmt.Sprintf (or a plain message) so failures include the actual pod name.
test/e2e/cluster_container_registry.go:336 - Gomega doesn't treat the optional description as a format string; the "%s" placeholder will be printed literally. Wrap in fmt.Sprintf so the ACR login server is shown in failure output.
test/e2e/cluster_container_registry.go:389 - This uses %q placeholders but Gomega does not printf-format the optional description args, so the placeholders will be printed literally. Use fmt.Sprintf (or a different matcher) so the mismatch details render correctly.
- Files reviewed: 90/94 changed files
- Comments generated: 0 new
- Review effort level: Lite
| func (c *clusterValidationSyncer) shouldProcess(serviceProviderCluster *coreapi.ServiceProviderCluster, cluster *coreapi.HCPOpenShiftCluster) bool { | ||
| condition := meta.FindStatusCondition(serviceProviderCluster.Status.Validations, c.validation.Name()) | ||
| if condition == nil || condition.Status != metav1.ConditionTrue { | ||
| return true | ||
| } | ||
| if keyed, ok := c.validation.(validationutils.InputKeyedClusterValidation); ok { | ||
| return keyed.InputKey(cluster) != serviceProviderCluster.Status.ValidationInputKeys[c.validation.Name()] | ||
| } | ||
| return false |
There was a problem hiding this comment.
We are going to enable continuous validation as part of this PR: #6439
We might not need this anymore, since the validation will run continuously.
There was a problem hiding this comment.
Will this merge before this PR? The main thing is the day 2 support for this. I was looking and this appears to be the first one that needs continuous validation because the value might be changes by a customer. Is that right?
There was a problem hiding this comment.
Suraj Patil (@patilsuraj767) Do we need to remove this then if #6439 handles this?
And does that mean this will be blocked on #6439, or can this merge then let #6439 modify the behavior
There was a problem hiding this comment.
I think it would be better to remove the InputKeyedClusterValidation type from this PR, as it introduces code that will no longer be required after #6439 is merged.
There was a problem hiding this comment.
Stripped it out Suraj Patil (@patilsuraj767)
Should be good to go
|
/retest |
1 similar comment
|
/retest |
There was a problem hiding this comment.
🟡 Changes recommended
The added dispatch-config tests currently assert string expectations against *string values (and the new CS extraction helper comment is misleading), which will cause CI failures and ambiguity until corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
internal/ocm/cluster_update_dispatch_config_test.go:1604
- rpConfig.ContainerRegistryPullManagedIdentityResourceID is a *string; comparing it directly to a string will always fail. Dereference (treat nil as empty) before asserting.
assert.Equal(t, miID.String(), rpConfig.ContainerRegistryPullManagedIdentityResourceID)
- Files reviewed: 90/94 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are blocking correctness issues in the new tests and in CS-vs-desired comparison logic (empty-string clearing signal) that can cause failing tests or a stuck Updating state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- Files reviewed: 90/94 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There is a potential nil dereference in the CS conversion clear-signal path (oldClusterServiceCluster.Azure().ContainerRegistry() without guarding Azure()), which can crash update processing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- Files reviewed: 90/94 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The keyed re-validation path can still be throttled by the controller’s retry cooldown gate, which can prevent timely re-validation after day-2 input changes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- Files reviewed: 90/94 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The updated cluster validation controller appears to bypass cooldown gating for failed/unknown validations, which can cause validations to run too frequently and hammer external dependencies.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- Files reviewed: 90/94 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The updated cooldown/shouldProcess ordering in the cluster validation controller can defeat retry throttling and cause repeated validations on every sync, and the new E2E test’s ACR name generation can violate Azure ACR lowercase naming rules.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
backend/pkg/controllers/cluster/validation/cluster_validation_controller.go:140
- The cooldown gate is now applied only when
shouldProcessreturns false (i.e., already-succeeded validations). For Failed/Unknown (and even Skipped) outcomesshouldProcessremains true, so validations can run on every sync event, defeatingEarliestRetryAfterthrottling and potentially causing repeated external calls. Consider applying the cooldown gate first, and only bypassing it when a keyed validation's input changed.
if !c.shouldProcess(cachedServiceProviderCluster, existingCluster) {
// No input change. Apply the cooldown gate.
if !c.retryCooldownChecker.CanSync(ctx, key) {
- Files reviewed: 90/94 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The keyed-validation cooldown bypass can unintentionally disable retry throttling after failures, risking excessive revalidation against external dependencies.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- Files reviewed: 90/94 changed files
- Comments generated: 1
- Review effort level: Lite
| if matches, message := c.clusterServiceClusterNodeDrainTimeoutSpecMatchesDesired(cluster.CustomerProperties.NodeDrainTimeoutMinutes, csCluster); !matches { | ||
| return false, message | ||
| } | ||
| if matches, message := c.clusterServiceClusterContainerRegistryPullMISpecMatchesDesired(cluster.CustomerProperties.Platform.ContainerRegistry.PullManagedIdentity, csCluster); !matches { |
There was a problem hiding this comment.
Also let's add a matcher that the hosted cluster cr has been updated.
See how it's done for other updates
There was a problem hiding this comment.
Added. Pushing it up
There was a problem hiding this comment.
Requested a small enhancement to the operation update controller.
Once that is done and Suraj Patil (@patilsuraj767) is happy with the changes in the validation then I am ready to lgtm
There was a problem hiding this comment.
🟡 Changes recommended
There is at least one concrete correctness issue (potential nil-pointer panic in the new E2E helper) and an unresolved versioning mismatch between the PR description and the API version where the schema change landed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- Files reviewed: 93/97 changed files
- Comments generated: 5
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The cluster validation controller currently cannot re-run a previously-passed validation when containerRegistry.managedIdentity changes, which undermines the intended day-2 revalidation behavior described in the PR.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- Files reviewed: 92/96 changed files
- Comments generated: 4
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness issues in newly added/updated test fixtures and state/validation behavior (including a doc/implementation mismatch that prevents day-2 revalidation after managed identity changes).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
test-integration/frontend/artifacts/FrontendCRUD/Cluster/create-reject-containerRegistry-managed-rg/01-httpCreate-cluster/expected-error.txt:1
- This integration fixture’s
expected-error.txtlooks truncated: the JSON string is missing the closing quote, and it doesn’t assert thetargetpath (unlike the other containerRegistry fixtures). This will either fail to match or weaken the test signal.
- Files reviewed: 90/94 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are at least two correctness issues (a non-compiling test loop and a potential nil dereference in backend validation) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
internal/validation/validate_cluster_test.go:995
- This loop won’t compile:
rangecan’t be used on anint(typ.NumField()returns an int). Use a standard index loop over the struct fields.
- Files reviewed: 90/94 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
internal/validation/validate_cluster_test.go introduces a non-compiling loop (for i := range typ.NumField()), which will break the build/test run.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
internal/validation/validate_cluster_test.go:994
- This loop won’t compile:
range typ.NumField()attempts to range over anint. Use a standard index loop overNumField()instead.
- Files reviewed: 90/94 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new backend validation dereferences clusterSubscription.Properties.TenantId without nil/empty checks, risking a panic in the controller when Subscription properties are missing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- Files reviewed: 90/94 changed files
- Comments generated: 1
- Review effort level: Lite
|
/hold cancel |
There was a problem hiding this comment.
🟡 Changes recommended
The TypeSpec/OpenAPI contract does not currently express the documented PATCH clear behavior (setting containerRegistry to null), which risks ARM schema rejection of legitimate requests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- Files reviewed: 90/94 changed files
- Comments generated: 1
- Review effort level: Lite
Summary
Add properties.platform.containerRegistry.managedIdentity to the v20261001preview ARM API, allowing customers to configure a user-assigned managed identity for worker node ACR image pulls.
ARO-24037
What changes
ARM API: ContainerRegistryProfile on PlatformProfile (v20261001preview). Day-2 mutable via PATCH. Set, change, or clear (null).
Admission: OCP >= 4.22 version gate.
Validation: Resource type check, managed resource group rejection, struct-level platform immutability preserved.
Backend: ContainerRegistryPullCredentialsPermissionValidation — checks CAPZ has assign/action on the MI. Re-validates on day-2 MI changes via InputKeyedClusterValidation.
Dispatch + CS conversion: Wired through clusterUpdateDispatchConfig for day-2 updates and clearing.
Cross-version preservation: Older API versions preserve the field through round-trips.
|
Todd Wolff (@twolff-gh): 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. |
Summary
Add
properties.platform.containerRegistry.managedIdentityto the v20261001preview ARM API, allowing customers to configure a user-assigned managed identity for worker node ACR image pulls.ARO-24037
What changes
ContainerRegistryProfileonPlatformProfile(v20261001preview). Day-2 mutable via PATCH. Set, change, or clear (null).ContainerRegistryPullCredentialsPermissionValidation— checks CAPZ has assign/action on the MI. Re-validates on day-2 MI changes viaInputKeyedClusterValidation.clusterUpdateDispatchConfigfor day-2 updates and clearing.Testing
Companion changes