Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
+ Coverage 58.70% 67.04% +8.33%
==========================================
Files 5 5
Lines 1109 1250 +141
==========================================
+ Hits 651 838 +187
+ Misses 425 365 -60
- Partials 33 47 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes reconciliation getting wedged when toggling service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol on an existing LoadBalancer Service by resolving existing CloudStack LB rules by their uniqueness tuple (public IP, IP protocol, public port) and updating rules in-place (including renaming) rather than attempting conflicting creates. Also includes related robustness fixes around CIDR list comparison, VPC ACL protocol handling, and management-server version parsing.
Changes:
- Reworks LB rule reconciliation into resolve/prune/apply phases, with rule lookup falling back from name to (public IP, IP protocol, public port) to support protocol toggles.
- Normalizes CIDR list parsing for rule comparisons and gates CIDR updates on CloudStack version support.
- Hardens management server version parsing (avoids panics on short versions) and expands unit test coverage, including regression tests for issue #2.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents that proxy-protocol toggling updates the rule in-place without port interruption. |
| cloudstack.go | Makes management-server version parsing resilient to short/4-part version strings. |
| cloudstack_test.go | Adds coverage for short/empty version strings to prevent panics and validate parsing behavior. |
| cloudstack_loadbalancer.go | Implements tuple-based LB rule resolution, phased prune/apply reconciliation, CIDR list normalization, and VPC ACL protocol fix. |
| cloudstack_loadbalancer_test.go | Adds tests for CIDR splitting, tuple-based rule matching, in-place protocol toggles, phased pruning behavior, and VPC ACL behavior. |
Suppressed comments (1)
cloudstack_loadbalancer.go:890
- When a rule must be recreated, checkLoadBalancerRule deletes it in CloudStack but leaves it in lb.rules. That leftover entry is then treated as obsolete and pruned, which can trigger a second DeleteLoadBalancerRule call (and potentially fail reconciliation with a "not found"/API error) even though the rule was already deleted in the resolve phase.
Remove the rule from lb.rules immediately after a successful delete so it can’t be pruned again later in the same reconciliation.
// Delete the load balancer rule so we can create a new one using the new values.
if err := lb.deleteLoadBalancerRule(lbRule); err != nil {
return nil, false, err
}
return nil, false, nil
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
vishesh92
force-pushed
the
fix-protocol-toggle
branch
from
August 27, 2026 07:59
345a563 to
bc832c4
Compare
vishesh92
force-pushed
the
fix-protocol-toggle
branch
from
September 17, 2026 07:12
bc832c4 to
1c70b28
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved stale-IP/ACL cleanup cases and mismatched ACL mock expectations affect correctness and test reliability.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
cloudstack_loadbalancer_test.go:3218
- As in the earlier ACL test, these expectations omit the variadic project option even though
updateNetworkACLpassescloudstack.WithProject(lb.projectID)to both calls. The mock therefore does not match the production invocation and this added test fails before checking the existing-rule path.
mockNetwork.EXPECT().GetNetworkByID("net-123").Return(networkResp, 1, nil),
mockNetworkACL.EXPECT().GetNetworkACLListByID("acl-456").Return(aclListResp, 1, nil),
- Files reviewed: 5/5 changed files
- Comments generated: 5
- Review effort level: Lite
vishesh92
force-pushed
the
fix-protocol-toggle
branch
from
September 17, 2026 09:45
1c70b28 to
6cfa3ad
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unknown-protocol conflicts and cross-network stale-rule cleanup can still wedge reconciliation or leave ACL resources behind.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
cloudstack_loadbalancer.go:759
- An obsolete rule with an unknown protocol is skipped here, but
partitionObsoleteRulesuses the public port alone to decide whether a leftover rule blocks a create. If this rule is on the current IP and holds a port needed by a new desired rule, the subsequent create still hits CloudStack's port-conflict error and reconciliation remains wedged. Parse and classify the port before rejecting the protocol so such a rule is deleted before the create, while handling its unknown firewall/ACL protocol separately.
protocol := ProtocolFromLoadBalancer(lbRule.Protocol)
if protocol == LoadBalancerProtocolInvalid {
klog.Errorf("Skipping obsolete load balancer rule %v with unknown protocol %v", lbRule.Name, lbRule.Protocol)
continue
cloudstack_loadbalancer.go:824
- This cleanup uses the network of the service currently being reconciled for every obsolete rule. A stale rule can be attached to an IP in a different network/VPC; in that case
claimedis also keyed without network identity, so a desired tuple in the current network can preserve the old network's ACL, or this branch can query/delete the wrong ACL list. Resolve the obsolete rule's network (for example from its IP/Networkid) and include that network in the claim before choosing the firewall/ACL cleanup path.
} else if isNetworkACLSupported(network.Service) {
// ACL rules belong to the network rather than an IP, so the claim always applies.
if claimed[o.tuple] {
klog.V(4).Infof("Keeping Network ACL rules of obsolete load balancer rule %v (%v:%v): still claimed by a service port", lbRule.Name, protocol, port)
} else {
cloudstack_loadbalancer.go:690
lb.ipAddrIDis not guaranteed to be the service's current/desired IP:getLoadBalancerassigns it from each listed rule, so when rules with different names exist on old and current IPs, the API response order can make the stale IP win. This filter would then match the stale rule and prune the actual current-IP rule, contrary to the intended stale-IP handling. Select the target fromspec.loadBalancerIP/published ingress (or otherwise validate the selected IP) before using it here.
// Only rules on the IP being reconciled towards are eligible; a rule on any other IP is left
// for the prune pass, which also cleans up the firewall rules it leaves behind.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
vishesh92
force-pushed
the
fix-protocol-toggle
branch
from
September 17, 2026 12:00
6cfa3ad to
7157a10
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
VPC cleanup can mis-scope Network ACLs when an obsolete rule omits its network ID.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
vishesh92
force-pushed
the
fix-protocol-toggle
branch
from
September 18, 2026 06:22
7157a10 to
97b1a72
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Explicit stale-IP reconciliation and cross-network firewall/ACL cleanup still have correctness gaps.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
cloudstack_loadbalancer.go:451
- When
service.Spec.LoadBalancerIPis set but the API returns only rules on the old IP,preferredIPis never observed in this loop, solb.ipAddr/lb.ipAddrIDremain stale.EnsureLoadBalancerthen seeshasLoadBalancerIP()as true and skipsgetLoadBalancerIP, causing the newly resolved rules to be created on the old address instead of the requested one; clear/re-resolve the selected IP when no rule matches the explicit preferred address before resolving rules.
if lb.ipAddr == "" || lb.ipAddr != preferredIP {
lb.ipAddr = lbRule.Publicip
lb.ipAddrID = lbRule.Publicipid
}
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
vishesh92
force-pushed
the
fix-protocol-toggle
branch
from
September 18, 2026 07:15
97b1a72 to
4c107fc
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Network ACL pruning can remove a tier-shared opening needed by another service, and the new VPC E2E assertion can race ACL reconciliation.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
kiranchavala
approved these changes
Sep 21, 2026
kiranchavala
left a comment
kiranchavala
left a comment
Member
There was a problem hiding this comment.
LGTM, Tested manually
kubectl expose deploy nginx --type=LoadBalancer --port=80 --name=lb-proxy
kubectl get svc lb-proxy -w # wait for EXTERNAL-IP
cmk list loadbalancerrules listall=true filter=id,name,protocol,publicport
Before fix
k describe svc lb-proxy
Name: lb-proxy
Namespace: default
Labels: <none>
Annotations: service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol: true
Selector: app=nginx
Type: LoadBalancer
IP Family Policy: SingleStack
IP Families: IPv4
IP: 10.104.141.145
IPs: 10.104.141.145
LoadBalancer Ingress: 10.0.59.124 (VIP)
Port: <unset> 80/TCP
TargetPort: 80/TCP
NodePort: <unset> 31196/TCP
Endpoints: 192.168.239.5:80,192.168.239.6:80
Session Affinity: None
External Traffic Policy: Cluster
Internal Traffic Policy: Cluster
Events:
Type Reason Age From Message
---- ------ ---- ---- -------
Normal EnsuredLoadBalancer 2m30s service-controller Ensured load balancer
Warning SyncLoadBalancerFailed 21s (x3 over 37s) service-controller Error syncing load balancer: failed to ensure load balancer: error creating load balancer rule a778efb5964f04dea842e6cedce84e3b-tcp-proxy-80: CloudStack API error 537 (CSExceptionErrorCode: 9999): The range specified, 80-80, conflicts with rule FirewallRule {"id":34,"networkId":210,"purpose":"LoadBalancing","state":"Active","uuid":"4000f82d-afc4-46b3-ab92-e00cf5b9e7fe"} which has 80-80
Normal EnsuringLoadBalancer 1s (x5 over 2m40s) service-controller Ensuring load balancer
╰─ kubectl annotate svc lb-proxy service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol="false" --overwrite
service/lb-proxy annotated
╰─ k describe svc lb-proxy
Name: lb-proxy
Namespace: default
Labels: <none>
Annotations: service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol: false
Selector: app=nginx
Type: LoadBalancer
IP Family Policy: SingleStack
IP Families: IPv4
IP: 10.104.141.145
IPs: 10.104.141.145
LoadBalancer Ingress: 10.0.59.124 (VIP)
Port: <unset> 80/TCP
TargetPort: 80/TCP
NodePort: <unset> 31196/TCP
Endpoints: 192.168.239.5:80,192.168.239.6:80
Session Affinity: None
External Traffic Policy: Cluster
Internal Traffic Policy: Cluster
Events:
Type Reason Age From Message
---- ------ ---- ---- -------
Normal EnsuringLoadBalancer 5s (x7 over 3m25s) service-controller Ensuring load balancer
Warning SyncLoadBalancerFailed 5s (x5 over 82s) service-controller Error syncing load balancer: failed to ensure load balancer: error creating load balancer rule a778efb5964f04dea842e6cedce84e3b-tcp-proxy-80: CloudStack API error 537 (CSExceptionErrorCode: 9999): The range specified, 80-80, conflicts with rule FirewallRule {"id":34,"networkId":210,"purpose":"LoadBalancing","state":"Active","uuid":"4000f82d-afc4-46b3-ab92-e00cf5b9e7fe"} which has 80-80
Normal EnsuredLoadBalancer 4s (x2 over 3m15s) service-controller Ensured load balancer
After fix
k describe svc lb-proxy
Name: lb-proxy
Namespace: default
Labels: <none>
Annotations: <none>
Selector: app=nginx
Type: LoadBalancer
IP Family Policy: SingleStack
IP Families: IPv4
IP: 10.111.255.47
IPs: 10.111.255.47
Port: <unset> 80/TCP
TargetPort: 80/TCP
NodePort: <unset> 30499/TCP
Endpoints: 192.168.149.132:80,192.168.149.133:80
Session Affinity: None
External Traffic Policy: Cluster
Internal Traffic Policy: Cluster
Events: <none>
╭─ ~ ✔ ╱ kubernetes-admin@kubernetes ╱ 03:15:34 PM
╰─ kubectl annotate svc lb-proxy service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol="true" --overwrite
service/lb-proxy annotated
╭─ ~ ✔ ╱ kubernetes-admin@kubernetes ╱ 03:15:58 PM
╰─ k describe svc lb-proxy
Name: lb-proxy
Namespace: default
Labels: <none>
Annotations: service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol: true
Selector: app=nginx
Type: LoadBalancer
IP Family Policy: SingleStack
IP Families: IPv4
IP: 10.111.255.47
IPs: 10.111.255.47
Port: <unset> 80/TCP
TargetPort: 80/TCP
NodePort: <unset> 30499/TCP
Endpoints: 192.168.149.132:80,192.168.149.133:80
Session Affinity: None
External Traffic Policy: Cluster
Internal Traffic Policy: Cluster
Events:
Type Reason Age From Message
---- ------ ---- ---- -------
Normal EnsuringLoadBalancer 11s service-controller Ensuring load balancer
Normal UpdatedLoadBalancer 10s service-controller Updated load balancer with new hosts
╭─ ~ ✔ ╱ kubernetes-admin@kubernetes ╱ 03:16:02 PM
╰─ kubectl annotate svc lb-proxy service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol="false" --overwrite
service/lb-proxy annotated
╭─ ~ ✔ ╱ kubernetes-admin@kubernetes ╱ 03:16:24 PM
╰─ k describe svc lb-proxy
Name: lb-proxy
Namespace: default
Labels: <none>
Annotations: service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol: false
Selector: app=nginx
Type: LoadBalancer
IP Family Policy: SingleStack
IP Families: IPv4
IP: 10.111.255.47
IPs: 10.111.255.47
LoadBalancer Ingress: 10.0.59.124 (VIP)
Port: <unset> 80/TCP
TargetPort: 80/TCP
NodePort: <unset> 30499/TCP
Endpoints: 192.168.149.132:80,192.168.149.133:80
Session Affinity: None
External Traffic Policy: Cluster
Internal Traffic Policy: Cluster
Events:
Type Reason Age From Message
---- ------ ---- ---- -------
Normal UpdatedLoadBalancer 35s service-controller Updated load balancer with new hosts
Normal EnsuredLoadBalancer 18s (x2 over 24s) service-controller Ensured load balancer
Normal EnsuringLoadBalancer 3s (x3 over 36s) service-controller Ensuring load balancer
vishesh92
force-pushed
the
fix-protocol-toggle
branch
from
September 21, 2026 12:34
4c107fc to
af3d418
Compare
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (3)
vishesh92
force-pushed
the
fix-protocol-toggle
branch
from
September 23, 2026 08:36
af3d418 to
7efee6e
Compare
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (3)
vishesh92
force-pushed
the
fix-protocol-toggle
branch
from
September 23, 2026 09:44
7efee6e to
b71760d
Compare
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (4)
checkLoadBalancerRuledecides a rule must be recreated, it deletes the CloudStack rule and… · NewsplitCIDRListonly treats the literal space' 'as whitespace in addition to commas. If… · New
Resolved since last review (1)
Comment on lines
+697
to
+705
| lbRule, needsUpdate, err := lb.checkLoadBalancerRule(lb.findLoadBalancerRule(lbRuleName, port, protocol), lbRuleName, port, protocol, service, version) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if lbRule != nil { | ||
| // Claim by the rule's actual name: after a protocol change it still carries the old one. | ||
| delete(lb.rules, lbRule.Name) | ||
| } |
Comment on lines
+442
to
+444
| // duplicate sweep from deleting the rule that clients and DNS are pointing at. The | ||
| // same address is the one the rules are reconciled towards when they span several, | ||
| // so which IP wins does not depend on the order CloudStack lists the rules in. |
|
|
||
| lb.ipAddr = lbRule.Publicip | ||
| lb.ipAddrID = lbRule.Publicipid | ||
| if lb.ipAddr == "" || lb.ipAddr != preferredIP { |
vishesh92
force-pushed
the
fix-protocol-toggle
branch
from
September 23, 2026 13:15
b71760d to
bf3a6d9
Compare
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 1
Open (8)
checkLoadBalancerRuledecides a rule must be recreated, it deletes the CloudStack rule and…Errorf). In larger clusters, a… · NewErrorf). In larger clusters, a… · NewlbRuleis non-nil andgetCIDRListfails, returningruleMissingis misleading (the rule… · NewsplitCIDRListonly treats a literal space (' ') as whitespace. If CloudStack (or future code… · Newportas"8085"but hard-codesPort: 8085separately. This can drift if the… · New
Resolved since last review (1)
Comment on lines
+812
to
+814
| klog.Errorf("Skipping obsolete load balancer rule %v with invalid public port %v: %v", lbRule.Name, lbRule.Publicport, err) | ||
| continue | ||
| } |
Comment on lines
+824
to
+827
| if protocol == LoadBalancerProtocolInvalid && !blocksACreate { | ||
| klog.Errorf("Skipping obsolete load balancer rule %v with unknown protocol %v", lbRule.Name, lbRule.Protocol) | ||
| continue | ||
| } |
|
|
||
| cidrList, err := lb.getCIDRList(service) | ||
| if err != nil { | ||
| return ruleMissing, err |
Comment on lines
+691
to
+695
| func splitCIDRList(cidrList string) []string { | ||
| return strings.FieldsFunc(cidrList, func(r rune) bool { | ||
| return r == ',' || r == ' ' | ||
| }) | ||
| } |
Comment on lines
+275
to
+281
| const port = "8085" | ||
| svc := f.CreateLBService(func(s *corev1.Service) { | ||
| s.Annotations = map[string]string{annotationProxyProtocol: "true"} | ||
| s.Spec.Ports = []corev1.ServicePort{ | ||
| {Name: "http", Port: 8085, Protocol: corev1.ProtocolTCP}, | ||
| } | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2.
Toggling
service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocolon anexisting LoadBalancer service wedges reconciliation permanently:
Cause: rule names embed the protocol (
<lb>-<protocol>-<port>) and rules are lookedup by name, so a protocol change misses the lookup and tries to create a rule on a port
the old rule still holds. The in-place update was already written but unreachable — a
name-keyed lookup can never return a rule whose protocol differs from the one requested.
Changes
findLoadBalancerRulematches on an exact name, then falls back to(public IP, IP protocol, public port) — the tuple CloudStack enforces uniqueness on.
A proxy-protocol toggle now resolves to the existing rule and updates it in place.
Rules on a stale IP are pruned rather than matched, which used to strand their
firewall rule.
SetNameso the name stops contradicting its protocol.deleted before the creates, everything else after, so a cleanup failure can't take a
service down. Blocking is keyed on port alone —
detectRulesConflictnever exemptsLoadBalancing pairs with differing protocols.
Also fixed:
updateNetworkACLcreated ACLs with
CSProtocol()(tcp-proxy, which CloudStack rejects) whilefiltering with
IPProtocol(). Both now useIPProtocol().Cidrlistwas split on" "while CloudStack returns it comma-separated.