Copilot commented on code in PR #103: URL: https://github.com/apache/cloudstack-kubernetes-provider/pull/103#discussion_r3851821752
########## README.md: ########## @@ -146,10 +161,118 @@ 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 Markdown table has an extra leading `|` on each row (`|| ...`), which typically renders as an empty first column. Use single leading pipes (`| ...`) so the table renders correctly. ########## README.md: ########## @@ -146,10 +161,118 @@ 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. + +To restrict access at both layers, set both: + +```yaml +apiVersion: v1 +kind: Service +metadata: + name: my-service + annotations: + service.beta.kubernetes.io/cloudstack-load-balancer-source-cidrs: "10.0.0.0/8" +spec: + type: LoadBalancer + loadBalancerSourceRanges: + - 10.0.0.0/8 + ports: + - port: 80 + protocol: TCP +``` + +The controller never opens the firewall implicitly; it always creates explicit firewall rules for +the ports it manages, and it removes firewall rules whose CIDR list no longer matches. + +### Assigning a Specific IP Address + +Set `spec.loadBalancerIP` to pin the load balancer to a known public IP: + +```yaml +spec: + type: LoadBalancer + loadBalancerIP: 10.1.1.218 +``` -**CloudStack Version:** Updating CIDR lists on existing load balancer rules requires CloudStack 4.22 or later. Creating new load balancer rules with CIDR lists works on earlier versions. +The address must be an existing public IP address visible to the configured account, otherwise the +service fails to sync with `could not find IP address`. It does not need to be associated with the +network beforehand: if the address is free, the controller associates it (with the VPC instead of +the network if the network belongs to a VPC). -**Note:** If the annotation is not set, the default behavior is to allow all sources (`0.0.0.0/0`). However, if you explicitly set the annotation to an empty value (`""`), this will result in an empty CIDR list, effectively blocking all traffic. +When the service is deleted, the IP is released only if the controller associated it — see +[`...-ip-associated-by-controller`](#servicebetakubernetesiocloudstack-load-balancer-ip-associated-by-controller) +above. Review Comment: The internal heading anchor in the link looks misspelled (`...kubernetesiocloudstack...` is missing the `s` in `kubernetesio`), which will break the jump link on GitHub. Update the fragment to match the auto-generated anchor for the corresponding heading. ########## README.md: ########## @@ -129,9 +137,16 @@ spec: **Default:** `"0.0.0.0/0"` (allows all sources) -**Description:** Specifies the source CIDR list for firewall rules on the CloudStack load balancer. This restricts which IP addresses can access the load balancer. +**Description:** Sets the source CIDR list on the CloudStack **load balancer rule**, restricting the source addresses that the load balancer rule accepts traffic from. + +This annotation does **not** configure the firewall. The firewall rule that is created alongside +the load balancer rule is derived from `spec.loadBalancerSourceRanges`, which defaults to +`0.0.0.0/0`. Setting only this annotation leaves the firewall open — see +[Restricting Source Traffic](#restricting-source-traffic). Review Comment: This wording can be read as implying that using only the annotation won’t restrict access, even though it still restricts traffic at the load balancer rule layer. Consider rephrasing to explicitly distinguish (a) traffic restriction at the LB rule vs (b) the firewall rule remaining `0.0.0.0/0`, so readers don’t misconfigure thinking the annotation is ineffective. -- 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]
