Align NodePool frontend CRUD to cluster CRUD on internal types #3460
Conversation
deads2k
requested review from
DennisJWheeler,
Nanyte25,
SudoBrendan,
avollmer-redhat,
bennerv,
geoberle,
hbhushan3,
janboll,
jharrington22,
jonathan34c,
kostola,
mbarnes,
mbukatov,
mgahagan73,
miquelsi,
mmazur,
mociarain,
oriAdler,
patriksuba,
raelga,
roivaz,
sclarkso,
stevekuznetsov,
tony-schndr,
trevorwilliams2025,
tsatam,
venkateshsredhat,
weherdh,
weinong and
whober0521
as code owners
|
Please rebase pull request. |
This brings our mutation into the same pattern of decode to internal, manipulate, validate, store, then finally convert back to external for encoding.
deads2k
force-pushed
the
cs-57-node-align
branch
from
c7ed1c7 to
a220735
Compare
| conversion.CopyReadOnlyClusterValues(newInternalCluster, oldInternalCluster) | ||
| newInternalCluster.SystemData = systemData |
There was a problem hiding this comment.
change to align to the update flow. Users shouldn't be able to set these at all.
| clusterUpdateOperation.TenantID = request.Header.Get(arm.HeaderNameHomeTenantID) | ||
| clusterUpdateOperation.ClientID = request.Header.Get(arm.HeaderNameClientObjectID) | ||
| clusterUpdateOperation.NotificationURI = request.Header.Get(arm.HeaderNameAsyncNotificationURI) |
There was a problem hiding this comment.
added for consistency. the ExposeOperation does the same work below, but this makes it read in parallel to create.
frontend/pkg/frontend/cluster.go
Outdated
| return err | ||
| } | ||
|
|
||
| // TODO is patch supposed to be status accepted? |
There was a problem hiding this comment.
Wondered about this. Nothing is failing, but should it be different?
There was a problem hiding this comment.
Yes, good catch. Response code should be 202 (Accepted).
Resource Provider Contract quote (emphasis mine):
Response
The response includes an HTTP status code, a set of response headers, and a response body.
Status Code
The resource provider must return 200 (OK) to indicate that the synchronous Patch operation completed successfully. 202 (Accepted) must be returned to indicate that the operation will complete asynchronously.
If the resource group or resource does not exist, 404 (NotFound) should be returned.
| // mergeToInternalCluster renders a CS Cluster object in JSON format, applying | ||
| // 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 mergeToInternalCluster(csCluster *arohcpv1alpha1.Cluster, internalCluster *api.HCPOpenShiftCluster) (*api.HCPOpenShiftCluster, error) { |
There was a problem hiding this comment.
next three functions are moved here and delegate to each other so they are consistent. There were two slightly different flows before.
| } | ||
|
|
||
| return f.updateHCPClusterInCosmos(ctx, writer, request, newInternalCluster, oldInternalCluster) | ||
| return f.readInternalClusterFromClusterService(ctx, internalCluster) |
There was a problem hiding this comment.
delegated for consistency (found when re-using in nodepool cluster lookup)
| logger.Error(fmt.Sprintf("%v", err), args...) // fmt used to handle nil | ||
|
|
||
| var ocmError *ocmerrors.Error | ||
| if errors.As(err, &ocmError) { |
There was a problem hiding this comment.
convenient for error handling when we're able to do this.
| } | ||
| } | ||
|
|
||
| if database.IsResponseError(err, http.StatusNotFound) { |
There was a problem hiding this comment.
convenient for error handling simplification.
| @@ -1,3 +1,4 @@ | |||
| InvalidRequestContent: properties.autoRepair: Forbidden: field is immutable | |||
There was a problem hiding this comment.
Not sure how this was missed before in decoding, but we're now honoring user intent and it fails validation as expected. Let me know if we shouldn't.
There was a problem hiding this comment.
That's correct. API spec has autoRepair as create-only.
|
/test e2e-parallel |
|
Same e2e tests fail in other runs, so probably not failure here. But this is a great indication of success otherwise! |
frontend/pkg/frontend/error.go
Outdated
| if err != nil { | ||
| arm.WriteInternalServerError(w) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Maybe don't throw an Internal Server Error here. Some endpoint paths don't form a valid resource ID (e.g. certain list endpoints). A nil resource ID is ok to pass to ocm.CSErrorToCloudError.
| if err != nil { | ||
| arm.WriteInternalServerError(w) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Same concern as above except arm.NewResourceNotFoundError does require a resource ID. I can't think of any cases where this will be a problem, so maybe it's fine until we discover it isn't.
Fixed all comments. Plan to merge on green. |
|
@deads2k: 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. |
3 participants
This brings our mutation into the same pattern of decode to internal, manipulate, validate, store, then finally convert back to external for encoding.
builds on #3379 to convert nodepools.