Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 26 additions & 0 deletions frontend/pkg/frontend/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines +269 to +273
}
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)
}
}
Comment on lines +269 to +288
if err := clusterIterator.GetError(); err != nil {
Comment on lines +269 to +289
return nil, fmt.Errorf("cannot list clusters for cluster admission: %w", err)
}
return admissionContext, nil
}

if op.Type != operation.Update {
return admissionContext, nil
}
Expand Down
19 changes: 19 additions & 0 deletions frontend/pkg/frontend/frontend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,22 @@ func TestDeploymentPreflight(t *testing.T) {
"subnetId": api.TestSubnetResourceID,
"networkSecurityGroupId": api.TestNetworkSecurityGroupResourceID,
},
"etcd": map[string]any{
"dataEncryption": map[string]any{
"keyManagementMode": "CustomerManaged",
"customerManaged": map[string]any{
"encryptionType": "KMS",
"kms": map[string]any{
"visibility": "Public",
"activeKey": map[string]any{
"name": "test-key",
"vaultName": "test-vault",
"version": "test-version",
},
},
},
},
},
},
},
expectStatus: arm.DeploymentPreflightStatusSucceeded,
Expand Down Expand Up @@ -475,6 +491,9 @@ func TestDeploymentPreflight(t *testing.T) {
{message: "Unsupported value: \"invisible\": supported values: \"Private\", \"Public\"", target: "properties.api.visibility"},
{message: "Required value", target: "properties.platform.subnetId"},
{message: "Required value", target: "properties.platform.networkSecurityGroupId"},
{message: "Unsupported value: \"PlatformManaged\": supported values: \"CustomerManaged\"", target: "properties.etcd.dataEncryption.keyManagementMode"},
{message: "Required value", target: "properties.platform.subnetId"},
{message: "Required value", target: "properties.platform.networkSecurityGroupId"},
},
},
{
Expand Down
147 changes: 147 additions & 0 deletions internal/admission/admit_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,16 @@ 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
// (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
// 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we need to check this? don't we check that for nodepools the subnet must be part of the VNet of the cluster?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This replicates the ValidateSubnetNotUsedByAnotherClusterNodePools() validation in CS. Node pool subnet must be in the same VNet as its parent cluster, but VNet sharing across clusters is not prevented

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Regarding subnets, does the following summarize the behavior?

in CS:

  • We allow reusing VNets between clusters
  • We do not allow reusing Subnets between clusters
  • We enforce that the node pool subnets must belong to the parent cluster VNet
  • We allow reusing Subnets between Node Pools within the same cluster
  • We do not allow reusing Subnets between Node Pools across clusters

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That all sounds correct

}

// ClusterAdmissionNodePool is a single node pool plus its prefetched service
Expand Down Expand Up @@ -222,6 +232,143 @@ 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))...)
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.
//
// 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 {
return nil
}

if admissionContext.OriginalCluster == nil {
return field.ErrorList{field.InternalError(fldPath, errors.New("original cluster is required for admission"))}
}

mrgPath := fldPath.Child("managedResourceGroup")
if len(newObj.ManagedResourceGroup) == 0 {
return field.ErrorList{field.Required(mrgPath, "")}
}
Comment on lines +263 to +266

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(
mrgPath,
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 {
return nil
}

subnetPath := fldPath.Child("subnetId")
if newObj.SubnetID == nil {
return field.ErrorList{field.Required(subnetPath, "")}
}
Comment on lines +298 to +301
Comment on lines +298 to +301
subnetID := newObj.SubnetID.String()
var errs field.ErrorList

for _, existing := range admissionContext.SubscriptionClusters {
existingSubnet := existing.CustomerProperties.Platform.SubnetID
if existingSubnet == nil {
errs = append(errs, field.InternalError(subnetPath, errors.New("existing cluster is missing subnetId")))
continue
}
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 nodePoolSubnet == nil {
errs = append(errs, field.InternalError(subnetPath, errors.New("existing node pool is missing subnetId")))
continue
}
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 {
return nil
}

nsgPath := fldPath.Child("networkSecurityGroupId")
if newObj.NetworkSecurityGroupID == nil {
return field.ErrorList{field.Required(nsgPath, "")}
}
Comment on lines +350 to +353
Comment on lines +350 to +353
nsgID := newObj.NetworkSecurityGroupID.String()
var errs field.ErrorList

for _, existing := range admissionContext.SubscriptionClusters {
existingNSG := existing.CustomerProperties.Platform.NetworkSecurityGroupID
if existingNSG == nil {
errs = append(errs, field.InternalError(nsgPath, errors.New("existing cluster is missing networkSecurityGroupId")))
continue
}
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
}
Expand Down
164 changes: 164 additions & 0 deletions internal/admission/admit_cluster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -684,3 +684,167 @@ 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",
Comment thread
miguelsorianod marked this conversation as resolved.
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{},
},
{
name: "create with nil new platform resource IDs returns required errors",
subscriptionClusters: []*api.HCPOpenShiftCluster{
makeCluster("existing-cluster", "mrg-existing", subnetID, nsgID),
},
newCluster: makeCluster("new-cluster", "mrg-new", nil, nil),
expectErrors: []utils.ExpectedError{
{FieldPath: "properties.platform.subnetId", Message: "Required value"},
{FieldPath: "properties.platform.networkSecurityGroupId", Message: "Required value"},
},
},
{
name: "create with existing cluster missing platform resource IDs returns internal errors",
subscriptionClusters: []*api.HCPOpenShiftCluster{
makeCluster("existing-cluster", "mrg-existing", nil, nil),
},
newCluster: makeCluster("new-cluster", "mrg-new", subnetID, nsgID),
expectErrors: []utils.ExpectedError{
{FieldPath: "properties.platform.subnetId", Message: "existing cluster is missing subnetId"},
{FieldPath: "properties.platform.networkSecurityGroupId", Message: "existing cluster is missing networkSecurityGroupId"},
},
},
{
name: "create with existing node pool missing subnet returns internal error",
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{
{FieldPath: "properties.platform.subnetId", Message: "existing node pool is missing subnetId"},
},
},
}

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)
})
}
}
Loading