adestis-mf commented on code in PR #115:
URL:
https://github.com/apache/cloudstack-kubernetes-provider/pull/115#discussion_r4550981646
##########
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:
The map only gets an entry when CloudStack refuses `listVPCs` for a VPC,
that is when the account's role lacks the permission. The VPCs come from the
networks of the cluster's nodes and of its load balancer rules, so they are the
account's own VPCs, usually one. A TTL would bring back the warning
periodically, which the map is there to avoid, so I'd keep it as it is.
##########
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:
`isNotAllowed` is the existing helper on `main`, used for the stickiness
policies and for taking over ACL rules; this PR adds one call site. A
structural check isn't possible with cloudstack-go v2.19.1: `CSError.Error()`
returns an `error` rather than a `string`, so `CSError` doesn't implement
`error`, and the client only returns the formatted error. The code survives
only in the message. The refusal of `listVPCs` came back in this form in the
live test. If cloudstack-go exposes the error type later, all call sites can
switch together.
##########
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:
Good point. `vpcReadRefused` is now a `sync.Map` value in `CSCloud`, whose
zero value is ready to use, and each sync gets a pointer to it, so the warning
is logged once per VPC however `CSCloud` is built (466eeab0, with a test that
builds it as a literal). Initializing it lazily in `getLoadBalancer` would be
an unsynchronized write, as syncs can run concurrently.
--
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]
