vishesh92 commented on code in PR #104:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/104#discussion_r4080085880
##########
cloudstack_loadbalancer.go:
##########
@@ -681,11 +639,372 @@ 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, in the network that rule belongs to. 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.
+func (lb *loadBalancer) pruneNetworkACLRule(o obsoleteRule, claimed
map[portProtocol]bool, networkID string) error {
+ lbRule, port := o.rule, int(o.tuple.publicPort)
+
+ if 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)
+ return nil
Review Comment:
Correct that the ACL entry is tier-wide and shared, but this is how the ACL
path has worked since it landed in #69: updateNetworkACL matches on protocol
and port with no ownership marker, so two Services on one tier already share a
single entry, and both the previous cleanup loop and EnsureLoadBalancerDeleted
delete it the same way. This PR only narrows that: an opening this Service
still claims is kept, and the delete is scoped to the obsolete rule's own
network rather than the reconciled one. Fixing the cross-Service case means
tagging the openings the controller creates and deleting only tagged entries,
which also has to cover the delete path and the public IP release, so I'll
track it in https://github.com/apache/cloudstack-kubernetes-provider/issues/107
rather than this PR.
--
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]
