feat: stop using clusers-service for frontend externalauth reads - #5110
Conversation
| // the necessary conversions for the API version of the request. | ||
| // TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos | ||
| func mergeToInternalExternalAuth(csEternalAuth *arohcpv1alpha1.ExternalAuth, internalObj *api.HCPOpenShiftClusterExternalAuth) (*api.HCPOpenShiftClusterExternalAuth, error) { | ||
| mergedExternalAuth, err := ocm.ConvertCStoExternalAuth(internalObj.ID, csEternalAuth) |
There was a problem hiding this comment.
Pending to decide if we fully remove the ocm.ConvertCStoExternalAuth function itself and the functions/methods it calls exclusively. It depends on the decision taken in #4610 (comment)
There was a problem hiding this comment.
As decided there we can remove the call so I removed the ocm.ConvertCStoExternalAuth own function. I kept a couple of other functions it was using because they convert from individual CS API attribute constants values to the corresponding api attributes constant values in the RP.
There was a problem hiding this comment.
at the end I removed them because the linter was complaining about it. The functions were convertUsernameClaimPrefixPolicyCSToRP and convertExternalAuthClientTypeCSToRP. I wanted to keep them because someone could do it wrong and just convert the string 1:1 without proper mapping and having those functions available could help / give a hint.
|
I reviewed the customer properties side I think we currently have them all in Cosmos. This should make the removal safe. I also reviewed defaulting of them and I think we should be covered too. There are some pending validations to be moved from CS to RP but that should not be a blocker for removal of the read path in frontend. Although I did a review from CS side it would be great to have a doublecheck. |
There was a problem hiding this comment.
Pull request overview
This PR removes RP Frontend read-time dependency on Clusters Service for ExternalAuth resources by serving ExternalAuth GET/LIST exclusively from Cosmos, and deletes the backend migration controller that previously backfilled missing customer properties into Cosmos.
Changes:
- Frontend ExternalAuth LIST/GET now reads directly from Cosmos and stops merging/hydrating from Clusters Service.
- Frontend update flow stops re-reading/merging ExternalAuth state from Clusters Service prior to validation and write.
- Backend ExternalAuth customer-properties migration controller (and its tests) is removed and no longer started by the backend app.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/pkg/frontend/external_auth.go | Removes Clusters Service read/merge logic from ExternalAuth GET/LIST and update paths; list now verifies parent cluster existence via Cosmos only. |
| backend/pkg/controllers/externalauthpropertiescontroller/external_auth_customer_properties_migration.go | Removes the controller that migrated missing ExternalAuth customer properties from Clusters Service into Cosmos. |
| backend/pkg/controllers/externalauthpropertiescontroller/external_auth_customer_properties_migration_test.go | Removes tests for the deleted migration controller. |
| backend/pkg/app/backend.go | Stops wiring/running the deleted ExternalAuth migration controller. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| updating := oldInternalExternalAuth != nil | ||
| if updating { | ||
| // re-write oldInternalCluster for as long as cluster-service needs to be consulted for pre-existing state. | ||
| oldInternalExternalAuth, err = f.readInternalExternalAuthFromClusterService(ctx, oldInternalExternalAuth) | ||
| if err != nil { | ||
| return utils.TrackError(err) | ||
| } | ||
| if err := checkForProvisioningStateConflict(ctx, f.dbClient, database.OperationRequestUpdate, oldInternalExternalAuth.ID, oldInternalExternalAuth.Properties.ProvisioningState); err != nil { | ||
| return utils.TrackError(err) | ||
| } | ||
|
|
||
| switch request.Method { | ||
| case http.MethodPut: | ||
| return f.updateExternalAuth(writer, request, oldInternalExternalAuth) |
| // the most recently specified casing to the client and must not | ||
| // normalize or return a toupper or tolower form of the resource | ||
| // group or resource name. The resource group name and resource | ||
| // name must come from the URL and not the request body. | ||
| if !strings.EqualFold(internalExternalAuth.ID.String(), resourceID.String()) { | ||
| return nil, fmt.Errorf("unexpected resourceID: %s", internalExternalAuth.ID.String()) | ||
| } | ||
| internalExternalAuth.ID = resourceID | ||
|
|
||
| return f.readInternalExternalAuthFromClusterService(ctx, internalExternalAuth) | ||
|
|
||
| } | ||
|
|
||
| // readInternalExternalAuthFromClusterService takes an internal ExternalAuth read from cosmos, retrieves the corresponding cluster-service data, | ||
| // merges the states together, and returns the internal representation. | ||
| func (f *Frontend) readInternalExternalAuthFromClusterService(ctx context.Context, oldInternalExternalAuth *api.HCPOpenShiftClusterExternalAuth) (*api.HCPOpenShiftClusterExternalAuth, error) { | ||
| oldClusterServiceExternalAuth, err := f.clusterServiceClient.GetExternalAuth(ctx, oldInternalExternalAuth.ServiceProviderProperties.ClusterServiceID) | ||
| if err != nil { | ||
| return nil, utils.TrackError(err) | ||
| } | ||
|
|
||
| // TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos | ||
| oldInternalExternalAuth, err = mergeToInternalExternalAuth(oldClusterServiceExternalAuth, oldInternalExternalAuth) | ||
| if err != nil { | ||
| return nil, utils.TrackError(err) | ||
| } | ||
|
|
||
| return oldInternalExternalAuth, nil | ||
| return internalExternalAuth, nil |
|
/hold until some comments and questions are resolved |
e606ccb to
0821dc0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Verify the parent cluster exists so we return 404 instead of an empty list for a non-existent cluster (Cosmos List is prefix-based) | ||
| _, err = f.dbClient.HCPClusters(subscriptionID, resourceGroupName).Get(ctx, resourceName) | ||
| if err != nil { | ||
| return utils.TrackError(err) | ||
| } |
There was a problem hiding this comment.
Is this then wrong and also in #5058 ? Should we add an inner error check here on database not found error where we convert to an arm.NewResourceNotFoundError ?
There was a problem hiding this comment.
No, if this is correct, make the middleware/filter correct
There was a problem hiding this comment.
Yes, the message is correct. Because it's a DB not found error the api error handling layer attempts to extract the resource id from the context but because the routes layer doesn't add resourceid into the context it writes it as an internal server error.
I originally considered adding the MiddlewareResourceID as suggested but it's strange to return that in the specific case when the cluster retrieval fails on a list endpoint of another type of resource: what's not found is the cluster and not the whole resource id which is something like :/subscriptions/11111111-1111-1111-1111-111111111111/resourceGroups/testResourceGroup/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/testCluster/externalAuths. If we were to add the MiddlewareResourceID to all resource list routes and for example a DB not found error occurs on the cluster GET during an external auths list and we don't do any inner conversion the returned error would be:
"error": {
"code": "ResourceNotFound",
"message": "The resource 'hcpOpenShiftClusters/externalAuths/' under resource group 'testResourceGroup' was not found.",
"target": "/subscriptions/11111111-1111-1111-1111-111111111111/resourceGroups/testResourceGroup/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/testCluster/externalAuths"
}
which doesn't really seem accurate and it has an odd/incorrect messaging.
That's why I suggested the inner error check directly on database notfound and conversion.
There was a problem hiding this comment.
After having discussed this, we've decided that although the error message and target are not accurate, we consider the fact of having the 404 and dealing at it at the middleware layer instead of within the handler itself more important. We will at some point try to improve the middleware level handling.
I added the MiddlewareResourceID in the postmuxMiddleware of the resource list endpoints. This should apply automatically to other resource list handlers.
|
/lgtm |
|
I pushed a new change that adds |
All customer provided properties are now being stored in Cosmos during external auth creation. For previously existing external auth resources in the RP we ran a a controller that persisted all potentially missing customer properties to Cosmos by retrieving the information from Clusters Service and persisting it in Cosmos. With this, we can now remove all read interaction with Clusters Service from the RP Frontend. This simplifies the read path on Frontend and it will allow us to later fully disconnect the RP Frontend from Clusters Service when we disconnect the write path.
0821dc0 to
b15d9b6
Compare
To add more detail on this, after discussion through other channels we decided to add it even when the "old" external auth we receive from cosmos contains the data with |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, 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 |
|
/hold cancel |
All customer provided properties are now being stored in Cosmos during external auth creation. For previously existing external auth resources in the RP we ran a a controller that persisted all potentially missing customer properties to Cosmos by retrieving the information from Clusters Service and persisting it in Cosmos.
With this, we can now remove all read interaction with Clusters Service from the RP Frontend. This simplifies the read path on Frontend and it will allow us to later fully disconnect the RP Frontend from Clusters Service when we disconnect the write path.