Copilot commented on code in PR #115:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/115#discussion_r4550564321
##########
cloudstack_loadbalancer.go:
##########
@@ -333,6 +348,64 @@ func isNetworkACLSupported(services
[]cloudstack.NetworkServiceInternal) bool {
return false
}
+// vpcHasFirewall reports whether the VPC a network belongs to has the
Firewall service. Since
+// CloudStack 4.23 a VPC offering can include it. A tier lists the service
only if its own network
+// offering includes it, and the VPC virtual router drops traffic to a public
IP with a load
+// balancer rule unless a firewall rule admits it, on top of the tier's
Network ACL. A network
+// outside a VPC, or on an earlier release, has no VPC firewall.
+//
+// The answer is remembered per VPC. An account that may not call listVPCs is
treated as if the
+// VPC had no Firewall service, as earlier releases did. The account
CloudStack's Kubernetes
+// service creates for clusters in projects is one such account, and most VPCs
have no Firewall
+// service, so the warning that names the missing API is logged once per VPC.
+func (lb *loadBalancer) vpcHasFirewall(network *cloudstack.Network) (bool,
error) {
+ if network.Vpcid == "" || lb.version.LT(vpcFirewallVersion) {
+ return false, nil
+ }
+ if hasFirewall, ok := lb.vpcFirewalls[network.Vpcid]; ok {
+ return hasFirewall, nil
+ }
+
+ hasFirewall := false
+ vpc, _, err := lb.VPC.GetVPCByID(network.Vpcid,
cloudstack.WithProject(lb.projectID))
+ switch {
+ case err != nil && isNotAllowed(err):
+ if lb.firstVPCReadRefusal(network.Vpcid) {
+ klog.Warningf("Cannot read VPC %v to check for the
Firewall service, so no firewall rules are managed there; allow the account to
call listVPCs: %v", network.Vpcid, err)
+ } else {
+ klog.V(4).Infof("Cannot read VPC %v to check for the
Firewall service for load balancer %v: %v", network.Vpcid, lb.name, err)
Review Comment:
The new behavior depends on reliably identifying the “API not allowed” case,
but `isNotAllowed(err)` currently relies on substring matching the error text.
That’s brittle (message formats may change) and could lead to silently skipping
firewall-rule management when it shouldn’t. Prefer checking CloudStack errors
structurally (e.g., type-assert to the cloudstack-go error type and check the
numeric error code / API error code) instead of matching `err.Error()`.
##########
cloudstack.go:
##########
@@ -69,6 +70,9 @@ type CSCloud struct {
region string
version semver.Version
clientBuilder cloudprovider.ControllerClientBuilder
+
+ // vpcReadRefused holds the VPCs the account may not read, so that is
logged once per VPC.
+ vpcReadRefused *sync.Map
Review Comment:
`vpcReadRefused` is an unbounded, never-cleared cache of refused VPC IDs. In
long-lived controllers with many distinct VPCs (or repeated project churn),
this can cause unbounded memory growth. Consider bounding this (e.g., LRU/TTL
cache), or periodically resetting it (e.g., per resync interval) while still
keeping the “log once per VPC” behavior.
##########
cloudstack_loadbalancer.go:
##########
@@ -473,6 +568,8 @@ func (cs *CSCloud) getLoadBalancer(service *corev1.Service)
(*loadBalancer, erro
name: cs.GetLoadBalancerName(context.TODO(), "",
service),
projectID: cs.projectID,
rules: make(map[string]*cloudstack.LoadBalancerRule),
+ vpcReadRefused: cs.vpcReadRefused,
+ version: cs.version,
}
Review Comment:
`vpcReadRefused` is assumed to be non-nil to achieve the documented “warn
once per VPC until restart” behavior, but `CSCloud` can be constructed outside
`newCSCloud` (notably in tests or in alternative wiring) and leave this nil,
which will make warnings repeat every time. Consider defensively initializing
`cs.vpcReadRefused` (or `lb.vpcReadRefused`) here when it’s nil so behavior is
consistent regardless of construction path.
--
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]
