diff --git a/frontend/pkg/frontend/cluster.go b/frontend/pkg/frontend/cluster.go index 105479fc94..5203ad28f4 100644 --- a/frontend/pkg/frontend/cluster.go +++ b/frontend/pkg/frontend/cluster.go @@ -18,13 +18,17 @@ import ( "context" "encoding/json" "fmt" + "maps" "net/http" "strings" "k8s.io/utils/ptr" + azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" "github.com/Azure/azure-sdk-for-go/sdk/data/azcosmos" + arohcpv1alpha1 "github.com/openshift-online/ocm-sdk-go/arohcp/v1alpha1" + "github.com/Azure/ARO-HCP/internal/api" "github.com/Azure/ARO-HCP/internal/api/arm" "github.com/Azure/ARO-HCP/internal/conversion" @@ -48,7 +52,7 @@ func (f *Frontend) GetHCPCluster(writer http.ResponseWriter, request *http.Reque return err } - resultingInternalCluster, err := f.GetInternalClusterFromStorage(ctx, resourceID) + resultingInternalCluster, err := f.getInternalClusterFromStorage(ctx, resourceID) if err != nil { return err } @@ -128,11 +132,9 @@ func (f *Frontend) ArmResourceListClusters(writer http.ResponseWriter, request * pagedResponse.AddValue(jsonBytes) } } - err = csIterator.GetError() - // Check for iteration error. - if err != nil { - return ocm.CSErrorToCloudError(err, nil, writer.Header()) + if err := csIterator.GetError(); err != nil { + return err } _, err = arm.WriteJSONResponse(writer, http.StatusOK, pagedResponse) @@ -173,7 +175,7 @@ func (f *Frontend) CreateOrUpdateHCPCluster(writer http.ResponseWriter, request updating := oldInternalCluster != nil if updating { // re-write oldInternalCluster for as long as cluster-service needs to be consulted for pre-existing state. - oldInternalCluster, err = f.readInternalClusterFromClusterService(ctx, oldInternalCluster, writer.Header()) + oldInternalCluster, err = f.readInternalClusterFromClusterService(ctx, oldInternalCluster) if err != nil { return err } @@ -310,12 +312,12 @@ func (f *Frontend) createHCPCluster(writer http.ResponseWriter, request *http.Re clusterCreateOperation.TenantID = request.Header.Get(arm.HeaderNameHomeTenantID) clusterCreateOperation.ClientID = request.Header.Get(arm.HeaderNameClientObjectID) clusterCreateOperation.NotificationURI = request.Header.Get(arm.HeaderNameAsyncNotificationURI) - operationCosmosID := transaction.CreateOperationDoc(clusterCreateOperation, nil) + operationCosmosUID := transaction.CreateOperationDoc(clusterCreateOperation, nil) transaction.OnSuccess(addOperationResponseHeaders(writer, request, clusterCreateOperation.NotificationURI, clusterCreateOperation.OperationID)) // set fields that were not known until the operation doc instance was created. // TODO once we we have separate creation/validation of operation documents, this can be done ahead of time. - newInternalCluster.ServiceProviderProperties.ActiveOperationID = operationCosmosID + newInternalCluster.ServiceProviderProperties.ActiveOperationID = operationCosmosUID newInternalCluster.ServiceProviderProperties.ProvisioningState = clusterCreateOperation.Status cosmosUID, err := f.dbClient.HCPClusters(newInternalCluster.ID.SubscriptionID, newInternalCluster.ID.ResourceGroupName).AddCreateToTransaction(ctx, transaction, newInternalCluster, nil) @@ -357,44 +359,6 @@ func (f *Frontend) createHCPCluster(writer http.ResponseWriter, request *http.Re return nil } -// readInternalClusterFromClusterService takes an internal Cluster read from cosmos, retrieves the corresponding cluster-service data, -// merges the states together, and returns the internal representation. -// TODO remove the header it takes and collapse that to some general error handling. -func (f *Frontend) readInternalClusterFromClusterService(ctx context.Context, oldInternalCluster *api.HCPOpenShiftCluster, header http.Header) (*api.HCPOpenShiftCluster, error) { - logger := LoggerFromContext(ctx) - - oldClusterServiceCluster, err := f.clusterServiceClient.GetCluster(ctx, oldInternalCluster.ServiceProviderProperties.ClusterServiceID) - if err != nil { - return nil, ocm.CSErrorToCloudError(err, oldInternalCluster.ID, header) - } - - mergedOldClusterServiceCluster, err := ocm.ConvertCStoHCPOpenShiftCluster(oldInternalCluster.ID, oldClusterServiceCluster) - if err != nil { - logger.Error(err.Error()) - return nil, arm.NewInternalServerError() - } - - // Do not set the TrackedResource.Tags field here. We need - // the Tags map to remain nil so we can see if the request - // body included a new set of resource tags. - - mergedOldClusterServiceCluster.SystemData = oldInternalCluster.SystemData - mergedOldClusterServiceCluster.ServiceProviderProperties.ProvisioningState = oldInternalCluster.ServiceProviderProperties.ProvisioningState - mergedOldClusterServiceCluster.ServiceProviderProperties.ActiveOperationID = oldInternalCluster.ServiceProviderProperties.ActiveOperationID - mergedOldClusterServiceCluster.ServiceProviderProperties.ClusterServiceID = oldInternalCluster.ServiceProviderProperties.ClusterServiceID - mergedOldClusterServiceCluster.ServiceProviderProperties.CosmosUID = oldInternalCluster.ServiceProviderProperties.CosmosUID - if mergedOldClusterServiceCluster.Identity == nil { - mergedOldClusterServiceCluster.Identity = &arm.ManagedServiceIdentity{} - } - if oldInternalCluster.Identity != nil { - mergedOldClusterServiceCluster.Identity.PrincipalID = oldInternalCluster.Identity.PrincipalID - mergedOldClusterServiceCluster.Identity.TenantID = oldInternalCluster.Identity.TenantID - mergedOldClusterServiceCluster.Identity.Type = oldInternalCluster.Identity.Type - } - - return mergedOldClusterServiceCluster, nil -} - func decodeDesiredClusterReplace(ctx context.Context, oldInternalCluster *api.HCPOpenShiftCluster) (*api.HCPOpenShiftCluster, error) { versionedInterface, err := VersionFromContext(ctx) if err != nil { @@ -472,10 +436,63 @@ func (f *Frontend) updateHCPCluster(writer http.ResponseWriter, request *http.Re return err } - return f.updateHCPClusterInCosmos(ctx, writer, request, newInternalCluster, oldInternalCluster) + return f.updateHCPClusterInCosmos(ctx, writer, request, http.StatusOK, newInternalCluster, oldInternalCluster) +} + +func decodeDesiredClusterPatch(ctx context.Context, oldInternalCluster *api.HCPOpenShiftCluster) (*api.HCPOpenShiftCluster, error) { + versionedInterface, err := VersionFromContext(ctx) + if err != nil { + return nil, err + } + body, err := BodyFromContext(ctx) + if err != nil { + return nil, err + } + systemData, err := SystemDataFromContext(ctx) + if err != nil { + return nil, err + } + + // TODO find a way to represent the desired change without starting from internal state here (very confusing) + // TODO we appear to lack a test, but this seems to take an original, apply the patch and unmarshal the result, meaning the above patch step is just incorrect. + var newExternalCluster = versionedInterface.NewHCPOpenShiftCluster(oldInternalCluster) + if err := api.ApplyRequestBody(http.MethodPatch, body, newExternalCluster); err != nil { + return nil, err + } + newInternalCluster := &api.HCPOpenShiftCluster{} + newExternalCluster.Normalize(newInternalCluster) + + // ServiceProviderProperties contains two types of information + // 1. values that a user cannot change because the external type does not expose the information. + // We must overwrite those values with the oldInternalCluster values so the values don't change, because the user's input will always be empty. + // 2. values that a user cannot change due to validation requirements, but the user *can* specify the values. + // We are overwriting these values that we consider to be status values. + // We do this because if a user has read a value, then modified it, then replaces it, we don't want to produce + // validation errors on status fields that the user isn't trying to modify. + conversion.CopyReadOnlyClusterValues(newInternalCluster, oldInternalCluster) + newInternalCluster.SystemData = systemData + // Clear the user-assigned identities map since that is reconstructed from Cluster Service data. + // TODO we'd like to have the instance complete when we go to validate it. Right now validation fails if we clear this. + // TODO we probably update validation to require this field is cleared. + //newInternalCluster.Identity.UserAssignedIdentities = nil + + return newInternalCluster, nil } -func (f *Frontend) updateHCPClusterInCosmos(ctx context.Context, writer http.ResponseWriter, request *http.Request, newInternalCluster, oldInternalCluster *api.HCPOpenShiftCluster) error { +func (f *Frontend) patchHCPCluster(writer http.ResponseWriter, request *http.Request, oldInternalCluster *api.HCPOpenShiftCluster) error { + // PATCH requests overlay the request body onto a resource struct + // that represents an existing resource to be updated. + ctx := request.Context() + + newInternalCluster, err := decodeDesiredClusterPatch(ctx, oldInternalCluster) + if err != nil { + return err + } + + return f.updateHCPClusterInCosmos(ctx, writer, request, http.StatusAccepted, newInternalCluster, oldInternalCluster) +} + +func (f *Frontend) updateHCPClusterInCosmos(ctx context.Context, writer http.ResponseWriter, request *http.Request, httpStatusCode int, newInternalCluster, oldInternalCluster *api.HCPOpenShiftCluster) error { logger := LoggerFromContext(ctx) versionedInterface, err := VersionFromContext(ctx) @@ -509,15 +526,18 @@ func (f *Frontend) updateHCPClusterInCosmos(ctx context.Context, writer http.Res pk := database.NewPartitionKey(oldInternalCluster.ID.SubscriptionID) transaction := f.dbClient.NewTransaction(pk) - operationDoc := database.NewOperationDocument(database.OperationRequestUpdate, oldInternalCluster.ID, oldInternalCluster.ServiceProviderProperties.ClusterServiceID, correlationData) - operationID := transaction.CreateOperationDoc(operationDoc, nil) + clusterUpdateOperation := database.NewOperationDocument(database.OperationRequestUpdate, oldInternalCluster.ID, oldInternalCluster.ServiceProviderProperties.ClusterServiceID, correlationData) + clusterUpdateOperation.TenantID = request.Header.Get(arm.HeaderNameHomeTenantID) + clusterUpdateOperation.ClientID = request.Header.Get(arm.HeaderNameClientObjectID) + clusterUpdateOperation.NotificationURI = request.Header.Get(arm.HeaderNameAsyncNotificationURI) + operationCosmosUID := transaction.CreateOperationDoc(clusterUpdateOperation, nil) - f.ExposeOperation(writer, request, operationID, transaction) + f.ExposeOperation(writer, request, operationCosmosUID, transaction) var patchOperations database.ResourceDocumentPatchOperations - patchOperations.SetActiveOperationID(&operationID) - patchOperations.SetProvisioningState(operationDoc.Status) + patchOperations.SetActiveOperationID(&operationCosmosUID) + patchOperations.SetProvisioningState(clusterUpdateOperation.Status) // Record the latest system data values form ARM, if present. patchOperations.SetSystemData(newInternalCluster.SystemData) @@ -563,62 +583,66 @@ func (f *Frontend) updateHCPClusterInCosmos(ctx context.Context, writer http.Res return err } - _, err = arm.WriteJSONResponse(writer, http.StatusOK, responseBytes) + _, err = arm.WriteJSONResponse(writer, httpStatusCode, responseBytes) if err != nil { return err } return nil } -func decodeDesiredClusterPatch(ctx context.Context, oldInternalCluster *api.HCPOpenShiftCluster) (*api.HCPOpenShiftCluster, error) { - versionedInterface, err := VersionFromContext(ctx) +// 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) { + clusterServiceBasedInternalCluster, err := ocm.ConvertCStoHCPOpenShiftCluster(internalCluster.ID, csCluster) if err != nil { return nil, err } - body, err := BodyFromContext(ctx) - if err != nil { - return nil, err + + clusterServiceBasedInternalCluster.SystemData = internalCluster.SystemData + clusterServiceBasedInternalCluster.Tags = maps.Clone(internalCluster.Tags) + clusterServiceBasedInternalCluster.ServiceProviderProperties.ProvisioningState = internalCluster.ServiceProviderProperties.ProvisioningState + clusterServiceBasedInternalCluster.ServiceProviderProperties.ActiveOperationID = internalCluster.ServiceProviderProperties.ActiveOperationID + clusterServiceBasedInternalCluster.ServiceProviderProperties.ClusterServiceID = internalCluster.ServiceProviderProperties.ClusterServiceID + clusterServiceBasedInternalCluster.ServiceProviderProperties.CosmosUID = internalCluster.ServiceProviderProperties.CosmosUID + if clusterServiceBasedInternalCluster.Identity == nil { + clusterServiceBasedInternalCluster.Identity = &arm.ManagedServiceIdentity{} } - systemData, err := SystemDataFromContext(ctx) + + if internalCluster.Identity != nil { + clusterServiceBasedInternalCluster.Identity.PrincipalID = internalCluster.Identity.PrincipalID + clusterServiceBasedInternalCluster.Identity.TenantID = internalCluster.Identity.TenantID + clusterServiceBasedInternalCluster.Identity.Type = internalCluster.Identity.Type + } + + return clusterServiceBasedInternalCluster, nil +} + +// readInternalClusterFromClusterService takes an internal Cluster read from cosmos, retrieves the corresponding cluster-service data, +// merges the states together, and returns the internal representation. +// TODO remove the header it takes and collapse that to some general error handling. +func (f *Frontend) readInternalClusterFromClusterService(ctx context.Context, oldInternalCluster *api.HCPOpenShiftCluster) (*api.HCPOpenShiftCluster, error) { + oldClusterServiceCluster, err := f.clusterServiceClient.GetCluster(ctx, oldInternalCluster.ServiceProviderProperties.ClusterServiceID) if err != nil { return nil, err } - // TODO find a way to represent the desired change without starting from internal state here (very confusing) - // TODO we appear to lack a test, but this seems to take an original, apply the patch and unmarshal the result, meaning the above patch step is just incorrect. - var newExternalCluster = versionedInterface.NewHCPOpenShiftCluster(oldInternalCluster) - if err := api.ApplyRequestBody(http.MethodPatch, body, newExternalCluster); err != nil { + // TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos + oldInternalCluster, err = mergeToInternalCluster(oldClusterServiceCluster, oldInternalCluster) + if err != nil { return nil, err } - newInternalCluster := &api.HCPOpenShiftCluster{} - newExternalCluster.Normalize(newInternalCluster) - - // ServiceProviderProperties contains two types of information - // 1. values that a user cannot change because the external type does not expose the information. - // We must overwrite those values with the oldInternalCluster values so the values don't change, because the user's input will always be empty. - // 2. values that a user cannot change due to validation requirements, but the user *can* specify the values. - // We are overwriting these values that we consider to be status values. - // We do this because if a user has read a value, then modified it, then replaces it, we don't want to produce - // validation errors on status fields that the user isn't trying to modify. - newInternalCluster.SystemData = systemData - newInternalCluster.ServiceProviderProperties = oldInternalCluster.ServiceProviderProperties - // Clear the user-assigned identities map since that is reconstructed from Cluster Service data. - // TODO we'd like to have the instance complete when we go to validate it. Right now validation fails if we clear this. - // TODO we probably update validation to require this field is cleared. - //newInternalCluster.Identity.UserAssignedIdentities = nil - return newInternalCluster, nil + return oldInternalCluster, nil } -func (f *Frontend) patchHCPCluster(writer http.ResponseWriter, request *http.Request, oldInternalCluster *api.HCPOpenShiftCluster) error { - // PATCH requests overlay the request body onto a resource struct - // that represents an existing resource to be updated. - ctx := request.Context() - - newInternalCluster, err := decodeDesiredClusterPatch(ctx, oldInternalCluster) +func (f *Frontend) getInternalClusterFromStorage(ctx context.Context, resourceID *azcorearm.ResourceID) (*api.HCPOpenShiftCluster, error) { + internalCluster, err := f.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).Get(ctx, resourceID.Name) + if database.IsResponseError(err, http.StatusNotFound) { + return nil, arm.NewResourceNotFoundError(resourceID) + } if err != nil { - return err + return nil, err } - - return f.updateHCPClusterInCosmos(ctx, writer, request, newInternalCluster, oldInternalCluster) + return f.readInternalClusterFromClusterService(ctx, internalCluster) } diff --git a/frontend/pkg/frontend/error.go b/frontend/pkg/frontend/error.go index d5ec8149cd..993de5bbfc 100644 --- a/frontend/pkg/frontend/error.go +++ b/frontend/pkg/frontend/error.go @@ -20,7 +20,11 @@ import ( "fmt" "net/http" + ocmerrors "github.com/openshift-online/ocm-sdk-go/errors" + "github.com/Azure/ARO-HCP/internal/api/arm" + "github.com/Azure/ARO-HCP/internal/database" + "github.com/Azure/ARO-HCP/internal/ocm" ) // erroringHTTPHandler is an http handler that leaves error reporting to a higher layer @@ -53,6 +57,13 @@ func writeError(ctx context.Context, w http.ResponseWriter, err error, args ...i logger.Error(fmt.Sprintf("%v", err), args...) // fmt used to handle nil + var ocmError *ocmerrors.Error + if errors.As(err, &ocmError) { + resourceID, _ := ResourceIDFromContext(ctx) // used for error reporting + arm.WriteCloudError(w, ocm.CSErrorToCloudError(err, resourceID, w.Header())) + return nil + } + var cloudErr *arm.CloudError if err != nil && errors.As(err, &cloudErr) { if cloudErr != nil { // difference between interface is nil and the content is nil @@ -61,6 +72,16 @@ func writeError(ctx context.Context, w http.ResponseWriter, err error, args ...i } } + if database.IsResponseError(err, http.StatusNotFound) { + resourceID, err := ResourceIDFromContext(ctx) // used for error reporting + if err != nil { + arm.WriteInternalServerError(w) + return nil + } + arm.WriteCloudError(w, arm.NewResourceNotFoundError(resourceID)) + return nil + } + arm.WriteInternalServerError(w) return nil } diff --git a/frontend/pkg/frontend/frontend.go b/frontend/pkg/frontend/frontend.go index 833cb7462f..d7ae3db32f 100644 --- a/frontend/pkg/frontend/frontend.go +++ b/frontend/pkg/frontend/frontend.go @@ -20,7 +20,6 @@ import ( "errors" "fmt" "log/slog" - "maps" "net" "net/http" "os" @@ -774,31 +773,6 @@ func (f *Frontend) OperationStatus(writer http.ResponseWriter, request *http.Req return nil } -// 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) { - clusterServiceBasedInternalCluster, err := ocm.ConvertCStoHCPOpenShiftCluster(internalCluster.ID, csCluster) - if err != nil { - return nil, err - } - - clusterServiceBasedInternalCluster.SystemData = internalCluster.SystemData - clusterServiceBasedInternalCluster.Tags = maps.Clone(internalCluster.Tags) - clusterServiceBasedInternalCluster.ServiceProviderProperties.ProvisioningState = internalCluster.ServiceProviderProperties.ProvisioningState - if clusterServiceBasedInternalCluster.Identity == nil { - clusterServiceBasedInternalCluster.Identity = &arm.ManagedServiceIdentity{} - } - - if internalCluster.Identity != nil { - clusterServiceBasedInternalCluster.Identity.PrincipalID = internalCluster.Identity.PrincipalID - clusterServiceBasedInternalCluster.Identity.TenantID = internalCluster.Identity.TenantID - clusterServiceBasedInternalCluster.Identity.Type = internalCluster.Identity.Type - } - - return clusterServiceBasedInternalCluster, nil -} - func getSubscriptionDifferences(oldSub, newSub *arm.Subscription) []string { var messages []string @@ -944,7 +918,7 @@ func (f *Frontend) OperationResult(writer http.ResponseWriter, request *http.Req } case cosmosOperation.InternalID.Kind() == arohcpv1alpha1.ClusterKind: - resultingInternalCluster, err := f.GetInternalClusterFromStorage(ctx, cosmosOperation.ExternalID) + resultingInternalCluster, err := f.getInternalClusterFromStorage(ctx, cosmosOperation.ExternalID) if err != nil { return err } @@ -954,15 +928,11 @@ func (f *Frontend) OperationResult(writer http.ResponseWriter, request *http.Req } case cosmosOperation.ExternalID.ResourceType.String() == api.NodePoolResourceType.String(): - internalObj, err := f.dbClient.HCPClusters(cosmosOperation.ExternalID.SubscriptionID, cosmosOperation.ExternalID.ResourceGroupName).NodePools(cosmosOperation.ExternalID.Parent.Name).Get(ctx, cosmosOperation.ExternalID.Name) + resultingInternalNodePool, err := f.getInternalNodePoolFromStorage(ctx, cosmosOperation.ExternalID) if err != nil { return err } - clusterServiceObj, err := f.clusterServiceClient.GetNodePool(ctx, internalObj.ServiceProviderProperties.ClusterServiceID) - if err != nil { - return ocm.CSErrorToCloudError(err, resourceID, nil) - } - responseBody, err = mergeToExternalNodePool(clusterServiceObj, internalObj, versionedInterface) + responseBody, err = arm.MarshalJSON(versionedInterface.NewHCPOpenShiftClusterNodePool(resultingInternalNodePool)) if err != nil { return err } diff --git a/frontend/pkg/frontend/helpers.go b/frontend/pkg/frontend/helpers.go index ff483dc481..ca2ae00fb0 100644 --- a/frontend/pkg/frontend/helpers.go +++ b/frontend/pkg/frontend/helpers.go @@ -358,26 +358,3 @@ func (f *Frontend) DeleteResource(ctx context.Context, transaction database.DBTr return operationID, nil } - -func (f *Frontend) GetInternalClusterFromStorage(ctx context.Context, resourceID *azcorearm.ResourceID) (*api.HCPOpenShiftCluster, error) { - internalCluster, err := f.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).Get(ctx, resourceID.Name) - if database.IsResponseError(err, http.StatusNotFound) { - return nil, arm.NewResourceNotFoundError(resourceID) - } - if err != nil { - return nil, err - } - - csCluster, err := f.clusterServiceClient.GetCluster(ctx, internalCluster.ServiceProviderProperties.ClusterServiceID) - if err != nil { - return nil, ocm.CSErrorToCloudError(err, resourceID, nil) - } - - // TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos - internalCluster, err = mergeToInternalCluster(csCluster, internalCluster) - if err != nil { - return nil, err - } - - return internalCluster, nil -} diff --git a/frontend/pkg/frontend/node_pool.go b/frontend/pkg/frontend/node_pool.go index 780e4e7ffe..6fde14ad32 100644 --- a/frontend/pkg/frontend/node_pool.go +++ b/frontend/pkg/frontend/node_pool.go @@ -15,13 +15,16 @@ package frontend import ( + "context" + "encoding/json" "fmt" "maps" "net/http" "strings" - "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/utils/ptr" + azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" "github.com/Azure/azure-sdk-for-go/sdk/data/azcosmos" arohcpv1alpha1 "github.com/openshift-online/ocm-sdk-go/arohcp/v1alpha1" @@ -42,33 +45,25 @@ func (f *Frontend) GetNodePool(writer http.ResponseWriter, request *http.Request if err != nil { return err } - resourceID, err := ResourceIDFromContext(ctx) // used for error reporting + resourceID, err := ResourceIDFromContext(ctx) if err != nil { return err } - internalObj, err := f.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).NodePools(resourceID.Parent.Name).Get(ctx, resourceID.Name) - if database.IsResponseError(err, http.StatusNotFound) { - return arm.NewResourceNotFoundError(resourceID) - } + resultingInternalNodePool, err := f.getInternalNodePoolFromStorage(ctx, resourceID) if err != nil { return err } - - clusterServiceObj, err := f.clusterServiceClient.GetNodePool(ctx, internalObj.ServiceProviderProperties.ClusterServiceID) - if err != nil { - return ocm.CSErrorToCloudError(err, resourceID, nil) - } - - responseBody, err := mergeToExternalNodePool(clusterServiceObj, internalObj, versionedInterface) + responseBytes, err := arm.MarshalJSON(versionedInterface.NewHCPOpenShiftClusterNodePool(resultingInternalNodePool)) if err != nil { return err } - _, err = arm.WriteJSONResponse(writer, http.StatusOK, responseBody) + _, err = arm.WriteJSONResponse(writer, http.StatusOK, responseBytes) if err != nil { return err } + return nil } @@ -83,9 +78,9 @@ func (f *Frontend) ArmResourceListNodePools(writer http.ResponseWriter, request subscriptionID := request.PathValue(PathSegmentSubscriptionID) resourceGroupName := request.PathValue(PathSegmentResourceGroupName) - resourceName := request.PathValue(PathSegmentResourceName) + clusterName := request.PathValue(PathSegmentResourceName) - internalCluster, err := f.dbClient.HCPClusters(subscriptionID, resourceGroupName).Get(ctx, resourceName) + internalCluster, err := f.dbClient.HCPClusters(subscriptionID, resourceGroupName).Get(ctx, clusterName) if err != nil { return err } @@ -93,7 +88,7 @@ func (f *Frontend) ArmResourceListNodePools(writer http.ResponseWriter, request pagedResponse := arm.NewPagedResponse() nodePoolsByClusterServiceID := make(map[string]*api.HCPOpenShiftClusterNodePool) - internalNodePoolIterator, err := f.dbClient.HCPClusters(subscriptionID, resourceGroupName).NodePools(resourceName).List(ctx, dbListOptionsFromRequest(request)) + internalNodePoolIterator, err := f.dbClient.HCPClusters(subscriptionID, resourceGroupName).NodePools(clusterName).List(ctx, dbListOptionsFromRequest(request)) if err != nil { return err } @@ -123,16 +118,21 @@ func (f *Frontend) ArmResourceListNodePools(writer http.ResponseWriter, request csIterator := f.clusterServiceClient.ListNodePools(internalCluster.ServiceProviderProperties.ClusterServiceID, query) for csNodePool := range csIterator.Items(ctx) { if internalNodePool, ok := nodePoolsByClusterServiceID[csNodePool.ID()]; ok { - value, err := mergeToExternalNodePool(csNodePool, internalNodePool, versionedInterface) + internalNodePool, err = mergeToInternalNodePool(csNodePool, internalNodePool) + if err != nil { + return err + } + resultingExternalCluster := versionedInterface.NewHCPOpenShiftClusterNodePool(internalNodePool) + jsonBytes, err := arm.MarshalJSON(resultingExternalCluster) if err != nil { return err } - pagedResponse.AddValue(value) + pagedResponse.AddValue(jsonBytes) } } - err = csIterator.GetError() - if err != nil { - return ocm.CSErrorToCloudError(err, nil, writer.Header()) + // Check for iteration error. + if err := csIterator.GetError(); err != nil { + return err } _, err = arm.WriteJSONResponse(writer, http.StatusOK, pagedResponse) @@ -157,6 +157,91 @@ func (f *Frontend) CreateOrUpdateNodePool(writer http.ResponseWriter, request *h // PATCH requests overlay the request body onto a resource struct // that represents an existing resource to be updated. + ctx := request.Context() + + resourceID, err := ResourceIDFromContext(ctx) + if err != nil { + return err + } + + nodePoolCosmosClient := f.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).NodePools(resourceID.Parent.Name) + oldInternalNodePool, err := nodePoolCosmosClient.Get(ctx, resourceID.Name) + if err != nil && !database.IsResponseError(err, http.StatusNotFound) { + return err + } + + updating := oldInternalNodePool != nil + if updating { + // re-write oldInternalCluster for as long as cluster-service needs to be consulted for pre-existing state. + oldInternalNodePool, err = f.readInternalNodePoolFromClusterService(ctx, oldInternalNodePool) + if err != nil { + return err + } + // CheckForProvisioningStateConflict does not log conflict errors + // but does log unexpected errors like database failures. + if err := checkForProvisioningStateConflict(ctx, f.dbClient, database.OperationRequestUpdate, oldInternalNodePool.ID, oldInternalNodePool.Properties.ProvisioningState); err != nil { + return err + } + + switch request.Method { + case http.MethodPut: + return f.updateNodePool(writer, request, oldInternalNodePool) + case http.MethodPatch: + return f.patchNodePool(writer, request, oldInternalNodePool) + default: + return fmt.Errorf("unsupported method %s", request.Method) + } + } + + switch request.Method { + case http.MethodPut: + return f.createNodePool(writer, request) + case http.MethodPatch: + return arm.NewResourceNotFoundError(resourceID) + default: + return fmt.Errorf("unsupported method %s", request.Method) + } +} + +func decodeDesiredNodePoolCreate(ctx context.Context) (*api.HCPOpenShiftClusterNodePool, error) { + versionedInterface, err := VersionFromContext(ctx) + if err != nil { + return nil, err + } + resourceID, err := ResourceIDFromContext(ctx) + if err != nil { + return nil, err + } + body, err := BodyFromContext(ctx) + if err != nil { + return nil, err + } + systemData, err := SystemDataFromContext(ctx) + if err != nil { + return nil, err + } + + externalNodePoolFromRequest := versionedInterface.NewHCPOpenShiftClusterNodePool(&api.HCPOpenShiftClusterNodePool{}) + if err := json.Unmarshal(body, &externalNodePoolFromRequest); err != nil { + return nil, err + } + if err := externalNodePoolFromRequest.SetDefaultValues(externalNodePoolFromRequest); err != nil { + return nil, err + } + + newInternalNodePool := &api.HCPOpenShiftClusterNodePool{} + externalNodePoolFromRequest.Normalize(newInternalNodePool) + // TrackedResource info doesn't appear to come from the external resource information + conversion.CopyReadOnlyTrackedResourceValues(&newInternalNodePool.TrackedResource, ptr.To(arm.NewTrackedResource(resourceID))) + + // set fields that were not included during the conversion, because the user does not provide them or because the + // data is determined live on read. + newInternalNodePool.SystemData = systemData + + return newInternalNodePool, nil +} + +func (f *Frontend) createNodePool(writer http.ResponseWriter, request *http.Request) error { ctx := request.Context() logger := LoggerFromContext(ctx) @@ -164,225 +249,279 @@ func (f *Frontend) CreateOrUpdateNodePool(writer http.ResponseWriter, request *h if err != nil { return err } - resourceID, err := ResourceIDFromContext(ctx) if err != nil { return err } - - body, err := BodyFromContext(ctx) + correlationData, err := CorrelationDataFromContext(ctx) if err != nil { return err } - systemData, err := SystemDataFromContext(ctx) + newInternalNodePool, err := decodeDesiredNodePoolCreate(ctx) if err != nil { return err } - correlationData, err := CorrelationDataFromContext(ctx) + // Node pool validation checks some fields against the parent cluster + // so we have to request the cluster from Cluster Service. + hcpCluster, err := f.getInternalClusterFromStorage(ctx, resourceID.Parent) if err != nil { return err } - pk := database.NewPartitionKey(resourceID.SubscriptionID) + validationErrs := validation.ValidateNodePoolCreate(ctx, newInternalNodePool) + // in addition to static validation, we have validation based on the state of the hcp cluster + validationErrs = append(validationErrs, admission.AdmitNodePool(newInternalNodePool, hcpCluster)...) + if err := arm.CloudErrorFromFieldErrors(validationErrs); err != nil { + return err + } - nodePoolCosmosClient := f.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).NodePools(resourceID.Parent.Name) - internalOldNodePool, err := nodePoolCosmosClient.Get(ctx, resourceID.Name) - if err != nil && !database.IsResponseError(err, http.StatusNotFound) { + logger.Info(fmt.Sprintf("creating resource %s", resourceID)) + csNodePoolBuilder, err := ocm.BuildCSNodePool(ctx, newInternalNodePool, false) + if err != nil { + return err + } + csNodePool, err := f.clusterServiceClient.PostNodePool(ctx, hcpCluster.ServiceProviderProperties.ClusterServiceID, csNodePoolBuilder) + if err != nil { return err } - var updating = (internalOldNodePool != nil) - var operationRequest database.OperationRequest + newInternalNodePool.ServiceProviderProperties.ClusterServiceID, err = api.NewInternalID(csNodePool.HREF()) + if err != nil { + return err + } - var externalOldNodePool api.VersionedHCPOpenShiftClusterNodePool - var externalNewNodePool api.VersionedHCPOpenShiftClusterNodePool - var successStatusCode int + pk := database.NewPartitionKey(newInternalNodePool.ID.SubscriptionID) + transaction := f.dbClient.NewTransaction(pk) - if updating { - { // scope to ensure temporary variables don't escape - csNodePool, err := f.clusterServiceClient.GetNodePool(ctx, internalOldNodePool.ServiceProviderProperties.ClusterServiceID) - if err != nil { - logger.Error(fmt.Sprintf("failed to fetch CS node pool for %s: %v", resourceID, err)) - return ocm.CSErrorToCloudError(err, resourceID, writer.Header()) - } + createNodePoolOperation := database.NewOperationDocument(database.OperationRequestCreate, newInternalNodePool.ID, newInternalNodePool.ServiceProviderProperties.ClusterServiceID, correlationData) + createNodePoolOperation.TenantID = request.Header.Get(arm.HeaderNameHomeTenantID) + createNodePoolOperation.ClientID = request.Header.Get(arm.HeaderNameClientObjectID) + createNodePoolOperation.NotificationURI = request.Header.Get(arm.HeaderNameAsyncNotificationURI) + operationCosmosID := transaction.CreateOperationDoc(createNodePoolOperation, nil) + transaction.OnSuccess(addOperationResponseHeaders(writer, request, createNodePoolOperation.NotificationURI, createNodePoolOperation.OperationID)) - mergedOldNodePool := ocm.ConvertCStoNodePool(resourceID, csNodePool) + // set fields that were not known until the operation doc instance was created. + // TODO once we we have separate creation/validation of operation documents, this can be done ahead of time. + newInternalNodePool.ServiceProviderProperties.ActiveOperationID = operationCosmosID + newInternalNodePool.Properties.ProvisioningState = createNodePoolOperation.Status - // Do not set the TrackedResource.Tags field here. We need - // the Tags map to remain nil so we can see if the request - // body included a new set of resource tags. + nodePoolCosmosClient := f.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).NodePools(resourceID.Parent.Name) + cosmosUID, err := nodePoolCosmosClient.AddCreateToTransaction(ctx, transaction, newInternalNodePool, nil) + if err != nil { + return err + } - mergedOldNodePool.SystemData = internalOldNodePool.SystemData - mergedOldNodePool.Properties.ProvisioningState = internalOldNodePool.Properties.ProvisioningState - mergedOldNodePool.ServiceProviderProperties.CosmosUID = internalOldNodePool.ServiceProviderProperties.CosmosUID - mergedOldNodePool.ServiceProviderProperties.ClusterServiceID = internalOldNodePool.ServiceProviderProperties.ClusterServiceID + transactionResult, err := transaction.Execute(ctx, &azcosmos.TransactionalBatchOptions{ + EnableContentResponseOnWrite: true, + }) + if err != nil { + return err + } - // internalOldNodePool gets overwritten (for now), by the content from cluster-service which is authoritative for now. - internalOldNodePool = mergedOldNodePool - } + // Read back the resource document so the response body is accurate. + resultingUncastInternalNodePool, err := transactionResult.GetItem(cosmosUID) + if err != nil { + return err + } + resultingInternalNodePool, ok := resultingUncastInternalNodePool.(*api.HCPOpenShiftClusterNodePool) + if !ok { + return fmt.Errorf("unexpected type %T", resultingUncastInternalNodePool) + } - operationRequest = database.OperationRequestUpdate + // TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos + resultingInternalNodePool, err = mergeToInternalNodePool(csNodePool, resultingInternalNodePool) + if err != nil { + return err + } + responseBytes, err := arm.MarshalJSON(versionedInterface.NewHCPOpenShiftClusterNodePool(resultingInternalNodePool)) + if err != nil { + return err + } - // This is slightly repetitive for the sake of clarify on PUT vs PATCH. - switch request.Method { - case http.MethodPut: - // Initialize versionedRequestNodePool to include both - // non-zero default values and current read-only values. - reqNodePool := api.NewDefaultHCPOpenShiftClusterNodePool(resourceID) - - // Some optional create-only fields have dynamic default - // values that are determined downstream of this phase of - // request processing. To ensure idempotency, add these - // values to the target struct for the incoming request. - reqNodePool.Properties.Version.ID = internalOldNodePool.Properties.Version.ID - reqNodePool.Properties.Platform.SubnetID = internalOldNodePool.Properties.Platform.SubnetID - - externalOldNodePool = versionedInterface.NewHCPOpenShiftClusterNodePool(internalOldNodePool) - externalNewNodePool = versionedInterface.NewHCPOpenShiftClusterNodePool(reqNodePool) - - // read-only values are an internal concern since they're the source, so we convert. - // this could be faster done purely externally, but this allows a single set of rules for copying read only fields. - newTemporaryInternal := &api.HCPOpenShiftClusterNodePool{} - externalNewNodePool.Normalize(newTemporaryInternal) - conversion.CopyReadOnlyNodePoolValues(newTemporaryInternal, internalOldNodePool) - externalNewNodePool = versionedInterface.NewHCPOpenShiftClusterNodePool(newTemporaryInternal) - - successStatusCode = http.StatusOK - case http.MethodPatch: - externalOldNodePool = versionedInterface.NewHCPOpenShiftClusterNodePool(internalOldNodePool) - externalNewNodePool = versionedInterface.NewHCPOpenShiftClusterNodePool(internalOldNodePool) - successStatusCode = http.StatusAccepted - } + _, err = arm.WriteJSONResponse(writer, http.StatusCreated, responseBytes) + if err != nil { + return err + } + return nil +} - // CheckForProvisioningStateConflict does not log conflict errors - // but does log unexpected errors like database failures. +func decodeDesiredNodePoolReplace(ctx context.Context, oldInternalNodePool *api.HCPOpenShiftClusterNodePool) (*api.HCPOpenShiftClusterNodePool, error) { + versionedInterface, err := VersionFromContext(ctx) + if err != nil { + return nil, err + } - if err := checkForProvisioningStateConflict(ctx, f.dbClient, operationRequest, internalOldNodePool.ID, internalOldNodePool.Properties.ProvisioningState); err != nil { - return err - } + // Decoding for update has a series of semantics for determining the final desired update + // 1. exact user request + // 2. defaults for that version + // 3. if not set, the values that the user doesn't necessary have to set but are not static defaults. These from from the old value. + // 4. values that are missing because the external type doesn't represent them + // 5. values that might change because our machinery changes them. - } else { - operationRequest = database.OperationRequestCreate + body, err := BodyFromContext(ctx) + if err != nil { + return nil, err + } + systemData, err := SystemDataFromContext(ctx) + if err != nil { + return nil, err + } - switch request.Method { - case http.MethodPut: - // Initialize top-level resource fields from the request path. - // If the request body specifies these fields, validation should - // accept them as long as they match (case-insensitively) values - // from the request path. - hcpNodePool := api.NewDefaultHCPOpenShiftClusterNodePool(resourceID) - - externalOldNodePool = versionedInterface.NewHCPOpenShiftClusterNodePool(hcpNodePool) - externalNewNodePool = versionedInterface.NewHCPOpenShiftClusterNodePool(hcpNodePool) - successStatusCode = http.StatusCreated - case http.MethodPatch: - // PATCH requests never create a new resource. - return arm.NewResourceNotFoundError(resourceID) - } + // Initialize versionedRequestNodePool to include both + // non-zero default values and current read-only values. + // Exact user request + externalNodePoolFromRequest := versionedInterface.NewHCPOpenShiftClusterNodePool(&api.HCPOpenShiftClusterNodePool{}) + if err := json.Unmarshal(body, &externalNodePoolFromRequest); err != nil { + return nil, err } - if err := api.ApplyRequestBody(request.Method, body, externalNewNodePool); err != nil { - return err + // Default values + if err := externalNodePoolFromRequest.SetDefaultValues(externalNodePoolFromRequest); err != nil { + return nil, err } - // Node pool validation checks some fields against the parent cluster - // so we have to request the cluster from Cluster Service. + newInternalNodePool := &api.HCPOpenShiftClusterNodePool{} + externalNodePoolFromRequest.Normalize(newInternalNodePool) - cluster, err := f.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).Get(ctx, resourceID.Parent.Name) + // values a user doesn't have to provide, but are not static defaults (set dynamically during create). Set these from old value + if len(newInternalNodePool.Properties.Version.ID) == 0 { + newInternalNodePool.Properties.Version.ID = oldInternalNodePool.Properties.Version.ID + } + if len(newInternalNodePool.Properties.Platform.SubnetID) == 0 { + newInternalNodePool.Properties.Platform.SubnetID = oldInternalNodePool.Properties.Platform.SubnetID + } + + // ServiceProviderProperties contains two types of information + // 1. values that a user cannot change because the external type does not expose the information. + // We must overwrite those values with the oldInternalCluster values so the values don't change, because the user's input will always be empty. + // 2. values that a user cannot change due to validation requirements, but the user *can* specify the values. + // We are overwriting these values that we consider to be status values. + // We do this because if a user has read a value, then modified it, then replaces it, we don't want to produce + // validation errors on status fields that the user isn't trying to modify. + conversion.CopyReadOnlyNodePoolValues(newInternalNodePool, oldInternalNodePool) + newInternalNodePool.SystemData = systemData + + // Clear the user-assigned identities map since that is reconstructed from Cluster Service data. + // TODO we'd like to have the instance complete when we go to validate it. Right now validation fails if we clear this. + // TODO we probably update validation to require this field is cleared. + //newInternalCluster.Identity.UserAssignedIdentities = nil + + return newInternalNodePool, nil +} + +func (f *Frontend) updateNodePool(writer http.ResponseWriter, request *http.Request, oldInternalNodePool *api.HCPOpenShiftClusterNodePool) error { + ctx := request.Context() + + newInternalNodePool, err := decodeDesiredNodePoolReplace(ctx, oldInternalNodePool) if err != nil { return err } - csCluster, err := f.clusterServiceClient.GetCluster(ctx, cluster.ServiceProviderProperties.ClusterServiceID) + return f.updateNodePoolInCosmos(ctx, writer, request, http.StatusOK, newInternalNodePool, oldInternalNodePool) +} + +func decodeDesiredNodePoolPatch(ctx context.Context, oldInternalNodePool *api.HCPOpenShiftClusterNodePool) (*api.HCPOpenShiftClusterNodePool, error) { + versionedInterface, err := VersionFromContext(ctx) if err != nil { - return ocm.CSErrorToCloudError(err, resourceID.Parent, writer.Header()) + return nil, err } - - hcpCluster, err := ocm.ConvertCStoHCPOpenShiftCluster(resourceID.Parent, csCluster) + body, err := BodyFromContext(ctx) if err != nil { - return err + return nil, err + } + systemData, err := SystemDataFromContext(ctx) + if err != nil { + return nil, err } + // TODO find a way to represent the desired change without starting from internal state here (very confusing) + // TODO we appear to lack a test, but this seems to take an original, apply the patch and unmarshal the result, meaning the above patch step is just incorrect. + newExternalNodePool := versionedInterface.NewHCPOpenShiftClusterNodePool(oldInternalNodePool) + if err := api.ApplyRequestBody(http.MethodPatch, body, newExternalNodePool); err != nil { + return nil, err + } newInternalNodePool := &api.HCPOpenShiftClusterNodePool{} - externalNewNodePool.Normalize(newInternalNodePool) + newExternalNodePool.Normalize(newInternalNodePool) - var validationErrs field.ErrorList - if updating { - oldInternalNodePool := &api.HCPOpenShiftClusterNodePool{} - externalOldNodePool.Normalize(oldInternalNodePool) - validationErrs = validation.ValidateNodePoolUpdate(ctx, newInternalNodePool, oldInternalNodePool) - // in addition to static validation, we have validation based on the state of the hcp cluster - validationErrs = append(validationErrs, admission.AdmitNodePool(newInternalNodePool, hcpCluster)...) + conversion.CopyReadOnlyNodePoolValues(newInternalNodePool, oldInternalNodePool) + newInternalNodePool.SystemData = systemData - } else { - validationErrs = validation.ValidateNodePoolCreate(ctx, newInternalNodePool) - // in addition to static validation, we have validation based on the state of the hcp cluster - validationErrs = append(validationErrs, admission.AdmitNodePool(newInternalNodePool, hcpCluster)...) + // Clear the user-assigned identities map since that is reconstructed from Cluster Service data. + // TODO we'd like to have the instance complete when we go to validate it. Right now validation fails if we clear this. + // TODO we probably update validation to require this field is cleared. + //newInternalCluster.Identity.UserAssignedIdentities = nil - } - if err := arm.CloudErrorFromFieldErrors(validationErrs); err != nil { + return newInternalNodePool, nil +} + +func (f *Frontend) patchNodePool(writer http.ResponseWriter, request *http.Request, oldInternalNodePool *api.HCPOpenShiftClusterNodePool) error { + // PATCH requests overlay the request body onto a resource struct + // that represents an existing resource to be updated. + ctx := request.Context() + + newInternalNodePool, err := decodeDesiredNodePoolPatch(ctx, oldInternalNodePool) + if err != nil { return err } - csNodePoolBuilder, err := ocm.BuildCSNodePool(ctx, newInternalNodePool, updating) + return f.updateNodePoolInCosmos(ctx, writer, request, http.StatusAccepted, newInternalNodePool, oldInternalNodePool) +} + +func (f *Frontend) updateNodePoolInCosmos(ctx context.Context, writer http.ResponseWriter, request *http.Request, httpStatusCode int, newInternalNodePool, oldInternalNodePool *api.HCPOpenShiftClusterNodePool) error { + logger := LoggerFromContext(ctx) + + versionedInterface, err := VersionFromContext(ctx) + if err != nil { + return err + } + correlationData, err := CorrelationDataFromContext(ctx) if err != nil { return err } - var csNodePool *arohcpv1alpha1.NodePool + // Node pool validation checks some fields against the parent cluster + // so we have to request the cluster from Cluster Service. + cluster, err := f.getInternalClusterFromStorage(ctx, oldInternalNodePool.ID.Parent) + if err != nil { + return ocm.CSErrorToCloudError(err, oldInternalNodePool.ID.Parent, writer.Header()) + } - if updating { - logger.Info(fmt.Sprintf("updating resource %s", resourceID)) - csNodePool, err = f.clusterServiceClient.UpdateNodePool(ctx, internalOldNodePool.ServiceProviderProperties.ClusterServiceID, csNodePoolBuilder) - if err != nil { - return ocm.CSErrorToCloudError(err, resourceID, writer.Header()) - } - } else { - logger.Info(fmt.Sprintf("creating resource %s", resourceID)) - cluster, err := f.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).Get(ctx, resourceID.Parent.Name) - if err != nil { - return err - } + validationErrs := validation.ValidateNodePoolUpdate(ctx, newInternalNodePool, oldInternalNodePool) + // in addition to static validation, we have validation based on the state of the hcp cluster + validationErrs = append(validationErrs, admission.AdmitNodePool(newInternalNodePool, cluster)...) + if err := arm.CloudErrorFromFieldErrors(validationErrs); err != nil { + return err + } - csNodePool, err = f.clusterServiceClient.PostNodePool(ctx, cluster.ServiceProviderProperties.ClusterServiceID, csNodePoolBuilder) - if err != nil { - return ocm.CSErrorToCloudError(err, resourceID, writer.Header()) - } + csNodePoolBuilder, err := ocm.BuildCSNodePool(ctx, newInternalNodePool, true) + if err != nil { + return err + } - newInternalNodePool.ServiceProviderProperties.ClusterServiceID, err = api.NewInternalID(csNodePool.HREF()) - if err != nil { - return err - } + logger.Info(fmt.Sprintf("updating resource %s", oldInternalNodePool.ID)) + csNodePool, err := f.clusterServiceClient.UpdateNodePool(ctx, oldInternalNodePool.ServiceProviderProperties.ClusterServiceID, csNodePoolBuilder) + if err != nil { + return err } + pk := database.NewPartitionKey(oldInternalNodePool.ID.SubscriptionID) transaction := f.dbClient.NewTransaction(pk) - operationDoc := database.NewOperationDocument(operationRequest, newInternalNodePool.ID, newInternalNodePool.ServiceProviderProperties.ClusterServiceID, correlationData) - operationID := transaction.CreateOperationDoc(operationDoc, nil) - - f.ExposeOperation(writer, request, operationID, transaction) + nodePoolUpdateOperation := database.NewOperationDocument(database.OperationRequestUpdate, newInternalNodePool.ID, newInternalNodePool.ServiceProviderProperties.ClusterServiceID, correlationData) + nodePoolUpdateOperation.TenantID = request.Header.Get(arm.HeaderNameHomeTenantID) + nodePoolUpdateOperation.ClientID = request.Header.Get(arm.HeaderNameClientObjectID) + nodePoolUpdateOperation.NotificationURI = request.Header.Get(arm.HeaderNameAsyncNotificationURI) + operationCosmosUID := transaction.CreateOperationDoc(nodePoolUpdateOperation, nil) - cosmosUID := "" - if !updating { - cosmosUID, err = nodePoolCosmosClient.AddCreateToTransaction(ctx, transaction, newInternalNodePool, nil) - if err != nil { - return err - } - } else { - cosmosUID = internalOldNodePool.ServiceProviderProperties.CosmosUID - } + f.ExposeOperation(writer, request, operationCosmosUID, transaction) var patchOperations database.ResourceDocumentPatchOperations - patchOperations.SetActiveOperationID(&operationID) - patchOperations.SetProvisioningState(operationDoc.Status) - - // Record the latest system data values from ARM, if present. - if systemData != nil { - patchOperations.SetSystemData(systemData) - } + patchOperations.SetActiveOperationID(&operationCosmosUID) + patchOperations.SetProvisioningState(nodePoolUpdateOperation.Status) + patchOperations.SetSystemData(newInternalNodePool.SystemData) // Here the difference between a nil map and an empty map is significant. // If the Tags map is nil, that means it was omitted from the request body, @@ -393,7 +532,7 @@ func (f *Frontend) CreateOrUpdateNodePool(writer http.ResponseWriter, request *h patchOperations.SetTags(newInternalNodePool.Tags) } - transaction.PatchResourceDoc(cosmosUID, patchOperations, nil) + transaction.PatchResourceDoc(oldInternalNodePool.ServiceProviderProperties.CosmosUID, patchOperations, nil) transactionResult, err := transaction.Execute(ctx, &azcosmos.TransactionalBatchOptions{ EnableContentResponseOnWrite: true, @@ -403,21 +542,26 @@ func (f *Frontend) CreateOrUpdateNodePool(writer http.ResponseWriter, request *h } // Read back the resource document so the response body is accurate. - resultingCosmosObj, err := transactionResult.GetResourceDoc(cosmosUID) + resultingUncastInternalNodePool, err := transactionResult.GetItem(oldInternalNodePool.ServiceProviderProperties.CosmosUID) if err != nil { return err } - resultingInternalObj, err := database.ResourceDocumentToInternalAPI[api.HCPOpenShiftClusterNodePool, database.NodePool](resultingCosmosObj) + resultingInternalNodePool, ok := resultingUncastInternalNodePool.(*api.HCPOpenShiftClusterNodePool) + if !ok { + return fmt.Errorf("unexpected type %T", resultingUncastInternalNodePool) + } + + // TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos + resultingInternalNodePool, err = mergeToInternalNodePool(csNodePool, resultingInternalNodePool) if err != nil { return err } - - responseBody, err := mergeToExternalNodePool(csNodePool, resultingInternalObj, versionedInterface) + responseBytes, err := arm.MarshalJSON(versionedInterface.NewHCPOpenShiftClusterNodePool(resultingInternalNodePool)) if err != nil { return err } - _, err = arm.WriteJSONResponse(writer, successStatusCode, responseBody) + _, err = arm.WriteJSONResponse(writer, httpStatusCode, responseBytes) if err != nil { return err } @@ -425,11 +569,48 @@ func (f *Frontend) CreateOrUpdateNodePool(writer http.ResponseWriter, request *h } // the necessary conversions for the API version of the request. -func mergeToExternalNodePool(csNodePool *arohcpv1alpha1.NodePool, internalNodePool *api.HCPOpenShiftClusterNodePool, versionedInterface api.Version) ([]byte, error) { - hcpNodePool := ocm.ConvertCStoNodePool(internalNodePool.ID, csNodePool) - hcpNodePool.SystemData = internalNodePool.SystemData - hcpNodePool.Tags = maps.Clone(internalNodePool.Tags) - hcpNodePool.Properties.ProvisioningState = internalNodePool.Properties.ProvisioningState +func mergeToInternalNodePool(clusterServiceNod *arohcpv1alpha1.NodePool, internalNodePool *api.HCPOpenShiftClusterNodePool) (*api.HCPOpenShiftClusterNodePool, error) { + mergedOldClusterServiceNodePool := ocm.ConvertCStoNodePool(internalNodePool.ID, clusterServiceNod) + + // Do not set the TrackedResource.Tags field here. We need + // the Tags map to remain nil so we can see if the request + // body included a new set of resource tags. + mergedOldClusterServiceNodePool.SystemData = internalNodePool.SystemData + mergedOldClusterServiceNodePool.Tags = maps.Clone(internalNodePool.Tags) + mergedOldClusterServiceNodePool.Properties.ProvisioningState = internalNodePool.Properties.ProvisioningState + mergedOldClusterServiceNodePool.ServiceProviderProperties.CosmosUID = internalNodePool.ServiceProviderProperties.CosmosUID + mergedOldClusterServiceNodePool.ServiceProviderProperties.ClusterServiceID = internalNodePool.ServiceProviderProperties.ClusterServiceID + + return mergedOldClusterServiceNodePool, nil +} + +func (f *Frontend) getInternalNodePoolFromStorage(ctx context.Context, resourceID *azcorearm.ResourceID) (*api.HCPOpenShiftClusterNodePool, error) { + internalNodePool, err := f.dbClient.HCPClusters(resourceID.SubscriptionID, resourceID.ResourceGroupName).NodePools(resourceID.Parent.Name).Get(ctx, resourceID.Name) + if database.IsResponseError(err, http.StatusNotFound) { + return nil, arm.NewResourceNotFoundError(resourceID) + } + if err != nil { + return nil, err + } + + return f.readInternalNodePoolFromClusterService(ctx, internalNodePool) + +} + +// readInternalNodePoolFromClusterService takes an internal NodePool read from cosmos, retrieves the corresponding cluster-service data, +// merges the states together, and returns the internal representation. +// TODO remove the header it takes and collapse that to some general error handling. +func (f *Frontend) readInternalNodePoolFromClusterService(ctx context.Context, oldInternalNodePool *api.HCPOpenShiftClusterNodePool) (*api.HCPOpenShiftClusterNodePool, error) { + oldClusterServiceNodePool, err := f.clusterServiceClient.GetNodePool(ctx, oldInternalNodePool.ServiceProviderProperties.ClusterServiceID) + if err != nil { + return nil, err + } + + // TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos + oldInternalNodePool, err = mergeToInternalNodePool(oldClusterServiceNodePool, oldInternalNodePool) + if err != nil { + return nil, err + } - return arm.MarshalJSON(versionedInterface.NewHCPOpenShiftClusterNodePool(hcpNodePool)) + return oldInternalNodePool, nil } diff --git a/frontend/test/simulate/artifacts/NodePoolMutation/immutability/expected-errors.txt b/frontend/test/simulate/artifacts/NodePoolMutation/immutability/expected-errors.txt index 8da54e6b00..64fd2fbdae 100644 --- a/frontend/test/simulate/artifacts/NodePoolMutation/immutability/expected-errors.txt +++ b/frontend/test/simulate/artifacts/NodePoolMutation/immutability/expected-errors.txt @@ -1,3 +1,4 @@ +InvalidRequestContent: properties.autoRepair: Forbidden: field is immutable InvalidRequestContent: properties.platform.vmSize: Forbidden: field is immutable InvalidRequestContent: properties.platform.availabilityZone: Forbidden: field is immutable InvalidRequestContent: properties.platform.enableEncryptionAtHost: Forbidden: field is immutable diff --git a/frontend/test/simulate/mutation_test_utils.go b/frontend/test/simulate/mutation_test_utils.go index 57be38c639..c2c2b688cf 100644 --- a/frontend/test/simulate/mutation_test_utils.go +++ b/frontend/test/simulate/mutation_test_utils.go @@ -196,10 +196,9 @@ func trivialPassThroughClusterServiceMock(t *testing.T, testInfo *SimulationTest require.NoError(t, testInfo.AddMockData(t.Name()+"_nodePools", internalIDToNodePool)) testInfo.MockClusterServiceClient.EXPECT().PostNodePool(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, clusterID ocm.InternalID, builder *arohcpv1alpha1.NodePoolBuilder) (*arohcpv1alpha1.NodePool, error) { justID := rand.String(10) - builder.ID(justID) nodePoolInternalID := clusterID.String() + "/node_pools/" + justID - builder = builder.HREF(nodePoolInternalID) - ret, err := builder.Build() + + ret, err := builder.ID(justID).HREF(nodePoolInternalID).Build() if err != nil { return nil, err } diff --git a/internal/api/types_nodepool.go b/internal/api/types_nodepool.go index 7206932783..565eb6ffbe 100644 --- a/internal/api/types_nodepool.go +++ b/internal/api/types_nodepool.go @@ -63,8 +63,9 @@ type HCPOpenShiftClusterNodePoolProperties struct { } type HCPOpenShiftClusterNodePoolServiceProviderProperties struct { - CosmosUID string `json:"cosmosUID,omitempty"` - ClusterServiceID InternalID `json:"clusterServiceID,omitempty"` + CosmosUID string `json:"cosmosUID,omitempty"` + ClusterServiceID InternalID `json:"clusterServiceID,omitempty"` + ActiveOperationID string `json:"activeOperationId,omitempty"` } // NodePoolVersionProfile represents the worker node pool version. diff --git a/internal/api/v20240610preview/conversion_fuzz_test.go b/internal/api/v20240610preview/conversion_fuzz_test.go index 0d63f4e211..8cfaa9bc65 100644 --- a/internal/api/v20240610preview/conversion_fuzz_test.go +++ b/internal/api/v20240610preview/conversion_fuzz_test.go @@ -50,6 +50,8 @@ func TestRoundTripInternalExternalInternal(t *testing.T) { }, func(j *api.HCPOpenShiftClusterNodePoolServiceProviderProperties, c randfill.Continue) { c.FillNoCustom(j) + // ActiveOperationID does not roundtrip through the external type because it is purely an internal detail + j.ActiveOperationID = "" // CosmosUID does not roundtrip through the external type because it is purely an internal detail j.CosmosUID = "" // ClusterServiceID does not roundtrip through the external type because it is purely an internal detail diff --git a/internal/api/v20251223preview/conversion_fuzz_test.go b/internal/api/v20251223preview/conversion_fuzz_test.go index 6a3f0c0ea5..0b6fe9859f 100644 --- a/internal/api/v20251223preview/conversion_fuzz_test.go +++ b/internal/api/v20251223preview/conversion_fuzz_test.go @@ -50,6 +50,8 @@ func TestRoundTripInternalExternalInternal(t *testing.T) { }, func(j *api.HCPOpenShiftClusterNodePoolServiceProviderProperties, c randfill.Continue) { c.FillNoCustom(j) + // ActiveOperationID does not roundtrip through the external type because it is purely an internal detail + j.ActiveOperationID = "" // CosmosUID does not roundtrip through the external type because it is purely an internal detail j.CosmosUID = "" // ClusterServiceID does not roundtrip through the external type because it is purely an internal detail diff --git a/internal/conversion/readonly_nodepool.go b/internal/conversion/readonly_nodepool.go index ca0a412a47..45a316fbb6 100644 --- a/internal/conversion/readonly_nodepool.go +++ b/internal/conversion/readonly_nodepool.go @@ -21,11 +21,7 @@ import ( func CopyReadOnlyNodePoolValues(dest, src *api.HCPOpenShiftClusterNodePool) { // the old code appeared to shallow copies only - - dest.ID = src.ID - dest.Name = src.Name - dest.Type = src.Type - dest.SystemData = src.SystemData + CopyReadOnlyTrackedResourceValues(&dest.TrackedResource, &src.TrackedResource) switch { case hasClusterIdentityToSet(src.Identity) && dest.Identity == nil: @@ -38,4 +34,5 @@ func CopyReadOnlyNodePoolValues(dest, src *api.HCPOpenShiftClusterNodePool) { } dest.Properties.ProvisioningState = src.Properties.ProvisioningState + dest.ServiceProviderProperties = src.ServiceProviderProperties } diff --git a/internal/database/convert_nodepool.go b/internal/database/convert_nodepool.go index 07c35d9221..b1075b31d1 100644 --- a/internal/database/convert_nodepool.go +++ b/internal/database/convert_nodepool.go @@ -35,10 +35,9 @@ func InternalToCosmosNodePool(internalObj *api.HCPOpenShiftClusterNodePool) (*No }, NodePoolProperties: NodePoolProperties{ ResourceDocument: ResourceDocument{ - ResourceID: internalObj.ID, - InternalID: internalObj.ServiceProviderProperties.ClusterServiceID, - // TODO - //ActiveOperationID: "", + ResourceID: internalObj.ID, + InternalID: internalObj.ServiceProviderProperties.ClusterServiceID, + ActiveOperationID: internalObj.ServiceProviderProperties.ActiveOperationID, ProvisioningState: internalObj.Properties.ProvisioningState, Identity: toCosmosIdentity(internalObj.Identity), SystemData: internalObj.SystemData, @@ -61,6 +60,7 @@ func InternalToCosmosNodePool(internalObj *api.HCPOpenShiftClusterNodePool) (*No cosmosObj.InternalState.InternalAPI.Tags = nil cosmosObj.InternalState.InternalAPI.ServiceProviderProperties.CosmosUID = "" cosmosObj.InternalState.InternalAPI.ServiceProviderProperties.ClusterServiceID = ocm.InternalID{} + cosmosObj.InternalState.InternalAPI.ServiceProviderProperties.ActiveOperationID = "" return cosmosObj, nil } @@ -90,6 +90,7 @@ func CosmosToInternalNodePool(cosmosObj *NodePool) (*api.HCPOpenShiftClusterNode internalObj.Tags = copyTags(cosmosObj.Tags) internalObj.ServiceProviderProperties.CosmosUID = cosmosObj.ID internalObj.ServiceProviderProperties.ClusterServiceID = cosmosObj.InternalID + internalObj.ServiceProviderProperties.ActiveOperationID = cosmosObj.ActiveOperationID return internalObj, nil } diff --git a/internal/database/transaction.go b/internal/database/transaction.go index a6465d1b0c..a55a739cba 100644 --- a/internal/database/transaction.go +++ b/internal/database/transaction.go @@ -280,11 +280,11 @@ func (r *cosmosDBTransactionResult) GetItem(cosmosUID string) (any, error) { } switch strings.ToLower(typedDoc.ResourceType) { - case strings.ToLower(api.ProviderNamespace + "/" + api.ClusterResourceTypeName): + case strings.ToLower(api.ClusterResourceType.String()): return getCastResult[api.HCPOpenShiftCluster, HCPCluster](r, cosmosUID) - case strings.ToLower(api.ProviderNamespace + "/" + api.NodePoolResourceTypeName): + case strings.ToLower(api.NodePoolResourceType.String()): return getCastResult[api.HCPOpenShiftClusterNodePool, NodePool](r, cosmosUID) - case strings.ToLower(api.ProviderNamespace + "/" + api.ExternalAuthResourceTypeName): + case strings.ToLower(api.ExternalAuthResourceType.String()): return getCastResult[api.HCPOpenShiftClusterExternalAuth, ExternalAuth](r, cosmosUID) default: return nil, fmt.Errorf("unknown resource type '%s'", typedDoc.ResourceType)