DaanHoogland commented on code in PR #109:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/109#discussion_r4335412000
##########
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: %w", 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
+ }
Review Comment:
whoever thought that a switch on a random set of expressions with unrelated
field contained in them was a good feature for a structured programming
language???
--
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]
