unrevert of read from cosmos - #4610
Conversation
|
David Eads (@deads2k) The way ARO-HCP/internal/ocm/convert.go Line 311 in 428d5ac This ARO-HCP/internal/ocm/convert.go Line 318 in 428d5ac be something like
We also need to ensure that the migration identity controller can handle the case of only some values being partially set i.e NeedsWork re-written in a way that it says work is needed if any of the identity elements have a nil clientId or principalId |
428d5ac to
978e85c
Compare
| } | ||
|
|
||
| // Clear the user-assigned identities map since that is built by a controller. The defaults will be set next and valid until we know the actual values. | ||
| newInternalCluster.Identity.UserAssignedIdentities = nil |
There was a problem hiding this comment.
Behavior question I don't know the answer to. Do we need to validate the user provided .Identity.UserAssignedIdentities for some reason for ARM even though our long-held authoritative source is actually in .CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators (etc).
If we need to validate that every entry the user provided is present, we need to remove the defaulting I added in this PR and add the information only during reads.
There was a problem hiding this comment.
Ben Vesel (@bennerv) Matthew Barnes (@mbarnes) will know more;
I believe we could also get away by not overwriting the values and the EnsureDefaults() method.
By that I mean persist whatever we received from the user for the .Identity property as ARM would've validate that prior to the frontend processing the request. We remove the EnsureDefaults().
Then in the controller we sync the properties for the .Identity only if the clientId or principalId are empty for a given entry. When syncing the properties, if CS hasn't set it i.e ok = false then we make sure to live that as nil so that it'll be omitted from the returned json.
There was a problem hiding this comment.
I'm not sure I understand the scope of your question wrt to validation so forgive me if I overanswer.
The .Identity.UserAssignedIdentities part of the API is the means by which user-assigned managed identities actually get assigned to the cluster resource by ARM.
I believe ARM does some validation here before the request reaches the RP, insofar as it will catch references to identities that don't exist. That would be worth verifying though.
The ARM requirement that makes this section tricky is that we have to provide the client and principal ID for each key in .Identity.UserAssignedIdentities whenever we return the cluster resource in a response (aside from the initial PUT response). Those values must be obtained from the Managed Identity Azure service.
Currently Cluster Service obtains those values and supplies them to the RP, which means that in order to satisfy the ARM requirement there has to be a strict 1:1 mapping of keys in the .Identity.UserAssignedIdentities map to a key in either the ...OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators map or to the ...OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity field.
Once the backend is obtaining the client and principal IDs itself, we could relax this constraint slightly by tolerating extraneous keys in .Identity.UserAssignedIdentities that don't appear under ...OperatorsAuthentication.UserAssignedIdentities. Vice versa, however, won't work. If a key in ...OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators does not appear in .Identity.UserAssignedIdenties, then that would mean the identity is not actually assigned to the cluster resource and won't be any good to the control plane operator.
In terms of the content of the .Identity.UserAssignedIdentities map, the value for each key must be an empty JSON object or -- in the case of a cluster update -- I believe it can be a JSON object that matches exactly what would be returned in a GET request (i.e. a JSON object with clientId and principalId keys and values that match what Cluster Service provides).
There was a problem hiding this comment.
Ok, I think the summary of all that is, "we must validate the user input as provided. we must return frontend values with all the keys and either accurate or empty values." Doable. Will update.
| for _, operatorIdentityResourceID := range cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators { | ||
| if cluster.Identity == nil { | ||
| cluster.Identity = &arm.ManagedServiceIdentity{} | ||
| } | ||
| if cluster.Identity.UserAssignedIdentities == nil { | ||
| cluster.Identity.UserAssignedIdentities = make(map[string]*arm.UserAssignedIdentity) | ||
| } | ||
|
|
||
| if _, ok := cluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()]; !ok { | ||
| cluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()] = &arm.UserAssignedIdentity{} | ||
| } | ||
| } |
There was a problem hiding this comment.
It's not needed for data plane; only ControlPlane + SMI identities should be in the cluster.Identity. S
Something like
ARO-HCP/demo/bicep/cluster.bicep
Line 574 in 978e85c
For DP we don't request credentials directly from the MI RP but rather perform federations on them so they don't need to be in this list. However for the CP + SMI, do interact with the MI RP and hence the need for the .Identity stanza which makes ARM give the FPA credential ability to retrieve credentials for these identities from the MI RP
| return true | ||
| } | ||
| } | ||
| for _, operatorIdentityResourceID := range existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators { |
There was a problem hiding this comment.
Same as #4610 (comment) we don't need to do this for DP identities
| clientID, _ := operatorIdentity.GetClientID() | ||
| principalID, _ := operatorIdentity.GetPrincipalID() | ||
| internalCluster.Identity.UserAssignedIdentities[operatorIdentity.ResourceID()] = &arm.UserAssignedIdentity{ | ||
| ret[operatorIdentity.ResourceID()] = &arm.UserAssignedIdentity{ | ||
| ClientID: &clientID, | ||
| PrincipalID: &principalID, | ||
| } |
There was a problem hiding this comment.
This should be written as so that we don't set the value to "" and potentially returning it to the end user when CS hasn't set it yet.
uai := &arm.UserAssignedIdentity{}
if clientID, ok := operatorIdentity.GetClientID(); ok {
uai.ClientID = &clientID
}
if principalID, ok := operatorIdentity.GetPrincipalID(); ok {
uai.PrincipalID = &principalID
}
ret[operatorIdentity.ResourceID()] = uaiCS only sets this value async (as part of the cluster provisioning process and not during cluster creation).
This will avoid putting us in a situation where we return something like this
..
identity: {
"..../kms": {"clientID": "", "principalID": ""}
}back to the end user when they do a GET and once they attempt a PUT using the gotten response it'll fail.
| clientID, _ := mi.ServiceManagedIdentity().GetClientID() | ||
| principalID, _ := mi.ServiceManagedIdentity().GetPrincipalID() | ||
| internalCluster.Identity.UserAssignedIdentities[mi.ServiceManagedIdentity().ResourceID()] = &arm.UserAssignedIdentity{ | ||
| ret[mi.ServiceManagedIdentity().ResourceID()] = &arm.UserAssignedIdentity{ | ||
| ClientID: &clientID, | ||
| PrincipalID: &principalID, | ||
| } |
There was a problem hiding this comment.
978e85c to
9e05098
Compare
9e05098 to
7f7d6e5
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/hold |
Manyanda Chitimbo (machi1990)
left a comment
There was a problem hiding this comment.
Took another round of reviews.
Just 1 one comment and bumping 2 old related comments that I think will be critical to address
|
|
||
| controlPlaneExists := false | ||
| for _, resourceID := range existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators { | ||
| if resourceID != nil && resourceID.String() == operatorIdentityResourceIDString { |
There was a problem hiding this comment.
| if resourceID != nil && resourceID.String() == operatorIdentityResourceIDString { | |
| if resourceID.String() == operatorIdentityResourceIDString { |
is the nil check needed?
| clientID, _ := operatorIdentity.GetClientID() | ||
| principalID, _ := operatorIdentity.GetPrincipalID() | ||
| internalCluster.Identity.UserAssignedIdentities[operatorIdentity.ResourceID()] = &arm.UserAssignedIdentity{ | ||
| ret[operatorIdentity.ResourceID()] = &arm.UserAssignedIdentity{ | ||
| ClientID: &clientID, | ||
| PrincipalID: &principalID, | ||
| } |
| clientID, _ := mi.ServiceManagedIdentity().GetClientID() | ||
| principalID, _ := mi.ServiceManagedIdentity().GetPrincipalID() | ||
| internalCluster.Identity.UserAssignedIdentities[mi.ServiceManagedIdentity().ResourceID()] = &arm.UserAssignedIdentity{ | ||
| ret[mi.ServiceManagedIdentity().ResourceID()] = &arm.UserAssignedIdentity{ | ||
| ClientID: &clientID, | ||
| PrincipalID: &principalID, | ||
| } |
There was a problem hiding this comment.
Manyanda Chitimbo (machi1990)
left a comment
There was a problem hiding this comment.
/lgtm
|
/retest |
|
/hold cancel |
53ffef9 to
3d07b56
Compare
| ServiceProviderProperties: api.HCPOpenShiftClusterServiceProviderProperties{ | ||
| DNS: api.ServiceProviderDNSProfile{ | ||
| BaseDomain: cluster.DNS().BaseDomain(), | ||
| }, | ||
| Console: api.ServiceProviderConsoleProfile{ | ||
| URL: cluster.Console().URL(), | ||
| }, | ||
| API: api.ServiceProviderAPIProfile{ | ||
| URL: cluster.API().URL(), | ||
| }, | ||
| Platform: api.ServiceProviderPlatformProfile{ | ||
| IssuerURL: cluster.Azure().OidcIssuerUrl(), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Looks like this removal is problematic
{fail [github.com/Azure/ARO-HCP/test/e2e/cluster_authorized_cidrs_connectivity.go:145]: cluster Properties.API.URL was nil
Expected
<*string | 0x0>: nil
not to be nil fail [github.com/Azure/ARO-HCP/test/e2e/cluster_authorized_cidrs_connectivity.go:145]: cluster Properties.API.URL was nil
Expected
<*string | 0x0>: nil
not to be nil}
There was a problem hiding this comment.
it passed on a retest. Looks like it was just a flake
… mutation Rather than clearing entirely, this change has the frontend create a valid default. We also default on reading from storage so the return value is always valid for the RP.
If the strings aren't longer than zero, they aren't valid and confuse clients.
7f85975 to
83ab41e
Compare
There was a problem hiding this comment.
Pull request overview
This PR shifts cluster reads/responses further toward Cosmos-only state (removing several legacy Cluster Service → RP conversion/merge paths) and updates integration fixtures accordingly, while adding logic to “complete” identity.userAssignedIdentities based on operator identity references.
Changes:
- Remove legacy Cluster Service → RP cluster conversion and related tests/controllers; reduce frontend reliance on Cluster Service for reads/listing.
- Introduce frontend identity “completion” to reconcile/prune
identity.userAssignedIdentitiesfrom operator identity references. - Update integration test artifacts/fixtures to reflect the new identity payload shape and Cosmos document shape.
Reviewed changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/pkg/frontend/cluster.go |
Removes CS merge on list/read; adds completeClusterIdentity and calls it during create/update/read. |
internal/ocm/convert.go |
Deletes legacy CS→RP conversion helpers; adds GetClusterServiceUserAssignedIdentities helper. |
internal/ocm/convert_test.go |
Removes tests tied to deleted legacy conversion path. |
internal/database/convert_defaults_consistency_test.go |
Removes defaults-consistency test that depended on legacy CS→RP conversion. |
internal/database/convert_cluster.go |
Adjusts Cosmos identity conversion to deep-copy values when present. |
backend/pkg/controllers/clusterpropertiescontroller/identity_migration.go |
Updates identity migration to use GetClusterServiceUserAssignedIdentities; expands NeedsWork logic. |
backend/pkg/controllers/clusterpropertiescontroller/identity_migration_test.go |
Updates identity migration tests for operator identity references. |
backend/pkg/controllers/clusterpropertiescontroller/cluster_customer_properties_migration*.go |
Removes the customer properties migration controller and its tests. |
backend/pkg/app/backend.go |
Stops registering/running the removed customer properties migration controller. |
test-integration/frontend/artifacts/** |
Updates fixtures for Cosmos cluster docs, identity payload shape, and expected validation errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TODO this is bad, see above TODOs. We want to validate what we store. | ||
| newInternalCluster.Identity.UserAssignedIdentities = nil | ||
| // we must validate using user provided .Identity.UserAssignedIdentities because that is the intent expressed by the user to allow | ||
| // us to use these identities. The information contained in those key is not trusted to be accurate, so we clear this field and set to |
There was a problem hiding this comment.
Could you expand on this? I thought the end-user needs to provide both .Identity.UserAssignedIdentities (as it's a requirement by ARM) as well as the .platform.operatorsAuthentication.* data and we would just limit what we do to validate all the expected data is there and consistent between them instead of modifying .Identity with something different than they have provided.
There was a problem hiding this comment.
We validate the identity in the call above. We clear the input before storage and set it to an empty valid value.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 23 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| return utils.TrackError(err) | ||
| } | ||
| clustersByClusterServiceID := make(map[string]*api.HCPOpenShiftCluster) | ||
| for _, internalCluster := range internalClusterIterator.Items(ctx) { | ||
| if internalCluster.ServiceProviderProperties.ClusterServiceID == nil { | ||
| // TODO this will be removed during our switch to read only from cosmos. | ||
| // we can still merge now since the value will never be nil until both the read path is fixed and this PR makes it to prod. | ||
| continue | ||
| resultingExternalCluster := versionedInterface.NewHCPOpenShiftCluster(internalCluster) | ||
| jsonBytes, err := arm.MarshalJSON(resultingExternalCluster) | ||
| if err != nil { | ||
| return utils.TrackError(err) |
| if val, ok := cluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()]; !ok || val == nil { | ||
| if existingValue, hasExisting := existingUserAssignedIdentity[operatorIdentityResourceID.String()]; hasExisting { | ||
| cluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()] = existingValue.DeepCopy() | ||
| } else { | ||
| cluster.Identity.UserAssignedIdentities[operatorIdentityResourceID.String()] = &arm.UserAssignedIdentity{} | ||
| } |
| if val, ok := cluster.Identity.UserAssignedIdentities[serviceManagedIdentity.String()]; !ok || val == nil { | ||
| if existingValue, hasExisting := existingUserAssignedIdentity[serviceManagedIdentity.String()]; hasExisting { | ||
| cluster.Identity.UserAssignedIdentities[serviceManagedIdentity.String()] = existingValue.DeepCopy() | ||
| } else { | ||
| cluster.Identity.UserAssignedIdentities[serviceManagedIdentity.String()] = &arm.UserAssignedIdentity{} | ||
| } |
| if serviceManagedIdentity := existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity; serviceManagedIdentity != nil { | ||
| expectedIdentityResourceIDs[serviceManagedIdentity.String()] = struct{}{} | ||
| } |
| "message": "Invalid value: \"/subscriptions/different-sub/resourceGroups/some-resource-group/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/test-subnet\": must not be the same resource group name: \"some-resource-group\"", | ||
| "target": "properties.platform.subnetId" | ||
| } | ||
| { | ||
| "code": "InvalidRequestContent", |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, machi1990, miguelsorianod 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 |
|
David Eads (@deads2k): The following tests 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. |
|
/retest-required |
/hold
need to add fixes from #4412