Copilot commented on code in PR #104:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/104#discussion_r4044384296
##########
cloudstack_loadbalancer.go:
##########
@@ -681,11 +638,322 @@ func (lb *loadBalancer) getCIDRList(service
*corev1.Service) ([]string, error) {
return cidrList, nil
}
-// checkLoadBalancerRule checks if the rule already exists and if it does, if
it can be updated. If
-// it does exist but cannot be updated, it will delete the existing rule so it
can be created again.
-func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port
corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service,
version semver.Version) (*cloudstack.LoadBalancerRule, bool, error) {
- lbRule, ok := lb.rules[lbRuleName]
- if !ok {
+// splitCIDRList splits the CIDR list of an existing CloudStack rule into its
entries.
+// CloudStack has reported these both comma and space separated, and a CIDR
can contain
+// neither character, so treat both as separators.
+func splitCIDRList(cidrList string) []string {
+ return strings.FieldsFunc(cidrList, func(r rune) bool {
+ return r == ',' || r == ' '
+ })
+}
+
+// resolveLoadBalancerRules maps every service port to the load balancer rule
that should
+// represent it, claiming each match as it goes so that what remains in
lb.rules is exactly
+// the obsolete set and no rule can be claimed twice.
+func (lb *loadBalancer) resolveLoadBalancerRules(service *corev1.Service,
version semver.Version) ([]desiredLBRule, error) {
+ desired := make([]desiredLBRule, 0, len(service.Spec.Ports))
+
+ for _, port := range service.Spec.Ports {
+ // Construct the protocol name first, we need it a few times
+ protocol := ProtocolFromServicePort(port, service)
+ if protocol == LoadBalancerProtocolInvalid {
+ return nil, fmt.Errorf("unsupported load balancer
protocol: %v", port.Protocol)
+ }
+
+ // All ports have their own load balancer rule, so add the port
to lbName to keep the names unique.
+ lbRuleName := fmt.Sprintf("%s-%s-%d", lb.name, protocol,
port.Port)
+
+ lbRule, needsUpdate, err :=
lb.checkLoadBalancerRule(lb.findLoadBalancerRule(lbRuleName, port, protocol),
lbRuleName, port, protocol, service, version)
+ if err != nil {
+ return nil, err
+ }
+
+ if lbRule != nil {
+ // Claim by the rule's actual name: after a protocol
change it still carries the old one.
+ delete(lb.rules, lbRule.Name)
+ }
+
+ desired = append(desired, desiredLBRule{
+ name: lbRuleName,
+ port: port,
+ protocol: protocol,
+ existing: lbRule,
+ update: needsUpdate,
+ })
+ }
+
+ return desired, nil
+}
+
+// findLoadBalancerRule locates the existing CloudStack rule for a desired
service port. It
+// prefers an exact name match, then falls back to matching on the tuple. That
fallback is what
+// lets a protocol change (tcp <-> tcp-proxy) update the existing rule instead
of creating a
+// conflicting one.
+//
+// Only rules on the IP being reconciled towards are eligible; a rule on any
other IP is left
+// for the prune pass, which also cleans up the firewall rules it leaves
behind.
+func (lb *loadBalancer) findLoadBalancerRule(lbRuleName string, port
corev1.ServicePort, protocol LoadBalancerProtocol) *cloudstack.LoadBalancerRule
{
+ if lbRule, ok := lb.rules[lbRuleName]; ok && lbRule.Publicipid ==
lb.ipAddrID {
+ return lbRule
+ }
+
+ publicPort := strconv.Itoa(int(port.Port))
+ var names []string
+ for name, lbRule := range lb.rules {
+ if lbRule.Publicipid == lb.ipAddrID &&
+ ProtocolFromLoadBalancer(lbRule.Protocol).IPProtocol()
== protocol.IPProtocol() &&
+ lbRule.Publicport == publicPort {
+ names = append(names, name)
+ }
+ }
+ if len(names) == 0 {
+ return nil
+ }
+
+ // Map iteration order is randomized; sort so the pick is deterministic.
+ sort.Strings(names)
+ if len(names) > 1 {
+ klog.Warningf("Multiple load balancer rules match %s port %s:
%v; using %v", protocol.IPProtocol(), publicPort, names, names[0])
+ }
+ return lb.rules[names[0]]
+}
+
+// portProtocol is the tuple CloudStack refuses to place two load balancer
rules on, and that
+// firewall and network ACL rules are keyed on. IPProtocol maps both tcp and
tcp-proxy to
+// "tcp", so a tcp and a tcp-proxy rule on one port share a tuple, and one
firewall/ACL rule.
+type portProtocol struct {
+ ipProtocol string
+ publicPort int32
+}
+
+// obsoleteRule is a rule no desired service port claimed, with its tuple
already parsed.
+type obsoleteRule struct {
+ rule *cloudstack.LoadBalancerRule
+ protocol LoadBalancerProtocol
+ tuple portProtocol
+}
+
+// partitionObsoleteRules splits the rules left in lb.rules — those no desired
port claimed —
+// into the ones holding a tuple that a rule still to be created needs, and
the rest.
+func (lb *loadBalancer) partitionObsoleteRules(desired []desiredLBRule)
(blocking, rest []obsoleteRule) {
+ // CloudStack refuses two load balancer rules with overlapping public
port ranges on one
+ // IP whatever their protocols, so the port alone decides what blocks a
create. Note this
+ // is deliberately coarser than the firewall/ACL claim, which is per
protocol because
+ // firewall rules are.
+ neededPorts := make(map[int32]bool)
+ for _, d := range desired {
+ if d.existing == nil {
+ neededPorts[d.port.Port] = true
+ }
+ }
+
+ // Iterate in name order so the prune sequence is reproducible.
+ names := make([]string, 0, len(lb.rules))
+ for name := range lb.rules {
+ names = append(names, name)
+ }
+ sort.Strings(names)
+
+ for _, name := range names {
+ lbRule := lb.rules[name]
+
+ port, err := strconv.ParseInt(lbRule.Publicport, 10, 32)
+ if err != nil {
+ klog.Errorf("Skipping obsolete load balancer rule %v
with invalid public port %v: %v", lbRule.Name, lbRule.Publicport, err)
+ continue
+ }
+
+ // Conflicts are per public IP, so only a rule on the IP being
reconciled towards can
+ // block a create.
+ blocksACreate := lbRule.Publicipid == lb.ipAddrID &&
neededPorts[int32(port)]
+
+ // A protocol the provider cannot interpret leaves its firewall
or ACL rule
+ // unresolvable, so such a rule is normally left alone. One
holding a port a create
+ // needs still has to go, or CloudStack rejects that create as
a port conflict.
+ protocol := ProtocolFromLoadBalancer(lbRule.Protocol)
+ if protocol == LoadBalancerProtocolInvalid && !blocksACreate {
+ klog.Errorf("Skipping obsolete load balancer rule %v
with unknown protocol %v", lbRule.Name, lbRule.Protocol)
+ continue
+ }
+
+ obsolete := obsoleteRule{
+ rule: lbRule,
+ protocol: protocol,
+ tuple: portProtocol{protocol.IPProtocol(),
int32(port)},
+ }
+
+ if blocksACreate {
+ blocking = append(blocking, obsolete)
+ } else {
+ rest = append(rest, obsolete)
+ }
+ }
+
+ return blocking, rest
+}
+
+// ruleNetworkID is the network whose ACL rules an existing load balancer rule
was opened in, or
+// "" when that network cannot be established. CloudStack omits the network on
rules of some
+// network types; such a rule is known to belong to the network being
reconciled towards only
+// when it sits on the public IP being reconciled towards.
+func (lb *loadBalancer) ruleNetworkID(lbRule *cloudstack.LoadBalancerRule)
string {
+ if lbRule.Networkid != "" {
+ return lbRule.Networkid
+ }
+ if lbRule.Publicipid == lb.ipAddrID {
+ return lb.networkID
+ }
+ return ""
+}
+
+// claimedTuples are the tuples the desired service ports still need, and
whose firewall or
+// network ACL rules therefore have to survive a prune.
+func claimedTuples(desired []desiredLBRule) map[portProtocol]bool {
+ claimed := make(map[portProtocol]bool, len(desired))
+ for _, d := range desired {
+ claimed[portProtocol{d.protocol.IPProtocol(), d.port.Port}] =
true
+ }
+ return claimed
+}
+
+// pruneFirewallRule deletes the firewall rule admitting traffic to an
obsolete load balancer
+// rule. Firewall rules belong to a single public IP, so a claim only covers a
rule on the IP
+// the service is being reconciled towards.
+func (lb *loadBalancer) pruneFirewallRule(o obsoleteRule, claimed
map[portProtocol]bool) error {
+ lbRule, port := o.rule, int(o.tuple.publicPort)
+
+ if claimed[o.tuple] && lbRule.Publicipid == lb.ipAddrID {
+ klog.V(4).Infof("Keeping firewall rules of obsolete load
balancer rule %v (%v:%v:%v): still claimed by a service port", lbRule.Name,
o.protocol, lbRule.Publicip, port)
+ return nil
+ }
+
+ klog.V(4).Infof("Deleting firewall rules associated with load balancer
rule: %v (%v:%v:%v)", lbRule.Name, o.protocol, lbRule.Publicip, port)
+ _, err := lb.deleteFirewallRule(lbRule.Publicipid, port, o.protocol)
+ return err
+}
+
+// pruneNetworkACLRule deletes the network ACL rule admitting traffic to an
obsolete load
+// balancer rule. ACL rules belong to a network rather than an IP, so a claim
only covers a rule
+// in the network the service is being reconciled towards. A rule whose
network cannot be
+// established is left alone: an ACL rule deleted in a guessed network could
be the only opening
+// another service has.
+func (lb *loadBalancer) pruneNetworkACLRule(o obsoleteRule, claimed
map[portProtocol]bool) error {
+ lbRule, port := o.rule, int(o.tuple.publicPort)
+ networkID := lb.ruleNetworkID(lbRule)
+
+ switch {
+ case networkID == "":
+ klog.Warningf("Leaving the Network ACL rule of obsolete load
balancer rule %v (%v:%v) in place: its network is unknown", lbRule.Name,
o.protocol, port)
+ case claimed[o.tuple] && networkID == lb.networkID:
+ klog.V(4).Infof("Keeping Network ACL rules of obsolete load
balancer rule %v (%v:%v:%v): still claimed by a service port", lbRule.Name,
o.protocol, networkID, port)
+ default:
+ klog.V(4).Infof("Deleting Network ACL rules associated with
load balancer rule: %v (%v:%v:%v)", lbRule.Name, o.protocol, networkID, port)
+ if _, err := lb.deleteNetworkACLRule(port, o.protocol,
networkID); err != nil {
+ return err
+ }
+ }
+
+ return nil
+}
+
+// pruneRuleOpening deletes the firewall or network ACL rule admitting traffic
to an obsolete
+// load balancer rule, unless a desired service port still claims that same
opening. A rule with
+// an uninterpretable protocol keeps its opening, which cannot be identified
without one.
+func (lb *loadBalancer) pruneRuleOpening(o obsoleteRule, claimed
map[portProtocol]bool, network *cloudstack.Network) error {
+ switch {
+ case o.protocol == LoadBalancerProtocolInvalid:
+ klog.Warningf("Leaving the firewall or Network ACL rule of
obsolete load balancer rule %v in place: unknown protocol %v", o.rule.Name,
o.rule.Protocol)
+ return nil
+ case isFirewallSupported(network.Service):
+ return lb.pruneFirewallRule(o, claimed)
+ case isNetworkACLSupported(network.Service):
+ return lb.pruneNetworkACLRule(o, claimed)
Review Comment:
The cleanup mechanism is chosen from the current network's services, not
from the obsolete rule's network. If a stale rule came from a VPC/NetworkACL
tier while the service now reconciles on a Firewall tier (or vice versa), this
calls the wrong delete helper, then still deletes the load-balancer rule and
leaves the old ACL/firewall opening behind. Resolve the obsolete rule's
network/service type, or clean the corresponding mechanism before deleting the
rule.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
