Skip to content

Commit 9414bc3

Browse files
dharaneeshvrddharaneeshvrd
authored
Fix priority between network and DHCP server (#2269)
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
1 parent 680b5d8 commit 9414bc3

7 files changed

+89
-37
lines changed

‎api/v1beta2/ibmpowervscluster_types.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ type IBMPowerVSClusterSpec struct {
4242
// 1. in the case of DHCPServer.Name is not set the name will be DHCPSERVER<CLUSTER_NAME>_Private.
4343
// 2. if DHCPServer.Name is set the name will be DHCPSERVER<DHCPServer.Name>_Private.
4444
// when Network.ID is set, its expected that there exist a network in PowerVS workspace with id or else system will give error.
45-
// 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.
45+
// 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.
4646
// Network.RegEx is not yet supported and system will ignore the value.
4747
Network IBMPowerVSResourceReference `json:"network"`
4848

‎cloud/scope/powervs_cluster.go‎

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -872,10 +872,11 @@ func (s *PowerVSClusterScope) createServiceInstance(ctx context.Context) (*resou
872872
}
873873

874874
// ReconcileNetwork reconciles network
875-
// 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.
875+
// 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.
876876
// 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.
877-
// 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.
878-
// If both IBMPowerVSCluster.Spec.Network & IBMPowerVSCluster.Spec.DHCPServer is not set, by default DHCP service will be created to setup cluster's network.
877+
// Cannot set both IBMPowerVSCluster.Spec.Network & IBMPowerVSCluster.Spec.DHCPServer since it will cause collision during network creation if both are provided.
878+
// 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.
879+
// Note: DHCP network name would be in `DHCPSERVER<Network.name or DHCPServer.name>_Private` this format.
879880
func (s *PowerVSClusterScope) ReconcileNetwork(ctx context.Context) (bool, error) {
880881
log := ctrl.LoggerFrom(ctx)
881882
if s.GetNetworkID() != nil {
@@ -958,12 +959,7 @@ func (s *PowerVSClusterScope) checkDHCPServer(ctx context.Context) (*string, err
958959
}
959960

960961
// if user provides DHCP server name then we can use network name to match the existing DHCP server
961-
var networkName string
962-
if s.DHCPServer() != nil && s.DHCPServer().Name != nil {
963-
networkName = dhcpNetworkName(*s.DHCPServer().Name)
964-
} else {
965-
networkName = dhcpNetworkName(s.InfraCluster())
966-
}
962+
networkName := dhcpNetworkName(*s.GetServiceName(infrav1beta2.ResourceTypeDHCPServer))
967963

968964
log.V(3).Info("Checking DHCP server's network list by network name", "name", networkName)
969965
dhcpServers, err := s.IBMPowerVSClient.GetAllDHCPServers()
@@ -2491,11 +2487,14 @@ func (s *PowerVSClusterScope) GetServiceName(resourceType infrav1beta2.ResourceT
24912487
return ptr.To(fmt.Sprintf("%s-serviceInstance", s.InfraCluster()))
24922488
}
24932489
return s.ServiceInstance().Name
2494-
case infrav1beta2.ResourceTypeNetwork:
2495-
if s.Network() == nil || s.Network().Name == nil {
2496-
return ptr.To(fmt.Sprintf("DHCPSERVER%s_Private", s.InfraCluster()))
2490+
case infrav1beta2.ResourceTypeDHCPServer:
2491+
if s.DHCPServer() != nil && s.DHCPServer().Name != nil {
2492+
return s.DHCPServer().Name
24972493
}
2498-
return s.Network().Name
2494+
if s.Network() != nil && s.Network().Name != nil {
2495+
return s.Network().Name
2496+
}
2497+
return ptr.To(s.InfraCluster())
24992498
case infrav1beta2.ResourceTypeVPC:
25002499
if s.VPC() == nil || s.VPC().Name == nil {
25012500
return ptr.To(fmt.Sprintf("%s-vpc", s.InfraCluster()))
@@ -2506,11 +2505,6 @@ func (s *PowerVSClusterScope) GetServiceName(resourceType infrav1beta2.ResourceT
25062505
return ptr.To(fmt.Sprintf("%s-transitgateway", s.InfraCluster()))
25072506
}
25082507
return s.TransitGateway().Name
2509-
case infrav1beta2.ResourceTypeDHCPServer:
2510-
if s.DHCPServer() == nil || s.DHCPServer().Name == nil {
2511-
return ptr.To(s.InfraCluster())
2512-
}
2513-
return s.DHCPServer().Name
25142508
case infrav1beta2.ResourceTypeCOSInstance:
25152509
if s.COSInstance() == nil || s.COSInstance().Name == "" {
25162510
return ptr.To(fmt.Sprintf("%s-cosinstance", s.InfraCluster()))

‎cloud/scope/powervs_cluster_test.go‎

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2766,22 +2766,6 @@ func TestGetServiceName(t *testing.T) {
27662766
},
27672767
expectedName: ptr.To("ServiceInstanceName"),
27682768
},
2769-
{
2770-
name: "Resource type is network and Network is nil",
2771-
resourceType: infrav1beta2.ResourceTypeNetwork,
2772-
clusterScope: PowerVSClusterScope{
2773-
IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ObjectMeta: metav1.ObjectMeta{Name: "ClusterName"}},
2774-
},
2775-
expectedName: ptr.To("DHCPSERVERClusterName_Private"),
2776-
},
2777-
{
2778-
name: "Resource type is network and Network is not nil",
2779-
resourceType: infrav1beta2.ResourceTypeNetwork,
2780-
clusterScope: PowerVSClusterScope{
2781-
IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Spec: infrav1beta2.IBMPowerVSClusterSpec{Network: infrav1beta2.IBMPowerVSResourceReference{Name: ptr.To("NetworkName")}}},
2782-
},
2783-
expectedName: ptr.To("NetworkName"),
2784-
},
27852769
{
27862770
name: "Resource type is vpc and VPC is nil",
27872771
resourceType: infrav1beta2.ResourceTypeVPC,
@@ -2830,6 +2814,14 @@ func TestGetServiceName(t *testing.T) {
28302814
},
28312815
expectedName: ptr.To("DHCPServerName"),
28322816
},
2817+
{
2818+
name: "Resource type is dhcp server and dhcpserver is not nil and network is not nil",
2819+
resourceType: infrav1beta2.ResourceTypeDHCPServer,
2820+
clusterScope: PowerVSClusterScope{
2821+
IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Spec: infrav1beta2.IBMPowerVSClusterSpec{Network: infrav1beta2.IBMPowerVSResourceReference{Name: ptr.To("NetworkName")}}},
2822+
},
2823+
expectedName: ptr.To("NetworkName"),
2824+
},
28332825
{
28342826
name: "Resource type is cos instance and cos instance is nil",
28352827
resourceType: infrav1beta2.ResourceTypeCOSInstance,

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -462,7 +462,7 @@ spec:
462462
1. in the case of DHCPServer.Name is not set the name will be DHCPSERVER<CLUSTER_NAME>_Private.
463463
2. if DHCPServer.Name is set the name will be DHCPSERVER<DHCPServer.Name>_Private.
464464
when Network.ID is set, its expected that there exist a network in PowerVS workspace with id or else system will give error.
465-
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.
465+
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.
466466
Network.RegEx is not yet supported and system will ignore the value.
467467
properties:
468468
id:

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -499,7 +499,7 @@ spec:
499499
1. in the case of DHCPServer.Name is not set the name will be DHCPSERVER<CLUSTER_NAME>_Private.
500500
2. if DHCPServer.Name is set the name will be DHCPSERVER<DHCPServer.Name>_Private.
501501
when Network.ID is set, its expected that there exist a network in PowerVS workspace with id or else system will give error.
502-
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.
502+
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.
503503
Network.RegEx is not yet supported and system will ignore the value.
504504
properties:
505505
id:

‎internal/webhooks/ibmpowervscluster.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,12 @@ func validateIBMPowerVSClusterNetwork(cluster *infrav1beta2.IBMPowerVSCluster) *
104104
if res, err := validateIBMPowerVSNetworkReference(cluster.Spec.Network); !res {
105105
return err
106106
}
107+
if (cluster.Spec.Network.Name != nil || cluster.Spec.Network.ID != nil) && (cluster.Spec.DHCPServer != nil && cluster.Spec.DHCPServer.Name != nil) {
108+
return field.Invalid(field.NewPath("spec.dhcpServer.name"), cluster.Spec.DHCPServer.Name, "either one of network or dhcpServer details can be provided")
109+
}
110+
if (cluster.Spec.Network.Name != nil || cluster.Spec.Network.ID != nil) && (cluster.Spec.DHCPServer != nil && cluster.Spec.DHCPServer.ID != nil) {
111+
return field.Invalid(field.NewPath("spec.dhcpServer.id"), cluster.Spec.DHCPServer.ID, "either one of network or dhcpServer details can be provided")
112+
}
107113
return nil
108114
}
109115

‎internal/webhooks/ibmpowervscluster_test.go‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,66 @@ func TestIBMPowerVSCluster_create(t *testing.T) {
7070
},
7171
wantErr: true,
7272
},
73+
{
74+
name: "Should error if both Network name and DHCP name are set",
75+
powervsCluster: &infrav1beta2.IBMPowerVSCluster{
76+
Spec: infrav1beta2.IBMPowerVSClusterSpec{
77+
ServiceInstanceID: "capi-si-id",
78+
Network: infrav1beta2.IBMPowerVSResourceReference{
79+
Name: ptr.To("capi-net"),
80+
},
81+
DHCPServer: &infrav1beta2.DHCPServer{
82+
Name: ptr.To("capi-dhcp"),
83+
},
84+
},
85+
},
86+
wantErr: true,
87+
},
88+
{
89+
name: "Should error if both Network id and DHCP name are set",
90+
powervsCluster: &infrav1beta2.IBMPowerVSCluster{
91+
Spec: infrav1beta2.IBMPowerVSClusterSpec{
92+
ServiceInstanceID: "capi-si-id",
93+
Network: infrav1beta2.IBMPowerVSResourceReference{
94+
ID: ptr.To("capi-net-id"),
95+
},
96+
DHCPServer: &infrav1beta2.DHCPServer{
97+
Name: ptr.To("capi-dhcp"),
98+
},
99+
},
100+
},
101+
wantErr: true,
102+
},
103+
{
104+
name: "Should error if both Network name and DHCP id are set",
105+
powervsCluster: &infrav1beta2.IBMPowerVSCluster{
106+
Spec: infrav1beta2.IBMPowerVSClusterSpec{
107+
ServiceInstanceID: "capi-si-id",
108+
Network: infrav1beta2.IBMPowerVSResourceReference{
109+
Name: ptr.To("capi-net"),
110+
},
111+
DHCPServer: &infrav1beta2.DHCPServer{
112+
ID: ptr.To("capi-dhcp-id"),
113+
},
114+
},
115+
},
116+
wantErr: true,
117+
},
118+
{
119+
name: "Should error if both Network id and DHCP id are set",
120+
powervsCluster: &infrav1beta2.IBMPowerVSCluster{
121+
Spec: infrav1beta2.IBMPowerVSClusterSpec{
122+
ServiceInstanceID: "capi-si-id",
123+
Network: infrav1beta2.IBMPowerVSResourceReference{
124+
ID: ptr.To("capi-net-id"),
125+
},
126+
DHCPServer: &infrav1beta2.DHCPServer{
127+
ID: ptr.To("capi-dhcp-id"),
128+
},
129+
},
130+
},
131+
wantErr: true,
132+
},
73133
}
74134

75135
for _, tc := range tests {

0 commit comments

Comments
 (0)