Copilot commented on code in PR #104:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/104#discussion_r4034181074
##########
cloudstack_loadbalancer.go:
##########
@@ -681,11 +634,261 @@ 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 {
Review Comment:
This filter assumes `lb.ipAddrID` is the service's current public IP, but
`getLoadBalancer` assigns that field from every returned rule, so a response
containing both current and stale-IP rules leaves the result dependent on API
order. In that case a valid rule on the published IP can be treated as obsolete
while reconciliation updates or creates rules on the stale IP. Select the
preferred/requested/status IP before resolving rules, or pass the target IP
explicitly.
##########
cloudstack_loadbalancer_test.go:
##########
@@ -2854,6 +3128,114 @@ func TestUpdateNetworkACL(t *testing.T) {
}
})
+ t.Run("tcp-proxy creates ACL rule with tcp protocol", func(t
*testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockNetwork := cloudstack.NewMockNetworkServiceIface(ctrl)
+ mockNetworkACL := cloudstack.NewMockNetworkACLServiceIface(ctrl)
+ networkResp := &cloudstack.Network{
+ Id: "net-123",
+ Aclid: "acl-456",
+ Service: []cloudstack.NetworkServiceInternal{},
+ }
+
+ aclListResp := &cloudstack.NetworkACLList{
+ Id: "acl-456",
+ Name: "custom-acl",
+ }
+
+ listParams := &cloudstack.ListNetworkACLsParams{}
+ listResp := &cloudstack.ListNetworkACLsResponse{
+ Count: 0,
+ NetworkACLs: []*cloudstack.NetworkACL{},
+ }
+
+ createParams := &cloudstack.CreateNetworkACLParams{}
+ createResp := &cloudstack.CreateNetworkACLResponse{
+ Id: "acl-rule-123",
+ }
+
+ gomock.InOrder(
+
mockNetwork.EXPECT().GetNetworkByID("net-123").Return(networkResp, 1, nil),
+
mockNetworkACL.EXPECT().GetNetworkACLListByID("acl-456").Return(aclListResp, 1,
nil),
Review Comment:
These expectations omit the variadic project option, but `updateNetworkACL`
always calls both methods with `cloudstack.WithProject(lb.projectID)` (even
when the project ID is empty). Gomock will see two arguments and reject these
one-argument expectations, so this new regression test fails before exercising
the ACL fix. Match the option as the existing test above does.
This issue also appears on line 3217 of the same file.
##########
cloudstack_loadbalancer.go:
##########
@@ -681,11 +634,261 @@ 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]
+
+ protocol := ProtocolFromLoadBalancer(lbRule.Protocol)
+ if protocol == LoadBalancerProtocolInvalid {
+ klog.Errorf("Skipping obsolete load balancer rule %v
with unknown protocol %v", lbRule.Name, lbRule.Protocol)
+ continue
+ }
+ 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
+ }
+
+ obsolete := obsoleteRule{
+ rule: lbRule,
+ protocol: protocol,
+ tuple: portProtocol{protocol.IPProtocol(),
int32(port)},
+ }
+
+ // Conflicts are per public IP, so only a rule on the IP being
reconciled towards can
+ // block a create.
+ if lbRule.Publicipid == lb.ipAddrID &&
neededPorts[obsolete.tuple.publicPort] {
+ blocking = append(blocking, obsolete)
+ } else {
+ rest = append(rest, obsolete)
+ }
+ }
+
+ return blocking, rest
+}
+
+// pruneRules deletes the given obsolete rules along with their firewall or
network ACL rules.
+// A firewall/ACL rule is kept when a desired port still claims the same
tuple, since the two
+// load balancer rules share it and pruning would strip the survivor of its
opening.
+//
+// A rule that fails to delete is reported but does not stop the others being
pruned.
+func (lb *loadBalancer) pruneRules(obsolete []obsoleteRule, desired
[]desiredLBRule, network *cloudstack.Network) error {
+ // Tuples the service still needs, and whose firewall/ACL rules
therefore have to survive.
+ claimed := make(map[portProtocol]bool, len(desired))
+ for _, d := range desired {
+ claimed[portProtocol{d.protocol.IPProtocol(), d.port.Port}] =
true
+ }
+
+ var firstErr error
+ recordErr := func(err error) {
+ klog.Errorf("Error pruning obsolete load balancer rule: %v",
err)
+ if firstErr == nil {
+ firstErr = err
+ }
+ }
+
+ for _, o := range obsolete {
+ lbRule, protocol, port := o.rule, o.protocol, o.tuple.publicPort
+
+ if isFirewallSupported(network.Service) {
+ // Firewall rules belong to a single public IP, so a
claim only covers a rule on
+ // the IP the service is being reconciled towards.
+ 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, protocol, lbRule.Publicip, port)
+ } else {
+ klog.V(4).Infof("Deleting firewall rules
associated with load balancer rule: %v (%v:%v:%v)", lbRule.Name, protocol,
lbRule.Publicip, port)
+ if _, err :=
lb.deleteFirewallRule(lbRule.Publicipid, int(port), protocol); err != nil {
+ recordErr(err)
+ continue
+ }
+ }
+ } else if isNetworkACLSupported(network.Service) {
+ // ACL rules belong to the network rather than an IP,
so the claim always applies.
+ if claimed[o.tuple] {
+ klog.V(4).Infof("Keeping Network ACL rules of
obsolete load balancer rule %v (%v:%v): still claimed by a service port",
lbRule.Name, protocol, port)
+ } else {
+ klog.V(4).Infof("Deleting Network ACL rules
associated with load balancer rule: %v (%v:%v)", lbRule.Name, protocol, port)
Review Comment:
Network ACLs are network-scoped, but `claimed` contains only protocol and
port and this branch keeps an obsolete rule's ACL whenever the desired rule has
the same tuple. If a service has stale rules from an old VPC/network, the
desired rule on the new network incorrectly protects the old ACL from deletion;
the deletion path also uses `lb.networkID` rather than the obsolete rule's
network. Include network identity in the claim and delete against the obsolete
rule's network.
##########
cloudstack_loadbalancer.go:
##########
@@ -681,11 +634,261 @@ 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]
+
+ protocol := ProtocolFromLoadBalancer(lbRule.Protocol)
+ if protocol == LoadBalancerProtocolInvalid {
+ klog.Errorf("Skipping obsolete load balancer rule %v
with unknown protocol %v", lbRule.Name, lbRule.Protocol)
+ continue
Review Comment:
This skips an obsolete rule before partitioning it. If CloudStack returns an
unrecognized protocol for a rule on the target IP and a needed port such as 80,
that rule still occupies the port, but no `blocking` entry is produced; the
apply phase then attempts the desired rule and can hit the same port-conflict
error this change is intended to avoid. Invalid protocols need a separate
cleanup/blocking path rather than being silently left in place.
##########
cloudstack.go:
##########
@@ -131,7 +131,7 @@ func (cs *CSCloud) getManagementServerVersion()
(semver.Version, error) {
parts := strings.Split(version, ".")
v, err := semver.ParseTolerant(strings.Join(parts[:min(len(parts), 3)],
"."))
if err != nil {
- klog.Errorf("failed to parse management server version: %v",
err)
+ klog.Errorf("failed to parse management server version %q: %v",
version, err)
Review Comment:
The advertised short-version startup fix is not present in this PR: the
unchanged production line 132 already bounds the slice with `min(len(parts),
3)`, and the existing `handles short version strings without panicking` test
already covers `4.22` and `4`. This hunk only changes logging, so either remove
the duplicate tests/claim or include the actual behavior change if it is still
required.
--
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]
