Copilot commented on code in PR #103: URL: https://github.com/apache/cloudstack-kubernetes-provider/pull/103#discussion_r3860337984
########## README.md: ########## @@ -146,10 +162,123 @@ spec: ``` **Format:** Comma-separated list of CIDR ranges. Spaces around commas are automatically trimmed. +Every entry must parse as a valid CIDR, otherwise the service fails to sync with an `invalid CIDR` error. + +**CloudStack Version:** Creating a rule with a CIDR list works on all supported versions. +*Changing* the CIDR list of an existing rule can only be done in place on CloudStack 4.22 or later. +On earlier versions the controller deletes the load balancer rule and recreates it with the new CIDR +list, which briefly interrupts traffic on that port. + +**Note:** If the annotation is not set, the load balancer rule allows all sources (`0.0.0.0/0`). +Setting it to an empty value (`""`) sends an empty CIDR list to CloudStack — it does not block all +traffic. + +#### `service.beta.kubernetes.io/cloudstack-load-balancer-ip-associated-by-controller` + +**Type:** Boolean (`"true"` or `"false"`) + +**Default:** Not set + +**Description:** Set by the controller, not by you. When the controller associates a public IP that +was not already allocated, it records that fact on the service with this annotation. On deletion the +annotation determines whether the IP is disassociated again: an IP the controller allocated is +released, an IP that was already allocated before the service existed is left in place. + +The controller also checks for other load balancer rules on the same IP before releasing it, so an +IP shared by several services is not disassociated while still in use. Do not set or remove this +annotation by hand — doing so can leak a public IP or release one that you allocated yourself. + +### Restricting Source Traffic + +There are two independent layers, and they are configured separately: + +| Layer | Configured by | Default | +| --- | --- | --- | +| CloudStack load balancer rule | `service.beta.kubernetes.io/cloudstack-load-balancer-source-cidrs` annotation | `0.0.0.0/0` | +| Firewall rule (isolated networks) | `spec.loadBalancerSourceRanges` | `0.0.0.0/0` | +| Network ACL (VPC networks) | Not configurable, always `0.0.0.0/0` | `0.0.0.0/0` | + +Traffic has to be allowed by both layers, so either setting alone is enough to block unwanted +sources on an isolated network. Setting both keeps the two rules consistent in CloudStack. +On a VPC network, `spec.loadBalancerSourceRanges` has no effect and the annotation is the only +way to restrict sources. Review Comment: This sentence states that on *any* VPC network `spec.loadBalancerSourceRanges` has no effect, but the later “VPC Networks” section notes that when the Firewall service is offered (e.g., CloudStack 4.23+ with an offering that includes Firewall), the controller manages firewall rules and applies `spec.loadBalancerSourceRanges`. Qualify this statement to only apply to VPC networks that use Network ACLs. ########## README.md: ########## @@ -146,10 +162,123 @@ spec: ``` **Format:** Comma-separated list of CIDR ranges. Spaces around commas are automatically trimmed. +Every entry must parse as a valid CIDR, otherwise the service fails to sync with an `invalid CIDR` error. + +**CloudStack Version:** Creating a rule with a CIDR list works on all supported versions. +*Changing* the CIDR list of an existing rule can only be done in place on CloudStack 4.22 or later. +On earlier versions the controller deletes the load balancer rule and recreates it with the new CIDR +list, which briefly interrupts traffic on that port. + +**Note:** If the annotation is not set, the load balancer rule allows all sources (`0.0.0.0/0`). +Setting it to an empty value (`""`) sends an empty CIDR list to CloudStack — it does not block all +traffic. + +#### `service.beta.kubernetes.io/cloudstack-load-balancer-ip-associated-by-controller` + +**Type:** Boolean (`"true"` or `"false"`) + +**Default:** Not set + +**Description:** Set by the controller, not by you. When the controller associates a public IP that +was not already allocated, it records that fact on the service with this annotation. On deletion the +annotation determines whether the IP is disassociated again: an IP the controller allocated is +released, an IP that was already allocated before the service existed is left in place. + +The controller also checks for other load balancer rules on the same IP before releasing it, so an +IP shared by several services is not disassociated while still in use. Do not set or remove this +annotation by hand — doing so can leak a public IP or release one that you allocated yourself. + +### Restricting Source Traffic + +There are two independent layers, and they are configured separately: + +| Layer | Configured by | Default | +| --- | --- | --- | +| CloudStack load balancer rule | `service.beta.kubernetes.io/cloudstack-load-balancer-source-cidrs` annotation | `0.0.0.0/0` | +| Firewall rule (isolated networks) | `spec.loadBalancerSourceRanges` | `0.0.0.0/0` | +| Network ACL (VPC networks) | Not configurable, always `0.0.0.0/0` | `0.0.0.0/0` | Review Comment: The table row implies VPC networks always use Network ACLs that are not configurable and always allow `0.0.0.0/0`. Later in the README you describe VPC networks that offer the Firewall service (CloudStack 4.23+) where firewall rules are used instead and `spec.loadBalancerSourceRanges` is applied. Consider qualifying this row to only cover VPC networks that use Network ACLs (i.e., without the Firewall service). -- 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]
