Copilot commented on code in PR #109:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/109#discussion_r4257590581
##########
cloudstack_loadbalancer.go:
##########
@@ -394,6 +404,11 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx
context.Context, clusterName st
}
}
+ // Releasing the IP would make CloudStack revoke the rules kept for the
retry.
+ if aclErr != nil {
+ return errors.Join(aclErr, sweepErr)
+ }
Review Comment:
`errors.Join` requires Go 1.20+. If the project supports Go < 1.20, this
introduces a compile-time break. If older Go versions must be supported, prefer
returning `aclErr` when non-nil and separately logging/attaching `sweepErr` (or
implement a compatible multi-error pattern).
##########
test/e2e/vpc_test.go:
##########
@@ -24,10 +24,12 @@ package e2e
import (
"context"
"os"
+ "slices"
"strconv"
"strings"
"testing"
+ "github.com/apache/cloudstack-go/v2/cloudstack"
Review Comment:
Importing `slices` requires Go 1.21+. If the repo’s supported Go version
(go.mod/CI toolchain) is < 1.21, this will fail to compile; in that case,
replace it with a small local helper (or an existing project utility) to check
membership in `[]string`. If the repo already targets Go 1.21+, consider
adding/confirming that in go.mod and CI to make the dependency explicit.
##########
test/e2e/vpc_test.go:
##########
@@ -326,3 +329,188 @@ func TestVPC_ProxyProtocolACL(t *testing.T) {
t.Errorf("ACL rules for port %s = %d, want exactly 1 across the
toggle", port, n)
}
}
+
+// aclRulesOnPort returns the ingress tcp ACL rules on the list for one port.
+func aclRulesOnPort(f *Framework, aclID, port string)
([]*cloudstack.NetworkACL, error) {
+ rules, err := f.ACLRules(aclID)
+ if err != nil {
+ return nil, err
+ }
+ var onPort []*cloudstack.NetworkACL
+ for _, r := range rules {
+ if r.Startport == port && r.Endport == port &&
strings.EqualFold(r.Protocol, "tcp") &&
+ strings.EqualFold(r.Traffictype, "Ingress") {
+ onPort = append(onPort, r)
+ }
+ }
+ return onPort, nil
+}
+
+// hasACLRuleWithReason reports whether one of the rules carries the reason.
+func hasACLRuleWithReason(rules []*cloudstack.NetworkACL, reason string) bool {
+ for _, r := range rules {
+ if r.Reason == reason {
+ return true
+ }
+ }
+ return false
+}
+
+// requireNoACLRules stops the test when any rule for the port, whatever its
protocol or
+// direction, is left over from an earlier run, since the tests below count
rules on their port.
+func requireNoACLRules(f *Framework, aclID, port string) {
+ f.T.Helper()
+ rules, err := f.ACLRules(aclID)
+ if err != nil {
+ f.T.Fatalf("listing ACL rules for port %s: %v", port, err)
+ }
+ leftover := false
+ for _, r := range rules {
+ if r.Startport == port && r.Endport == port {
+ f.T.Errorf("leftover ACL rule %s for port %s (%s %s,
reason %q); delete it before rerunning",
+ r.Id, port, r.Traffictype, r.Protocol, r.Reason)
+ leftover = true
+ }
+ }
+ if leftover {
+ f.T.FailNow()
+ }
+}
Review Comment:
The comment says this fails the test when “any rule for the port”
(regardless of protocol/direction) is left over, but the check only catches
rules whose `Startport` and `Endport` exactly equal `port`. A leftover rule
that covers the port via a range (e.g. 80–90) would not be detected, yet can
still affect tier behavior and make these rule-counting tests unreliable.
Either (a) adjust the comment to explicitly say “exact single-port rules only”,
or (b) implement a range-inclusive check by parsing ports and flagging any rule
where `start <= port <= end` (and handling empty start/end safely).
##########
cloudstack_loadbalancer.go:
##########
@@ -2057,51 +2196,39 @@ func (lb *loadBalancer) deleteFirewallRule(publicIpId
string, publicPort int, pr
return deleted, err
}
-// Delete Network ACLs deletes the Network ACL rule associated with the
ip:port:protocol combo
+// deleteNetworkACLRule deletes this load balancer's Network ACL rules for a
port on a tier and
+// leaves every other rule alone, so another Service using the same port keeps
it open. A rule
+// CloudStack still shows as Deleting is deleted again, which retries applying
the list; a failure
+// there is only logged, as CloudStack removes the rule on its next successful
apply. Every rule
+// is tried and the first error is returned.
func (lb *loadBalancer) deleteNetworkACLRule(publicPort int, protocol
LoadBalancerProtocol, networkID string) (bool, error) {
- p := lb.NetworkACL.NewListNetworkACLsParams()
- p.SetListall(true)
- p.SetNetworkid(networkID)
- if lb.projectID != "" {
- p.SetProjectid(lb.projectID)
- }
-
- networkAcls, err := listAll(p, func() (int, []*cloudstack.NetworkACL,
error) {
- r, err := lb.NetworkACL.ListNetworkACLs(p)
- if err != nil {
- return 0, nil, err
- }
-
- return r.Count, r.NetworkACLs, nil
- })
+ aclRules, err := lb.listNetworkACLRules(networkID,
protocol.IPProtocol(), publicPort)
if err != nil {
return false, fmt.Errorf("error fetching Network ACL rules
Network ID %v: %v", networkID, err)
Review Comment:
This error path should wrap the underlying error with `%w` instead of
formatting it with `%v`, so callers/tests can unwrap and match specific failure
types. Suggested change: keep the message but use `%w` for `err`.
--
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]
