Skip to content

Commit 74462ee

Browse files
CodeBleuCodeBleu
committed
Added unit tests & minor change isolated_network
 * Adjusted some logic in isolated_network.go file so that the unit tests would work better for create Egress/Routing firewall rules * Attempted to add/update unit tests
1 parent f1ec86a commit 74462ee

File tree

4 files changed

+135
-87
lines changed

4 files changed

+135
-87
lines changed

‎pkg/cloud/isolated_network.go‎

Lines changed: 24 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -188,17 +188,14 @@ func (c *client) OpenFirewallRules(isoNet *infrav1.CloudStackIsolatedNetwork) (r
188188
protocols := []string{NetworkProtocolTCP, NetworkProtocolUDP, NetworkProtocolICMP}
189189
for _, proto := range protocols {
190190
if err := c.createFirewallRule(isoNet, proto); err != nil {
191-
retErr = err
191+
c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err)
192+
return errors.Wrapf(err, "failed creating firewall rule for network ID %s", isoNet.Spec.ID)
192193
}
193194
}
194195

195-
// Mark firewall rules as opened if no errors occurred
196-
if retErr == nil {
197-
isoNet.Status.FirewallRulesOpened = true
198-
}
199-
200-
c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(retErr)
201-
return retErr
196+
// Mark firewall rules as opened if all rules were created successfully
197+
isoNet.Status.FirewallRulesOpened = true
198+
return nil
202199
}
203200

204201
// Helper function to check if all required firewall rules exist
@@ -259,35 +256,29 @@ func (c *client) getNetwork(networkID string) (*cloudstack.Network, error) {
259256

260257
// Helper function to create a firewall rule for a given protocol
261258
func (c *client) createFirewallRule(isoNet *infrav1.CloudStackIsolatedNetwork, proto string) error {
262-
var p interface{}
263259
if isoNet.Status.RoutingMode != "" {
264-
p = c.cs.Firewall.NewCreateRoutingFirewallRuleParams(isoNet.Spec.ID, proto)
265-
} else {
266-
p = c.cs.Firewall.NewCreateEgressFirewallRuleParams(isoNet.Spec.ID, proto)
267-
}
268-
269-
// Set ICMP-specific parameters
270-
if proto == "icmp" {
271-
if routingP, ok := p.(*cloudstack.CreateRoutingFirewallRuleParams); ok {
272-
routingP.SetIcmptype(-1)
273-
routingP.SetIcmpcode(-1)
274-
} else if egressP, ok := p.(*cloudstack.CreateEgressFirewallRuleParams); ok {
275-
egressP.SetIcmptype(-1)
276-
egressP.SetIcmpcode(-1)
260+
// Handle routing firewall rules
261+
p := c.cs.Firewall.NewCreateRoutingFirewallRuleParams(isoNet.Spec.ID, proto)
262+
if proto == "icmp" {
263+
p.SetIcmptype(-1)
264+
p.SetIcmpcode(-1)
277265
}
266+
_, err := c.cs.Firewall.CreateRoutingFirewallRule(p)
267+
if err != nil && !c.isIgnorableError(err) {
268+
return errors.Wrapf(err, "failed creating routing firewall rule for network ID %s protocol %s", isoNet.Spec.ID, proto)
269+
}
270+
return nil
278271
}
279272

280-
// Create the firewall rule
281-
var err error
282-
if routingP, ok := p.(*cloudstack.CreateRoutingFirewallRuleParams); ok {
283-
_, err = c.cs.Firewall.CreateRoutingFirewallRule(routingP)
284-
} else if egressP, ok := p.(*cloudstack.CreateEgressFirewallRuleParams); ok {
285-
_, err = c.cs.Firewall.CreateEgressFirewallRule(egressP)
273+
// Handle egress firewall rules
274+
p := c.cs.Firewall.NewCreateEgressFirewallRuleParams(isoNet.Spec.ID, proto)
275+
if proto == "icmp" {
276+
p.SetIcmptype(-1)
277+
p.SetIcmpcode(-1)
286278
}
287-
288-
// Ignore specific errors
279+
_, err := c.cs.Firewall.CreateEgressFirewallRule(p)
289280
if err != nil && !c.isIgnorableError(err) {
290-
return errors.Wrapf(err, "failed creating firewall rule for network ID %s protocol %s", isoNet.Spec.ID, proto)
281+
return errors.Wrapf(err, "failed creating egress firewall rule for network ID %s protocol %s", isoNet.Spec.ID, proto)
291282
}
292283
return nil
293284
}
@@ -396,9 +387,9 @@ func (c *client) GetOrCreateIsolatedNetwork(
396387
isoNet *infrav1.CloudStackIsolatedNetwork,
397388
csCluster *infrav1.CloudStackCluster,
398389
) error {
399-
// Get or create the isolated network itself and resolve details into passed custom resources.
400390
net := isoNet.Network()
401-
if err := c.ResolveNetwork(net); err != nil { // Doesn't exist, create isolated network.
391+
err := c.ResolveNetwork(net)
392+
if err != nil {
402393
if err = c.CreateIsolatedNetwork(fd, isoNet); err != nil {
403394
return errors.Wrap(err, "creating a new isolated network")
404395
}

‎pkg/cloud/isolated_network_test.go‎

Lines changed: 107 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,8 @@ var _ = ginkgo.Describe("Network", func() {
7070
ginkgo.It("calls to create an isolated network when not found", func() {
7171
dummies.Zone1.Network = dummies.ISONet1
7272
dummies.Zone1.Network.ID = ""
73+
dummies.CSISONet1.Spec.VPC = nil // Explicitly disable VPC for CSISONet1.
74+
dummies.ISONet1.VPC = nil // Explicitly disable VPC for ISONet1.
7375

7476
nos.EXPECT().GetNetworkOfferingID(gomock.Any()).Return("someOfferingID", 1, nil)
7577
ns.EXPECT().NewCreateNetworkParams(gomock.Any(), gomock.Any(), gomock.Any()).
@@ -85,6 +87,15 @@ var _ = ginkgo.Describe("Network", func() {
8587
as.EXPECT().NewAssociateIpAddressParams().Return(&csapi.AssociateIpAddressParams{})
8688
as.EXPECT().AssociateIpAddress(gomock.Any())
8789
ns.EXPECT().GetNetworkByID(dummies.ISONet1.ID, gomock.Any()).Return(&csapi.Network{Egressdefaultpolicy: false}, 1, nil)
90+
91+
// Mock firewall rule check: no existing rules, so create TCP, UDP, ICMP rules
92+
fs.EXPECT().NewListEgressFirewallRulesParams().Return(&csapi.ListEgressFirewallRulesParams{})
93+
fs.EXPECT().ListEgressFirewallRules(gomock.Any()).Return(&csapi.ListEgressFirewallRulesResponse{
94+
Count: 0,
95+
EgressFirewallRules: []*csapi.EgressFirewallRule{},
96+
}, nil)
97+
98+
// Mock firewall rule creation
8899
fs.EXPECT().NewCreateEgressFirewallRuleParams(dummies.ISONet1.ID, gomock.Any()).
89100
DoAndReturn(func(_ string, protocol string) *csapi.CreateEgressFirewallRuleParams {
90101
p := &csapi.CreateEgressFirewallRuleParams{}
@@ -102,17 +113,29 @@ var _ = ginkgo.Describe("Network", func() {
102113
fs.EXPECT().CreateEgressFirewallRule(&csapi.CreateEgressFirewallRuleParams{}).
103114
Return(&csapi.CreateEgressFirewallRuleResponse{}, nil).Times(2),
104115
fs.EXPECT().CreateEgressFirewallRule(ruleParamsICMP).
105-
Return(&csapi.CreateEgressFirewallRuleResponse{}, nil))
116+
Return(&csapi.CreateEgressFirewallRuleResponse{}, nil),
117+
)
106118

107-
// Will add cluster tag once to Network and once to PublicIP.
108-
createdByResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{{Key: cloud.CreatedByCAPCTagName, Value: "1"}}}
119+
// Expect 6 ListTags calls: 3 for network (AddCreatedByCAPCTag, AddClusterTag x2),
120+
// 3 for public IP (GetPublicIP, AddClusterTag, AddCreatedByCAPCTag)
121+
emptyResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{}}
122+
clusterTagResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{{Key: cloud.CreatedByCAPCTagName, Value: "1"}}}
109123
gomock.InOrder(
110-
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}),
111-
rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil),
112-
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}),
113-
rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil))
114-
115-
// Will add creation and cluster tags to network and PublicIP.
124+
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // For AddCreatedByCAPCTag (Network)
125+
rs.EXPECT().ListTags(gomock.Any()).Return(emptyResponse, nil),
126+
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // For AddClusterTag (Network, IsCapcManaged)
127+
rs.EXPECT().ListTags(gomock.Any()).Return(clusterTagResponse, nil),
128+
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // For AddClusterTag (Network, AddTags)
129+
rs.EXPECT().ListTags(gomock.Any()).Return(clusterTagResponse, nil),
130+
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // For GetPublicIP (PublicIpAddress)
131+
rs.EXPECT().ListTags(gomock.Any()).Return(clusterTagResponse, nil),
132+
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // For AddClusterTag (PublicIpAddress)
133+
rs.EXPECT().ListTags(gomock.Any()).Return(clusterTagResponse, nil),
134+
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // For AddCreatedByCAPCTag (PublicIpAddress)
135+
rs.EXPECT().ListTags(gomock.Any()).Return(emptyResponse, nil),
136+
)
137+
138+
// Expect 4 CreateTags calls: 2 for network (cluster + created-by), 2 for public IP (cluster + created-by)
116139
rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()).
117140
Return(&csapi.CreateTagsParams{}).Times(4)
118141
rs.EXPECT().CreateTags(gomock.Any()).Return(&csapi.CreateTagsResponse{}, nil).Times(4)
@@ -136,11 +159,24 @@ var _ = ginkgo.Describe("Network", func() {
136159
})
137160
})
138161

139-
ginkgo.Context("for a closed firewall", func() {
140-
ginkgo.It("OpenFirewallRule asks CloudStack to open the firewall", func() {
141-
dummies.Zone1.Network = dummies.ISONet1
142-
ns.EXPECT().GetNetworkByID(dummies.ISONet1.ID, gomock.Any()).Return(&csapi.Network{Egressdefaultpolicy: false}, 1, nil)
143-
fs.EXPECT().NewCreateEgressFirewallRuleParams(dummies.ISONet1.ID, gomock.Any()).
162+
ginkgo.Context("OpenFirewallRules", func() {
163+
ginkgo.It("creates firewall rules when none exist", func() {
164+
isoNet := dummies.CSISONet1
165+
isoNet.Status.RoutingMode = "" // Egress rules
166+
isoNet.Spec.ID = "net-123"
167+
168+
// Mock GetNetworkByID to return a network with Egressdefaultpolicy set to false
169+
ns.EXPECT().GetNetworkByID(isoNet.Spec.ID, gomock.Any()).Return(&csapi.Network{Egressdefaultpolicy: false}, 1, nil)
170+
171+
// Mock firewall rule check: no existing rules
172+
fs.EXPECT().NewListEgressFirewallRulesParams().Return(&csapi.ListEgressFirewallRulesParams{})
173+
fs.EXPECT().ListEgressFirewallRules(gomock.Any()).Return(&csapi.ListEgressFirewallRulesResponse{
174+
Count: 0,
175+
EgressFirewallRules: []*csapi.EgressFirewallRule{},
176+
}, nil)
177+
178+
// Mock firewall rule creation for TCP, UDP, ICMP
179+
fs.EXPECT().NewCreateEgressFirewallRuleParams(isoNet.Spec.ID, gomock.Any()).
144180
DoAndReturn(func(_ string, protocol string) *csapi.CreateEgressFirewallRuleParams {
145181
p := &csapi.CreateEgressFirewallRuleParams{}
146182
if protocol == "icmp" {
@@ -149,45 +185,62 @@ var _ = ginkgo.Describe("Network", func() {
149185
}
150186
return p
151187
}).Times(3)
188+
fs.EXPECT().CreateEgressFirewallRule(gomock.Any()).Return(&csapi.CreateEgressFirewallRuleResponse{}, nil).Times(3)
152189

153-
ruleParamsICMP := &csapi.CreateEgressFirewallRuleParams{}
154-
ruleParamsICMP.SetIcmptype(-1)
155-
ruleParamsICMP.SetIcmpcode(-1)
156-
gomock.InOrder(
157-
fs.EXPECT().CreateEgressFirewallRule(&csapi.CreateEgressFirewallRuleParams{}).
158-
Return(&csapi.CreateEgressFirewallRuleResponse{}, nil).Times(2),
159-
fs.EXPECT().CreateEgressFirewallRule(ruleParamsICMP).
160-
Return(&csapi.CreateEgressFirewallRuleResponse{}, nil))
190+
err := client.OpenFirewallRules(isoNet)
191+
gomega.Expect(err).To(gomega.BeNil())
192+
gomega.Expect(isoNet.Status.FirewallRulesOpened).To(gomega.BeTrue())
193+
})
161194

162-
gomega.Ω(client.OpenFirewallRules(dummies.CSISONet1)).Should(gomega.Succeed())
195+
ginkgo.It("skips creating firewall rules if they already exist", func() {
196+
isoNet := dummies.CSISONet1
197+
isoNet.Status.RoutingMode = "" // Egress rules
198+
isoNet.Spec.ID = "net-123"
199+
200+
// Mock GetNetworkByID to return a network with Egressdefaultpolicy set to false
201+
ns.EXPECT().GetNetworkByID(isoNet.Spec.ID, gomock.Any()).Return(&csapi.Network{Egressdefaultpolicy: false}, 1, nil)
202+
203+
// Mock firewall rule check: all required rules exist
204+
fs.EXPECT().NewListEgressFirewallRulesParams().Return(&csapi.ListEgressFirewallRulesParams{})
205+
fs.EXPECT().ListEgressFirewallRules(gomock.Any()).Return(&csapi.ListEgressFirewallRulesResponse{
206+
Count: 3,
207+
EgressFirewallRules: []*csapi.EgressFirewallRule{
208+
{Protocol: "tcp"},
209+
{Protocol: "udp"},
210+
{Protocol: "icmp", Icmptype: -1, Icmpcode: -1},
211+
},
212+
}, nil)
213+
214+
err := client.OpenFirewallRules(isoNet)
215+
gomega.Expect(err).To(gomega.BeNil())
216+
gomega.Expect(isoNet.Status.FirewallRulesOpened).To(gomega.BeTrue())
163217
})
164-
})
165218

166-
ginkgo.Context("for an open firewall", func() {
167-
ginkgo.It("OpenFirewallRule asks CloudStack to open the firewall anyway, but doesn't fail", func() {
168-
dummies.Zone1.Network = dummies.ISONet1
219+
ginkgo.It("handles error during firewall rule creation", func() {
220+
isoNet := dummies.CSISONet1
221+
isoNet.Status.RoutingMode = "" // Egress rules
222+
isoNet.Spec.ID = "net-123"
169223

170-
ns.EXPECT().GetNetworkByID(dummies.ISONet1.ID, gomock.Any()).Return(&csapi.Network{Egressdefaultpolicy: false}, 1, nil)
171-
fs.EXPECT().NewCreateEgressFirewallRuleParams(dummies.ISONet1.ID, gomock.Any()).
172-
DoAndReturn(func(_ string, protocol string) *csapi.CreateEgressFirewallRuleParams {
173-
p := &csapi.CreateEgressFirewallRuleParams{}
174-
if protocol == "icmp" {
175-
p.SetIcmptype(-1)
176-
p.SetIcmpcode(-1)
177-
}
178-
return p
179-
}).Times(3)
224+
// Mock GetNetworkByID to return a network with Egressdefaultpolicy set to false
225+
ns.EXPECT().GetNetworkByID(isoNet.Spec.ID, gomock.Any()).Return(&csapi.Network{Egressdefaultpolicy: false}, 1, nil)
180226

181-
ruleParamsICMP := &csapi.CreateEgressFirewallRuleParams{}
182-
ruleParamsICMP.SetIcmptype(-1)
183-
ruleParamsICMP.SetIcmpcode(-1)
184-
gomock.InOrder(
185-
fs.EXPECT().CreateEgressFirewallRule(&csapi.CreateEgressFirewallRuleParams{}).
186-
Return(&csapi.CreateEgressFirewallRuleResponse{}, nil).Times(2),
187-
fs.EXPECT().CreateEgressFirewallRule(ruleParamsICMP).
188-
Return(&csapi.CreateEgressFirewallRuleResponse{}, nil))
227+
// Mock firewall rule check: no existing rules
228+
fs.EXPECT().NewListEgressFirewallRulesParams().Return(&csapi.ListEgressFirewallRulesParams{})
229+
fs.EXPECT().ListEgressFirewallRules(gomock.Any()).Return(&csapi.ListEgressFirewallRulesResponse{
230+
Count: 0,
231+
EgressFirewallRules: []*csapi.EgressFirewallRule{},
232+
}, nil)
233+
234+
// Mock firewall rule creation: fail on UDP
235+
fs.EXPECT().NewCreateEgressFirewallRuleParams(isoNet.Spec.ID, "tcp").Return(&csapi.CreateEgressFirewallRuleParams{})
236+
fs.EXPECT().CreateEgressFirewallRule(gomock.Any()).Return(&csapi.CreateEgressFirewallRuleResponse{}, nil)
237+
fs.EXPECT().NewCreateEgressFirewallRuleParams(isoNet.Spec.ID, "udp").Return(&csapi.CreateEgressFirewallRuleParams{})
238+
fs.EXPECT().CreateEgressFirewallRule(gomock.Any()).Return(nil, errors.New("failed to create UDP rule"))
189239

190-
gomega.Ω(client.OpenFirewallRules(dummies.CSISONet1)).Should(gomega.Succeed())
240+
err := client.OpenFirewallRules(isoNet)
241+
gomega.Expect(err).To(gomega.HaveOccurred())
242+
gomega.Expect(err.Error()).To(gomega.ContainSubstring("failed creating egress firewall rule for network ID net-123 protocol udp"))
243+
gomega.Expect(isoNet.Status.FirewallRulesOpened).To(gomega.BeFalse())
191244
})
192245
})
193246

@@ -252,17 +305,19 @@ var _ = ginkgo.Describe("Network", func() {
252305
aip := &csapi.AssociateIpAddressParams{}
253306
as.EXPECT().NewAssociateIpAddressParams().Return(aip)
254307
as.EXPECT().AssociateIpAddress(aip).Return(&csapi.AssociateIpAddressResponse{}, nil)
255-
// Will add cluster tag once to Network and once to PublicIP.
256308
createdByResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{{Key: cloud.CreatedByCAPCTagName, Value: "1"}}}
309+
emptyResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{}}
257310
gomock.InOrder(
258-
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}),
259-
rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil))
260-
261-
// Will add creation and cluster tags to network and PublicIP.
311+
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // AddClusterTag: IsCapcManaged
312+
rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil),
313+
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // AddClusterTag: AddTags
314+
rs.EXPECT().ListTags(gomock.Any()).Return(emptyResponse, nil),
315+
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // AddCreatedByCAPCTag: AddTags
316+
rs.EXPECT().ListTags(gomock.Any()).Return(emptyResponse, nil),
317+
)
262318
rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()).
263319
Return(&csapi.CreateTagsParams{}).Times(2)
264320
rs.EXPECT().CreateTags(gomock.Any()).Return(&csapi.CreateTagsResponse{}, nil).Times(2)
265-
266321
gomega.Ω(client.AssociatePublicIPAddress(dummies.CSFailureDomain1, dummies.CSISONet1, dummies.CSCluster)).Should(gomega.Succeed())
267322
})
268323

‎pkg/cloud/tags_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,8 +156,8 @@ var _ = ginkgo.Describe("Tag Unit Tests", func() {
156156
createdByCAPCResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{{Key: cloud.CreatedByCAPCTagName, Value: "1"}}}
157157
rtlp := &csapi.ListTagsParams{}
158158
ctp := &csapi.CreateTagsParams{}
159-
rs.EXPECT().NewListTagsParams().Return(rtlp)
160-
rs.EXPECT().ListTags(rtlp).Return(createdByCAPCResponse, nil)
159+
rs.EXPECT().NewListTagsParams().Return(rtlp).Times(2)
160+
rs.EXPECT().ListTags(rtlp).Return(createdByCAPCResponse, nil).Times(2)
161161
rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()).Return(ctp)
162162
rs.EXPECT().CreateTags(ctp).Return(&csapi.CreateTagsResponse{}, nil)
163163
gomega.Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.CSISONet1.Spec.ID, dummies.CSCluster)).Should(gomega.Succeed())

‎pkg/cloud/vpc_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,8 @@ var _ = ginkgo.Describe("VPC", func() {
169169
vs.EXPECT().NewCreateVPCParams(dummyVPC.Name, dummyVPC.Name, offeringID, dummyFD.Spec.Zone.ID).Return(createVPCParams)
170170
vs.EXPECT().CreateVPC(createVPCParams).Return(createVPCResponse, nil)
171171
rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()).Return(&csapi.CreateTagsParams{})
172+
rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{})
173+
rs.EXPECT().ListTags(gomock.Any()).Return(&csapi.ListTagsResponse{}, nil)
172174
rs.EXPECT().CreateTags(gomock.Any()).Return(&csapi.CreateTagsResponse{}, nil)
173175

174176
gomega.Ω(client.CreateVPC(&dummyFD, &dummyVPC)).Should(gomega.Succeed())

0 commit comments

Comments
 (0)