Skip to content

stop using cluster-service for frontend nodepool reads #5058

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 6 commits into from
May 4, 2026
Merged

stop using cluster-service for frontend nodepool reads #5058

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d37ae94
stop using cluster-service for frontend nodepool reads
deads2k Apr 28, 2026
0988da4
clarify nodepool frontend behavior
deads2k Apr 28, 2026
a3b87f7
add admission mutation for nodepools
deads2k Apr 28, 2026
7563f98
refine admission code for nodepools
deads2k Apr 30, 2026
605f0ae
add test for nodepool admission subnet
deads2k Apr 30, 2026
a9d34fe
address subnet pointer comparison and a few docs
deads2k May 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 39 additions & 104 deletions frontend/pkg/frontend/node_pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
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"
ocmerrors "github.com/openshift-online/ocm-sdk-go/errors"

"github.com/Azure/ARO-HCP/internal/admission"
Expand Down Expand Up @@ -74,7 +73,6 @@ func (f *Frontend) GetNodePool(writer http.ResponseWriter, request *http.Request

func (f *Frontend) ArmResourceListNodePools(writer http.ResponseWriter, request *http.Request) error {
ctx := request.Context()
logger := utils.LoggerFromContext(ctx)

versionedInterface, err := VersionFromContext(ctx)
if err != nil {
Expand All @@ -85,23 +83,19 @@ func (f *Frontend) ArmResourceListNodePools(writer http.ResponseWriter, request
resourceGroupName := request.PathValue(PathSegmentResourceGroupName)
clusterName := request.PathValue(PathSegmentResourceName)

internalCluster, err := f.dbClient.HCPClusters(subscriptionID, resourceGroupName).Get(ctx, clusterName)
if err != nil {
return utils.TrackError(err)
}
if internalCluster.ServiceProviderProperties.ClusterServiceID == nil {
return utils.TrackError(fmt.Errorf("cluster %s has no ClusterServiceID", internalCluster.ID))
}

pagedResponse := arm.NewPagedResponse()

nodePoolsByClusterServiceID := make(map[string]*api.HCPOpenShiftClusterNodePool)
internalNodePoolIterator, err := f.dbClient.HCPClusters(subscriptionID, resourceGroupName).NodePools(clusterName).List(ctx, dbListOptionsFromRequest(request))
if err != nil {
return utils.TrackError(err)
Comment on lines 92 to 96

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ArmResourceListNodePools no longer verifies that the parent cluster exists before listing node pools. Because the Cosmos List() call is prefix-based, this will return 200 with an empty list for a non-existent cluster rather than a 404 (and differs from ArmResourceListExternalAuths which does a parent Get first). Consider fetching the cluster from storage first (for existence) and returning NotFound when absent.

Copilot uses AI. Check for mistakes.
}
for _, nodePool := range internalNodePoolIterator.Items(ctx) {
nodePoolsByClusterServiceID[nodePool.ServiceProviderProperties.ClusterServiceID.ID()] = nodePool
resultingExternalNodePool := versionedInterface.NewHCPOpenShiftClusterNodePool(nodePool)
jsonBytes, err := arm.MarshalJSON(resultingExternalNodePool)
if err != nil {
return utils.TrackError(err)
}
pagedResponse.AddValue(jsonBytes)
}
err = internalNodePoolIterator.GetError()
if err != nil {
Expand All @@ -114,35 +108,6 @@ func (f *Frontend) ArmResourceListNodePools(writer http.ResponseWriter, request
return utils.TrackError(err)
}

// Build a Cluster Service query that looks for
// the specific IDs returned by the Cosmos query.
queryIDs := make([]string, 0, len(nodePoolsByClusterServiceID))
for key := range nodePoolsByClusterServiceID {
queryIDs = append(queryIDs, "'"+key+"'")
}
query := fmt.Sprintf("id in (%s)", strings.Join(queryIDs, ", "))
logger.Info(fmt.Sprintf("Searching Cluster Service for %q", query))

csIterator := f.clusterServiceClient.ListNodePools(*internalCluster.ServiceProviderProperties.ClusterServiceID, query)
for csNodePool := range csIterator.Items(ctx) {
if internalNodePool, ok := nodePoolsByClusterServiceID[csNodePool.ID()]; ok {
internalNodePool, err = mergeToInternalNodePool(csNodePool, internalNodePool, f.azureLocation)
if err != nil {
return utils.TrackError(err)
}
resultingExternalNodePool := versionedInterface.NewHCPOpenShiftClusterNodePool(internalNodePool)
jsonBytes, err := arm.MarshalJSON(resultingExternalNodePool)
if err != nil {
return utils.TrackError(err)
}
pagedResponse.AddValue(jsonBytes)
}
}
// Check for iteration error.
if err := csIterator.GetError(); err != nil {
return utils.TrackError(err)
}

_, err = arm.WriteJSONResponse(writer, http.StatusOK, pagedResponse)
if err != nil {
return utils.TrackError(err)
Expand Down Expand Up @@ -180,11 +145,6 @@ func (f *Frontend) CreateOrUpdateNodePool(writer http.ResponseWriter, request *h

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 utils.TrackError(err)
}
if err := checkForProvisioningStateConflict(ctx, f.dbClient, database.OperationRequestUpdate, oldInternalNodePool.ID, oldInternalNodePool.Properties.ProvisioningState); err != nil {
return utils.TrackError(err)
}
Expand Down Expand Up @@ -254,6 +214,17 @@ func decodeDesiredNodePoolCreate(ctx context.Context, azureLocation string) (*ap
return newInternalNodePool, nil
}

func (f *Frontend) newNodePoolAdmissionContext(ctx context.Context, nodePoolResourceID *azcorearm.ResourceID) (*admission.NodePoolAdmissionContext, error) {
cluster, err := f.getInternalClusterFromStorage(ctx, nodePoolResourceID.Parent)
if err != nil {
return nil, utils.TrackError(err)
}

return &admission.NodePoolAdmissionContext{
Cluster: cluster,
}, nil
Comment thread
miguelsorianod marked this conversation as resolved.
Resolved
}
Comment on lines +223 to +227

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newNodePoolAdmissionContext currently doesn’t use ctx and always returns nil error, which adds indirection/signature complexity without functional value. Consider returning *admission.NodePoolAdmissionContext directly (no error) and dropping ctx, or add the intended error-producing logic now (e.g., context-building that can actually fail).

Copilot uses AI. Check for mistakes.

func (f *Frontend) createNodePool(writer http.ResponseWriter, request *http.Request) error {
ctx := request.Context()
logger := utils.LoggerFromContext(ctx)
Expand Down Expand Up @@ -291,11 +262,19 @@ func (f *Frontend) createNodePool(writer http.ResponseWriter, request *http.Requ
return utils.TrackError(fmt.Errorf("cluster %s has no ClusterServiceID", cluster.ID))
}

validationOp := operation.Operation{
admissionContext, err := f.newNodePoolAdmissionContext(ctx, resourceID)
if err != nil {
return utils.TrackError(err)
}
restOperation := operation.Operation{
Type: operation.Create,
Options: validation.AFECsToValidationOptions(subscription.GetRegisteredFeatures()),
}
validationErrs := validation.ValidateNodePool(ctx, validationOp, newInternalNodePool, nil)
if mutationErrs := admission.MutateNodePool(ctx, admissionContext, restOperation, newInternalNodePool, nil); len(mutationErrs) > 0 {
return utils.TrackError(arm.CloudErrorFromFieldErrors(mutationErrs))
}

validationErrs := validation.ValidateNodePool(ctx, restOperation, newInternalNodePool, nil)
// in addition to static validation, we have validation based on the state of the hcp cluster
validationErrs = append(validationErrs, admission.AdmitNodePool(newInternalNodePool, nil, cluster)...)
if err := arm.CloudErrorFromFieldErrors(validationErrs); err != nil {
Expand Down Expand Up @@ -363,12 +342,6 @@ func (f *Frontend) createNodePool(writer http.ResponseWriter, request *http.Requ
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, f.azureLocation)
Comment thread
deads2k marked this conversation as resolved.
Resolved
if err != nil {
return utils.TrackError(err)
}
responseBytes, err := arm.MarshalJSON(versionedInterface.NewHCPOpenShiftClusterNodePool(resultingInternalNodePool))
if err != nil {
return utils.TrackError(err)
Expand Down Expand Up @@ -569,15 +542,22 @@ func (f *Frontend) updateNodePoolInCosmos(ctx context.Context, writer http.Respo
return utils.TrackError(err)
}

validationOp := operation.Operation{
admissionContext, err := f.newNodePoolAdmissionContext(ctx, oldInternalNodePool.ID)
if err != nil {
return utils.TrackError(err)
}
restOperation := operation.Operation{
Type: operation.Update,
Options: validation.AFECsToValidationOptions(subscription.GetRegisteredFeatures()),
}
if mutationErrs := admission.MutateNodePool(ctx, admissionContext, restOperation, newInternalNodePool, nil); len(mutationErrs) > 0 {
Comment thread
miguelsorianod marked this conversation as resolved.
Resolved
return utils.TrackError(arm.CloudErrorFromFieldErrors(mutationErrs))
}

validationErrs := validation.ValidateNodePool(ctx, validationOp, newInternalNodePool, oldInternalNodePool)
validationErrs := validation.ValidateNodePool(ctx, restOperation, newInternalNodePool, oldInternalNodePool)
// in addition to static validation, we have validation based on the state of the hcp cluster
// AdmitNodePoolUpdate includes AdmitNodePool checks plus version upgrade validation
validationErrs = append(validationErrs, admission.AdmitNodePoolUpdate(newInternalNodePool, oldInternalNodePool, cluster, spNodePool, spCluster, validationOp)...)
validationErrs = append(validationErrs, admission.AdmitNodePoolUpdate(newInternalNodePool, oldInternalNodePool, cluster, spNodePool, spCluster, restOperation)...)
if err := arm.CloudErrorFromFieldErrors(validationErrs); err != nil {
return utils.TrackError(err)
}
Expand All @@ -588,7 +568,7 @@ func (f *Frontend) updateNodePoolInCosmos(ctx context.Context, writer http.Respo
}

logger.Info(fmt.Sprintf("updating resource %s", oldInternalNodePool.ID))
csNodePool, err := f.clusterServiceClient.UpdateNodePool(ctx, oldInternalNodePool.ServiceProviderProperties.ClusterServiceID, csNodePoolBuilder)
_, err = f.clusterServiceClient.UpdateNodePool(ctx, oldInternalNodePool.ServiceProviderProperties.ClusterServiceID, csNodePoolBuilder)
if err != nil {
return utils.TrackError(err)
}
Expand Down Expand Up @@ -647,11 +627,6 @@ func (f *Frontend) updateNodePoolInCosmos(ctx context.Context, writer http.Respo
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, f.azureLocation)
if err != nil {
return utils.TrackError(err)
}
responseBytes, err := arm.MarshalJSON(versionedInterface.NewHCPOpenShiftClusterNodePool(resultingInternalNodePool))
if err != nil {
return utils.TrackError(err)
Expand Down Expand Up @@ -774,29 +749,6 @@ func (f *Frontend) addDeleteNodePoolToTransaction(ctx context.Context, writer ht
return nil
}

// the necessary conversions for the API version of the request.
// TODO remove azureLocation once we have migrated all records to store the azureLocation
func mergeToInternalNodePool(clusterServiceNode *arohcpv1alpha1.NodePool, internalNodePool *api.HCPOpenShiftClusterNodePool, azureLocation string) (*api.HCPOpenShiftClusterNodePool, error) {
mergedOldClusterServiceNodePool, err := ocm.ConvertCStoNodePool(internalNodePool.ID, azureLocation, clusterServiceNode)
if err != nil {
return nil, utils.TrackError(err)
}

// this does not use conversion.CopyReadOnly* because some ServiceProvider properties come from cluster-service-only or live reads
mergedOldClusterServiceNodePool.SystemData = internalNodePool.SystemData.DeepCopy()
mergedOldClusterServiceNodePool.Tags = maps.Clone(internalNodePool.Tags)
mergedOldClusterServiceNodePool.Properties.ProvisioningState = internalNodePool.Properties.ProvisioningState
mergedOldClusterServiceNodePool.ServiceProviderProperties = *internalNodePool.ServiceProviderProperties.DeepCopy()

// Properties.Version is the desired version; read it from Cosmos when set. Cluster Service reflects the
// actual node pool version and is updated when the upgrade completes.
if len(internalNodePool.Properties.Version.ID) > 0 {
mergedOldClusterServiceNodePool.Properties.Version = internalNodePool.Properties.Version
}

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.IsNotFoundError(err) {
Expand Down Expand Up @@ -825,23 +777,6 @@ func (f *Frontend) getInternalNodePoolFromStorage(ctx context.Context, resourceI
}
internalNodePool.ID = resourceID

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.
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, utils.TrackError(err)
}

// TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos
oldInternalNodePool, err = mergeToInternalNodePool(oldClusterServiceNodePool, oldInternalNodePool, f.azureLocation)
if err != nil {
return nil, utils.TrackError(err)
}
return internalNodePool, nil

return oldInternalNodePool, nil
}
35 changes: 35 additions & 0 deletions internal/admission/admit_nodepool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,54 @@
package admission

import (
"context"
"fmt"
"strings"

"github.com/blang/semver/v4"

"k8s.io/apimachinery/pkg/api/operation"
"k8s.io/apimachinery/pkg/api/safe"
"k8s.io/apimachinery/pkg/util/validation/field"

"github.com/Azure/ARO-HCP/internal/api"
"github.com/Azure/ARO-HCP/internal/utils/apihelpers"
"github.com/Azure/ARO-HCP/internal/validation"
)

type NodePoolAdmissionContext struct {
Cluster *api.HCPOpenShiftCluster
}

Comment on lines 32 to +38

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NodePoolAdmissionContext and MutateNodePool are newly exported but lack GoDoc comments. Add doc comments (starting with the identifier name) to avoid lint failures and to make the new admission/mutation entry points discoverable for callers.

Suggested change
type NodePoolAdmissionContext struct {
Cluster *api.HCPOpenShiftCluster
}
// NodePoolAdmissionContext carries additional cluster context used during
// node pool admission and mutation.
type NodePoolAdmissionContext struct {
Cluster *api.HCPOpenShiftCluster
}
// MutateNodePool applies admission-time mutations to a node pool and returns
// any field errors encountered while mutating the object.

Copilot uses AI. Check for mistakes.
func MutateNodePool(ctx context.Context, admissionContext *NodePoolAdmissionContext, op operation.Operation, newObj, oldObj *api.HCPOpenShiftClusterNodePool) field.ErrorList {
errs := field.ErrorList{}

//Properties HCPOpenShiftClusterNodePoolProperties `json:"properties"`
errs = append(errs, mutateNodePoolProperties(ctx, admissionContext, op, field.NewPath("properties"), &newObj.Properties, safe.Field(oldObj, validation.ToNodePoolProperties))...)

return errs
}

func mutateNodePoolProperties(ctx context.Context, admissionContext *NodePoolAdmissionContext, op operation.Operation, fldPath *field.Path, newObj, oldObj *api.HCPOpenShiftClusterNodePoolProperties) field.ErrorList {
errs := field.ErrorList{}

errs = append(errs, mutateNodePoolPlatform(ctx, admissionContext, op, fldPath.Child("platform"), &newObj.Platform, safe.Field(oldObj, validation.ToNodePoolPropertiesPlatform))...)

return errs
}

func mutateNodePoolPlatform(ctx context.Context, admissionContext *NodePoolAdmissionContext, op operation.Operation, fldPath *field.Path, newObj, oldObj *api.NodePoolPlatformProfile) field.ErrorList {
errs := field.ErrorList{}

if op.Type == operation.Create {
if newObj.SubnetID == nil {
newObj.SubnetID = admissionContext.Cluster.CustomerProperties.Platform.DeepCopy().SubnetID
}
}

return errs
}

// AdmitNodePool performs non-static checks of nodepool. Checks that require more information than is contained inside of
// of the nodepool instance itself.
func AdmitNodePool(newNodePool, oldNodePool *api.HCPOpenShiftClusterNodePool, cluster *api.HCPOpenShiftCluster) field.ErrorList {
Expand Down
Loading
Loading