diff --git a/pkg/cloud/isolated_network.go b/pkg/cloud/isolated_network.go index e55fe3ca..8b00dcf4 100644 --- a/pkg/cloud/isolated_network.go +++ b/pkg/cloud/isolated_network.go @@ -231,6 +231,11 @@ func (c *client) GetLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNetwork) // ReconcileLoadBalancerRules manages the loadbalancer rules for all ports. func (c *client) ReconcileLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNetwork, csCluster *infrav1.CloudStackCluster) error { + // If there is no public IP address associated with the isonet, do nothing. + if isoNet.Status.PublicIPID == "" { + return nil + } + lbr, err := c.GetLoadBalancerRules(isoNet) if err != nil { c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) @@ -238,22 +243,33 @@ func (c *client) ReconcileLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNe } portsAndIDs := mapExistingLoadBalancerRules(lbr) - ports := gatherPorts(csCluster) - lbRuleIDs, err := c.ensureLoadBalancerRules(isoNet, ports, portsAndIDs, csCluster) - if err != nil { - return err - } + if csCluster.Spec.APIServerLoadBalancer.IsEnabled() { + // Load balancer enabled, reconcile the rules. + ports := gatherPorts(csCluster) - if err := c.cleanupObsoleteLoadBalancerRules(portsAndIDs, ports); err != nil { - return err - } + lbRuleIDs, err := c.ensureLoadBalancerRules(isoNet, ports, portsAndIDs, csCluster) + if err != nil { + return err + } - if len(lbRuleIDs) > 1 { - capcstrings.Canonicalize(lbRuleIDs) - } + if err := c.cleanupObsoleteLoadBalancerRules(portsAndIDs, ports); err != nil { + return err + } - isoNet.Status.LoadBalancerRuleIDs = lbRuleIDs + if len(lbRuleIDs) > 1 { + capcstrings.Canonicalize(lbRuleIDs) + } + + isoNet.Status.LoadBalancerRuleIDs = lbRuleIDs + } else { + // Load balancer disabled, delete all rules. + if err := c.cleanupAllLoadBalancerRules(portsAndIDs); err != nil { + return err + } + + isoNet.Status.LoadBalancerRuleIDs = []string{} + } return nil } @@ -262,7 +278,18 @@ func (c *client) ReconcileLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNe func mapExistingLoadBalancerRules(lbr []*cloudstack.LoadBalancerRule) map[string]string { portsAndIDs := make(map[string]string) for _, rule := range lbr { - portsAndIDs[rule.Publicport] = rule.Id + // Check if the rule is managed by CAPC. + capcManaged := false + for _, t := range rule.Tags { + if t.Key == CreatedByCAPCTagName && t.Value == "1" { + capcManaged = true + + break + } + } + if capcManaged { + portsAndIDs[rule.Publicport] = rule.Id + } } return portsAndIDs @@ -317,6 +344,17 @@ func (c *client) cleanupObsoleteLoadBalancerRules(portsAndIDs map[string]string, return nil } +// cleanupAllLoadBalancerRules deletes all load balancer rules created by CAPC. +func (c *client) cleanupAllLoadBalancerRules(portsAndIDs map[string]string) error { + for _, ruleID := range portsAndIDs { + if err := c.deleteLoadBalancerRuleByID(ruleID); err != nil { + return err + } + } + + return nil +} + // deleteLoadBalancerRuleByID wraps the deletion logic with error handling. func (c *client) deleteLoadBalancerRuleByID(ruleID string) error { success, err := c.DeleteLoadBalancerRule(ruleID) @@ -351,12 +389,24 @@ func (c *client) CreateLoadBalancerRule(isoNet *infrav1.CloudStackIsolatedNetwor return "", err } + if err := c.AddCreatedByCAPCTag(ResourceTypeLoadBalancerRule, resp.Id); err != nil { + return "", errors.Wrap(err, "adding created by CAPC tag") + } return resp.Id, nil } // DeleteLoadBalancerRule deletes an existing load balancer rule. func (c *client) DeleteLoadBalancerRule(id string) (bool, error) { + isCAPCManaged, err := c.IsCapcManaged(ResourceTypeLoadBalancerRule, id) + if err != nil { + return false, err + } + + if !isCAPCManaged { + return false, errors.Errorf("firewall rule with id %s is not managed by CAPC", id) + } + p := c.csAsync.LoadBalancer.NewDeleteLoadBalancerRuleParams(id) resp, err := c.csAsync.LoadBalancer.DeleteLoadBalancerRule(p) if err != nil { @@ -384,30 +434,46 @@ func (c *client) GetFirewallRules(isoNet *infrav1.CloudStackIsolatedNetwork) ([] // ReconcileFirewallRules manages the firewall rules for all port <-> allowedCIDR combinations. func (c *client) ReconcileFirewallRules(isoNet *infrav1.CloudStackIsolatedNetwork, csCluster *infrav1.CloudStackCluster) error { + // If there is no public IP address associated with the isonet, do nothing. + if isoNet.Status.PublicIPID == "" { + return nil + } + fwr, err := c.GetFirewallRules(isoNet) if err != nil { c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) return errors.Wrap(err, "retrieving firewall rules") } - ports := gatherPorts(csCluster) - allowedCIDRS := getCanonicalAllowedCIDRs(isoNet, csCluster) portsAndIDs := mapExistingFirewallRules(fwr) - // A note on the implementation here: - // Due to the lack of a `cidrlist` parameter in UpdateFirewallRule, we have to manage - // firewall rules for every item in the list of allowed CIDRs. - // See https://github.com/apache/cloudstack/issues/8382 - if err := c.reconcileFirewallRulesForPorts(isoNet, fwr, ports, allowedCIDRS); err != nil { - return err - } + if csCluster.Spec.APIServerLoadBalancer.IsEnabled() { + // Load balancer enabled, reconcile firewall rules. + ports := gatherPorts(csCluster) + allowedCIDRS := getCanonicalAllowedCIDRs(isoNet, csCluster) - if err := c.cleanupObsoleteFirewallRules(portsAndIDs, ports); err != nil { - return err - } + // A note on the implementation here: + // Due to the lack of a `cidrlist` parameter in UpdateFirewallRule, we have to manage + // firewall rules for every item in the list of allowed CIDRs. + // See https://github.com/apache/cloudstack/issues/8382 + if err := c.reconcileFirewallRulesForPorts(isoNet, fwr, ports, allowedCIDRS); err != nil { + return err + } + + if err := c.cleanupObsoleteFirewallRules(portsAndIDs, ports); err != nil { + return err + } + + // Update the list of allowed CIDRs in the status + isoNet.Status.APIServerLoadBalancer.AllowedCIDRs = allowedCIDRS + } else { + // Load balancer disabled, remove all firewall rules. + if err := c.cleanupAllFirewallRules(portsAndIDs); err != nil { + return err + } - // Update the list of allowed CIDRs in the status - isoNet.Status.APIServerLoadBalancer.AllowedCIDRs = allowedCIDRS + isoNet.Status.APIServerLoadBalancer.AllowedCIDRs = []string{} + } return nil } @@ -423,11 +489,20 @@ func gatherPorts(csCluster *infrav1.CloudStackCluster) []int { } // mapExistingFirewallRules creates a lookup map for existing firewall rules based on their port. -func mapExistingFirewallRules(fwr []*cloudstack.FirewallRule) map[int]string { - portsAndIDs := make(map[int]string) +func mapExistingFirewallRules(fwr []*cloudstack.FirewallRule) map[int][]string { + portsAndIDs := make(map[int][]string) for _, rule := range fwr { - if rule.Startport == rule.Endport { - portsAndIDs[rule.Startport] = rule.Id + // Check if the rule is managed by CAPC. + capcManaged := false + for _, t := range rule.Tags { + if t.Key == CreatedByCAPCTagName && t.Value == "1" { + capcManaged = true + + break + } + } + if capcManaged && rule.Startport == rule.Endport { + portsAndIDs[rule.Startport] = append(portsAndIDs[rule.Startport], rule.Id) } } @@ -488,9 +563,24 @@ func (c *client) createMissingFirewallRules(isoNet *infrav1.CloudStackIsolatedNe } // cleanupObsoleteFirewallRules deletes firewall rules that are no longer needed. -func (c *client) cleanupObsoleteFirewallRules(portsAndIDs map[int]string, ports []int) error { - for port, ruleID := range portsAndIDs { +func (c *client) cleanupObsoleteFirewallRules(portsAndIDs map[int][]string, ports []int) error { + for port, ruleIDs := range portsAndIDs { if !slices.Contains(ports, port) { + for _, ruleID := range ruleIDs { + if err := c.deleteFirewallRuleByID(ruleID); err != nil { + return err + } + } + } + } + + return nil +} + +// cleanupAllFirewallRules deletes all firewall rules created by CAPC. +func (c *client) cleanupAllFirewallRules(portsAndIDs map[int][]string) error { + for _, ruleIDs := range portsAndIDs { + for _, ruleID := range ruleIDs { if err := c.deleteFirewallRuleByID(ruleID); err != nil { return err } @@ -520,18 +610,30 @@ func (c *client) CreateFirewallRule(isoNet *infrav1.CloudStackIsolatedNetwork, p p.SetStartport(port) p.SetEndport(port) p.SetCidrlist(cidrList) - _, err := c.cs.Firewall.CreateFirewallRule(p) + resp, err := c.cs.Firewall.CreateFirewallRule(p) if err != nil { c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) return err } + if err := c.AddCreatedByCAPCTag(ResourceTypeFirewallRule, resp.Id); err != nil { + return errors.Wrap(err, "adding created by CAPC tag") + } return nil } // DeleteFirewallRule deletes a firewall rule. func (c *client) DeleteFirewallRule(id string) (bool, error) { + isCAPCManaged, err := c.IsCapcManaged(ResourceTypeFirewallRule, id) + if err != nil { + return false, err + } + + if !isCAPCManaged { + return false, errors.Errorf("firewall rule with id %s is not managed by CAPC", id) + } + p := c.csAsync.Firewall.NewDeleteFirewallRuleParams(id) resp, err := c.csAsync.Firewall.DeleteFirewallRule(p) if err != nil { @@ -611,20 +713,36 @@ func (c *client) GetOrCreateIsolatedNetwork( isoNet.Spec.ControlPlaneEndpoint.Port = 6443 } - // Set up a load balancing rules to map VM ports to Public IP ports. + // Associate public IP with the load balancer if enabled. if csCluster.Spec.APIServerLoadBalancer.IsEnabled() { // Associate Public IP with CloudStackIsolatedNetwork if err := c.AssociatePublicIPAddress(fd, isoNet, csCluster); err != nil { return errors.Wrapf(err, "associating public IP address to csCluster") } + } - if err := c.ReconcileLoadBalancerRules(isoNet, csCluster); err != nil { - return errors.Wrap(err, "reconciling load balancing rules") - } + // Set up load balancing rules to map VM ports to Public IP ports. + if err := c.ReconcileLoadBalancerRules(isoNet, csCluster); err != nil { + return errors.Wrap(err, "reconciling load balancing rules") + } - if err := c.ReconcileFirewallRules(isoNet, csCluster); err != nil { - return errors.Wrap(err, "reconciling firewall rules") + // Set up firewall rules to manage access to load balancer public IP ports. + if err := c.ReconcileFirewallRules(isoNet, csCluster); err != nil { + return errors.Wrap(err, "reconciling firewall rules") + } + + if !csCluster.Spec.APIServerLoadBalancer.IsEnabled() { + // If the APIServerLoadBalancer has been disabled, release its IP unless it's the SNAT IP. + released, err := c.DisassociatePublicIPAddressIfNotInUse(isoNet) + if err != nil { + return errors.Wrap(err, "disassociating public IP address") + } + if released { + isoNet.Status.PublicIPID = "" } + + // Clear the load balancer status as it is disabled. + isoNet.Status.APIServerLoadBalancer = nil } } @@ -680,7 +798,7 @@ func (c *client) DisposeIsoNetResources( if err := c.DeleteClusterTag(ResourceTypeIPAddress, isoNet.Status.PublicIPID, csCluster); err != nil { return err } - if err := c.DisassociatePublicIPAddressIfNotInUse(isoNet); err != nil { + if _, err := c.DisassociatePublicIPAddressIfNotInUse(isoNet); err != nil { return err } } @@ -713,19 +831,25 @@ func (c *client) DeleteNetworkIfNotInUse(net infrav1.Network) (retError error) { } // DisassociatePublicIPAddressIfNotInUse removes a CloudStack public IP association from passed isolated network -// if it is no longer in use (indicated by in use tags). -func (c *client) DisassociatePublicIPAddressIfNotInUse(isoNet *infrav1.CloudStackIsolatedNetwork) (retError error) { +// if it is no longer in use (indicated by in use tags). It returns a bool indicating whether or not an IP was actually +// disassociated, and an error in case an error occurred. +func (c *client) DisassociatePublicIPAddressIfNotInUse(isoNet *infrav1.CloudStackIsolatedNetwork) (bool, error) { if tagsAllowDisposal, err := c.DoClusterTagsAllowDisposal(ResourceTypeIPAddress, isoNet.Status.PublicIPID); err != nil { - return err + return false, err } else if publicIP, _, err := c.cs.Address.GetPublicIpAddressByID(isoNet.Status.PublicIPID); err != nil { c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) - return err + return false, err } else if publicIP == nil || publicIP.Issourcenat { // Can't disassociate an address if it's the source NAT address. - return nil + return false, nil } else if tagsAllowDisposal { - return c.DisassociatePublicIPAddress(isoNet) + if err := c.DisassociatePublicIPAddress(isoNet); err != nil { + return false, err + } + + return true, nil } - return nil + + return false, nil } // DisassociatePublicIPAddress removes a CloudStack public IP association from passed isolated network. @@ -739,5 +863,6 @@ func (c *client) DisassociatePublicIPAddress(isoNet *infrav1.CloudStackIsolatedN p := c.cs.Address.NewDisassociateIpAddressParams(isoNet.Status.PublicIPID) _, retErr = c.cs.Address.DisassociateIpAddress(p) c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(retErr) + return retErr } diff --git a/pkg/cloud/isolated_network_test.go b/pkg/cloud/isolated_network_test.go index 78433e9b..730f43c1 100644 --- a/pkg/cloud/isolated_network_test.go +++ b/pkg/cloud/isolated_network_test.go @@ -17,6 +17,7 @@ limitations under the License. package cloud_test import ( + "k8s.io/utils/pointer" "strconv" csapi "github.com/apache/cloudstack-go/v2/cloudstack" @@ -119,16 +120,22 @@ var _ = Describe("Network", func() { lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).Return( &csapi.ListLoadBalancerRulesResponse{LoadBalancerRules: []*csapi.LoadBalancerRule{ - {Publicport: strconv.Itoa(int(dummies.EndPointPort)), Id: dummies.LBRuleID}}}, nil) + { + Id: dummies.LBRuleID, + Publicport: strconv.Itoa(int(dummies.EndPointPort)), + Tags: dummies.CreatedByCAPCTag, + }, + }}, nil) fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{}) fs.EXPECT().ListFirewallRules(gomock.Any()).Return( &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{ { + Id: dummies.FWRuleID, Cidrlist: "0.0.0.0/0", Startport: int(dummies.EndPointPort), Endport: int(dummies.EndPointPort), - Id: dummies.FWRuleID, + Tags: dummies.CreatedByCAPCTag, }, }}, nil) @@ -290,23 +297,38 @@ var _ = Describe("Network", func() { Context("The specific load balancer rule exists", func() { It("resolves the rule's ID", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).Return( &csapi.ListLoadBalancerRulesResponse{LoadBalancerRules: []*csapi.LoadBalancerRule{ - {Publicport: strconv.Itoa(int(dummies.EndPointPort)), Id: dummies.LBRuleID}}}, nil) + { + Id: dummies.LBRuleID, + Publicport: strconv.Itoa(int(dummies.EndPointPort)), + Tags: dummies.CreatedByCAPCTag, + }, + }}, nil) dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{} Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) Ω(dummies.CSISONet1.Status.LoadBalancerRuleIDs).Should(Equal(dummies.LoadBalancerRuleIDs)) }) - It("when API loadbalancer additional ports are defined, resolves all rule IDs", func() { + It("when API load balancer additional ports are defined, resolves all rule IDs", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID dummies.CSCluster.Spec.APIServerLoadBalancer.AdditionalPorts = append(dummies.CSCluster.Spec.APIServerLoadBalancer.AdditionalPorts, 456) lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).Return( &csapi.ListLoadBalancerRulesResponse{LoadBalancerRules: []*csapi.LoadBalancerRule{ - {Publicport: strconv.Itoa(int(dummies.EndPointPort)), Id: dummies.LBRuleID}, - {Publicport: strconv.Itoa(456), Id: "FakeLBRuleID2"}, + { + Id: dummies.LBRuleID, + Publicport: strconv.Itoa(int(dummies.EndPointPort)), + Tags: dummies.CreatedByCAPCTag, + }, + { + Id: "FakeLBRuleID2", + Publicport: strconv.Itoa(456), + Tags: dummies.CreatedByCAPCTag, + }, }}, nil) dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{} @@ -324,11 +346,18 @@ var _ = Describe("Network", func() { }) It("doesn't create a new load balancer rule on create", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()). Return(&csapi.ListLoadBalancerRulesResponse{ LoadBalancerRules: []*csapi.LoadBalancerRule{ - {Publicport: strconv.Itoa(int(dummies.EndPointPort)), Id: dummies.LBRuleID}}}, nil) + { + Id: dummies.LBRuleID, + Publicport: strconv.Itoa(int(dummies.EndPointPort)), + Tags: dummies.CreatedByCAPCTag, + }, + }, + }, nil) Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) Ω(dummies.CSISONet1.Status.LoadBalancerRuleIDs).Should(Equal(dummies.LoadBalancerRuleIDs)) @@ -402,6 +431,7 @@ var _ = Describe("Network", func() { Context("load balancer rule does not exist", func() { It("calls CloudStack to create a new load balancer rule", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()). Return(&csapi.ListLoadBalancerRulesResponse{ @@ -412,6 +442,9 @@ var _ = Describe("Network", func() { Return(&csapi.CreateLoadBalancerRuleResponse{Id: dummies.LBRuleID}, nil) lbs.EXPECT().NewDeleteLoadBalancerRuleParams(gomock.Any()).Times(0) lbs.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Times(0) + rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&csapi.CreateTagsParams{}).Times(1) + rs.EXPECT().CreateTags(gomock.Any()).Return(&csapi.CreateTagsResponse{}, nil).Times(1) Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) loadBalancerRuleIDs := []string{dummies.LBRuleID} @@ -419,11 +452,16 @@ var _ = Describe("Network", func() { }) It("when API load balancer additional ports are defined, creates additional rules", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID dummies.CSCluster.Spec.APIServerLoadBalancer.AdditionalPorts = append(dummies.CSCluster.Spec.APIServerLoadBalancer.AdditionalPorts, 456) lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).Return( &csapi.ListLoadBalancerRulesResponse{LoadBalancerRules: []*csapi.LoadBalancerRule{ - {Publicport: strconv.Itoa(int(dummies.EndPointPort)), Id: dummies.LBRuleID}, + { + Id: dummies.LBRuleID, + Publicport: strconv.Itoa(int(dummies.EndPointPort)), + Tags: dummies.CreatedByCAPCTag, + }, }}, nil) lbs.EXPECT().NewCreateLoadBalancerRuleParams(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). @@ -432,6 +470,9 @@ var _ = Describe("Network", func() { Return(&csapi.CreateLoadBalancerRuleResponse{Id: "2ndLBRuleID"}, nil) lbs.EXPECT().NewDeleteLoadBalancerRuleParams(gomock.Any()).Times(0) lbs.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Times(0) + rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&csapi.CreateTagsParams{}).Times(1) + rs.EXPECT().CreateTags(gomock.Any()).Return(&csapi.CreateTagsResponse{}, nil).Times(1) dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{dummies.LBRuleID} Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) @@ -440,17 +481,34 @@ var _ = Describe("Network", func() { }) It("when API load balancer additional ports are defined, and a port is removed, deletes related rules", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).Return( &csapi.ListLoadBalancerRulesResponse{LoadBalancerRules: []*csapi.LoadBalancerRule{ - {Publicport: strconv.Itoa(int(dummies.EndPointPort)), Id: dummies.LBRuleID}, - {Publicport: strconv.Itoa(456), Id: "2ndLBRuleID"}, + { + Id: dummies.LBRuleID, + Publicport: strconv.Itoa(int(dummies.EndPointPort)), + Tags: dummies.CreatedByCAPCTag, + }, + { + Id: "2ndLBRuleID", + Publicport: strconv.Itoa(456), + Tags: dummies.CreatedByCAPCTag, + }, }}, nil) lbs.EXPECT().NewDeleteLoadBalancerRuleParams(gomock.Any()). Return(&csapi.DeleteLoadBalancerRuleParams{}).Times(1) lbs.EXPECT().DeleteLoadBalancerRule(gomock.Any()). Return(&csapi.DeleteLoadBalancerRuleResponse{Success: true}, nil).Times(1) + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}).Times(1) + rs.EXPECT().ListTags(gomock.Any()).Return(&csapi.ListTagsResponse{ + Count: 1, + Tags: []*csapi.Tag{{ + Key: cloud.CreatedByCAPCTagName, + Value: "1", + }}, + }, nil).Times(1) dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{"2ndLBRuleID", dummies.LBRuleID} Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) @@ -459,6 +517,7 @@ var _ = Describe("Network", func() { }) It("Fails to resolve load balancer rule details", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()). Return(nil, fakeError) @@ -468,6 +527,7 @@ var _ = Describe("Network", func() { }) It("Fails to create a new load balancer rule.", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) lbs.EXPECT().ListLoadBalancerRules(gomock.Any()). Return(&csapi.ListLoadBalancerRulesResponse{ @@ -484,6 +544,7 @@ var _ = Describe("Network", func() { Context("The specific firewall rule exists", func() { It("does not call create or delete firewall rule", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{}) fs.EXPECT().ListFirewallRules(gomock.Any()).Return( &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{ @@ -502,20 +563,23 @@ var _ = Describe("Network", func() { }) It("calls delete firewall rule when there is a rule with a cidr not in allowed cidr list", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{}) fs.EXPECT().ListFirewallRules(gomock.Any()).Return( &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{ { + Id: dummies.FWRuleID, Cidrlist: "0.0.0.0/0", Startport: int(dummies.EndPointPort), Endport: int(dummies.EndPointPort), - Id: dummies.FWRuleID, + Tags: dummies.CreatedByCAPCTag, }, { + Id: "FakeFWRuleID2", Cidrlist: "192.168.1.0/24", Startport: int(dummies.EndPointPort), Endport: int(dummies.EndPointPort), - Id: "FakeFWRuleID2", + Tags: dummies.CreatedByCAPCTag, }, }}, nil) @@ -527,26 +591,37 @@ var _ = Describe("Network", func() { fs.EXPECT().DeleteFirewallRule(gomock.Any()).Return(&csapi.DeleteFirewallRuleResponse{Success: true}, nil).Times(1) fs.EXPECT().NewCreateFirewallRuleParams(gomock.Any(), gomock.Any()).Times(0) fs.EXPECT().CreateFirewallRule(gomock.Any()).Times(0) + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}).Times(1) + rs.EXPECT().ListTags(gomock.Any()).Return(&csapi.ListTagsResponse{ + Count: 1, + Tags: []*csapi.Tag{{ + Key: cloud.CreatedByCAPCTagName, + Value: "1", + }}, + }, nil).Times(1) Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) }) It("calls delete firewall rule when a port is removed from additionalPorts", func() { + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID // We pretend that port 6565 was removed from additionalPorts fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{}) fs.EXPECT().ListFirewallRules(gomock.Any()).Return( &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{ { + Id: dummies.FWRuleID, Cidrlist: "0.0.0.0/0", Startport: int(dummies.EndPointPort), Endport: int(dummies.EndPointPort), - Id: dummies.FWRuleID, + Tags: dummies.CreatedByCAPCTag, }, { + Id: "FakeFWRuleID2", Cidrlist: "0.0.0.0/0", Startport: 6565, Endport: 6565, - Id: "FakeFWRuleID2", + Tags: dummies.CreatedByCAPCTag, }, }}, nil) @@ -558,6 +633,14 @@ var _ = Describe("Network", func() { fs.EXPECT().DeleteFirewallRule(gomock.Any()).Return(&csapi.DeleteFirewallRuleResponse{Success: true}, nil).Times(1) fs.EXPECT().NewCreateFirewallRuleParams(gomock.Any(), gomock.Any()).Times(0) fs.EXPECT().CreateFirewallRule(gomock.Any()).Times(0) + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}).Times(1) + rs.EXPECT().ListTags(gomock.Any()).Return(&csapi.ListTagsResponse{ + Count: 1, + Tags: []*csapi.Tag{{ + Key: cloud.CreatedByCAPCTagName, + Value: "1", + }}, + }, nil).Times(1) Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) }) @@ -580,6 +663,9 @@ var _ = Describe("Network", func() { }).Times(1) fs.EXPECT().CreateFirewallRule(gomock.Any()).Return(&csapi.CreateFirewallRuleResponse{}, nil).Times(1) fs.EXPECT().DeleteFirewallRule(gomock.Any()).Times(0) + rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&csapi.CreateTagsParams{}).Times(1) + rs.EXPECT().CreateTags(gomock.Any()).Return(&csapi.CreateTagsResponse{}, nil).Times(1) Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) }) @@ -590,10 +676,11 @@ var _ = Describe("Network", func() { fs.EXPECT().ListFirewallRules(gomock.Any()).Return( &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{ { + Id: "FakeFWRuleID2", Cidrlist: "192.168.1.0/24", Startport: int(dummies.EndPointPort), Endport: int(dummies.EndPointPort), - Id: "FakeFWRuleID2", + Tags: dummies.CreatedByCAPCTag, }, }}, nil) @@ -612,6 +699,17 @@ var _ = Describe("Network", func() { return p }).Times(1) fs.EXPECT().CreateFirewallRule(gomock.Any()).Return(&csapi.CreateFirewallRuleResponse{}, nil).Times(1) + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}).Times(1) + rs.EXPECT().ListTags(gomock.Any()).Return(&csapi.ListTagsResponse{ + Count: 1, + Tags: []*csapi.Tag{{ + Key: cloud.CreatedByCAPCTagName, + Value: "1", + }}, + }, nil).Times(1) + rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&csapi.CreateTagsParams{}).Times(1) + rs.EXPECT().CreateTags(gomock.Any()).Return(&csapi.CreateTagsResponse{}, nil).Times(1) Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) }) @@ -643,6 +741,9 @@ var _ = Describe("Network", func() { }).Times(1), fs.EXPECT().CreateFirewallRule(gomock.Any()).Return(&csapi.CreateFirewallRuleResponse{}, nil).Times(1), ) + rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&csapi.CreateTagsParams{}).Times(2) + rs.EXPECT().CreateTags(gomock.Any()).Return(&csapi.CreateTagsResponse{}, nil).Times(2) fs.EXPECT().DeleteFirewallRule(gomock.Any()).Times(0) @@ -692,8 +793,86 @@ var _ = Describe("Network", func() { fs.EXPECT().CreateFirewallRule(gomock.Any()).Return(&csapi.CreateFirewallRuleResponse{}, nil).Times(1), ) fs.EXPECT().DeleteFirewallRule(gomock.Any()).Times(0) + rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&csapi.CreateTagsParams{}).Times(3) + rs.EXPECT().CreateTags(gomock.Any()).Return(&csapi.CreateTagsResponse{}, nil).Times(3) + + Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) + }) + }) + + Context("With the API server load balancer enabled", func() { + It("Removes all firewall and load balancer rules if the API server load balancer is disabled", func() { + dummies.CSCluster.Spec.APIServerLoadBalancer.AllowedCIDRs = append(dummies.CSCluster.Spec.APIServerLoadBalancer.AllowedCIDRs, + "192.168.1.0/24", + "192.168.2.0/24") + dummies.CSCluster.Spec.APIServerLoadBalancer.Enabled = pointer.Bool(false) + dummies.CSISONet1.Status.PublicIPID = dummies.PublicIPID + + lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{}) + lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).Return( + &csapi.ListLoadBalancerRulesResponse{LoadBalancerRules: []*csapi.LoadBalancerRule{ + { + Id: dummies.LBRuleID, + Publicport: strconv.Itoa(int(dummies.EndPointPort)), + Tags: dummies.CreatedByCAPCTag, + }, + }}, nil) + + fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{}) + fs.EXPECT().ListFirewallRules(gomock.Any()).Return( + &csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{ + { + Id: "FakeFWRuleID1", + Cidrlist: "192.168.1.0/24", + Startport: int(dummies.EndPointPort), + Endport: int(dummies.EndPointPort), + Tags: dummies.CreatedByCAPCTag, + }, + { + Id: "FakeFWRuleID2", + Cidrlist: "192.168.2.0/24", + Startport: int(dummies.EndPointPort), + Endport: int(dummies.EndPointPort), + Tags: dummies.CreatedByCAPCTag, + }, + }}, nil) + gomock.InOrder( + lbs.EXPECT().NewDeleteLoadBalancerRuleParams(dummies.LBRuleID). + Return(&csapi.DeleteLoadBalancerRuleParams{}).Times(1), + lbs.EXPECT().DeleteLoadBalancerRule(gomock.Any()). + Return(&csapi.DeleteLoadBalancerRuleResponse{Success: true}, nil).Times(1), + + fs.EXPECT().NewDeleteFirewallRuleParams("FakeFWRuleID1").DoAndReturn(func(ruleid string) *csapi.DeleteFirewallRuleParams { + p := &csapi.DeleteFirewallRuleParams{} + p.SetId(ruleid) + return p + }), + fs.EXPECT().DeleteFirewallRule(gomock.Any()).Return(&csapi.DeleteFirewallRuleResponse{Success: true}, nil).Times(1), + fs.EXPECT().NewDeleteFirewallRuleParams("FakeFWRuleID2").DoAndReturn(func(ruleid string) *csapi.DeleteFirewallRuleParams { + p := &csapi.DeleteFirewallRuleParams{} + p.SetId(ruleid) + return p + }), + fs.EXPECT().DeleteFirewallRule(gomock.Any()).Return(&csapi.DeleteFirewallRuleResponse{Success: true}, nil).Times(1), + ) + + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}).Times(3) + rs.EXPECT().ListTags(gomock.Any()).Return(&csapi.ListTagsResponse{ + Count: 1, + Tags: []*csapi.Tag{{ + Key: cloud.CreatedByCAPCTagName, + Value: "1", + }}, + }, nil).Times(3) + + fs.EXPECT().NewCreateFirewallRuleParams(gomock.Any(), gomock.Any()).Times(0) + fs.EXPECT().CreateFirewallRule(gomock.Any()).Times(0) + + Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed()) + Ω(dummies.CSISONet1.Status.LoadBalancerRuleIDs).Should(Equal([]string{})) }) }) diff --git a/pkg/cloud/tags.go b/pkg/cloud/tags.go index a679b30b..bf6a8b48 100644 --- a/pkg/cloud/tags.go +++ b/pkg/cloud/tags.go @@ -39,10 +39,12 @@ type TagIface interface { type ResourceType string const ( - ClusterTagNamePrefix = "CAPC_cluster_" - CreatedByCAPCTagName = "created_by_CAPC" - ResourceTypeNetwork ResourceType = "Network" - ResourceTypeIPAddress ResourceType = "PublicIpAddress" + ClusterTagNamePrefix = "CAPC_cluster_" + CreatedByCAPCTagName = "created_by_CAPC" + ResourceTypeNetwork ResourceType = "Network" + ResourceTypeIPAddress ResourceType = "PublicIpAddress" + ResourceTypeLoadBalancerRule ResourceType = "LoadBalancerRule" + ResourceTypeFirewallRule ResourceType = "FirewallRule" ) // ignoreAlreadyPresentErrors returns nil if the error is an already present tag error. @@ -54,6 +56,7 @@ func ignoreAlreadyPresentErrors(err error, rType ResourceType, rID string) error return nil } +// IsCapcManaged checks whether the resource has the CreatedByCAPCTag. func (c *client) IsCapcManaged(resourceType ResourceType, resourceID string) (bool, error) { tags, err := c.GetTags(resourceType, resourceID) if err != nil { diff --git a/pkg/cloud/tags_test.go b/pkg/cloud/tags_test.go index 03bccc09..ccb07d8f 100644 --- a/pkg/cloud/tags_test.go +++ b/pkg/cloud/tags_test.go @@ -103,7 +103,7 @@ var _ = Describe("Tag Unit Tests", func() { // Verify tags tags, err := client.GetTags(cloud.ResourceTypeNetwork, dummies.CSISONet1.Spec.ID) Ω(err).Should(BeNil()) - Ω(tags[dummies.CreatedByCapcKey]).Should(Equal("")) + Ω(tags[cloud.CreatedByCAPCTagName]).Should(Equal("")) Ω(tags[dummies.CSClusterTagKey]).Should(Equal("")) }) diff --git a/test/dummies/v1beta1/vars.go b/test/dummies/v1beta1/vars.go index 06d51160..8bd3ee14 100644 --- a/test/dummies/v1beta1/vars.go +++ b/test/dummies/v1beta1/vars.go @@ -71,8 +71,6 @@ var ( // Declare exported dummy vars. CSClusterTagKey string CSClusterTagVal string CSClusterTag map[string]string - CreatedByCapcKey string - CreatedByCapcVal string LBRuleID string PublicIPID string EndPointHost string @@ -134,8 +132,6 @@ func SetDummyTagVars() { CSClusterTagKey = "CAPC_cluster_" + string(CSCluster.ObjectMeta.UID) CSClusterTagVal = "1" CSClusterTag = map[string]string{CSClusterTagVal: CSClusterTagVal} - CreatedByCapcKey = "create_by_CAPC" - CreatedByCapcVal = "" Tag1Key = "test_tag1" Tag1Val = "arbitrary_value1" Tag2Key = "test_tag2" diff --git a/test/dummies/v1beta2/vars.go b/test/dummies/v1beta2/vars.go index 6e7d3d47..1adc572c 100644 --- a/test/dummies/v1beta2/vars.go +++ b/test/dummies/v1beta2/vars.go @@ -75,8 +75,6 @@ var ( // Declare exported dummy vars. CSClusterTagKey string CSClusterTagVal string CSClusterTag map[string]string - CreatedByCapcKey string - CreatedByCapcVal string LBRuleID string PublicIPID string EndPointHost string @@ -139,8 +137,6 @@ func SetDummyTagVars() { CSClusterTagKey = "CAPC_cluster_" + string(CSCluster.ObjectMeta.UID) CSClusterTagVal = "1" CSClusterTag = map[string]string{CSClusterTagVal: CSClusterTagVal} - CreatedByCapcKey = "create_by_CAPC" - CreatedByCapcVal = "" Tag1Key = "test_tag1" Tag1Val = "arbitrary_value1" Tag2Key = "test_tag2" diff --git a/test/dummies/v1beta3/vars.go b/test/dummies/v1beta3/vars.go index a98906c3..f0d31b1e 100644 --- a/test/dummies/v1beta3/vars.go +++ b/test/dummies/v1beta3/vars.go @@ -76,8 +76,7 @@ var ( // Declare exported dummy vars. CSClusterTagKey string CSClusterTagVal string CSClusterTag map[string]string - CreatedByCapcKey string - CreatedByCapcVal string + CreatedByCAPCTag []csapi.Tags FWRuleID string LBRuleID string LoadBalancerRuleIDs []string @@ -144,8 +143,6 @@ func SetDummyTagVars() { CSClusterTagKey = "CAPC_cluster_" + string(CSCluster.ObjectMeta.UID) CSClusterTagVal = "1" CSClusterTag = map[string]string{CSClusterTagVal: CSClusterTagVal} - CreatedByCapcKey = "create_by_CAPC" - CreatedByCapcVal = "" Tag1Key = "test_tag1" Tag1Val = "arbitrary_value1" Tag2Key = "test_tag2" @@ -153,6 +150,12 @@ func SetDummyTagVars() { Tag1 = map[string]string{Tag2Key: Tag2Val} Tag2 = map[string]string{Tag2Key: Tag2Val} Tags = map[string]string{Tag1Key: Tag1Val, Tag2Key: Tag2Val} + CreatedByCAPCTag = []csapi.Tags{ + { + Key: cloud.CreatedByCAPCTagName, + Value: "1", + }, + } } func SetCSMachineOwner() {