Copilot commented on code in PR #104:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/104#discussion_r4080666038
##########
cloudstack_loadbalancer.go:
##########
@@ -157,96 +174,33 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx
context.Context, clusterName string, s
klog.V(4).Infof("Load balancer %v is associated with IP %v", lb.name,
lb.ipAddr)
- 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)
-
- // If the load balancer rule exists and is up-to-date, we move
on to the next rule.
- lbRule, needsUpdate, err :=
lb.checkLoadBalancerRule(lbRuleName, port, protocol, service, cs.version)
- if err != nil {
- return nil, err
- }
-
- if lbRule != nil {
- if needsUpdate {
- klog.V(4).Infof("Updating load balancer rule:
%v", lbRuleName)
- if err := lb.updateLoadBalancerRule(lbRuleName,
protocol, service, cs.version); err != nil {
- return nil, err
- }
- // Delete the rule from the map, to prevent it
being deleted.
- delete(lb.rules, lbRuleName)
- } else {
- klog.V(4).Infof("Load balancer rule %v is
up-to-date", lbRuleName)
- // Delete the rule from the map, to prevent it
being deleted.
- delete(lb.rules, lbRuleName)
- }
- } else {
- klog.V(4).Infof("Creating load balancer rule: %v",
lbRuleName)
- lbRule, err = lb.createLoadBalancerRule(lbRuleName,
port, protocol, service)
- if err != nil {
- return nil, err
- }
-
- klog.V(4).Infof("Assigning hosts (%v) to load balancer
rule: %v", lb.hostIDs, lbRuleName)
- if err = lb.assignHostsToRule(lbRule, lb.hostIDs); err
!= nil {
- return nil, err
- }
- }
-
- network, count, err := lb.Network.GetNetworkByID(lb.networkID,
cloudstack.WithProject(lb.projectID))
- if err != nil {
- if count == 0 {
- return nil, err
- }
- return nil, err
- }
+ // Resolve every service port to the rule that should represent it.
+ desired, err := lb.resolveLoadBalancerRules(service, cs.version)
+ if err != nil {
+ return nil, err
+ }
- if lbRule != nil {
- if isFirewallSupported(network.Service) {
- klog.V(4).Infof("Creating firewall rules for
load balancer rule: %v (%v:%v:%v)", lbRuleName, protocol, lbRule.Publicip,
port.Port)
- if _, err :=
lb.updateFirewallRule(lbRule.Publicipid, int(port.Port), protocol,
service.Spec.LoadBalancerSourceRanges); err != nil {
- return nil, err
- }
- } else if isNetworkACLSupported(network.Service) {
- klog.V(4).Infof("Creating ACL rules for load
balancer rule: %v (%v:%v:%v)", lbRuleName, protocol, lbRule.Publicip, port.Port)
- if _, err :=
lb.updateNetworkACL(int(port.Port), protocol, network.Id); err != nil {
- return nil, err
- }
- }
- }
+ network, _, err := lb.Network.GetNetworkByID(lb.networkID,
cloudstack.WithProject(lb.projectID))
+ if err != nil {
+ return nil, err
}
Review Comment:
`EnsureLoadBalancer` now ignores the `count` return value from
`GetNetworkByID`. Elsewhere in this PR (`networkByID`) you explicitly handle
`count == 0` as \"not found\" with `network = nil`. If `GetNetworkByID` can
return `(nil, 0, nil)` for a missing network, the current code will pass a nil
`network` into `pruneRules`/`applyLoadBalancerRules` and crash when
dereferencing `network.Id` or `network.Service`. Capture `count` and return a
clear error when `count == 0` (or `network == nil`) before proceeding.
--
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]
