Feat: Add support for stickyness policy

[vishesh92]

This needs changes in this PR: apache/cloudstack-go#133

Fixes #75

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.84979% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.97%. Comparing base (7d27f2f) to head (435a548).

Files with missing lines	Patch %	Lines
cloudstack_loadbalancer.go	94.84%	7 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
+ Coverage   67.04%   71.97%   +4.93%     
==========================================
  Files           5        5              
  Lines        1250     1470     +220     
==========================================
+ Hits          838     1058     +220     
+ Misses        365      361       -4     
- Partials       47       51       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Adds support for configuring CloudStack Load Balancer stickiness policy via Kubernetes Service annotations (fixing the request in #75), and updates module dependencies to use a CloudStack Go SDK version that supports the required stickiness-policy API parameters.

Changes:

• Introduces Service annotations for stickiness method name + params and reconciles stickiness policies during EnsureLoadBalancer.
• Extends load balancer discovery to also fetch existing stickiness policies for rules.
• Bumps github.com/apache/cloudstack-go/v2 to v2.19.1 and updates Go module dependencies accordingly.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.

File	Description
cloudstack_loadbalancer.go	Adds stickiness annotations, policy reconciliation logic, and stickiness policy listing/CRUD.
go.mod	Bumps CloudStack Go SDK to v2.19.1 and adjusts dependency classification.
go.sum	Updates checksums for the bumped CloudStack Go SDK version.

Suppressed comments (2)

cloudstack_loadbalancer.go:833

• CreateLBStickinessPolicy response is assumed to contain at least one Stickinesspolicy entry; if the API returns an empty slice this will panic. Add a defensive check and return an explicit error (and drop the commented-out return).

	return &cloudstack.LBStickinessPolicyStickinesspolicy{
		Methodname: stickynessPolicy.Stickinesspolicy[0].Methodname,
		Params:     stickynessPolicy.Stickinesspolicy[0].Params,
		Id:         stickynessPolicy.Stickinesspolicy[0].Id,
		Name:       stickynessPolicy.Stickinesspolicy[0].Name,

cloudstack_loadbalancer.go:1282

• parseStickynessParams trims the full key=value token but not the key/value themselves, so annotations like "cookie = abc" will produce a key of "cookie " and won’t match expected params. Trim both sides when populating the map.

		if len(parts) == 2 {
			params[parts[0]] = parts[1]
		}

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

🟡 Changes recommended

Orchestration-path test coverage and the documented comment/example corrections remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

cloudstack_loadbalancer.go:198

• The new tests cover the helper methods independently, but do not exercise this orchestration path for a new rule, a parameter change, or annotation removal. A regression in the ordering/map cleanup around delete-and-recreate could therefore pass all added tests; add an EnsureLoadBalancer test with mocked list/create/delete policy calls.

			stickinessPolicy, stickinessPolicyNeedsUpdate, err := lb.checkStickinessPolicy(lbRule, service)
			if err != nil {
				return nil, err
			}
			if stickinessPolicyNeedsUpdate {

• Files reviewed: 4/5 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Clean up the load-balancer rule when stickiness policy creation fails before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 6/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate load-balancer reconciliation issues remain, along with a documentation inconsistency.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

README.md:242

• This example adds length=52 to AppCookie, but the preceding contract lists cookie-name for AppCookie and says CloudStack rejects unknown parameters. As written, the documentation advertises a configuration that can fail with the documented sync error; remove length=52 or expand the supported-key description only if this is a valid CloudStack parameter.

    service.beta.kubernetes.io/cloudstack-load-balancer-stickiness-method-param: "cookie-name=JSESSIONID,length=52"

cloudstack_loadbalancer.go:519

• getLoadBalancer is also called by UpdateLoadBalancer, which only reconciles backend hosts. This adds one listLBStickinessPolicies API request per rule to every node/update reconciliation, even when no stickiness annotation is set, and makes host-only updates fail if this optional policy lookup is unavailable. Load policies only in the Ensure/Delete paths or otherwise avoid this per-rule lookup in the shared discovery path.

		lbStickinessPoliciesParams := cs.client.LoadBalancer.NewListLBStickinessPoliciesParams()
		lbStickinessPoliciesParams.SetLbruleid(lbRule.Id)
		lbStickinessPolicies, err := cs.client.LoadBalancer.ListLBStickinessPolicies(lbStickinessPoliciesParams)

• Files reviewed: 6/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved issues remain in value round-tripping, rejected replacement retries, and rollback validation.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

cloudstack_loadbalancer.go:874

• When the replacement is rejected, EnsureLoadBalancer returns this error (as the new e2e test expects), so the service controller retries with the same invalid annotation. Each retry deletes and recreates the previously valid policy, repeatedly resetting HAProxy affinity state and changing the policy ID even though the desired change was not accepted. Avoid repeating this destructive replacement for an unchanged rejected configuration (for example, by caching/short-circuiting permanent validation failures or adding a non-destructive preflight).

	if err := lb.deleteStickinessPolicy(existing.Id); err != nil {
		return err
	}

	if _, err := lb.createStickinessPolicy(lbRuleName, lbRule.Id, service); err != nil {
		return lb.restoreStickinessPolicy(lbRuleName, lbRule.Id, existing, err)

cloudstack_loadbalancer.go:887

• The rollback path treats any nil-error response as a successful restore, but CloudStack can return an empty Stickinesspolicy list; the normal creation path already treats that response as an error at lines 910-911. In that case a rejected replacement leaves the rule without the old policy while reporting only the original failure, contradicting the rollback guarantee. Validate the restore response before declaring the previous policy recovered.

	if _, err := lb.LoadBalancer.CreateLBStickinessPolicy(p); err != nil {
		return fmt.Errorf("%v (restoring the previous stickiness policy failed too: %v)", cause, err)

• Files reviewed: 7/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Copilot review overview

🟡 Changes recommended

Two moderate issues remain in stickiness policy replacement and fallback flag handling.

Review effort: Lite
Findings: 1

Open (1)

• Preserve existing policy if replacement creation fails · New

Resolved since last review (1)

• SplitN makes values such as expr=a=b and key= look supported here, but CloudStack serializes…

[Damans227]

tested this on a kvm lab with a 1.24 cluster running this PR's provider (83b07d4). both methods work end to end.

LbCookie on port 80, SourceBased on port 8090. cmk list lbstickinesspolicies for the two rules:

"methodname": "LbCookie",    "params": {"cookie-name": "SERVERID", "nocache": "true"}
"methodname": "SourceBased", "params": {"expire": "30m", "tablesize": "200k"}

router haproxy config:

listen 10_0_60_84-80
	server 10_0_60_84-80_0 10.1.1.55:31150 check cookie 10_1_1_55-31150
	server 10_0_60_84-80_1 10.1.1.153:31150 check cookie 10_1_1_153-31150
	cookie SERVERID insert  nocache
listen 10_0_60_85-8090
	stick-table type ip size 200k expire 30m
	stick on src

10 requests each, two app copies on separate nodes:

--- LbCookie, no cookie kept:
      5 Hostname: whoami-589b5665ff-98l7m
      5 Hostname: whoami-589b5665ff-llrqn
--- LbCookie, cookie kept:
     10 Hostname: whoami-589b5665ff-llrqn
--- SourceBased, same client:
     10 Hostname: whoami-589b5665ff-llrqn

setting cookie-name=APPID replaced the policy (set-cookie: APPID=10_1_1_55-31150). removing the annotation removed the policy and requests split again.

bad input fails and cloudstack is left as is:

stickiness method LbCookie only works on plain HTTP ports: set appProtocol: http on them
stickiness method "Bogus" in annotation service.beta.kubernetes.io/cloudstack-load-balancer-stickiness-method-name is not supported on this network

[DaanHoogland]

clgtm, (but I don't like inline comments)

[Copilot]

Copilot review overview

🔵 Needs a closer look

Two moderate issues remain in stickiness capability validation and failure-safe policy reconciliation.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Preserve existing policy if replacement creation fails

[DaanHoogland]

merging on Daman's test report (and my review)