feat: add cluster's mi dataplane identity url to internal HCPOpenShiftCluster api type - #3838
Conversation
| type ServiceProviderPlatformProfile struct { | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| OperatorsAuthentication ServiceProviderOperatorsAuthenticationProfile `json:"operatorsAuthentication,omitempty"` |
There was a problem hiding this comment.
This adds .operatorsAuthentication.userAssignedIdentities.managedIdentitiesDataPlaneIdentityUrl to the serviceprovider properties. This nested structure imitates the corresponding levels on the customerprovider side where for example, we have .operatorsAuthentication.userAssignedIdentities.serviceManagedIdentity. This is similar to what's done for other areas like for example .customerProperties.dns.baseDomainPrefix and .serviceProviderProperties.dns.baseDomain
| type ServiceProviderPlatformProfile struct { | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| OperatorsAuthentication ServiceProviderOperatorsAuthenticationProfile `json:"operatorsAuthentication,omitempty"` |
There was a problem hiding this comment.
I considered using url.URL but it was serializing the struct when calling MarshalJSON and not the textual representation
| if cluster.Azure().OperatorsAuthentication() != nil { | ||
| if mi, ok := cluster.Azure().OperatorsAuthentication().GetManagedIdentities(); ok { | ||
| miDPURL := mi.ManagedIdentitiesDataPlaneIdentityUrl() | ||
| hcpcluster.ServiceProviderProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ManagedIdentitiesDataPlaneIdentityURL = miDPURL |
There was a problem hiding this comment.
When converting from CS to the internal HCPOpenShiftCluster data type we set the identity URL. As of now I am still unsure if this will be needed or not because at some point the clusters should store from the beginning that information. Maybe this is needed to move the previously existing ones temporarily at least.
There was a problem hiding this comment.
I'm willing to let it play out and tackle refinment when we remove cluster-service from the frontend. WIP here #3788 . We won't wait on it.
| type ServiceProviderPlatformProfile struct { | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| OperatorsAuthentication ServiceProviderOperatorsAuthenticationProfile `json:"operatorsAuthentication,omitempty"` |
There was a problem hiding this comment.
I understand that even though CS has the identity url information and we can set it when converting from CS https://github.com/Azure/ARO-HCP/pull/3838/files#r2708169679 we still want to store this in cosmosdb since the beginning when the cluster creation request is received in the rp frontend.
If that understanding is correct, I've been taking a look at the frontend code on how to set that property. I see that there's a ConvertToInternal method part of the VersionedCreatableResource[InternalAPIType any] interface that is called from the versioned cluster type to get the corresponding internal api type.
A possibility is to change the ConvertToInternal method signature to receive a http.Header. However, this has a significant impact because N versioned resource types implement that interface which means we would need to update those to receive it and at the moment only the cluster ones need it (maybe in the future more might need it?). Additionally, once that is done the functions where ConvertToInternal is called need to have the http Header available which means we would need to modify the signatures of those functions/methods to receive it. The same occurs with tests. And because this is part of that interface it impacts not only cluster types but also other resource types like nodepools, externalauth, ... . I am also unsure whether we see it as desirable to have http.Request type at that layer. Passing just the cluster identity url as a parameter to ConvertToInternal does not seem correct as it applies to other resources other than cluster.
Another possibility would be to set it after the call to ConvertInternal only for the cluster creation endpoint but that would make the conversion to internal done partially in different places.
Before going further let's discuss about how do we want to tackle this.
| type ServiceProviderPlatformProfile struct { | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| OperatorsAuthentication ServiceProviderOperatorsAuthenticationProfile `json:"operatorsAuthentication,omitempty"` |
There was a problem hiding this comment.
I added it as a serviceprovider property because that information is not returned to the end-user.
My understanding is that we shouldn't add attributes that we are not going to return to the end-user in customer properties, but I would like additional insights about this.
| type ServiceProviderPlatformProfile struct { | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| OperatorsAuthentication ServiceProviderOperatorsAuthenticationProfile `json:"operatorsAuthentication,omitempty"` |
There was a problem hiding this comment.
Figure out if when adding service provider properties we need to protect from some of them being serialized/deserialized in the api responses returned by the RP
Looking at the code my understanding is that unless the mapping is implemented in the versioned type (for example in internal/api/v20251223preview/hcpopenshiftclusters_methods.go and internal/api/v20240610preview/hcpopenshiftclusters_methods.go for clusters) it is not exposed and that the approach is that we as developers should be careful not to expose accidentally some of the ones we do not want to expose in the mapping layer.
Is this understanding accurate?
| type ServiceProviderPlatformProfile struct { | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| OperatorsAuthentication ServiceProviderOperatorsAuthenticationProfile `json:"operatorsAuthentication,omitempty"` |
There was a problem hiding this comment.
Figure out how to set the attribute so it is persisted on Cosmos
For newly created resources as long as it is defined in the internal api type and then set when instantiating it the information will be automatically stored to cosmos. For example, for clusters the internal api data type is fully serialized in cosmos in the corresponding cosmos document under the attribute .properties.internalState.internalAPI.
Is this understanding accurate?
For previously existing resources TODO figure out if some data is overwritten when received from CS.
| type ServiceProviderPlatformProfile struct { | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| OperatorsAuthentication ServiceProviderOperatorsAuthenticationProfile `json:"operatorsAuthentication,omitempty"` |
There was a problem hiding this comment.
Figure out if customer properties are automatically serialized on responses (to gain a better understanding)
Based on my understanding, it is not automatic. It depends on developers to implement the appropriate mapping, being careful to not accidentally expose information we don't want to expose.
Is this understanding accurate?
Aside from that, is the idea that we should always return in the api response all customer properties?
| type ServiceProviderPlatformProfile struct { | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| IssuerURL string `json:"issuerUrl,omitempty"` | ||
| OperatorsAuthentication ServiceProviderOperatorsAuthenticationProfile `json:"operatorsAuthentication,omitempty"` |
There was a problem hiding this comment.
Figure out how to ensure that for previously existing clusters we start storing the data
When looking at the code I don't observe any place where for previously existing resources the cluster info is received from CS and stored in backend.
Is this accurate or am I missing something?
There was a problem hiding this comment.
I don't observe any place where for previously existing resources the cluster info is received from CS and stored in backend.
That is correct. The information only goes one way. We had just merged a PR that pulled in data from cluster-service, but the data in cosmos doesn't currently match the data in cluster-service, so the migration failed.
| // ManagedIdentitiesDataPlaneIdentityURL is the Managed Identities Data Plane | ||
| // Identity URL associated with the cluster. It is the URL that will be used | ||
| // to communicate with the Managed Identities Resource Provider (MI RP) | ||
| ManagedIdentitiesDataPlaneIdentityURL string `json:"managedIdentitiesDataPlaneIdentityUrl,omitempty"` |
There was a problem hiding this comment.
How many of these do we have for a cluster? If we only have one, just move it directly onto the HCPOpenShiftClusterServiceProviderProperties. String is fine.
Is this value ever updated via the ARM API?
Please write this value in decodeDesiredClusterCreate. We'll migrate our existing data soon.
There was a problem hiding this comment.
How many of these do we have for a cluster? If we only have one, just move it directly onto the HCPOpenShiftClusterServiceProviderProperties
As of now it's this attribute. See #3838 (comment) on a reason why I decided to keep the nesting. It also conceptually belongs to the "operators authentication managed identities" concept so I think it's still useful to keep that context and makes it easier to search when looking at the information together with the customerproperties. It also has space for grow in case it is needed in the future without needing to migrate/change.
There was a problem hiding this comment.
Is this value ever updated via the ARM API?
I don't know the details on the ARM side around the header. It's one of the TODO points in the original description of the MR. I've asked about it and I am waiting for some insights around this.
There was a problem hiding this comment.
Please write this value in decodeDesiredClusterCreate. We'll migrate our existing data soon.
Changed. I added it just after the call to ConvertToInternal. I also modified the decodeDesiredClusterCreate to now receive a http.Header so we can retrieve that information.
There was a problem hiding this comment.
It's one per cluster and the nesting for operators is super deep. Move it to the top.
There was a problem hiding this comment.
Moved to the top
|
just the scoping question and then lgtm.
put that on the field comments too. That helped me more than the godoc. |
a905977 to
84ff569
Compare
Done |
| j.CosmosUID = "" | ||
| // ClusterServiceID does not roundtrip through the external type because it is purely an internal detail | ||
| j.ClusterServiceID = ocm.InternalID{} | ||
| // ManagedIdentitiesDataPlaneIdentityURL does not roundtrip through the external type because |
There was a problem hiding this comment.
Unsetting the attribute in the fuzzing tests because we set it outside of the convertToInternal function
|
|
||
| // We set the managed identities data plane identity URL associated to the cluster from the | ||
| // http header 'X-Ms-Identity-Url'. | ||
| newInternalCluster.ServiceProviderProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ManagedIdentitiesDataPlaneIdentityURL = |
There was a problem hiding this comment.
is this field required on create? If so, add a validation rule
There was a problem hiding this comment.
It is required in create cluster but we think not during cluster arm preflight.
I implemented conditional validation based on K8s Operation validation.
84ff569 to
7add1fc
Compare
| errs = append(errs, validate.ImmutableByReflect(ctx, op, fldPath.Child("platform"), &newObj.Platform, safe.Field(oldObj, toServiceProviderPlatform))...) | ||
| errs = append(errs, validateServiceProviderPlatformProfile(ctx, op, fldPath.Child("platform"), &newObj.Platform, safe.Field(oldObj, toServiceProviderPlatform))...) | ||
|
|
||
| // ManagedIdentitiesDataPlaneIdentityURL string `json:"managedIdentitiesDataPlaneIdentityUrl,omitempty"` |
There was a problem hiding this comment.
we have existing data that won't have this. so if oldObj exists and this content does not, then the result cannot be required.
There was a problem hiding this comment.
if oldObj != nil && oldObj.Field != nil{
// required
}
There was a problem hiding this comment.
Changed
There was a problem hiding this comment.
I changed it, but, is the extra check needed?
Even if internally the data is not migrated yet, aren't we getting the old value from CS (which is always required), as this MR added that too? I am trying to think a case where it could happen that old comes without a value but I can't think of any as of now.
There was a problem hiding this comment.
Validation is calledon the decoded data, not the data from cluster-service. Validation is (intentionally) not impacted by clusterservice.
| const ( | ||
| // ManagedIdentitiesDataPlaneIdentityURLNotRequiredOperationOption is an operation option that indicates that the managed identities | ||
| // data plane identity URL is not required during validation. This is used on Preflight requests on the Cluster resource. | ||
| ManagedIdentitiesDataPlaneIdentityURLNotRequiredOperationOption = "ManagedIdentitiesDataPlaneIdentityURLNotRequired" |
There was a problem hiding this comment.
no negation in variable names. Is this ManagedIdentitiesDataPlaneIdentityURLOptional
There was a problem hiding this comment.
Changed
c0cb0da to
144b2cc
Compare
| req.Raw().Header.Set(arm.HeaderNameHomeTenantID, api.TestTenantID) | ||
|
|
||
| // Only set X-Ms-Identity-Url header for cluster create/update requests | ||
| if d.isClusterCreateOrUpdateRequest(req) { |
There was a problem hiding this comment.
For the integration tests we need to simulate the HTTP header that would be passed by ARM in AME based environments.
There was a problem hiding this comment.
is it only sometimes in arm or all the time?
There was a problem hiding this comment.
Only sometimes.
In non AME environments the ARM mechanism does not exist so we need to send the http header ourselves.
In AME environments, ARM sets the header but not for all resource types nor actions.
There was a problem hiding this comment.
In AME environments, ARM sets the header but not for all resource types nor actions.
add to the comment in code.
There was a problem hiding this comment.
introduced additional doc
144b2cc to
7699c1d
Compare
| if oldObj == nil || oldObj.ManagedIdentitiesDataPlaneIdentityURL != "" { | ||
| if !op.HasOption(ManagedIdentitiesDataPlaneIdentityURLOptionalOperationOption) { | ||
| errs = append(errs, validate.RequiredValue(ctx, op, fldPath.Child("managedIdentitiesDataPlaneIdentityUrl"), &newObj.ManagedIdentitiesDataPlaneIdentityURL, nil)...) | ||
| errs = append(errs, URL(ctx, op, fldPath.Child("managedIdentitiesDataPlaneIdentityUrl"), &newObj.ManagedIdentitiesDataPlaneIdentityURL, nil)...) |
There was a problem hiding this comment.
this is unconditional, with empty treated as ok.
There was a problem hiding this comment.
Move outside the if block.
There was a problem hiding this comment.
Moved outside both ifs. It is always executed now
| // the Managed Identities Dataplane service is not available the http header | ||
| // is set to a dummy value by our tools/testsuites/developers when | ||
| // creating ARO-HCP Clusters | ||
| ManagedIdentitiesDataPlaneIdentityURL string `json:"managedIdentitiesDataPlaneIdentityUrl,omitempty"` |
There was a problem hiding this comment.
| ManagedIdentitiesDataPlaneIdentityURL string `json:"managedIdentitiesDataPlaneIdentityUrl,omitempty"` | |
| ManagedIdentitiesDataPlaneIdentityURL string `json:"managedIdentitiesDataPlaneIdentityURL,omitempty"` |
There was a problem hiding this comment.
changed
David Eads (deads2k)
left a comment
There was a problem hiding this comment.
minor comments, lgtm otherwise.
|
/retest |
0687c2f to
1e9d8d4
Compare
|
/lgtm |
|
/hold cancel |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/remove-lgtm This PR has conflicts. |
fc6aeff to
86018e9
Compare
|
/retest |
|
/lgtm |
86018e9 to
cac1303
Compare
|
Miguel Soriano (@miguelsorianod): 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. |
…tCluster api type
cac1303 to
78f5869
Compare
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, geoberle, 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 |
To be able to interact with Microsoft's Managed Identities Dataplane (MI DP) service, we need to use the cluster-level identity url endpoint.
When an ARO-HCP Cluster is created, ARM sends a HTTP header
X-Ms-Identity-Urlthat contains the cluster's identity url. For ARO-HCP environments where the Managed Identities Dataplane service is not available the http header is set to a dummy value by our tools/testsuites/developers when creating ARO-HCP Clusters.We are going to start interacting with the Managed Identities Dataplane service in the backend RP. This means we need to be able to access the identity URL from it. As of now the cluster's identity URL is only sent to CS but it is not persisted in Cosmos nor accessible from the internal HCPOpenShiftCluster in backend logic. In #3820 you can see an example of code we want to introduce that interacts with the MI DP where we need the cluster's identity url but it is not available.
This PR introduces it as a serviceprovider property in the HCPOpenShiftCluster internal api type.
TODO: