Skip to content

Commit 42b871b

Browse files
bnallapetabnallapeta
committed
unit tests: add cases for nil network and SG override
Signed-off-by: Bharath Nallapeta <bnallapeta@mirantis.com>
1 parent 96f2da1 commit 42b871b

File tree

3 files changed

+116
-26
lines changed

3 files changed

+116
-26
lines changed

‎controllers/openstackcluster_controller.go‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -497,7 +497,10 @@ func (r *OpenStackClusterReconciler) reconcileBastionServer(ctx context.Context,
497497
}
498498

499499
// If the bastion is found but the spec has changed, we need to delete it and reconcile.
500-
bastionServerSpec := bastionToOpenStackServerSpec(openStackCluster)
500+
bastionServerSpec, err := bastionToOpenStackServerSpec(openStackCluster)
501+
if err != nil {
502+
return nil, true, err
503+
}
501504
if !bastionNotFound && server != nil && !apiequality.Semantic.DeepEqual(bastionServerSpec, &server.Spec) {
502505
scope.Logger().Info("Bastion spec has changed, re-creating the OpenStackServer object")
503506
if err := r.deleteBastion(ctx, scope, cluster, openStackCluster); err != nil {
@@ -543,7 +546,10 @@ func (r *OpenStackClusterReconciler) getBastionServer(ctx context.Context, openS
543546
// createBastionServer creates the OpenStackServer object for the bastion server.
544547
// It returns the OpenStackServer object and an error if any.
545548
func (r *OpenStackClusterReconciler) createBastionServer(ctx context.Context, openStackCluster *infrav1.OpenStackCluster, cluster *clusterv1.Cluster) (*infrav1alpha1.OpenStackServer, error) {
546-
bastionServerSpec := bastionToOpenStackServerSpec(openStackCluster)
549+
bastionServerSpec, err := bastionToOpenStackServerSpec(openStackCluster)
550+
if err != nil {
551+
return nil, err
552+
}
547553
bastionServer := &infrav1alpha1.OpenStackServer{
548554
ObjectMeta: metav1.ObjectMeta{
549555
Labels: map[string]string{
@@ -571,7 +577,7 @@ func (r *OpenStackClusterReconciler) createBastionServer(ctx context.Context, op
571577

572578
// bastionToOpenStackServerSpec converts the OpenStackMachineSpec for the bastion to an OpenStackServerSpec.
573579
// It returns the OpenStackServerSpec and an error if any.
574-
func bastionToOpenStackServerSpec(openStackCluster *infrav1.OpenStackCluster) *infrav1alpha1.OpenStackServerSpec {
580+
func bastionToOpenStackServerSpec(openStackCluster *infrav1.OpenStackCluster) (*infrav1alpha1.OpenStackServerSpec, error) {
575581
bastion := openStackCluster.Spec.Bastion
576582
if bastion == nil {
577583
bastion = &infrav1.Bastion{}
@@ -586,9 +592,12 @@ func bastionToOpenStackServerSpec(openStackCluster *infrav1.OpenStackCluster) *i
586592
if bastion.AvailabilityZone != nil {
587593
az = *bastion.AvailabilityZone
588594
}
589-
openStackServerSpec := openStackMachineSpecToOpenStackServerSpec(bastion.Spec, openStackCluster.Spec.IdentityRef, compute.InstanceTags(bastion.Spec, openStackCluster), az, nil, getBastionSecurityGroupID(openStackCluster), openStackCluster.Status.Network.ID)
595+
openStackServerSpec, err := openStackMachineSpecToOpenStackServerSpec(bastion.Spec, openStackCluster.Spec.IdentityRef, compute.InstanceTags(bastion.Spec, openStackCluster), az, nil, getBastionSecurityGroupID(openStackCluster), openStackCluster.Status.Network)
596+
if err != nil {
597+
return nil, err
598+
}
590599

591-
return openStackServerSpec
600+
return openStackServerSpec, nil
592601
}
593602

594603
func bastionName(clusterResourceName string) string {

‎controllers/openstackmachine_controller.go‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,18 @@ func (r *OpenStackMachineReconciler) getMachineServer(ctx context.Context, openS
479479

480480
// openStackMachineSpecToOpenStackServerSpec converts an OpenStackMachineSpec to an OpenStackServerSpec.
481481
// It returns the OpenStackServerSpec object and an error if there is any.
482-
func openStackMachineSpecToOpenStackServerSpec(openStackMachineSpec *infrav1.OpenStackMachineSpec, identityRef infrav1.OpenStackIdentityReference, tags []string, failureDomain string, userDataRef *corev1.LocalObjectReference, defaultSecGroup *string, defaultNetworkID string) *infrav1alpha1.OpenStackServerSpec {
482+
func openStackMachineSpecToOpenStackServerSpec(openStackMachineSpec *infrav1.OpenStackMachineSpec, identityRef infrav1.OpenStackIdentityReference, tags []string, failureDomain string, userDataRef *corev1.LocalObjectReference, defaultSecGroup *string, clusterNetwork *infrav1.NetworkStatusWithSubnets) (*infrav1alpha1.OpenStackServerSpec, error) {
483+
// Determine default network ID if the cluster status exposes one.
484+
var defaultNetworkID string
485+
if clusterNetwork != nil {
486+
defaultNetworkID = clusterNetwork.ID
487+
}
488+
489+
// If no cluster network is available AND the machine spec did not define any ports with a network, we cannot choose a network.
490+
if defaultNetworkID == "" && len(openStackMachineSpec.Ports) == 0 {
491+
return nil, capoerrors.Terminal(infrav1.InvalidMachineSpecReason, "no network configured: cluster network is missing and machine spec does not define ports with a network")
492+
}
493+
483494
openStackServerSpec := &infrav1alpha1.OpenStackServerSpec{
484495
AdditionalBlockDevices: openStackMachineSpec.AdditionalBlockDevices,
485496
ConfigDrive: openStackMachineSpec.ConfigDrive,
@@ -540,7 +551,7 @@ func openStackMachineSpecToOpenStackServerSpec(openStackMachineSpec *infrav1.Ope
540551
}
541552
openStackServerSpec.Ports = serverPorts
542553

543-
return openStackServerSpec
554+
return openStackServerSpec, nil
544555
}
545556

546557
// reconcileMachineServer reconciles the OpenStackServer object for the OpenStackMachine.
@@ -589,18 +600,10 @@ func (r *OpenStackMachineReconciler) getOrCreateMachineServer(ctx context.Contex
589600
}
590601
return openStackCluster.Spec.IdentityRef
591602
}()
592-
// Determine default network ID if the cluster status exposes one.
593-
var defaultNetworkID string
594-
if openStackCluster.Status.Network != nil {
595-
defaultNetworkID = openStackCluster.Status.Network.ID
596-
}
597-
598-
// If no cluster network is available AND the machine spec did not define any ports with a network, we cannot choose a network.
599-
if defaultNetworkID == "" && len(openStackMachine.Spec.Ports) == 0 {
600-
return nil, capoerrors.Terminal(infrav1.InvalidMachineSpecReason, "no network configured: cluster network is missing and machine spec does not define ports with a network")
603+
machineServerSpec, err := openStackMachineSpecToOpenStackServerSpec(&openStackMachine.Spec, identityRef, compute.InstanceTags(&openStackMachine.Spec, openStackCluster), failureDomain, userDataRef, getManagedSecurityGroup(openStackCluster, machine), openStackCluster.Status.Network)
604+
if err != nil {
605+
return nil, err
601606
}
602-
603-
machineServerSpec := openStackMachineSpecToOpenStackServerSpec(&openStackMachine.Spec, identityRef, compute.InstanceTags(&openStackMachine.Spec, openStackCluster), failureDomain, userDataRef, getManagedSecurityGroup(openStackCluster, machine), defaultNetworkID)
604607
machineServer = &infrav1alpha1.OpenStackServer{
605608
ObjectMeta: metav1.ObjectMeta{
606609
Labels: map[string]string{

‎controllers/openstackmachine_controller_test.go‎

Lines changed: 86 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,6 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
8282
ID: ptr.To(openStackCluster.Status.Network.ID),
8383
},
8484
SecurityGroups: []infrav1.SecurityGroupParam{
85-
{
86-
ID: ptr.To(openStackCluster.Status.WorkerSecurityGroup.ID),
87-
},
8885
{
8986
ID: ptr.To(extraSecurityGroupUUID),
9087
},
@@ -95,9 +92,11 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
9592
tags := []string{"tag1", "tag2"}
9693
userData := &corev1.LocalObjectReference{Name: "server-data-secret"}
9794
tests := []struct {
98-
name string
99-
spec *infrav1.OpenStackMachineSpec
100-
want *infrav1alpha1.OpenStackServerSpec
95+
name string
96+
spec *infrav1.OpenStackMachineSpec
97+
clusterNetwork *infrav1.NetworkStatusWithSubnets
98+
want *infrav1alpha1.OpenStackServerSpec
99+
wantErr bool
101100
}{
102101
{
103102
name: "Test a minimum OpenStackMachineSpec to OpenStackServerSpec conversion",
@@ -106,6 +105,7 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
106105
Image: image,
107106
SSHKeyName: sshKeyName,
108107
},
108+
clusterNetwork: openStackCluster.Status.Network,
109109
want: &infrav1alpha1.OpenStackServerSpec{
110110
Flavor: ptr.To(flavorName),
111111
IdentityRef: identityRef,
@@ -128,6 +128,7 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
128128
},
129129
},
130130
},
131+
clusterNetwork: openStackCluster.Status.Network,
131132
want: &infrav1alpha1.OpenStackServerSpec{
132133
Flavor: ptr.To(flavorName),
133134
IdentityRef: identityRef,
@@ -146,6 +147,7 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
146147
Image: image,
147148
SSHKeyName: sshKeyName,
148149
},
150+
clusterNetwork: openStackCluster.Status.Network,
149151
want: &infrav1alpha1.OpenStackServerSpec{
150152
Flavor: ptr.To(flavorName),
151153
FlavorID: ptr.To(flavorUUID),
@@ -164,6 +166,7 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
164166
Image: image,
165167
SSHKeyName: sshKeyName,
166168
},
169+
clusterNetwork: openStackCluster.Status.Network,
167170
want: &infrav1alpha1.OpenStackServerSpec{
168171
FlavorID: ptr.To(flavorUUID),
169172
IdentityRef: identityRef,
@@ -174,12 +177,87 @@ func TestOpenStackMachineSpecToOpenStackServerSpec(t *testing.T) {
174177
UserDataRef: userData,
175178
},
176179
},
180+
{
181+
name: "Cluster network nil, machine defines port network and overrides SG",
182+
spec: &infrav1.OpenStackMachineSpec{
183+
Ports: []infrav1.PortOpts{{
184+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
185+
}},
186+
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(extraSecurityGroupUUID)}},
187+
},
188+
clusterNetwork: nil,
189+
want: &infrav1alpha1.OpenStackServerSpec{
190+
IdentityRef: identityRef,
191+
Ports: []infrav1.PortOpts{{
192+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
193+
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(extraSecurityGroupUUID)}},
194+
}},
195+
Tags: tags,
196+
UserDataRef: userData,
197+
},
198+
},
199+
{
200+
name: "Cluster network nil, machine defines port network and falls back to cluster SG",
201+
spec: &infrav1.OpenStackMachineSpec{
202+
Ports: []infrav1.PortOpts{{
203+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
204+
}},
205+
},
206+
clusterNetwork: nil,
207+
want: &infrav1alpha1.OpenStackServerSpec{
208+
IdentityRef: identityRef,
209+
Ports: []infrav1.PortOpts{{
210+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
211+
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(workerSecurityGroupUUID)}},
212+
}},
213+
Tags: tags,
214+
UserDataRef: userData,
215+
},
216+
},
217+
{
218+
name: "Error case: no cluster network and no machine ports",
219+
spec: &infrav1.OpenStackMachineSpec{
220+
Flavor: ptr.To(flavorName),
221+
Image: image,
222+
SSHKeyName: sshKeyName,
223+
// No ports defined
224+
},
225+
clusterNetwork: nil,
226+
want: nil,
227+
wantErr: true,
228+
},
229+
{
230+
name: "Empty cluster network ID, machine defines explicit ports",
231+
spec: &infrav1.OpenStackMachineSpec{
232+
Flavor: ptr.To(flavorName),
233+
Image: image,
234+
Ports: []infrav1.PortOpts{{
235+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
236+
}},
237+
},
238+
clusterNetwork: &infrav1.NetworkStatusWithSubnets{NetworkStatus: infrav1.NetworkStatus{ID: ""}},
239+
want: &infrav1alpha1.OpenStackServerSpec{
240+
Flavor: ptr.To(flavorName),
241+
IdentityRef: identityRef,
242+
Image: image,
243+
Ports: []infrav1.PortOpts{{
244+
Network: &infrav1.NetworkParam{ID: ptr.To(networkUUID)},
245+
SecurityGroups: []infrav1.SecurityGroupParam{{ID: ptr.To(workerSecurityGroupUUID)}},
246+
}},
247+
Tags: tags,
248+
UserDataRef: userData,
249+
},
250+
},
177251
}
178252
for i := range tests {
179253
tt := tests[i]
180254
t.Run(tt.name, func(t *testing.T) {
181-
spec := openStackMachineSpecToOpenStackServerSpec(tt.spec, identityRef, tags, "", userData, &openStackCluster.Status.WorkerSecurityGroup.ID, openStackCluster.Status.Network.ID)
182-
if !reflect.DeepEqual(spec, tt.want) {
255+
spec, err := openStackMachineSpecToOpenStackServerSpec(tt.spec, identityRef, tags, "", userData, &openStackCluster.Status.WorkerSecurityGroup.ID, tt.clusterNetwork)
256+
if (err != nil) != tt.wantErr {
257+
t.Errorf("openStackMachineSpecToOpenStackServerSpec() error = %v, wantErr %v", err, tt.wantErr)
258+
return
259+
}
260+
if !tt.wantErr && !reflect.DeepEqual(spec, tt.want) {
183261
t.Errorf("openStackMachineSpecToOpenStackServerSpec() got = %+v, want %+v", spec, tt.want)
184262
}
185263
})

0 commit comments

Comments
 (0)