From 8460950dc9c62ef99b3822c266985bad56fee947 Mon Sep 17 00:00:00 2001 From: Jakob Gray <20209054+JakobGray@users.noreply.github.com> Date: Mon, 6 Jul 2026 12:19:17 -0400 Subject: [PATCH 1/3] feat(frontend): reject duplicate subnet, NSG, and MRG on cluster create Cluster Service no longer returns synchronous 400s for platform uniqueness checks after async CS create migration. Move those checks to the frontend so invalid cluster PUTs fail at admission time with the same error messages CS used in performSpecValidation. These checks are best-effort and may not catch concurrent creates. - Prefetch subscription clusters and node pools in newClusterAdmissionContext - Add admitClusterManagedResourceGroupName, admitClusterSubnetResourceID, and admitClusterNetworkSecurityGroupResourceID (CREATE only) - Add NSG same-subscription and not-in-MRG rules to static validation (parity with validateAroHcpClusterNetworkSecurityGroupResourceId) --- frontend/pkg/frontend/cluster.go | 26 ++++ internal/admission/admit_cluster.go | 113 +++++++++++++++ internal/admission/admit_cluster_test.go | 129 ++++++++++++++++++ .../validation/hcpopenshiftcluster_test.go | 1 + internal/validation/validate_cluster.go | 5 + .../validate_cluster_comprehensive_test.go | 25 +++- .../create-with-4.19.json | 6 +- 7 files changed, 301 insertions(+), 4 deletions(-) diff --git a/frontend/pkg/frontend/cluster.go b/frontend/pkg/frontend/cluster.go index 3652e88b648..ee43f4a04a8 100644 --- a/frontend/pkg/frontend/cluster.go +++ b/frontend/pkg/frontend/cluster.go @@ -266,6 +266,32 @@ func (f *Frontend) newClusterAdmissionContext(ctx context.Context, op operation. OriginalCluster: originalCluster.DeepCopy(), } + if op.Type == operation.Create { + subscriptionID := originalCluster.ID.SubscriptionID + clusterIterator, err := f.resourcesDBClient.HCPClusters(subscriptionID, "").List(ctx, nil) + if err != nil { + return nil, fmt.Errorf("cannot list clusters for cluster admission: %w", err) + } + for _, cluster := range clusterIterator.Items(ctx) { + admissionContext.SubscriptionClusters = append(admissionContext.SubscriptionClusters, cluster) + + nodePoolIterator, err := f.resourcesDBClient.HCPClusters(subscriptionID, cluster.ID.ResourceGroupName).NodePools(cluster.ID.Name).List(ctx, nil) + if err != nil { + return nil, fmt.Errorf("cannot list node pools for cluster admission: %w", err) + } + for _, nodePool := range nodePoolIterator.Items(ctx) { + admissionContext.SubscriptionNodePools = append(admissionContext.SubscriptionNodePools, nodePool) + } + if err := nodePoolIterator.GetError(); err != nil { + return nil, fmt.Errorf("cannot list node pools for cluster admission: %w", err) + } + } + if err := clusterIterator.GetError(); err != nil { + return nil, fmt.Errorf("cannot list clusters for cluster admission: %w", err) + } + return admissionContext, nil + } + if op.Type != operation.Update { return admissionContext, nil } diff --git a/internal/admission/admit_cluster.go b/internal/admission/admit_cluster.go index 4fe23ed6c3e..802d4b778c1 100644 --- a/internal/admission/admit_cluster.go +++ b/internal/admission/admit_cluster.go @@ -53,6 +53,15 @@ type ClusterAdmissionContext struct { // ClusterNodePools is the list of node pools belonging to the cluster, used // for minor-version skew checks against the desired cluster version. ClusterNodePools []ClusterAdmissionNodePool + // SubscriptionClusters lists cluster documents in the same subscription, used + // for cross-cluster platform resource uniqueness on CREATE. + // The list is empty on UPDATE. + SubscriptionClusters []*api.HCPOpenShiftCluster + // SubscriptionNodePools lists node pool documents under SubscriptionClusters, + // used to ensure a cluster subnet is not already assigned to another cluster's + // node pool on CREATE. + // The list is empty on UPDATE. + SubscriptionNodePools []*api.HCPOpenShiftClusterNodePool } // ClusterAdmissionNodePool is a single node pool plus its prefetched service @@ -222,6 +231,110 @@ func admitClusterCustomerProperties(ctx context.Context, admissionContext *Clust errs := field.ErrorList{} errs = append(errs, admitClusterVersionProfile(ctx, admissionContext, op, fldPath.Child("version"), &newObj.Version, safe.Field(oldObj, validation.ToClusterCustomerPropertiesVersion))...) + platformPath := fldPath.Child("platform") + errs = append(errs, admitClusterManagedResourceGroupName(ctx, admissionContext, op, platformPath, &newObj.Platform)...) + errs = append(errs, admitClusterSubnetResourceID(ctx, admissionContext, op, platformPath, &newObj.Platform)...) + errs = append(errs, admitClusterNetworkSecurityGroupResourceID(ctx, admissionContext, op, platformPath, &newObj.Platform)...) + + return errs +} + +// admitClusterManagedResourceGroupName ensures the managed resource group name +// is unique within the subscription on CREATE. +// +// Best-effort only: compares against SubscriptionClusters prefetched before +// admission runs. Concurrent creates with the same MRG name can both succeed. +func admitClusterManagedResourceGroupName(_ context.Context, admissionContext *ClusterAdmissionContext, op operation.Operation, fldPath *field.Path, newObj *api.CustomerPlatformProfile) field.ErrorList { + if op.Type != operation.Create || len(newObj.ManagedResourceGroup) == 0 { + return nil + } + + subscriptionID := admissionContext.OriginalCluster.ID.SubscriptionID + var errs field.ErrorList + + for _, existing := range admissionContext.SubscriptionClusters { + if strings.EqualFold(newObj.ManagedResourceGroup, existing.CustomerProperties.Platform.ManagedResourceGroup) { + errs = append(errs, field.Invalid( + fldPath.Child("managedResourceGroup"), + newObj.ManagedResourceGroup, + fmt.Sprintf("Cluster with managed resource group name '%s' in subscription '%s' "+ + "already exists, please provide a unique managed resource group name", + newObj.ManagedResourceGroup, subscriptionID), + )) + break + } + } + + return errs +} + +// admitClusterSubnetResourceID ensures that the subnet ID is not already in use by any other +// cluster or node pool within the same subscription when creating a new cluster. +// +// Best-effort only: compares against SubscriptionClusters and SubscriptionNodePools +// prefetched before admission runs. Concurrent creates (or a create racing with a +// node pool create) using the same subnet can both succeed. +func admitClusterSubnetResourceID(_ context.Context, admissionContext *ClusterAdmissionContext, op operation.Operation, fldPath *field.Path, newObj *api.CustomerPlatformProfile) field.ErrorList { + if op.Type != operation.Create || newObj.SubnetID == nil { + return nil + } + + subnetPath := fldPath.Child("subnetId") + subnetID := newObj.SubnetID.String() + var errs field.ErrorList + + for _, existing := range admissionContext.SubscriptionClusters { + existingSubnet := existing.CustomerProperties.Platform.SubnetID + if strings.EqualFold(subnetID, existingSubnet.String()) { + errs = append(errs, field.Invalid( + subnetPath, + subnetID, + fmt.Sprintf("Subnet '%s' is already in use by another cluster", subnetID), + )) + break + } + } + + for _, nodePool := range admissionContext.SubscriptionNodePools { + nodePoolSubnet := nodePool.Properties.Platform.SubnetID + if strings.EqualFold(subnetID, nodePoolSubnet.String()) { + errs = append(errs, field.Invalid( + subnetPath, + subnetID, + fmt.Sprintf("Subnet '%s' is already in use by another cluster", subnetID), + )) + break + } + } + + return errs +} + +// admitClusterNetworkSecurityGroupResourceID ensures that the network security group ID is not already in use by any other +// cluster within the same subscription when creating a new cluster. +// +// Best-effort only: compares against SubscriptionClusters prefetched before +// admission runs. Concurrent creates with the same NSG can both succeed. +func admitClusterNetworkSecurityGroupResourceID(_ context.Context, admissionContext *ClusterAdmissionContext, op operation.Operation, fldPath *field.Path, newObj *api.CustomerPlatformProfile) field.ErrorList { + if op.Type != operation.Create || newObj.NetworkSecurityGroupID == nil { + return nil + } + + nsgPath := fldPath.Child("networkSecurityGroupId") + nsgID := newObj.NetworkSecurityGroupID.String() + var errs field.ErrorList + + for _, existing := range admissionContext.SubscriptionClusters { + existingNSG := existing.CustomerProperties.Platform.NetworkSecurityGroupID + if strings.EqualFold(nsgID, existingNSG.String()) { + errs = append(errs, field.Invalid( + nsgPath, + nsgID, + fmt.Sprintf("Network Security Group '%s' is already in use by another cluster", nsgID), + )) + break + } + } return errs } diff --git a/internal/admission/admit_cluster_test.go b/internal/admission/admit_cluster_test.go index c48afc1f3c7..c48df217f98 100644 --- a/internal/admission/admit_cluster_test.go +++ b/internal/admission/admit_cluster_test.go @@ -684,3 +684,132 @@ func TestAdmitCluster_Update(t *testing.T) { }) } } + +func TestAdmitCluster_PlatformResourceIDs(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + const subscriptionID = "6b690bec-0c16-4ecb-8f67-781caf40bba7" + + subnetID := api.Must(azcorearm.ParseResourceID( + "/subscriptions/" + subscriptionID + "/resourceGroups/customer/providers/Microsoft.Network/virtualNetworks/vnet/subnets/cluster-subnet")) + otherSubnetID := api.Must(azcorearm.ParseResourceID( + "/subscriptions/" + subscriptionID + "/resourceGroups/other/providers/Microsoft.Network/virtualNetworks/vnet/subnets/other")) + nsgID := api.Must(azcorearm.ParseResourceID( + "/subscriptions/" + subscriptionID + "/resourceGroups/customer/providers/Microsoft.Network/networkSecurityGroups/cluster-nsg")) + otherNsgID := api.Must(azcorearm.ParseResourceID( + "/subscriptions/" + subscriptionID + "/resourceGroups/other/providers/Microsoft.Network/networkSecurityGroups/other-nsg")) + + makeCluster := func(name, managedResourceGroup string, subnet, nsg *azcorearm.ResourceID) *api.HCPOpenShiftCluster { + resourceID := api.Must(azcorearm.ParseResourceID(fmt.Sprintf( + "/subscriptions/%s/resourceGroups/rg/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/%s", + subscriptionID, name))) + return &api.HCPOpenShiftCluster{ + TrackedResource: arm.NewTrackedResource(resourceID, "eastus"), + CustomerProperties: api.HCPOpenShiftClusterCustomerProperties{ + Platform: api.CustomerPlatformProfile{ + ManagedResourceGroup: managedResourceGroup, + SubnetID: subnet, + NetworkSecurityGroupID: nsg, + }, + }, + } + } + + makeNodePool := func(clusterName, nodePoolName string, subnet *azcorearm.ResourceID) *api.HCPOpenShiftClusterNodePool { + resourceID := api.Must(azcorearm.ParseResourceID(fmt.Sprintf( + "/subscriptions/%s/resourceGroups/rg/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/%s/hcpOpenShiftClusterNodePools/%s", + subscriptionID, clusterName, nodePoolName))) + return &api.HCPOpenShiftClusterNodePool{ + TrackedResource: arm.NewTrackedResource(resourceID, "eastus"), + Properties: api.HCPOpenShiftClusterNodePoolProperties{ + Platform: api.NodePoolPlatformProfile{ + SubnetID: subnet, + }, + }, + } + } + + tests := []struct { + name string + subscriptionClusters []*api.HCPOpenShiftCluster + subscriptionNodePools []*api.HCPOpenShiftClusterNodePool + newCluster *api.HCPOpenShiftCluster + expectErrors []utils.ExpectedError + }{ + { + name: "create with empty subscription clusters", + subscriptionClusters: nil, + newCluster: makeCluster("new-cluster", "mrg-new", subnetID, nsgID), + expectErrors: []utils.ExpectedError{}, + }, + { + name: "create rejects duplicate subnet", + subscriptionClusters: []*api.HCPOpenShiftCluster{ + makeCluster("existing-cluster", "mrg-existing", subnetID, nsgID), + }, + newCluster: makeCluster("new-cluster", "mrg-new", subnetID, otherNsgID), + expectErrors: []utils.ExpectedError{ + {FieldPath: "properties.platform.subnetId", Message: "already in use by another cluster"}, + }, + }, + { + name: "create rejects duplicate network security group", + subscriptionClusters: []*api.HCPOpenShiftCluster{ + makeCluster("existing-cluster", "mrg-existing", subnetID, nsgID), + }, + newCluster: makeCluster("new-cluster", "mrg-new", otherSubnetID, nsgID), + expectErrors: []utils.ExpectedError{ + {FieldPath: "properties.platform.networkSecurityGroupId", Message: "already in use by another cluster"}, + }, + }, + { + name: "create rejects duplicate managed resource group", + subscriptionClusters: []*api.HCPOpenShiftCluster{ + makeCluster("existing-cluster", "shared-mrg", subnetID, nsgID), + }, + newCluster: makeCluster("new-cluster", "shared-mrg", otherSubnetID, otherNsgID), + expectErrors: []utils.ExpectedError{ + {FieldPath: "properties.platform.managedResourceGroup", Message: "please provide a unique managed resource group name"}, + }, + }, + { + name: "create rejects duplicate subnet used by node pool", + subscriptionClusters: []*api.HCPOpenShiftCluster{ + makeCluster("existing-cluster", "mrg-existing", otherSubnetID, nsgID), + }, + subscriptionNodePools: []*api.HCPOpenShiftClusterNodePool{ + makeNodePool("existing-cluster", "workers", subnetID), + }, + newCluster: makeCluster("new-cluster", "mrg-new", subnetID, otherNsgID), + expectErrors: []utils.ExpectedError{ + {FieldPath: "properties.platform.subnetId", Message: "already in use by another cluster"}, + }, + }, + { + name: "create allows distinct platform values", + subscriptionClusters: []*api.HCPOpenShiftCluster{ + makeCluster("existing-cluster", "mrg-existing", subnetID, nsgID), + }, + newCluster: makeCluster("new-cluster", "mrg-new", otherSubnetID, otherNsgID), + expectErrors: []utils.ExpectedError{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + admissionContext := &ClusterAdmissionContext{ + OriginalCluster: tt.newCluster.DeepCopy(), + SubscriptionClusters: tt.subscriptionClusters, + SubscriptionNodePools: tt.subscriptionNodePools, + } + + errs := AdmitCluster(ctx, admissionContext, operation.Operation{Type: operation.Create}, tt.newCluster, nil) + + utils.VerifyErrorsMatch(t, tt.expectErrors, errs) + }) + } +} diff --git a/internal/validation/hcpopenshiftcluster_test.go b/internal/validation/hcpopenshiftcluster_test.go index 8810a9c5e6a..de86a69a941 100644 --- a/internal/validation/hcpopenshiftcluster_test.go +++ b/internal/validation/hcpopenshiftcluster_test.go @@ -806,6 +806,7 @@ func TestClusterValidate(t *testing.T) { // Use a different resource group name to avoid a subnet ID error. SubnetID: api.Must(azcorearm.ParseResourceID(path.Join("/subscriptions", api.TestSubscriptionID, "resourceGroups", "anotherResourceGroup", "providers", "Microsoft.Network", "virtualNetworks", api.TestVirtualNetworkName, "subnets", api.TestSubnetName))), VnetIntegrationSubnetID: api.Must(azcorearm.ParseResourceID(path.Join("/subscriptions", api.TestSubscriptionID, "resourceGroups", "anotherResourceGroup", "providers", "Microsoft.Network", "virtualNetworks", api.TestVirtualNetworkName, "subnets", api.TestVnetIntegrationSubnetName))), + NetworkSecurityGroupID: api.Must(azcorearm.ParseResourceID(path.Join("/subscriptions", api.TestSubscriptionID, "resourceGroups", "anotherResourceGroup", "providers", "Microsoft.Network", "networkSecurityGroups", api.TestNetworkSecurityGroupName))), }, }, }, diff --git a/internal/validation/validate_cluster.go b/internal/validation/validate_cluster.go index b431c5bd6ff..1632e81fafc 100644 --- a/internal/validation/validate_cluster.go +++ b/internal/validation/validate_cluster.go @@ -175,6 +175,8 @@ func validateResourceIDsAgainstClusterID(ctx context.Context, op operation.Opera errs = append(errs, DifferentResourceGroupName(ctx, op, field.NewPath("customerProperties", "platform", "managedResourceGroup"), &newCluster.CustomerProperties.Platform.ManagedResourceGroup, nil, newCluster.ID.ResourceGroupName)...) errs = append(errs, SameSubscription(ctx, op, field.NewPath("customerProperties", "platform", "subnetId"), newCluster.CustomerProperties.Platform.SubnetID, nil, newCluster.ID.SubscriptionID)...) errs = append(errs, DifferentResourceGroupNameFromResourceID(ctx, op, field.NewPath("customerProperties", "platform", "subnetId"), newCluster.CustomerProperties.Platform.SubnetID, nil, newCluster.CustomerProperties.Platform.ManagedResourceGroup)...) + errs = append(errs, SameSubscription(ctx, op, field.NewPath("customerProperties", "platform", "networkSecurityGroupId"), newCluster.CustomerProperties.Platform.NetworkSecurityGroupID, nil, newCluster.ID.SubscriptionID)...) + errs = append(errs, DifferentResourceGroupNameFromResourceID(ctx, op, field.NewPath("customerProperties", "platform", "networkSecurityGroupId"), newCluster.CustomerProperties.Platform.NetworkSecurityGroupID, nil, newCluster.CustomerProperties.Platform.ManagedResourceGroup)...) errs = append(errs, SameSubscription(ctx, op, field.NewPath("customerProperties", "platform", "vnetIntegrationSubnetId"), newCluster.CustomerProperties.Platform.VnetIntegrationSubnetID, nil, newCluster.ID.SubscriptionID)...) for operatorName, operatorIdentity := range newCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ControlPlaneOperators { @@ -633,6 +635,9 @@ func validateCustomerPlatformProfile(ctx context.Context, op operation.Operation errs = append(errs, validate.RequiredPointer(ctx, op, fldPath.Child("networkSecurityGroupId"), newObj.NetworkSecurityGroupID, safe.Field(oldObj, toPlatformNetworkSecurityGroupID))...) errs = append(errs, immutableByReflect(ctx, op, fldPath.Child("networkSecurityGroupId"), newObj.NetworkSecurityGroupID, safe.Field(oldObj, toPlatformNetworkSecurityGroupID))...) errs = append(errs, RestrictedResourceIDWithResourceGroup(ctx, op, fldPath.Child("networkSecurityGroupId"), newObj.NetworkSecurityGroupID, safe.Field(oldObj, toPlatformNetworkSecurityGroupID), "Microsoft.Network/networkSecurityGroups")...) + // Note: SameSubscription and DifferentResourceGroupNameFromResourceID for + // networkSecurityGroupId are performed at the cluster peer-field level in + // validateResourceIDsAgainstClusterID. //OperatorsAuthentication OperatorsAuthenticationProfile `json:"operatorsAuthentication,omitempty"` errs = append(errs, immutableByReflect(ctx, op, fldPath.Child("operatorsAuthentication"), &newObj.OperatorsAuthentication, safe.Field(oldObj, toPlatformOperatorsAuthentication))...) diff --git a/internal/validation/validate_cluster_comprehensive_test.go b/internal/validation/validate_cluster_comprehensive_test.go index b73b9db4da3..4c230e1f5a3 100644 --- a/internal/validation/validate_cluster_comprehensive_test.go +++ b/internal/validation/validate_cluster_comprehensive_test.go @@ -369,7 +369,7 @@ func TestValidateClusterCreate(t *testing.T) { name: "wrong NSG resource type - create", cluster: func() *api.HCPOpenShiftCluster { c := createValidCluster() - c.CustomerProperties.Platform.NetworkSecurityGroupID = api.Must(azcorearm.ParseResourceID("/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/test-rg/providers/Microsoft.Network/virtualNetworks/test-vnet")) + c.CustomerProperties.Platform.NetworkSecurityGroupID = api.Must(azcorearm.ParseResourceID("/subscriptions/0465bc32-c654-41b8-8d87-9815d7abe8f6/resourceGroups/test-rg/providers/Microsoft.Network/virtualNetworks/test-vnet")) return c }(), expectErrors: []utils.ExpectedError{ @@ -744,6 +744,7 @@ func TestValidateClusterCreate(t *testing.T) { }(), expectErrors: []utils.ExpectedError{ {Message: "must not be the same resource group name", FieldPath: "customerProperties.platform.subnetId"}, + {Message: "must not be the same resource group name", FieldPath: "customerProperties.platform.networkSecurityGroupId"}, {Message: "must not be the same resource group name", FieldPath: "customerProperties.platform.vnetIntegrationSubnetId"}, {Message: "must not be the same resource group name", FieldPath: "customerProperties.platform.managedResourceGroup"}, {Message: "must not be the same resource group name", FieldPath: "customerProperties.platform.operatorsAuthentication.userAssignedIdentities.controlPlaneOperators[test-operator]"}, @@ -761,6 +762,28 @@ func TestValidateClusterCreate(t *testing.T) { {Message: "must be in the same Azure subscription", FieldPath: "customerProperties.platform.subnetId"}, }, }, + { + name: "network security group in different subscription - create", + cluster: func() *api.HCPOpenShiftCluster { + c := createValidCluster() + c.CustomerProperties.Platform.NetworkSecurityGroupID = api.Must(azcorearm.ParseResourceID("/subscriptions/different-sub/resourceGroups/test-rg/providers/Microsoft.Network/networkSecurityGroups/test-nsg")) + return c + }(), + expectErrors: []utils.ExpectedError{ + {Message: "must be in the same Azure subscription", FieldPath: "customerProperties.platform.networkSecurityGroupId"}, + }, + }, + { + name: "network security group in managed resource group - create", + cluster: func() *api.HCPOpenShiftCluster { + c := createValidCluster() + c.CustomerProperties.Platform.NetworkSecurityGroupID = api.Must(azcorearm.ParseResourceID("/subscriptions/0465bc32-c654-41b8-8d87-9815d7abe8f6/resourceGroups/managed-rg/providers/Microsoft.Network/networkSecurityGroups/test-nsg")) + return c + }(), + expectErrors: []utils.ExpectedError{ + {Message: "must not be the same resource group name", FieldPath: "customerProperties.platform.networkSecurityGroupId"}, + }, + }, { name: "vnet integration subnet is optional - create", cluster: func() *api.HCPOpenShiftCluster { diff --git a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features-no-afec/04-httpCreate-cluster-with-4.19/create-with-4.19.json b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features-no-afec/04-httpCreate-cluster-with-4.19/create-with-4.19.json index f2f6e196386..a7255f312dc 100644 --- a/test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features-no-afec/04-httpCreate-cluster-with-4.19/create-with-4.19.json +++ b/test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features-no-afec/04-httpCreate-cluster-with-4.19/create-with-4.19.json @@ -36,10 +36,10 @@ "serviceCidr": "172.30.0.0/16" }, "platform": { - "managedResourceGroup": "managed-resource-group-name", - "networkSecurityGroupId": "/subscriptions/a1b2c3d4-0000-0000-0000-afec00000000/resourceGroups/bar/providers/Microsoft.Network/networkSecurityGroups/nsg", + "managedResourceGroup": "managed-resource-group-create-with-419", + "networkSecurityGroupId": "/subscriptions/a1b2c3d4-0000-0000-0000-afec00000000/resourceGroups/bar/providers/Microsoft.Network/networkSecurityGroups/nsg-create-with-419", "outboundType": "LoadBalancer", - "subnetId": "/subscriptions/a1b2c3d4-0000-0000-0000-afec00000000/resourceGroups/bar/providers/Microsoft.Network/virtualNetworks/vnet/subnets/subnet" + "subnetId": "/subscriptions/a1b2c3d4-0000-0000-0000-afec00000000/resourceGroups/bar/providers/Microsoft.Network/virtualNetworks/vnet/subnets/subnet-create-with-419" }, "version": { "channelGroup": "stable", From ce624d57a47cf3aa3714c099ca856b8da8fda17c Mon Sep 17 00:00:00 2001 From: Jakob Gray <20209054+JakobGray@users.noreply.github.com> Date: Mon, 13 Jul 2026 14:20:02 -0400 Subject: [PATCH 2/3] Address comments --- internal/admission/admit_cluster.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/internal/admission/admit_cluster.go b/internal/admission/admit_cluster.go index 802d4b778c1..57298abb7aa 100644 --- a/internal/admission/admit_cluster.go +++ b/internal/admission/admit_cluster.go @@ -53,7 +53,8 @@ type ClusterAdmissionContext struct { // ClusterNodePools is the list of node pools belonging to the cluster, used // for minor-version skew checks against the desired cluster version. ClusterNodePools []ClusterAdmissionNodePool - // SubscriptionClusters lists cluster documents in the same subscription, used + // SubscriptionClusters lists cluster documents in the same subscription + // (not including the current cluster being admitted), used // for cross-cluster platform resource uniqueness on CREATE. // The list is empty on UPDATE. SubscriptionClusters []*api.HCPOpenShiftCluster @@ -231,14 +232,20 @@ func admitClusterCustomerProperties(ctx context.Context, admissionContext *Clust errs := field.ErrorList{} errs = append(errs, admitClusterVersionProfile(ctx, admissionContext, op, fldPath.Child("version"), &newObj.Version, safe.Field(oldObj, validation.ToClusterCustomerPropertiesVersion))...) - platformPath := fldPath.Child("platform") - errs = append(errs, admitClusterManagedResourceGroupName(ctx, admissionContext, op, platformPath, &newObj.Platform)...) - errs = append(errs, admitClusterSubnetResourceID(ctx, admissionContext, op, platformPath, &newObj.Platform)...) - errs = append(errs, admitClusterNetworkSecurityGroupResourceID(ctx, admissionContext, op, platformPath, &newObj.Platform)...) + errs = append(errs, admitClusterPlatform(ctx, admissionContext, op, fldPath.Child("platform"), &newObj.Platform)...) return errs } +func admitClusterPlatform(ctx context.Context, admissionContext *ClusterAdmissionContext, op operation.Operation, fldPath *field.Path, newObj *api.CustomerPlatformProfile) field.ErrorList { + errs := field.ErrorList{} + + errs = append(errs, admitClusterManagedResourceGroupName(ctx, admissionContext, op, fldPath, newObj)...) + errs = append(errs, admitClusterSubnetResourceID(ctx, admissionContext, op, fldPath, newObj)...) + errs = append(errs, admitClusterNetworkSecurityGroupResourceID(ctx, admissionContext, op, fldPath, newObj)...) + return errs +} + // admitClusterManagedResourceGroupName ensures the managed resource group name // is unique within the subscription on CREATE. // From aac6f9ed7fd892f5ab847c996a71986a28849669 Mon Sep 17 00:00:00 2001 From: Jakob Gray <20209054+JakobGray@users.noreply.github.com> Date: Mon, 13 Jul 2026 19:56:40 -0400 Subject: [PATCH 3/3] Add nil safety checks --- internal/admission/admit_cluster.go | 18 ++++++++++------ internal/admission/admit_cluster_test.go | 27 ++++++++++++++++++++++++ internal/validation/validate_cluster.go | 2 ++ 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/internal/admission/admit_cluster.go b/internal/admission/admit_cluster.go index 57298abb7aa..0df413ea5c8 100644 --- a/internal/admission/admit_cluster.go +++ b/internal/admission/admit_cluster.go @@ -30,6 +30,8 @@ import ( "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/ARO-HCP/internal/api" "github.com/Azure/ARO-HCP/internal/api/arm" "github.com/Azure/ARO-HCP/internal/utils/apihelpers" @@ -291,8 +293,7 @@ func admitClusterSubnetResourceID(_ context.Context, admissionContext *ClusterAd var errs field.ErrorList for _, existing := range admissionContext.SubscriptionClusters { - existingSubnet := existing.CustomerProperties.Platform.SubnetID - if strings.EqualFold(subnetID, existingSubnet.String()) { + if resourceIDsEqualFold(newObj.SubnetID, existing.CustomerProperties.Platform.SubnetID) { errs = append(errs, field.Invalid( subnetPath, subnetID, @@ -303,8 +304,7 @@ func admitClusterSubnetResourceID(_ context.Context, admissionContext *ClusterAd } for _, nodePool := range admissionContext.SubscriptionNodePools { - nodePoolSubnet := nodePool.Properties.Platform.SubnetID - if strings.EqualFold(subnetID, nodePoolSubnet.String()) { + if resourceIDsEqualFold(newObj.SubnetID, nodePool.Properties.Platform.SubnetID) { errs = append(errs, field.Invalid( subnetPath, subnetID, @@ -332,8 +332,7 @@ func admitClusterNetworkSecurityGroupResourceID(_ context.Context, admissionCont var errs field.ErrorList for _, existing := range admissionContext.SubscriptionClusters { - existingNSG := existing.CustomerProperties.Platform.NetworkSecurityGroupID - if strings.EqualFold(nsgID, existingNSG.String()) { + if resourceIDsEqualFold(newObj.NetworkSecurityGroupID, existing.CustomerProperties.Platform.NetworkSecurityGroupID) { errs = append(errs, field.Invalid( nsgPath, nsgID, @@ -346,6 +345,13 @@ func admitClusterNetworkSecurityGroupResourceID(_ context.Context, admissionCont return errs } +func resourceIDsEqualFold(a, b *azcorearm.ResourceID) bool { + if a == nil || b == nil { + return false + } + return strings.EqualFold(a.String(), b.String()) +} + // admitClusterVersionProfile runs admission checks when properties.version // changes (skew against active control-plane versions and existing node pool // minor skew). On CREATE there is no prior version to compare against, so this diff --git a/internal/admission/admit_cluster_test.go b/internal/admission/admit_cluster_test.go index c48df217f98..21ccfeb19eb 100644 --- a/internal/admission/admit_cluster_test.go +++ b/internal/admission/admit_cluster_test.go @@ -795,6 +795,33 @@ func TestAdmitCluster_PlatformResourceIDs(t *testing.T) { newCluster: makeCluster("new-cluster", "mrg-new", otherSubnetID, otherNsgID), expectErrors: []utils.ExpectedError{}, }, + { + name: "create with nil new platform resource IDs does not panic", + subscriptionClusters: []*api.HCPOpenShiftCluster{ + makeCluster("existing-cluster", "mrg-existing", subnetID, nsgID), + }, + newCluster: makeCluster("new-cluster", "mrg-new", nil, nil), + expectErrors: []utils.ExpectedError{}, + }, + { + name: "create with existing cluster missing platform resource IDs does not panic", + subscriptionClusters: []*api.HCPOpenShiftCluster{ + makeCluster("existing-cluster", "mrg-existing", nil, nil), + }, + newCluster: makeCluster("new-cluster", "mrg-new", subnetID, nsgID), + expectErrors: []utils.ExpectedError{}, + }, + { + name: "create with existing node pool missing subnet does not panic", + subscriptionClusters: []*api.HCPOpenShiftCluster{ + makeCluster("existing-cluster", "mrg-existing", otherSubnetID, nsgID), + }, + subscriptionNodePools: []*api.HCPOpenShiftClusterNodePool{ + makeNodePool("existing-cluster", "workers", nil), + }, + newCluster: makeCluster("new-cluster", "mrg-new", subnetID, otherNsgID), + expectErrors: []utils.ExpectedError{}, + }, } for _, tt := range tests { diff --git a/internal/validation/validate_cluster.go b/internal/validation/validate_cluster.go index 1632e81fafc..44b1b76aa40 100644 --- a/internal/validation/validate_cluster.go +++ b/internal/validation/validate_cluster.go @@ -173,6 +173,8 @@ func validateResourceIDsAgainstClusterID(ctx context.Context, op operation.Opera // Validate that managed resource group is different from cluster resource group errs = append(errs, DifferentResourceGroupName(ctx, op, field.NewPath("customerProperties", "platform", "managedResourceGroup"), &newCluster.CustomerProperties.Platform.ManagedResourceGroup, nil, newCluster.ID.ResourceGroupName)...) + // NOTE: Our admission logic expects that the subnet and network security group are in the same subscription as the cluster. + // If these validations are removed, the admission logic should also be updated. errs = append(errs, SameSubscription(ctx, op, field.NewPath("customerProperties", "platform", "subnetId"), newCluster.CustomerProperties.Platform.SubnetID, nil, newCluster.ID.SubscriptionID)...) errs = append(errs, DifferentResourceGroupNameFromResourceID(ctx, op, field.NewPath("customerProperties", "platform", "subnetId"), newCluster.CustomerProperties.Platform.SubnetID, nil, newCluster.CustomerProperties.Platform.ManagedResourceGroup)...) errs = append(errs, SameSubscription(ctx, op, field.NewPath("customerProperties", "platform", "networkSecurityGroupId"), newCluster.CustomerProperties.Platform.NetworkSecurityGroupID, nil, newCluster.ID.SubscriptionID)...)