Copilot commented on code in PR #104:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/104#discussion_r4081232981
##########
cloudstack_loadbalancer.go:
##########
@@ -711,11 +669,373 @@ 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)
+ }
Review Comment:
When `checkLoadBalancerRule` decides a rule must be recreated, it deletes
the CloudStack rule and returns `nil`. In `resolveLoadBalancerRules`, the rule
is only removed from `lb.rules` when the returned `lbRule != nil`, so the
just-deleted rule can remain in `lb.rules` and be picked up later as ‘obsolete’
and pruned again (including deleting firewall/ACL openings). This can lead to
double-delete errors and avoidable traffic interruption (openings pruned before
the replacement rule is created). Fix by ensuring the deleted rule is also
removed from `lb.rules` in the recreate path (e.g., delete by the existing
rule’s actual name before returning), or by having `resolveLoadBalancerRules`
remove the candidate it passed into `checkLoadBalancerRule` when
`checkLoadBalancerRule` returns `nil` after deletion.
##########
cloudstack_loadbalancer.go:
##########
@@ -485,7 +439,9 @@ func (cs *CSCloud) getLoadBalancer(service *corev1.Service)
(*loadBalancer, erro
lbRules = dedupeByID(lbRules, func(rule *cloudstack.LoadBalancerRule)
string { return rule.Id })
// Keeping the rule on the address the Service is already published on
stops a
- // duplicate sweep from deleting the rule that clients and DNS are
pointing at.
+ // duplicate sweep from deleting the rule that clients and DNS are
pointing at. The
+ // same address is the one the rules are reconciled towards when they
span several,
+ // so which IP wins does not depend on the order CloudStack lists the
rules in.
Review Comment:
The comment says IP selection ‘does not depend on the order CloudStack lists
the rules in’, but the condition `lb.ipAddr != preferredIP` causes `lb.ipAddr`
to be overwritten on each iteration when `preferredIP == \"\"` (common when
neither `spec.loadBalancerIP` nor `status.loadBalancer.ingress` is set). That
makes the resolved IP depend on list order and can cause flapping. Consider
updating the logic to only switch to `preferredIP` when `preferredIP != \"\"`
and the current rule matches it, otherwise keep the first resolved IP; or
update the comment if order-dependence is intended in that case.
##########
cloudstack_loadbalancer.go:
##########
@@ -711,11 +669,373 @@ 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 == ' '
+ })
+}
Review Comment:
`splitCIDRList` only treats the literal space `' '` as whitespace in
addition to commas. If CloudStack (or annotations) ever include other
whitespace (tabs/newlines), the split will not behave as intended. A small
robustness improvement is to treat any Unicode whitespace as a separator (e.g.,
use a whitespace predicate) while still splitting on commas.
##########
cloudstack_loadbalancer.go:
##########
@@ -509,8 +465,10 @@ func (cs *CSCloud) getLoadBalancer(service
*corev1.Service) (*loadBalancer, erro
klog.Warningf("Load balancer for service %v/%v has
rules associated with different IP's: %v, %v", service.Namespace, service.Name,
lb.ipAddr, lbRule.Publicip)
}
- lb.ipAddr = lbRule.Publicip
- lb.ipAddrID = lbRule.Publicipid
+ if lb.ipAddr == "" || lb.ipAddr != preferredIP {
Review Comment:
The comment says IP selection ‘does not depend on the order CloudStack lists
the rules in’, but the condition `lb.ipAddr != preferredIP` causes `lb.ipAddr`
to be overwritten on each iteration when `preferredIP == \"\"` (common when
neither `spec.loadBalancerIP` nor `status.loadBalancer.ingress` is set). That
makes the resolved IP depend on list order and can cause flapping. Consider
updating the logic to only switch to `preferredIP` when `preferredIP != \"\"`
and the current rule matches it, otherwise keep the first resolved IP; or
update the comment if order-dependence is intended in that case.
--
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]
