Add a simulated dev/test environment and simulator-based e2e CI

[vishesh92]

Closes #4, which asked for documentation on running kube-apiserver and
cloudstack-simulator locally so automated test workloads could follow.

Today nothing exercises the CCM end to end. configFromEnv() skips unless
CS_API_URL and friends are set and nothing sets them, so EnsureLoadBalancer,
UpdateLoadBalancer and EnsureLoadBalancerDeleted have no coverage at all. The
README also links a Docker Hub image that no longer exists.

What this adds

A one-command local environment under hack/e2e: CloudStack simulator,
advanced zone, admin API keys, a kind cluster, CloudStack VMs matching the kind
nodes, and the CCM deployed from deployment.yaml. Every CloudStack call is a
plain cmk invocation, so the scripts run the same commands the docs tell you to
run.

docs/development.md, which walks the same steps by hand: what each port is
for, why cloud-provider: external is needed, the node-name and node-IP contract
between kind and CloudStack, both CCM run modes, the VPC scenario, and a
symptom-to-cause troubleshooting table.

A Go e2e suite in test/e2e, 17 tests across load balancer lifecycle, node
initialization, annotations and session affinity, and the VPC/network ACL path.
It sits behind the e2e build tag, so make test and go build ./... are
unaffected, and it needs no new module dependencies.

Using it

make e2e-up        # simulator + kind + VMs + CCM
make test-e2e      # load balancer, node and annotation tests
make e2e-vpc       # project, VPC, custom ACL, re-point the CCM
make test-e2e-vpc  # VPC/ACL tests
make e2e-down

Bugs this surfaced, fixed here

• Project scoping. Three call sites fetched resources without the configured
  project. On a project-owned VPC every LoadBalancer Service failed with error fetching Network with ID and never got an ingress address, and the public IP
  leaked on delete.
• getNetworkIDFromIPAddress. It returned the wrong error variable after a
  failed network lookup, guarded on Networkid while looking up
  Associatednetworkid, and could return an empty ID with a nil error.
  GetNetworkByID does not reject an empty ID, it matches an unfiltered network
  list, so that resolved to an arbitrary network instead of failing.
• Duplicate load balancer rules. CloudStack does not enforce unique rule
  names, but loadBalancer.rules is keyed by name, so a duplicate displaced its
  twin and then survived deletion with no Service referencing it. Duplicates are
  now swept on reconcile and on delete, along with the firewall rule and public
  IP only they used. The rule that survives is the one on the published address,
  and a sweep that fails during deletion is logged rather than returned so a
  Service cannot stick in Terminating.
• Startup panic. getManagementServerVersion sliced the version string to
  three components unchecked, so a server reporting 4.22, or a bare 24 under
  CloudStack's new versioning scheme, crashed the controller at startup.

CI

.github/workflows/e2e-simulator.yml runs on pull requests, pushes to main,
and workflow_dispatch. The matrix is the latest two Kubernetes minors against
CloudStack 4.22.1.0 and 4.20.2.0. The CloudStack axis is real branch
coverage, not just version coverage: 4.22 and later update a rule's CIDR list in
place, earlier releases delete and recreate it. Cells run in parallel behind one
shared image build, with the simulator and kind node images cached, so wall-clock
is close to a single run. The job needs no secrets.

Notes for reviewers

• The CCM must run with an admin role. The default User role cannot call
  listManagementServersMetrics, which is a hard startup gate. Noted in the
  README.
• The simulator image is amd64 only.
• kind supplies its own provider ID, so the CCM's provider-ID assignment is not
  exercised and that test skips under kind.
• Known follow-ups, deliberately out of scope: a public IP can still leak if the
  rule delete succeeds and the release then fails, the duplicate sweep's in-use
  check counts only load balancer rules, and UpdateLoadBalancer does not sweep.
• Not promoted to a required check in .asf.yaml until the job proves stable.

[Copilot]

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

[codecov-commenter]

Codecov Report

❌ Patch coverage is 82.60870% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.97%. Comparing base (151452f) to head (b4ee398).

Files with missing lines	Patch %	Lines
cloudstack_loadbalancer.go	82.08%	8 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #105      +/-   ##
==========================================
+ Coverage   50.05%   53.97%   +3.92%     
==========================================
  Files           4        4              
  Lines         975     1032      +57     
==========================================
+ Hits          488      557      +69     
+ Misses        473      449      -24     
- Partials       14       26      +12     

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

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

[Copilot]

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.

Suppressed comments (1)

cloudstack_loadbalancer.go:487

• The netErr branch returns the wrong error variable (it returns err from the previous GetPublicIpAddressByID call). If the first call succeeded (err==nil) but GetNetworkByID fails, this currently returns a nil error and masks the failure.

		network, _, netErr := cs.client.Network.GetNetworkByID(ip.Associatednetworkid, cloudstack.WithProject(cs.projectID))
		if netErr != nil {
			klog.Errorf("Failed to fetch the network for id: %v", ip.Associatednetworkid)
			return "", err
		}

[Copilot]

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.

[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.

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated 5 comments.

[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.

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.

[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.

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 2 comments.

[Copilot]

🟡 Changes recommended

There is at least one gofmt/formatting issue in a changed Go file (cloudstack_loadbalancer.go) that is likely to fail CI/lint and should be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 26/27 changed files
• Comments generated: 1
• Review effort level: Lite

[DaanHoogland]

clgtm , no production code.

[sureshanaparti]

should we've to still keep this, or can remove

[vishesh92]

workflow_dispatch is to allow manual execution of the action. IMO, it's good to have

[Copilot]

🔵 Needs a closer look

The harness teardown and simulator bring-up scripts have a couple of concrete operational/security issues (persisted cmk credentials; simulator version overrides can be ignored when reusing an existing container) that should be addressed before merge.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

hack/e2e/10-simulator-up.sh:52

• 10-simulator-up.sh reuses an existing container named $SIM_NAME without checking whether it was created from the currently requested $SIM_IMAGE/$SIM_TAG. If a developer changes SIM_TAG (or SIM_IMAGE) and reruns the script, they can silently keep running the old simulator version while thinking the override took effect. Consider comparing the existing container image to $SIM_IMAGE and recreating the container when they differ.
  hack/e2e/99-down.sh:35
• 99-down.sh leaves behind hack/e2e/_out/cmk.ini (created by cmk_init) which contains CloudStack admin credentials. Since this script is documented as tearing down everything the harness created, it should remove cmk.ini as well (and optionally the artifacts directory) to avoid persisting credentials and large logs on disk.

• Files reviewed: 26/27 changed files
• Comments generated: 0 new
• Review effort level: Lite

[sureshanaparti]

@vishesh92 can you check the below.

• Duplicate-rule cleanup assumes same public IP (please confirm or guard)
  In getLoadBalancer, a name-collided rule hits continue before the lb.ipAddr consistency check, and deleteDuplicateRules only deletes the LB rule. If a duplicate ever had a different public IP, its firewall rule / IP would leak on cleanup — the exact class of bug this PR fixes elsewhere. Can we either add a guard, or a one-line comment confirming duplicates are guaranteed to share IP/port/protocol?

• Guard the e2e job like the build job
  build is gated on _if: github.repository == 'apache/...' || workflow_dispatc_h, but e2e (needs: build) has no matching if. A workflow_dispatch from a fork (or a repo rename) would leave e2e running with no artifact to download. Also cache-to: type=gha,mode=max is read-only on fork PRs and will warn. Suggest mirroring the if: onto e2e.

• make test now runs against the live simulator in CI
  Sourcing keys.env before make test activates the acceptance tests in cloudstack_test.go against the simulator. Intentional and useful, but it couples the unit-test signal to simulator health. Worth confirming that's the intent — or splitting the pure-unit run from the live run so a sim hiccup doesn't read as a code failure.

• Minor nits

  • .gitignore is missing a trailing newline.
  • retry() in hack/e2e/lib/log.sh looks unused — drop it?
  • docs/development.md says "Go 1.23 or later" while CI uses go-version-file: go.mod — keep in sync with go.mod.
  • majorMinorPatch is duplicated in cloudstack.go and test/e2e/framework.go (unavoidable across the package boundary) — worth a note so a future edit updates both.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain in load-balancer cleanup and e2e image rebuilding.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

cloudstack_loadbalancer.go:821

• The unusable-port/protocol branch in deleteDuplicateRule returns nil while leaving the CloudStack rule in place (lines 831-833), and this unconditional clear drops the only record of that rule. On the next reconcile or delete, getLoadBalancer keeps the valid twin in lb.rules, so this duplicate is never retried and its rule/firewall/IP can remain orphaned. Preserve failed entries or return an error that causes deletion to retry instead of clearing them as successfully processed.

	lb.duplicateRules = nil

• Files reviewed: 26/27 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved issues remain in e2e caching, simulator defaults, cleanup behavior, and e2e test handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (8)

Makefile:36

• Exporting this fallback URL globally changes the existing opt-in acceptance-test behavior: a developer who has CS_API_KEY and CS_SECRET_KEY in the shell but intentionally has no CS_API_URL now passes all three checks in configFromEnv() and make test attempts http://localhost:8080 instead of skipping. Keep the simulator default scoped to the e2e/CI invocation, or require an explicitly configured endpoint before enabling acceptance tests.

CS_API_URL ?= http://localhost:$(SIM_HOST_PORT)/client/api

README.md:58

• getManagementServerVersion returns the API error immediately when listManagementServersMetrics is unauthorized; it only produces no management servers found after a successful response with Count == 0. This text therefore gives the wrong symptom for the missing-role permission failure and can mislead users debugging startup.

The account must also be allowed to call `listManagementServersMetrics`, which the controller uses
on startup to determine the management server version. This is a root admin API and is **not**
included in the default `User` role; without it the controller exits immediately with
`no management servers found`.

cloudstack_loadbalancer.go:836

• When the duplicate has an invalid port or protocol, getLoadBalancer has already omitted it from lb.rules, but this branch returns success and deleteDuplicateRules clears duplicateRules. A delete can therefore remove the kept rule and report success while orphaning this duplicate and its public IP; later reconciles repeat the no-op. Keep the item in a cleanup set or route it through a safe direct rule-deletion/retry path instead of silently forgetting it.

	if err != nil || protocol == LoadBalancerProtocolInvalid {
		klog.Warningf("Leaving duplicate rule %v (%v) in place: unusable public port %q or protocol %q", lbRule.Name, lbRule.Id, lbRule.Publicport, lbRule.Protocol)
		return nil

cloudstack_loadbalancer.go:438

• This still returns sweepErr from EnsureLoadBalancerDeleted, so any duplicate-cleanup failure is propagated to the service controller and can keep the Service in Terminating. That contradicts the PR's stated deletion behavior that sweep failures are logged rather than returned so deletion can complete. Please choose and document one contract: return nil after the primary resources are deleted, or retain retry semantics and update the PR description and test accordingly.

	return sweepErr

docs/development.md:417

• The controller returns the API authorization error directly when this call is denied; no management servers found is reserved for a successful empty response. Please describe the permission failure as an authorization/startup error here so the troubleshooting entry matches the actual failure path.

| CCM exits with `no management servers found` | The account cannot call `listManagementServersMetrics`. This is a root-admin API; the default `User` role does not include it. |

docs/development.md:265

• This section says to skip step 4, but cloud-config-host is generated by 40-ccm-deploy.sh in step 4; following the documented manual sequence leaves the --cloud-config path absent. The scale-down command is also shown only after the host process is launched, so a user can start two CCMs. Update the walkthrough to generate the host config and scale the in-cluster deployment down before starting the binary.

For interactive development and debugging, skip step 4 and run the binary
directly against the same environment:

test/e2e/annotations_test.go:171

• PublicIPByAddress explicitly returns (nil, nil) when the address is absent, and the regular delete test already treats that as a successful release. Returning (false, nil) for ip == nil here makes this test time out if CloudStack removes the released address from the listing instead of retaining a free record. Treat a nil record as released.

			if err != nil || ip == nil {
				return false, err
			}
			return ip.Allocated == "", nil

test/e2e/vpc_test.go:259

• PublicIPByAddress explicitly returns (nil, nil) when the address is absent, but this callback turns that valid released state into false and eventually times out. The non-VPC delete test handles ip == nil as success; this VPC regression test should do the same.

			if err != nil || ip == nil {
				return false, err
			}
			return ip.Allocated == "", nil

• Files reviewed: 26/27 changed files
• Comments generated: 3
• Review effort level: Lite

[vishesh92]

@vishesh92 can you check the below.

* Duplicate-rule cleanup assumes same public IP (please confirm or guard)
  In _getLoadBalancer_, a name-collided rule hits continue before the _lb.ipAddr_ consistency check, and _deleteDuplicateRules_ only deletes the LB rule. If a duplicate ever had a different public IP, its firewall rule / IP would leak on cleanup — the exact class of bug this PR fixes elsewhere. Can we either add a guard, or a one-line comment confirming duplicates are guaranteed to share IP/port/protocol?

* Guard the e2e job like the build job
  build is gated on _if: github.repository == 'apache/...' || workflow_dispatc_h, but e2e (needs: build) has no matching if. A workflow_dispatch from a fork (or a repo rename) would leave e2e running with no artifact to download. Also cache-to: type=gha,mode=max is read-only on fork PRs and will warn. Suggest mirroring the if: onto e2e.

* make test now runs against the live simulator in CI
  Sourcing _keys.env_ before make test activates the acceptance tests in _cloudstack_test.go_ against the simulator. Intentional and useful, but it couples the unit-test signal to simulator health. Worth confirming that's the intent — or splitting the pure-unit run from the live run so a sim hiccup doesn't read as a code failure.

* Minor nits
  
  * _.gitignore_ is missing a trailing newline.
  * _retry()_ in hack/e2e/lib/log.sh looks unused — drop it?
  * _docs/development.md_ says "Go 1.23 or later" while CI uses go-version-file: go.mod — keep in sync with go.mod.
  * _majorMinorPatch_ is duplicated in cloudstack.go and test/e2e/framework.go (unavoidable across the package boundary) — worth a note so a future edit updates both.

I have addressed these.

[sureshanaparti]

clgtm