Copilot commented on code in PR #109:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/109#discussion_r4256381094
##########
cloudstack_loadbalancer.go:
##########
@@ -361,9 +362,12 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx
context.Context, clusterName st
if err != nil {
klog.Errorf("Error parsing port: %v", err)
} else {
- networkId, err :=
cs.getNetworkIDFromIPAddress(lb.ipAddrID)
- if err != nil {
- return err
+ networkId := lbRule.Networkid
+ if networkId == "" {
+ networkId, err =
cs.getNetworkIDFromIPAddress(lb.ipAddrID)
Review Comment:
When `lbRule.Networkid` is empty, the fallback uses `lb.ipAddrID` rather
than the IP associated with the specific rule being deleted. If the service
ever has rules on different IPs (e.g., duplicates/orphans), this can look up
the wrong network and delete/list ACLs in the wrong tier. Prefer deriving the
network from `lbRule.Publicipid` (or another rule-specific identifier) in this
fallback path.
##########
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)
+ }
Review Comment:
This newly introduced error wraps the underlying error using `%s`, which
loses unwrap-ability and makes it harder for callers/tests to use
`errors.Is/As`. Use `%w` for the underlying error (and `%v` for values like
IDs) so the original error is preserved in the chain.
##########
test/e2e/framework.go:
##########
@@ -559,6 +559,60 @@ func (f *Framework) ACLRules(aclListID string)
([]*cloudstack.NetworkACL, error)
return resp.NetworkACLs, nil
}
+// aclRuleMarker is the reason the CCM gives the Network ACL rules it creates
for a load balancer.
+func aclRuleMarker(lbName string) string {
+ return "Managed by the CloudStack Kubernetes Provider for " + lbName
Review Comment:
The reason prefix string is duplicated here and in production
(`networkACLReasonPrefix`). Since the production comment explicitly notes this
text must not change, duplicating it in e2e increases drift risk (tests could
start failing for the wrong reason, or miss a real regression). Consider
exposing the prefix (or a helper) from the production package for tests to
consume, or centralizing it in a shared constant location.
##########
docs/development.md:
##########
@@ -420,7 +425,8 @@ so a run is reproducible; avoid floating tags like `latest`.
| Nodes keep the uninitialized taint; CCM logs `provided node ip for node
"..." is not valid` | The CloudStack VM's NIC IP does not match the IP kubelet
registered with. Recreate the VM with `ipaddress=` set to the kind node's
docker IP. |
| Services stay `<pending>`; CCM logs `none of the hosts matched the list of
VMs retrieved from CS API` | No CloudStack VM has a name matching a Kubernetes
node name. |
| CCM logs `found hosts that belong to different networks` | VMs matching the
node names exist on more than one network — typically leftovers from a previous
scenario. |
-| No ACL rules are created on a VPC network | The tier uses `default_allow` or
`default_deny`. The CCM only manages rules on custom ACL lists. |
+| No ACL rules are created on a VPC network | The tier uses `default_allow` or
`default_deny`; the CCM only manages rules on custom ACL lists. Or the list
already has an ingress rule for the port that the CCM did not create, which it
leaves the port to. |
Review Comment:
The phrase “leaves the port to” is grammatically incomplete/unclear.
Consider rewording to explicitly name what the port is left to (e.g., “left to
that rule / left to the existing operator rule”) to avoid confusion when
troubleshooting.
##########
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)
}
- networkAcls, err := listAll(networkAclParams, func() (int,
[]*cloudstack.NetworkACL, error) {
- networkAclResponse, err :=
lb.NetworkACL.ListNetworkACLs(networkAclParams)
+ aclRules, err := listAll(p, func() (int, []*cloudstack.NetworkACL,
error) {
+ r, err := lb.NetworkACL.ListNetworkACLs(p)
if err != nil {
return 0, nil, err
}
- return networkAclResponse.Count,
networkAclResponse.NetworkACLs, nil
+ return r.Count, r.NetworkACLs, nil
})
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)
+ return nil, err
}
- // find all network ACL rules that have a matching proto+port
- // a map may or may not be faster, but is a bit easier to understand
- filtered := make(map[*cloudstack.NetworkACL]bool)
- for _, netAclRule := range networkAcls {
- if netAclRule.Protocol == protocol.IPProtocol() &&
netAclRule.Startport == strconv.Itoa(publicPort) && netAclRule.Endport ==
strconv.Itoa(publicPort) {
- filtered[netAclRule] = true
+ port := strconv.Itoa(publicPort)
+ var matching []*cloudstack.NetworkACL
+ for _, aclRule := range dedupeByID(aclRules, func(r
*cloudstack.NetworkACL) string { return r.Id }) {
+ if strings.EqualFold(aclRule.Traffictype, "Ingress") &&
aclRule.Protocol == ipProtocol &&
+ aclRule.Startport == port && aclRule.Endport == port {
+ matching = append(matching, aclRule)
}
}
- if len(filtered) > 0 {
- klog.V(4).Infof("Network ACL rule for port %v and protocol %v
already exists. No need to added a duplicate rule", publicPort, protocol)
- return true, err
+ return matching, nil
+}
+
+// adoptNetworkACLRule marks a rule an earlier release created as this load
balancer's. Only the
+// reason is sent: an update is partial by default, so CloudStack keeps the
other fields. An account
+// that may not update ACL rules creates a rule of its own instead, and one
that may not create
+// them either keeps relying on the old rule, as earlier releases did.
+func (lb *loadBalancer) adoptNetworkACLRule(aclRule *cloudstack.NetworkACL,
aclID, networkID, ipProtocol string, publicPort int) error {
+ p := lb.NetworkACL.NewUpdateNetworkACLItemParams(aclRule.Id)
+ p.SetReason(lb.networkACLReason())
+
+ _, err := lb.NetworkACL.UpdateNetworkACLItem(p)
+ if err == nil {
+ klog.Infof("Adopted Network ACL rule %v for load balancer %v",
aclRule.Id, lb.name)
+ return nil
+ }
+ if !isNotAllowed(err) {
+ return fmt.Errorf("error adopting Network ACL rule %v: %w",
aclRule.Id, err)
+ }
+
+ klog.Warningf("Cannot adopt Network ACL rule %v for load balancer %v,
creating a new one; delete the old rule by hand: %v", aclRule.Id, lb.name, err)
+ err = lb.createNetworkACLRule(aclID, networkID, ipProtocol, publicPort)
+ if err != nil && isNotAllowed(err) {
+ klog.Warningf("Cannot create a Network ACL rule for load
balancer %v either; port %v stays open through rule %v: %v", lb.name,
publicPort, aclRule.Id, err)
+ return nil
}
- // create ACL rule
- // ACL rules only know tcp/udp/icmp, so tcp-proxy maps to tcp. This
also matches the
- // filter above, which would otherwise never find the rule again.
- acl := lb.NetworkACL.NewCreateNetworkACLParams(protocol.IPProtocol())
- acl.SetAclid(network.Aclid)
+ return err
+}
+
+// createNetworkACLRule opens a port on a tier with a rule marked as this load
balancer's. ACL
+// rules only know tcp, udp and icmp, so the rule takes the IP protocol and a
tcp-proxy port
+// opens tcp.
+func (lb *loadBalancer) createNetworkACLRule(aclID, networkID, ipProtocol
string, publicPort int) error {
+ acl := lb.NetworkACL.NewCreateNetworkACLParams(ipProtocol)
+ acl.SetAclid(aclID)
acl.SetAction("Allow")
- acl.SetCidrlist([]string{"0.0.0.0/0"})
+ acl.SetCidrlist([]string{defaultAllowedCIDR})
acl.SetStartport(publicPort)
acl.SetEndport(publicPort)
- acl.SetNetworkid(networkId)
+ acl.SetNetworkid(networkID)
acl.SetTraffictype("Ingress")
+ acl.SetReason(lb.networkACLReason())
- _, err = lb.NetworkACL.CreateNetworkACL(acl)
- if err != nil {
- return false, fmt.Errorf("error creating Network ACL for port:
%v, due to: %s", publicPort, err)
+ if _, err := lb.NetworkACL.CreateNetworkACL(acl); err != nil {
+ return fmt.Errorf("error creating Network ACL for port: %v, due
to: %s", publicPort, err)
Review Comment:
Same issue as above: `%s` prevents proper error unwrapping. Prefer `%w` for
`err` so higher-level code can preserve and inspect the original CloudStack
error.
--
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]
