Give each Service its own Network ACL rule

[vishesh92]

Problem

On a VPC tier the controller opened a Service port with one Network ACL rule matched only on
protocol and port. Every Service using that port shared it, and it was deleted when any of them
was deleted or moved off the port, so the other Services lost ingress until their next sync.
Rules an operator made for the same port were taken over and deleted the same way.

Change

• Each rule the controller creates carries the reason
  Managed by the CloudStack Kubernetes Provider for <lb name>, and only rules with the
  Service's own reason are deleted. Each Service gets its own rule, even when another Service
  already has one for the port.
• A rule created by an earlier release (allow, 0.0.0.0/0, one port, no reason) is taken over on
  the first sync with updateNetworkACLItem. If that is refused, the Service creates its own
  rule; if creating is refused too, it keeps using the old rule, as before.
• Rules someone else made for the port are left alone, and the controller adds none, as before.
  They are no longer deleted with the Service. Rules in a global ACL list are never taken over.
• EnsureLoadBalancerDeleted looks the ACL rule up in the load balancer rule's own network, and
  keeps a load balancer rule whose ACL rule could not be deleted, so the cleanup is retried.
• Unchanged: firewall rules, public IP handling, and rules stored with an upper-case protocol,
  which are still ignored.

The issue proposed resource tags. This uses the rule's reason instead: it is set in the create
call, so a new rule is never unmarked, and the account CloudStack's Kubernetes service creates
for project clusters cannot call createTags.

Upgrading

README has a new "Upgrading" section listing what users will notice, what to do before upgrading
and what to check after. In short: give hand-made 0.0.0.0/0 rules a reason, allow
updateNetworkACLItem, and upgrade every controller whose Services share a tier or ACL list.

Testing

• Unit tests for rule classification, listing, take-over, deferring to other rules, owner-scoped
  delete, and full EnsureLoadBalancer / EnsureLoadBalancerDeleted runs on a VPC tier.
• New e2e tests: TestVPC_ACLRulePerService (the issue's reproduction),
  TestVPC_LegacyACLRuleAdopted and TestVPC_OperatorACLRuleKept.
  TestVPC_ACLRulePerService fails against main with "port 80 is no longer open on the tier
  after deleting one of the two Services using it" and passes with this change.
• Simulator, CloudStack 4.22.1.0 and Kubernetes v1.37.0: phase 1 24 passed, 1 skipped
  (TestNode_ProviderID, kind sets provider IDs); phase 2 8 passed.

Fixes #107

[Copilot]

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

4 open findings

• The previous implementation listed ACLs using both aclid and networkid, but… · New
• requireNoACLRules is described as guarding tests that “count every rule on their port”, but it… · New
• requireNoACLRules is described as guarding tests that “count every rule on their port”, but it… · New
• strings.Contains(r.Cidrlist, "0.0.0.0/0") can return true for unrelated CIDRs like "10.0.0.0/0"… · New

What changed in this PR

This PR fixes VPC-tier Network ACL rule ownership so each Kubernetes Service gets (and deletes) only its own ACL rule, preventing shared-port Services or operator-created rules from being accidentally removed (fixes #107).

Changes:

• Add per-Service ownership via the CloudStack ACL rule reason, adopt legacy unowned rules when possible, and avoid touching operator rules.
• Adjust deletion flow to retain LB rules when ACL rule deletion fails, ensuring Kubernetes retries cleanup.
• Expand unit and e2e coverage for ownership/adoption/retention scenarios and document upgrade behavior.

File	Description
cloudstack_loadbalancer.go	Implements owned-per-Service ACL rules (reason-based), legacy adoption, and retry-safe deletion semantics.
cloudstack_loadbalancer_test.go	Adds unit coverage for ACL rule classification, listing, adoption, ownership-scoped create/delete, and end-to-end sync/delete flows.
test/​e2e/​vpc_test.go	Adds new VPC e2e regression tests for per-Service ACL rules, legacy adoption, and operator rule retention.
test/​e2e/​framework.go	Adds helper APIs for creating/listing/cleaning up ACL rules directly via CloudStack for e2e tests.
docs/​development.md	Documents new e2e assumptions about counting ACL rules and failure mode when leftovers exist.
README.md	Documents per-Service ACL rule behavior and adds an “Upgrading” section with operational guidance.

🧠 Review effort: Lite

---

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

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.83%. Comparing base (0bd7887) to head (a7a44ba).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #109      +/-   ##
==========================================
+ Coverage   71.97%   73.83%   +1.86%     
==========================================
  Files           5        5              
  Lines        1470     1525      +55     
==========================================
+ Hits         1058     1126      +68     
+ Misses        361      350      -11     
+ Partials       51       49       -2     

☔ 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]

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

5 open findings

• When lbRule.Networkid is empty, the fallback uses lb.ipAddrID rather than the IP associated… · New
• This newly introduced error wraps the underlying error using %s, which loses unwrap-ability and… · New
• Same issue as above: %s prevents proper error unwrapping. Prefer %w for err so higher-level… · New
• The reason prefix string is duplicated here and in production (networkACLReasonPrefix). Since the… · New
• The phrase “leaves the port to” is grammatically incomplete/unclear. Consider rewording to… · New

4 resolved since last review

• strings.Contains(r.Cidrlist, "0.0.0.0/0") can return true for unrelated CIDRs like "10.0.0.0/0"…
• requireNoACLRules is described as guarding tests that “count every rule on their port”, but it…
• requireNoACLRules is described as guarding tests that “count every rule on their port”, but it…
• The previous implementation listed ACLs using both aclid and networkid, but…

🧠 Review effort: Lite

[Copilot]

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

4 open findings

• errors.Join requires Go 1.20+. If the project supports Go < 1.20, this introduces a compile-time… · New
• Importing slices requires Go 1.21+. If the repo’s supported Go version (go.mod/CI toolchain) is <… · New
• The comment says this fails the test when “any rule for the port” (regardless of… · New
• This error path should wrap the underlying error with %w instead of formatting it with %v, so… · New

5 resolved since last review

• The reason prefix string is duplicated here and in production (networkACLReasonPrefix). Since the…
• Same issue as above: %s prevents proper error unwrapping. Prefer %w for err so higher-level…
• This newly introduced error wraps the underlying error using %s, which loses unwrap-ability and…
• When lbRule.Networkid is empty, the fallback uses lb.ipAddrID rather than the IP associated…
• The phrase “leaves the port to” is grammatically incomplete/unclear. Consider rewording to…

🧠 Review effort: Lite

[dheeraj12347]

Hi @vishesh92 , @DaanHoogland , I reviewed the changes for per-Service Network ACL ownership. The ownership and cleanup approach looks worth testing carefully, particularly when multiple Services share a port. One additional improvement is to use %w when wrapping underlying errors so callers can inspect the original errors. I also noticed the E2E helper requireNoACLRules checks only exact start/end port matches, so it may miss a leftover rule whose port range includes the requested port. The existing Copilot comment flags this as well.

[vishesh92]

Hi @vishesh92 , @DaanHoogland , I reviewed the changes for per-Service Network ACL ownership. The ownership and cleanup approach looks worth testing carefully, particularly when multiple Services share a port. One additional improvement is to use %w when wrapping underlying errors so callers can inspect the original errors. I also noticed the E2E helper requireNoACLRules checks only exact start/end port matches, so it may miss a leftover rule whose port range includes the requested port. The existing Copilot comment flags this as well.

Thanks for the review. we need to ensure that deleting a service doesn't delete a rule which is used by another service. So, each service will have it's own rule. We don't really care about the range conflicts for these changes.