Copilot commented on code in PR #109:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/109#discussion_r4229515472
##########
test/e2e/vpc_test.go:
##########
@@ -326,3 +328,177 @@ 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
+}
Review Comment:
`requireNoACLRules` is described as guarding tests that “count every rule on
their port”, but it currently only checks *ingress tcp* rules (via
`aclRulesOnPort`). This can lead to confusing failures if non-ingress or
non-tcp rules exist on the port (e.g., an egress rule will be ignored by
`requireNoACLRules` but could still affect other counting/assertions like
`countACLRules`). Consider aligning the “leftover” detection with whatever the
tests actually count/assert, either by broadening
`aclRulesOnPort`/`requireNoACLRules` to match that scope or tightening the
assertions to only consider ingress tcp.
##########
cloudstack_loadbalancer.go:
##########
@@ -1972,57 +1992,176 @@ func (lb *loadBalancer) updateNetworkACL(publicPort
int, protocol LoadBalancerPr
return true, err
}
- networkAclParams := lb.NetworkACL.NewListNetworkACLsParams()
- networkAclParams.SetAclid(network.Aclid)
- networkAclParams.SetNetworkid(networkId)
- networkAclParams.SetListall(true)
+ ipProtocol := protocol.IPProtocol()
+ aclRules, err := lb.listNetworkACLRules(networkId, ipProtocol,
publicPort)
+ if err != nil {
+ return false, fmt.Errorf("error fetching Network ACL with ID:
%v for network with id: %v, due to: %s", network.Aclid, networkId, err)
+ }
+
+ // Only a root admin may change the rules of a global list, the kind
that belongs to no VPC.
+ adoptable := networkAclList.Vpcid != ""
+ kind, aclRule := lb.decidingNetworkACLRule(aclRules, adoptable)
+ switch kind {
+ case aclRuleOwn:
+ klog.V(4).Infof("Network ACL rule %v already opens %v port %v
for load balancer %v", aclRule.Id, protocol, publicPort, lb.name)
+ return true, nil
+ case aclRuleOperator:
+ klog.Infof("Network ACL rule %v already covers %v port %v on
network %v; not adding one for load balancer %v", aclRule.Id, protocol,
publicPort, networkId, lb.name)
+ return true, nil
+ case aclRuleLegacy:
+ if err := lb.adoptNetworkACLRule(aclRule, network.Aclid,
networkId, ipProtocol, publicPort); err != nil {
+ return false, err
+ }
+ return true, nil
+ }
+
+ if err := lb.createNetworkACLRule(network.Aclid, networkId, ipProtocol,
publicPort); err != nil {
+ return false, err
+ }
+ return true, nil
+}
+
+// networkACLReasonPrefix starts the reason of every Network ACL rule the
controller creates.
+// The rest of the reason names the owning load balancer, and only rules
carrying a load
+// balancer's own reason are ever deleted for it, so this text must not change.
+const networkACLReasonPrefix = "Managed by the CloudStack Kubernetes Provider
for "
+
+// networkACLReason is the reason that marks a Network ACL rule as this load
balancer's.
+func (lb *loadBalancer) networkACLReason() string {
+ return networkACLReasonPrefix + lb.name
+}
+
+// networkACLRuleKind is what an existing Network ACL rule on a needed port
means to a load
+// balancer. The kinds are ordered by precedence.
+type networkACLRuleKind int
+
+const (
+ aclRuleIgnored networkACLRuleKind = iota // being deleted, or another
load balancer's
+ aclRuleOperator // someone else's; the port
is left to them
+ aclRuleLegacy // made by a release before
rules had owners
+ aclRuleOwn // this load balancer's
+)
+
+// classifyNetworkACLRule tells what an ingress rule on the needed port is to
the load balancer
+// whose reason is given. CloudStack shows a rule being revoked as Deleting,
and updating such a
+// rule would bring it back, so it never counts. A rule from an earlier
release has exactly the
+// shape those releases created; any other unmarked rule, such as one with a
restricted CIDR, a
+// deny rule or "0.0.0.0/0,::/0", is someone else's.
+func classifyNetworkACLRule(aclRule *cloudstack.NetworkACL, reason string)
networkACLRuleKind {
+ switch {
+ case aclRule.State == "Deleting":
+ return aclRuleIgnored
+ case aclRule.Reason == reason:
+ return aclRuleOwn
+ case strings.HasPrefix(aclRule.Reason, networkACLReasonPrefix):
+ return aclRuleIgnored
+ case aclRule.Reason == "" && aclRule.Action == "Allow" &&
+ compareStringSlice(splitCIDRList(aclRule.Cidrlist),
[]string{defaultAllowedCIDR}):
+ return aclRuleLegacy
+ }
+ return aclRuleOperator
+}
+
+// decidingNetworkACLRule picks the rule that decides what this load balancer
does about a port:
+// its own rule, else one from an earlier release to adopt, else someone
else's. A rule from an
+// earlier release counts as someone else's when it cannot be adopted.
+func (lb *loadBalancer) decidingNetworkACLRule(aclRules
[]*cloudstack.NetworkACL, adoptable bool) (networkACLRuleKind,
*cloudstack.NetworkACL) {
+ kind, deciding := aclRuleIgnored, (*cloudstack.NetworkACL)(nil)
+ for _, aclRule := range aclRules {
+ k := classifyNetworkACLRule(aclRule, lb.networkACLReason())
+ if k == aclRuleLegacy && !adoptable {
+ k = aclRuleOperator
+ }
+ if k > kind {
+ kind, deciding = k, aclRule
+ }
+ }
+ return kind, deciding
+}
+
+// listNetworkACLRules lists the ingress rules of a tier for exactly one port
and IP protocol.
+// Listing by network covers the ACL list the tier uses now. CloudStack stores
the protocol as it
+// was sent, and the protocol is compared exactly, as earlier releases did, so
rules stored in upper
+// case, such as the "TCP" rules CloudStack's Kubernetes service adds for its
own ports, are left
+// alone.
+func (lb *loadBalancer) listNetworkACLRules(networkID, ipProtocol string,
publicPort int) ([]*cloudstack.NetworkACL, error) {
+ p := lb.NetworkACL.NewListNetworkACLsParams()
+ p.SetNetworkid(networkID)
+ p.SetListall(true)
if lb.projectID != "" {
- networkAclParams.SetProjectid(lb.projectID)
+ p.SetProjectid(lb.projectID)
}
Review Comment:
The previous implementation listed ACLs using both `aclid` and `networkid`,
but `listNetworkACLRules` now filters only by `networkid`. Given the call sites
already know the tier’s ACL list ID (`network.Aclid`), adding `p.SetAclid(...)`
would reduce the amount of data paged through and lower the chance of
unintentionally including rules from outside the intended list (depending on
CloudStack API behavior). Concretely, consider passing `aclID` into
`listNetworkACLRules` and setting it on the request params.
##########
test/e2e/vpc_test.go:
##########
@@ -326,3 +328,177 @@ 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 rules for the port are left over from
an earlier run,
+// since the tests below count every rule on their port.
+func requireNoACLRules(f *Framework, aclID, port string) {
+ f.T.Helper()
+ rules, err := aclRulesOnPort(f, aclID, port)
+ if err != nil {
+ f.T.Fatalf("listing ACL rules for port %s: %v", port, err)
+ }
+ for _, r := range rules {
+ f.T.Errorf("leftover ACL rule %s for port %s (reason %q);
delete it before rerunning", r.Id, port, r.Reason)
+ }
+ if len(rules) > 0 {
Review Comment:
`requireNoACLRules` is described as guarding tests that “count every rule on
their port”, but it currently only checks *ingress tcp* rules (via
`aclRulesOnPort`). This can lead to confusing failures if non-ingress or
non-tcp rules exist on the port (e.g., an egress rule will be ignored by
`requireNoACLRules` but could still affect other counting/assertions like
`countACLRules`). Consider aligning the “leftover” detection with whatever the
tests actually count/assert, either by broadening
`aclRulesOnPort`/`requireNoACLRules` to match that scope or tightening the
assertions to only consider ingress tcp.
##########
test/e2e/vpc_test.go:
##########
@@ -326,3 +328,177 @@ 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 rules for the port are left over from
an earlier run,
+// since the tests below count every rule on their port.
+func requireNoACLRules(f *Framework, aclID, port string) {
+ f.T.Helper()
+ rules, err := aclRulesOnPort(f, aclID, port)
+ if err != nil {
+ f.T.Fatalf("listing ACL rules for port %s: %v", port, err)
+ }
+ for _, r := range rules {
+ f.T.Errorf("leftover ACL rule %s for port %s (reason %q);
delete it before rerunning", r.Id, port, r.Reason)
+ }
+ if len(rules) > 0 {
+ f.T.FailNow()
+ }
+}
+
+// TestVPC_ACLRulePerService is a regression test for issue #107: two Services
+// on one tier exposing the same port each get an ACL rule of their own, so
+// deleting one Service leaves the port open for the other. Earlier releases
+// shared one rule between them and deleted it with the first Service, which
+// this test reports as the port no longer being open.
+func TestVPC_ACLRulePerService(t *testing.T) {
+ f, aclID, _ := vpcFramework(t)
+
+ const port = "80"
+ requireNoACLRules(f, aclID, port)
+
+ first := f.CreateLBService(nil)
+ second := f.CreateLBService(func(s *corev1.Service) { s.Name =
"e2e-second" })
+ f.WaitForIngressIP(first)
+ f.WaitForIngressIP(second)
+ firstReason := aclRuleMarker(defaultLoadBalancerName(first))
+ secondReason := aclRuleMarker(defaultLoadBalancerName(second))
+
+ f.Eventually(lbSyncTimeout, lbSyncInterval, "port "+port+" to be open
on the tier",
+ func() (bool, error) {
+ rules, err := aclRulesOnPort(f, aclID, port)
+ return opensPortToAll(rules), err
+ })
+
+ f.DeleteServiceAndWait(first)
+
+ rules, err := aclRulesOnPort(f, aclID, port)
+ if err != nil {
+ t.Fatalf("listing ACL rules: %v", err)
+ }
+ if !opensPortToAll(rules) {
+ t.Fatalf("port %s is no longer open on the tier after deleting
one of the two Services using it", port)
+ }
+ if !hasACLRuleWithReason(rules, secondReason) {
+ t.Errorf("the remaining Service has no ACL rule of its own for
port %s", port)
+ }
+ f.Eventually(lbSyncTimeout, lbSyncInterval, "the deleted Service's ACL
rule to be removed",
+ func() (bool, error) {
+ rules, err := aclRulesOnPort(f, aclID, port)
+ return !hasACLRuleWithReason(rules, firstReason), err
+ })
+}
+
+// opensPortToAll reports whether one of the rules allows the port from
anywhere and is not being
+// deleted.
+func opensPortToAll(rules []*cloudstack.NetworkACL) bool {
+ for _, r := range rules {
+ if strings.EqualFold(r.Action, "Allow") && r.State !=
"Deleting" &&
+ strings.Contains(r.Cidrlist, "0.0.0.0/0") {
+ return true
+ }
+ }
+ return false
+}
Review Comment:
`strings.Contains(r.Cidrlist, "0.0.0.0/0")` can return true for unrelated
CIDRs like `"10.0.0.0/0"` (it contains the substring `"0.0.0.0/0"` starting at
index 1). Parse/split the CIDR list (similar to `splitCIDRList`) and check for
an exact entry match to `0.0.0.0/0` instead of substring matching.
--
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]
