stop using cluster-service for frontend nodepool reads by deads2k · Pull Request #5058 · Azure/ARO-HCP

David Eads (deads2k) · 2026-04-28T14:17:26Z

this simplifies the read path and allows us to later disconnect the write path.

Miguel Soriano (miguelsorianod) · 2026-04-28T16:02:35Z

/hold see #5058 (comment)

David Eads (deads2k) · 2026-04-28T17:11:32Z

/hold see #5058 (comment)

now addressed. Subnet is set via admission.

David Eads (deads2k) · 2026-04-28T17:15:44Z

/hold cancel

Copilot

Pull request overview

This PR removes the dependency on cluster-service for frontend NodePool read/list flows, shifting the read path to rely on Cosmos-backed NodePool documents and adding admission-time defaulting so newly created NodePools persist subnetId directly.

Changes:

Frontend NodePool list/get now serialize NodePools directly from Cosmos, removing cluster-service merge logic.
Added NodePool admission mutation to default a missing NodePool subnetId on create to the parent cluster subnet, plus unit tests.
Updated integration/version compliance artifacts to reflect subnetId now being present in NodePool payloads.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`frontend/pkg/frontend/node_pool.go`	Removes cluster-service read/merge from list/get and wires in admission mutation before validation on create/update.
`internal/admission/admit_nodepool.go`	Introduces NodePool mutation that defaults SubnetID on create using cluster context.
`internal/admission/admit_nodepool_test.go`	Adds unit coverage for subnet defaulting behavior in mutation.
`internal/validation/validate_nodepools.go`	Exposes helper accessors used by admission via `safe.Field`.
`internal/api/types_nodepool.go`	Updates NodePool platform comments around subnet/AZ behavior.
`test-integration/frontend/artifacts/VersionCompliance/NodePool/basic-nodepool/expected/list/2025-12-23-preview.json`	Updates expected list payload with `subnetId`.
`test-integration/frontend/artifacts/VersionCompliance/NodePool/basic-nodepool/expected/list/2024-06-10-preview.json`	Updates expected list payload with `subnetId`.
`test-integration/frontend/artifacts/VersionCompliance/NodePool/basic-nodepool/expected/get/2025-12-23-preview.json`	Updates expected get payload with `subnetId`.
`test-integration/frontend/artifacts/VersionCompliance/NodePool/basic-nodepool/expected/get/2024-06-10-preview.json`	Updates expected get payload with `subnetId`.
`test-integration/frontend/artifacts/FrontendCRUD/NodePool/create-current/06-cosmosCompare-confirm-content/nodepool-node-pool-02.json`	Updates expected Cosmos compare content with `subnetId`.
`test-integration/frontend/artifacts/FrontendCRUD/NodePool/create-current/06-cosmosCompare-confirm-content/nodepool-basic-node-pool.json`	Updates expected Cosmos compare content with `subnetId`.
`test-integration/frontend/artifacts/FrontendCRUD/Cluster/delete-with-pending-nodepool-operation/05-cosmosCompare-final-state/nodepool-test-nodepool.json`	Updates expected final Cosmos state with `subnetId`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-29T03:04:34Z

 var (
 	toNodePoolTrackedResource = func(oldObj *api.HCPOpenShiftClusterNodePool) *arm.TrackedResource { return &oldObj.TrackedResource }
-	toNodePoolProperties      = func(oldObj *api.HCPOpenShiftClusterNodePool) *api.HCPOpenShiftClusterNodePoolProperties {
+	ToNodePoolProperties      = func(oldObj *api.HCPOpenShiftClusterNodePool) *api.HCPOpenShiftClusterNodePoolProperties {
 		return &oldObj.Properties
 	}


ToNodePoolProperties is now an exported package-level variable holding a function. Consider making this an exported function instead (or keep it unexported and add a dedicated accessor) so other packages can’t accidentally reassign it, and to better communicate intent.

Copilot · 2026-04-29T03:04:34Z

 	}
-	toNodePoolPropertiesPlatform = func(oldObj *api.HCPOpenShiftClusterNodePoolProperties) *api.NodePoolPlatformProfile {
+	ToNodePoolPropertiesPlatform = func(oldObj *api.HCPOpenShiftClusterNodePoolProperties) *api.NodePoolPlatformProfile {
 		return &oldObj.Platform
 	}


Same concern as ToNodePoolProperties: ToNodePoolPropertiesPlatform is exported as a mutable package-level variable. Prefer an exported function (or other non-assignable API) to avoid accidental reassignment and make the API surface safer.

Copilot · 2026-04-29T03:04:34Z

 // NodePoolPlatformProfile represents a worker node pool configuration.
 // Visibility for the entire struct is "read create".
 type NodePoolPlatformProfile struct {
+	// Subnet can be nil and when it is nil, that means "use the cluster subnet field"


The SubnetID field comment is a bit misleading/ambiguous: mutation now defaults a nil SubnetID on CREATE to the cluster subnet, so it’s not really “nil means use the cluster subnet” at runtime. Suggest rewording to explicitly describe the defaulting behavior (and refer to SubnetID rather than “Subnet”).

Suggested change

// Subnet can be nil and when it is nil, that means "use the cluster subnet field"

// If SubnetID is nil on CREATE, mutation defaults it to the cluster subnet.

Copilot · 2026-04-29T03:04:34Z

 	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)


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.

Miguel Soriano (miguelsorianod) · 2026-04-29T12:49:13Z

+func TestMutateNodePool(t *testing.T) {
+	const (
+		clusterSubnet  = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/cluster-vnet/subnets/cluster-subnet"
+		nodePoolSubnet = "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.Network/virtualNetworks/np-vnet/subnets/np-subnet"


While reading this it made me think that we should have this validation, which is being performed on CS side:
" The subnet resource ID must belong to the same VNet as the parent cluster's subnet".

I think we are also missing API documentation about it at ARM level, because it's being enforced by CS but it's not documented at ARM API level.

There is additionally another restriction (this one is documented at ARM API level already) that we should enforce:

"Note that a subnet cannot be reused between ARO-HCP Clusters, however the same subnet can be used for NodePools of the same cluster"

There is additionally another restriction (this one is documented at ARM API level already) that we should enforce:

"Note that a subnet cannot be reused between ARO-HCP Clusters, however the same subnet can be used for NodePools of the same cluster"

Another PR. It likely requires a reservation type at the subscription level in the controllers and best effort in the frontend admission validation. How is clsuter-service ensuring it? Transaction lock?

While reading this it made me think that we should have this validation, which is being performed on CS side: " The subnet resource ID must belong to the same VNet as the parent cluster's subnet".

I think we are also missing API documentation about it at ARM level, because it's being enforced by CS but it's not documented at ARM API level.

Already present. pushed some tests showing it.

Another PR. It likely requires a reservation type at the subscription level in the controllers and best effort in the frontend admission validation. How is clsuter-service ensuring it? Transaction lock?

Yes. A postgresql AdvisoryLock is used where the key is the subnet resource id. Also it's important to notice that the check occurs when both creating a cluster and when creating a nodepool, and it needs to check whether other clusters already use it, but also whether other clusters' nodepools use it.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-30T20:09:56Z

+			oldObj: nodePoolWithSubnet(sameVNetSubnet),
+			newObj: nodePoolWithSubnet(sameVNetSubnet),


The test case name says “different VNet” but uses sameVNetSubnet for both oldObj and newObj, so it doesn’t cover the intended behavior. More importantly, the current AdmitNodePool change-detection uses pointer inequality (newSubnetID != oldSubnetID), which will treat equal-but-distinct *azcorearm.ResourceID values as “changed” and can incorrectly re-validate/reject updates (especially for legacy persisted data). Update the test to use differentVNetSubnet for both old/new (and expect no error when unchanged), and update AdmitNodePool to compare subnet IDs by value (e.g., normalized string/ID) rather than pointer identity.

Suggested change

oldObj: nodePoolWithSubnet(sameVNetSubnet),

newObj: nodePoolWithSubnet(sameVNetSubnet),

oldObj: nodePoolWithSubnet(differentVNetSubnet),

newObj: nodePoolWithSubnet(differentVNetSubnet),

Copilot · 2026-04-30T20:09:56Z

+func toNodePoolTrackedResource(oldObj *api.HCPOpenShiftClusterNodePool) *arm.TrackedResource {
+	return &oldObj.TrackedResource
+}
+


ToNodePoolProperties and ToNodePoolPropertiesPlatform are newly exported, but they don’t have GoDoc comments. If the repo runs linting (e.g., revive/golangci-lint) this typically fails CI; add doc comments starting with the function name, or keep them unexported and expose them another way.

Suggested change

// ToNodePoolProperties returns the properties of the provided node pool.

Copilot · 2026-04-30T20:09:56Z

+func ToNodePoolPropertiesPlatform(oldObj *api.HCPOpenShiftClusterNodePoolProperties) *api.NodePoolPlatformProfile {
+	return &oldObj.Platform
+}


ToNodePoolProperties and ToNodePoolPropertiesPlatform are newly exported, but they don’t have GoDoc comments. If the repo runs linting (e.g., revive/golangci-lint) this typically fails CI; add doc comments starting with the function name, or keep them unexported and expose them another way.

Copilot · 2026-04-30T20:09:57Z


+type NodePoolAdmissionContext struct {
+	Cluster *api.HCPOpenShiftCluster
+}
+


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 · 2026-04-30T20:09:57Z

+func (f *Frontend) newNodePoolAdmissionContext(ctx context.Context, cluster *api.HCPOpenShiftCluster) (*admission.NodePoolAdmissionContext, error) {
+	return &admission.NodePoolAdmissionContext{
+		Cluster: cluster,
+	}, nil
+}


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).

Miguel Soriano (miguelsorianod) · 2026-05-04T08:39:08Z

+	// Check only if it is a creating nodepool or a change in the Subnet.
+	// Compare by string value (not pointer identity) so equal-but-distinct
+	// *azcorearm.ResourceID values aren't treated as a change.
+	if newNodePool.Properties.Platform.SubnetID != nil && cluster.CustomerProperties.Platform.SubnetID != nil {


Is the cluster.CustomerProperties.Platform.SubnetID != nil check needed? The cluster's subnet is a required attribute on creation.

The bot wants it. We can keep arguing with it or deal with the cruft it leaves everywhere. I have no interest in bothering to argue.

I understand this only occurs when the content is changed so it shouldn't impact future PRs.

Seeing the nil check though implies this could be nil for some reason which makes it confusing to reason about.

I don't have a strong preference so non blocker for me.

Miguel Soriano (miguelsorianod) · 2026-05-04T11:27:18Z

/lgtm

openshift-ci · 2026-05-04T11:27:25Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stop using cluster-service for frontend nodepool reads

d37ae94

this simplifies the read path and allows us to later disconnect the write path.

openshift-ci Bot requested review from Gerd Oberlechner (geoberle) and Matthew Barnes (mbarnes) April 28, 2026 14:17

openshift-ci Bot added the approved label Apr 28, 2026

Miguel Soriano (miguelsorianod) reviewed Apr 28, 2026

View reviewed changes

Comment thread frontend/pkg/frontend/node_pool.go Outdated

Miguel Soriano (miguelsorianod) reviewed Apr 28, 2026

View reviewed changes

Comment thread internal/api/types_nodepool.go Outdated

Miguel Soriano (miguelsorianod) reviewed Apr 28, 2026

View reviewed changes

Comment thread frontend/pkg/frontend/node_pool.go

openshift-ci Bot added the do-not-merge/hold label Apr 28, 2026

clarify nodepool frontend behavior

0988da4

openshift-ci Bot removed the do-not-merge/hold label Apr 28, 2026

add admission mutation for nodepools

a3b87f7

Copilot AI review requested due to automatic review settings April 29, 2026 02:58

David Eads (deads2k) force-pushed the cs-161-nodepool-no-cluster-service branch from 410e122 to a3b87f7 Compare April 29, 2026 02:58

Copilot started reviewing on behalf of David Eads (deads2k) April 29, 2026 02:59 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

Miguel Soriano (miguelsorianod) reviewed Apr 29, 2026

View reviewed changes

David Eads (deads2k) mentioned this pull request Apr 29, 2026

Remove cluster-service to cosmos nodepool migration #5059

Merged

David Eads (deads2k) added 2 commits April 30, 2026 15:51

refine admission code for nodepools

7563f98

add test for nodepool admission subnet

605f0ae

Copilot AI review requested due to automatic review settings April 30, 2026 20:01

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Copilot started reviewing on behalf of David Eads (deads2k) April 30, 2026 20:22 View session

address subnet pointer comparison and a few docs

a9d34fe

Miguel Soriano (miguelsorianod) reviewed May 4, 2026

View reviewed changes

Comment thread internal/admission/admit_nodepool.go

openshift-ci Bot assigned Miguel Soriano (miguelsorianod) May 4, 2026

openshift-ci Bot added the lgtm label May 4, 2026

openshift-merge-bot Bot merged commit 5941153 into main May 4, 2026
15 checks passed

openshift-merge-bot Bot deleted the cs-161-nodepool-no-cluster-service branch May 4, 2026 14:20

Miguel Soriano (miguelsorianod) mentioned this pull request May 4, 2026

feat: stop using clusers-service for frontend externalauth reads #5110

Merged

	// Subnet can be nil and when it is nil, that means "use the cluster subnet field"
	// If SubnetID is nil on CREATE, mutation defaults it to the cluster subnet.

		oldObj: nodePoolWithSubnet(sameVNetSubnet),
		newObj: nodePoolWithSubnet(sameVNetSubnet),



	// ToNodePoolProperties returns the properties of the provided node pool.

Uh oh!

Conversation

David Eads (deads2k) commented Apr 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Miguel Soriano (miguelsorianod) commented Apr 28, 2026

Uh oh!

David Eads (deads2k) commented Apr 28, 2026

Uh oh!

David Eads (deads2k) commented Apr 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Miguel Soriano (miguelsorianod) Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Miguel Soriano (miguelsorianod) Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

David Eads (deads2k) Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

David Eads (deads2k) Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Miguel Soriano (miguelsorianod) May 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Miguel Soriano (miguelsorianod) May 4, 2026

Choose a reason for hiding this comment

Uh oh!

David Eads (deads2k) May 4, 2026

Choose a reason for hiding this comment

Uh oh!

Miguel Soriano (miguelsorianod) May 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Miguel Soriano (miguelsorianod) commented May 4, 2026

Uh oh!

Miguel Soriano (miguelsorianod) Apr 29, 2026 •

edited

Loading

Miguel Soriano (miguelsorianod) Apr 29, 2026 •

edited

Loading