Fix priority between network and DHCP server

Do not allow to set both network and DHCP server details which will create conflict while creating new DHCP server on whether to use network name or DHCP server name

Author: dharaneeshvrd

api/v1beta2/ibmpowervscluster_types.go

@@ -42,7 +42,7 @@ type IBMPowerVSClusterSpec struct {
 	// 1. in the case of DHCPServer.Name is not set the name will be DHCPSERVER<CLUSTER_NAME>_Private.
 	// 2. if DHCPServer.Name is set the name will be DHCPSERVER<DHCPServer.Name>_Private.
 	// when Network.ID is set, its expected that there exist a network in PowerVS workspace with id or else system will give error.
-	// when Network.Name is set, system will first check for network with Name in PowerVS workspace, if not exist network will be created by DHCP service.
+	// when Network.Name is set, system will first check for network with Name in PowerVS workspace, if not exist system will check DHCP network with given Network.name, if that also not exist, it will create a new DHCP service and name will be DHCPSERVER<Network.Name>_Private.
 	// Network.RegEx is not yet supported and system will ignore the value.
 	Network IBMPowerVSResourceReference `json:"network"`
 

cloud/scope/powervs_cluster.go

@@ -872,10 +872,11 @@ func (s *PowerVSClusterScope) createServiceInstance(ctx context.Context) (*resou
 }
 
 // ReconcileNetwork reconciles network
-// If only IBMPowerVSCluster.Spec.Network is set, network would be validated and if exists already will get used as cluster’s network or a new network will be created via DHCP service.
+// If only IBMPowerVSCluster.Spec.Network is set, network would be validated and if exists already will get used as cluster’s network or DHCP network would be validated with this name if not exits then a new network will be created via DHCP service.
 // If only IBMPowerVSCluster.Spec.DHCPServer is set, DHCP server would be validated and if exists already, will use DHCP server’s network as cluster network. If not a new DHCP service will be created and it’s network will be used.
-// If both IBMPowerVSCluster.Spec.Network & IBMPowerVSCluster.Spec.DHCPServer is set, network and DHCP server would be validated and if both exists already then network is belongs to given DHCP server or not would be validated.
-// If both IBMPowerVSCluster.Spec.Network & IBMPowerVSCluster.Spec.DHCPServer is not set, by default DHCP service will be created to setup cluster's network.
+// Cannot set both IBMPowerVSCluster.Spec.Network & IBMPowerVSCluster.Spec.DHCPServer since it will cause collision during network creation if both are provided.
+// If both IBMPowerVSCluster.Spec.Network & IBMPowerVSCluster.Spec.DHCPServer is not set, by default DHCP service will be created with the cluster name to setup cluster's network.
+// Note: DHCP network name would be in `DHCPSERVER<Network.name or DHCPServer.name>_Private` this format.
 func (s *PowerVSClusterScope) ReconcileNetwork(ctx context.Context) (bool, error) {
 	log := ctrl.LoggerFrom(ctx)
 	if s.GetNetworkID() != nil {
@@ -958,12 +959,7 @@ func (s *PowerVSClusterScope) checkDHCPServer(ctx context.Context) (*string, err
 	}
 
 	// if user provides DHCP server name then we can use network name to match the existing DHCP server
-	var networkName string
-	if s.DHCPServer() != nil && s.DHCPServer().Name != nil {
-		networkName = dhcpNetworkName(*s.DHCPServer().Name)
-	} else {
-		networkName = dhcpNetworkName(s.InfraCluster())
-	}
+	networkName := dhcpNetworkName(*s.GetServiceName(infrav1beta2.ResourceTypeDHCPServer))
 
 	log.V(3).Info("Checking DHCP server's network list by network name", "name", networkName)
 	dhcpServers, err := s.IBMPowerVSClient.GetAllDHCPServers()
@@ -2491,11 +2487,14 @@ func (s *PowerVSClusterScope) GetServiceName(resourceType infrav1beta2.ResourceT
 			return ptr.To(fmt.Sprintf("%s-serviceInstance", s.InfraCluster()))
 		}
 		return s.ServiceInstance().Name
-	case infrav1beta2.ResourceTypeNetwork:
-		if s.Network() == nil || s.Network().Name == nil {
-			return ptr.To(fmt.Sprintf("DHCPSERVER%s_Private", s.InfraCluster()))
+	case infrav1beta2.ResourceTypeDHCPServer:
+		if s.DHCPServer() != nil && s.DHCPServer().Name != nil {
+			return s.DHCPServer().Name
 		}
-		return s.Network().Name
+		if s.Network() != nil && s.Network().Name != nil {
+			return s.Network().Name
+		}
+		return ptr.To(s.InfraCluster())
 	case infrav1beta2.ResourceTypeVPC:
 		if s.VPC() == nil || s.VPC().Name == nil {
 			return ptr.To(fmt.Sprintf("%s-vpc", s.InfraCluster()))
@@ -2506,11 +2505,6 @@ func (s *PowerVSClusterScope) GetServiceName(resourceType infrav1beta2.ResourceT
 			return ptr.To(fmt.Sprintf("%s-transitgateway", s.InfraCluster()))
 		}
 		return s.TransitGateway().Name
-	case infrav1beta2.ResourceTypeDHCPServer:
-		if s.DHCPServer() == nil || s.DHCPServer().Name == nil {
-			return ptr.To(s.InfraCluster())
-		}
-		return s.DHCPServer().Name
 	case infrav1beta2.ResourceTypeCOSInstance:
 		if s.COSInstance() == nil || s.COSInstance().Name == "" {
 			return ptr.To(fmt.Sprintf("%s-cosinstance", s.InfraCluster()))

cloud/scope/powervs_cluster_test.go

@@ -2766,22 +2766,6 @@ func TestGetServiceName(t *testing.T) {
 			},
 			expectedName: ptr.To("ServiceInstanceName"),
 		},
-		{
-			name:         "Resource type is network and Network is nil",
-			resourceType: infrav1beta2.ResourceTypeNetwork,
-			clusterScope: PowerVSClusterScope{
-				IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ObjectMeta: metav1.ObjectMeta{Name: "ClusterName"}},
-			},
-			expectedName: ptr.To("DHCPSERVERClusterName_Private"),
-		},
-		{
-			name:         "Resource type is network and Network is not nil",
-			resourceType: infrav1beta2.ResourceTypeNetwork,
-			clusterScope: PowerVSClusterScope{
-				IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Spec: infrav1beta2.IBMPowerVSClusterSpec{Network: infrav1beta2.IBMPowerVSResourceReference{Name: ptr.To("NetworkName")}}},
-			},
-			expectedName: ptr.To("NetworkName"),
-		},
 		{
 			name:         "Resource type is vpc and VPC is nil",
 			resourceType: infrav1beta2.ResourceTypeVPC,
@@ -2830,6 +2814,14 @@ func TestGetServiceName(t *testing.T) {
 			},
 			expectedName: ptr.To("DHCPServerName"),
 		},
+		{
+			name:         "Resource type is dhcp server and dhcpserver is not nil and network is not nil",
+			resourceType: infrav1beta2.ResourceTypeDHCPServer,
+			clusterScope: PowerVSClusterScope{
+				IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Spec: infrav1beta2.IBMPowerVSClusterSpec{Network: infrav1beta2.IBMPowerVSResourceReference{Name: ptr.To("NetworkName")}}},
+			},
+			expectedName: ptr.To("NetworkName"),
+		},
 		{
 			name:         "Resource type is cos instance and cos instance is nil",
 			resourceType: infrav1beta2.ResourceTypeCOSInstance,

config/crd/bases/infrastructure.cluster.x-k8s.io_ibmpowervsclusters.yaml

@@ -462,7 +462,7 @@ spec:
                   1. in the case of DHCPServer.Name is not set the name will be DHCPSERVER<CLUSTER_NAME>_Private.
                   2. if DHCPServer.Name is set the name will be DHCPSERVER<DHCPServer.Name>_Private.
                   when Network.ID is set, its expected that there exist a network in PowerVS workspace with id or else system will give error.
-                  when Network.Name is set, system will first check for network with Name in PowerVS workspace, if not exist network will be created by DHCP service.
+                  when Network.Name is set, system will first check for network with Name in PowerVS workspace, if not exist system will check DHCP network with given Network.name, if that also not exist, it will create a new DHCP service and name will be DHCPSERVER<Network.Name>_Private.
                   Network.RegEx is not yet supported and system will ignore the value.
                 properties:
                   id:

config/crd/bases/infrastructure.cluster.x-k8s.io_ibmpowervsclustertemplates.yaml

@@ -499,7 +499,7 @@ spec:
                           1. in the case of DHCPServer.Name is not set the name will be DHCPSERVER<CLUSTER_NAME>_Private.
                           2. if DHCPServer.Name is set the name will be DHCPSERVER<DHCPServer.Name>_Private.
                           when Network.ID is set, its expected that there exist a network in PowerVS workspace with id or else system will give error.
-                          when Network.Name is set, system will first check for network with Name in PowerVS workspace, if not exist network will be created by DHCP service.
+                          when Network.Name is set, system will first check for network with Name in PowerVS workspace, if not exist system will check DHCP network with given Network.name, if that also not exist, it will create a new DHCP service and name will be DHCPSERVER<Network.Name>_Private.
                           Network.RegEx is not yet supported and system will ignore the value.
                         properties:
                           id:

internal/webhooks/ibmpowervscluster_test.go

@@ -70,6 +70,66 @@ func TestIBMPowerVSCluster_create(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "Should error if both Network name and DHCP name are set",
+			powervsCluster: &infrav1beta2.IBMPowerVSCluster{
+				Spec: infrav1beta2.IBMPowerVSClusterSpec{
+					ServiceInstanceID: "capi-si-id",
+					Network: infrav1beta2.IBMPowerVSResourceReference{
+						Name: ptr.To("capi-net"),
+					},
+					DHCPServer: &infrav1beta2.DHCPServer{
+						Name: ptr.To("capi-dhcp"),
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should error if both Network id and DHCP name are set",
+			powervsCluster: &infrav1beta2.IBMPowerVSCluster{
+				Spec: infrav1beta2.IBMPowerVSClusterSpec{
+					ServiceInstanceID: "capi-si-id",
+					Network: infrav1beta2.IBMPowerVSResourceReference{
+						ID: ptr.To("capi-net-id"),
+					},
+					DHCPServer: &infrav1beta2.DHCPServer{
+						Name: ptr.To("capi-dhcp"),
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should error if both Network name and DHCP id are set",
+			powervsCluster: &infrav1beta2.IBMPowerVSCluster{
+				Spec: infrav1beta2.IBMPowerVSClusterSpec{
+					ServiceInstanceID: "capi-si-id",
+					Network: infrav1beta2.IBMPowerVSResourceReference{
+						Name: ptr.To("capi-net"),
+					},
+					DHCPServer: &infrav1beta2.DHCPServer{
+						ID: ptr.To("capi-dhcp-id"),
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "Should error if both Network id and DHCP id are set",
+			powervsCluster: &infrav1beta2.IBMPowerVSCluster{
+				Spec: infrav1beta2.IBMPowerVSClusterSpec{
+					ServiceInstanceID: "capi-si-id",
+					Network: infrav1beta2.IBMPowerVSResourceReference{
+						ID: ptr.To("capi-net-id"),
+					},
+					DHCPServer: &infrav1beta2.DHCPServer{
+						ID: ptr.To("capi-dhcp-id"),
+					},
+				},
+			},
+			wantErr: true,
+		},
 	}
 
 	for _, tc := range tests {