[feat] Add VPC and Subnet retention and adoption #789

komer3 · 2025-07-02T22:19:46Z

What this PR does / why we need it:

This PR introduces a crucial feature for users who want to manage existing Linode VPCs with the Cluster API provider or who need to prevent the controller from deleting VPCs and subnets that are managed externally. It provides more granular control over the lifecycle of network resources.

NOTE: We currently don't have functionality to update predefined/already-created subnets. We only have create/delete operations at the moment.

The new functionality includes:

VPC and Subnet Retention: Users can now set a retain flag on a LinodeVPC. When this flag is set, the controller will not delete the VPC in the Linode API when the Kubernetes object is removed. This allows the VPC to persist beyond the lifecycle of the CAPI cluster.
Conditional Subnet Deletion: When a LinodeVPC is marked with the retain flag, the controller will still manage the subnets defined within it. Any subnet that is not also marked with a retain flag will be deleted by the controller during the LinodeVPC deletion reconciliation. This allows for fine-grained control over which network resources are managed by CAPI.
VPC and Subnet Adoption: The controller can now "adopt" an existing VPC by matching the LinodeVPC resource name to an existing VPC's label in the Linode API. It will also adopt any existing subnets within that VPC by matching labels, and create any subnets that are defined in the spec but do not yet exist.

Special notes for your reviewer:

This PR introduces several important changes. Please pay close attention to the following areas:

API Changes (api/v1alpha2/linodevpc_types.go):
- New Retain fields have been added to LinodeVPCSpec and VPCSubnetCreateOptions. Please review the documentation for these fields.
Deletion Logic (internal/controller/linodevpc_controller.go):
- The reconcileDelete function contains the core logic for the retention policy. The flow first checks for the VPC retain flag. If the flag is set, it proceeds to handleRetainedVPC, which will delete any subnets that are part of the LinodeVPC spec and do not have their own retain flag set. The VPC itself is not deleted.
Adoption Logic (internal/controller/linodevpc_controller_helpers.go):
- The reconcileVPC and reconcileExistingVPC helper functions contain the logic for adopting existing VPCs and subnets by label, and creating new ones if they don't exist. This is a critical piece of the adoption feature.
Unit Tests (internal/controller/linodevpc_controller_test.go):
- New tests have been added to validate the retention and adoption features. These tests demonstrate the correct behavior for deletion reconciliation, including the logic for retained VPCs and unretained subnets.

TODOs:

squashed commits
includes documentation
adds unit tests
adds or updates e2e tests

This commit introduces a retention policy for LinodeVPC resources, allowing users to prevent the deletion of VPCs and their subnets when the corresponding Kubernetes object is deleted. Key features: - A `retain` boolean field has been added to both `LinodeVPCSpec` and `VPCSubnetCreateOptions`. - A new `--enable-subnet-deletion` controller flag allows for the deletion of unretained subnets within a retained VPC. This is disabled by default to prevent accidental data loss. - The VPC reconciler now supports adopting existing VPCs and their subnets by matching labels. If a subnet from the spec is not found in the existing VPC, it will be created. Comprehensive unit tests have been added in isolated test suites to cover these new behaviors.

codecov · 2025-07-02T22:22:10Z

Codecov Report

Attention: Patch coverage is 60.97561% with 64 lines in your changes missing coverage. Please review.

Project coverage is 62.82%. Comparing base (fc3564a) to head (7a05a08).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/controller/linodevpc_controller.go	56.30%	44 Missing and 8 partials ⚠️
...nternal/controller/linodevpc_controller_helpers.go	75.00%	7 Missing and 4 partials ⚠️
cmd/main.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #789      +/-   ##
==========================================
- Coverage   63.01%   62.82%   -0.19%     
==========================================
  Files          71       71              
  Lines        7078     7177      +99     
==========================================
+ Hits         4460     4509      +49     
- Misses       2362     2401      +39     
- Partials      256      267      +11

☔ View full report in Codecov by Sentry.
📢 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.

internal/controller/linodevpc_controller.go

amold1 · 2025-07-02T22:58:10Z

internal/controller/linodevpc_controller_helpers.go

 		vpcScope.LinodeVPC.Spec.VPCID = &vpcs[0].ID
-		updateVPCSpecSubnets(vpcScope, &vpcs[0])
+
+		// build a map of existing subnets to easily check for existence


are existing subnets allowed to have linodes attached to them when we adopt the subnet and vpc for the cluster?

Yeah they can. I can see a customer wanting multiple clusters in the same VPC subnet. If there are linode in the existing subnet and use want to reuse that subnet, they should use retain flag for that subnet to make sure we don't delete it.

api/v1alpha2/linodevpc_types.go

internal/controller/linodevpc_controller.go

cmd/main.go

internal/controller/linodevpc_controller_helpers.go

scmeggitt · 2025-07-07T15:40:10Z

Does this mean that in order to adopt a vpc or subnet, the label needs to be provided? Can it still be adopted by just providing the ID at create time of the linodeVPC resource?

komer3 · 2025-07-08T14:47:02Z

Does this mean that in order to adopt a vpc or subnet, the label needs to be provided? Can it still be adopted by just providing the ID at create time of the linodeVPC resource?

No you don't need to provide the label (just ID will work). If you want, you can provide the label instead of subnetID. That'll go and find a subnet with that label. If not found, it'll create it.

AshleyDumaine · 2025-07-09T19:01:57Z

docs/src/topics/vpc.md

 ### Running cilium connectivity tests
 One can also run cilium connectivity tests to make sure networking works fine within VPC. Follow the steps defined in [cilium e2e tests](https://docs.cilium.io/en/stable/contributing/testing/e2e/) guide to install cilium binary, set the KUBECONFIG variable and then run `cilium connectivity tests`.
+
+```


accidental addition?

oops yeah let me remove that

looks like it came back? 🤔

internal/controller/linodevpc_controller.go

…eteRetainedSubnets to handleRetainedSubnets to better reflect functionality.

internal/controller/linodevpc_controller.go

rahulait

Overall, it looks good to me.

…undant logging in handleRetainedSubnets method for improved clarity and maintainability.

…tCreateOptions to ensure consistency in terminology. Refactor VPC fetching logic in LinodeVPCReconciler to improve code clarity and reduce redundancy by introducing a helper function for VPC retrieval.

Copilot

Pull Request Overview

This PR introduces VPC and Subnet retention flags plus adoption logic, letting users prevent deletion of existing network resources and adopt them by label.

Adds retain fields to API types, CRD schema, and documentation.
Refactors deletion reconciliation into separate helpers for retained vs. non-retained VPCs and subnets.
Implements adoption by matching labels and creating missing subnets, with corresponding client interface, wrappers, mocks, and tests.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
api/v1alpha2/linodevpc_types.go	Added `Retain` fields to `LinodeVPCSpec` and subnets
config/crd/bases/infrastructure..._linodevpcs.yaml	Schema defaults and docs for `retain` flags
internal/controller/linodevpc_controller.go	Split `reconcileDelete` into `deleteVPCResources` and retention handlers
internal/controller/linodevpc_controller_helpers.go	New `reconcileExistingVPC`, `getVPC`, and subnet adoption logic
internal/controller/linodevpc_controller_test.go	Tests covering retention and adoption behaviors
docs/src/topics/vpc.md & docs/src/reference/out.md	Documentation for retention and adoption features

Comments suppressed due to low confidence (3)

docs/src/topics/vpc.md:89

The text says this behavior is the default, but the field's default is false. Clarify that users must explicitly set spec.retain: true to retain the VPC.

- **`spec.retain`**: When set to `true` on the `LinodeVPC`, the VPC itself will not be deleted from Linode. This is the default and recommended behavior when adopting an existing VPC.

internal/controller/linodevpc_controller.go:452

This log and the subsequent error handling are always executed because they are placed outside the loop's conditional. It should only run when a subnet with attached nodes is found, for example by returning from inside the loop or wrapping this block in an if tracking that condition.

		logger.Info("VPC subnets still has node(s) attached")

…documentation

github-actions bot added the new-feature label Jul 2, 2025

amold1 reviewed Jul 2, 2025

View reviewed changes

internal/controller/linodevpc_controller.go Outdated Show resolved Hide resolved

amold1 reviewed Jul 2, 2025

View reviewed changes

rahulait reviewed Jul 3, 2025

View reviewed changes

api/v1alpha2/linodevpc_types.go Outdated Show resolved Hide resolved

rahulait reviewed Jul 3, 2025

View reviewed changes

internal/controller/linodevpc_controller.go Outdated Show resolved Hide resolved

eljohnson92 reviewed Jul 3, 2025

View reviewed changes

cmd/main.go Outdated Show resolved Hide resolved

rahulait reviewed Jul 3, 2025

View reviewed changes

internal/controller/linodevpc_controller_helpers.go Outdated Show resolved Hide resolved

Refactor plus address comments

1e5aa24

komer3 had a problem deploying to prod July 9, 2025 18:23 — with GitHub Actions Error

doc update

f19fa16

komer3 had a problem deploying to prod July 9, 2025 18:26 — with GitHub Actions Error

main merge

1c21089

komer3 had a problem deploying to prod July 9, 2025 18:31 — with GitHub Actions Error

Merge branch 'main' into retain-vpc

7f98d08

komer3 temporarily deployed to prod July 9, 2025 18:32 — with GitHub Actions Inactive

komer3 temporarily deployed to prod July 9, 2025 18:33 — with GitHub Actions Inactive

AshleyDumaine reviewed Jul 9, 2025

View reviewed changes

typo fix

1b1c6e4

komer3 temporarily deployed to prod July 9, 2025 19:19 — with GitHub Actions Inactive

rahulait reviewed Jul 9, 2025

View reviewed changes

internal/controller/linodevpc_controller.go Outdated Show resolved Hide resolved

Refactor method names in LinodeVPCReconciler for clarity: renamed del…

61bcf55

…eteRetainedSubnets to handleRetainedSubnets to better reflect functionality.

komer3 temporarily deployed to prod July 9, 2025 23:29 — with GitHub Actions Inactive

rahulait reviewed Jul 10, 2025

View reviewed changes

internal/controller/linodevpc_controller.go Outdated Show resolved Hide resolved

rahulait previously approved these changes Jul 10, 2025

View reviewed changes

Update event recorder name back to LinodeVPCReconciler and remove red…

d68d6c2

…undant logging in handleRetainedSubnets method for improved clarity and maintainability.

komer3 dismissed rahulait’s stale review via d68d6c2 July 10, 2025 16:55

komer3 temporarily deployed to prod July 10, 2025 16:56 — with GitHub Actions Inactive

rahulait previously approved these changes Jul 10, 2025

View reviewed changes

rahulait requested a review from Copilot July 10, 2025 16:57

This comment was marked as outdated.

Sign in to view

Correct comments and documentation for the 'retain' field in VPCSubne…

13aa533

…tCreateOptions to ensure consistency in terminology. Refactor VPC fetching logic in LinodeVPCReconciler to improve code clarity and reduce redundancy by introducing a helper function for VPC retrieval.

komer3 dismissed rahulait’s stale review via 13aa533 July 10, 2025 17:18

komer3 temporarily deployed to prod July 10, 2025 17:19 — with GitHub Actions Inactive

Lint fix - Add sentinel error for VPC

0ec58d2

komer3 temporarily deployed to prod July 10, 2025 17:36 — with GitHub Actions Inactive

rahulait requested a review from Copilot July 10, 2025 17:38

Copilot AI reviewed Jul 10, 2025

View reviewed changes

rahulait previously approved these changes Jul 10, 2025

View reviewed changes

Add note regarding limitations on updating predefined subnets in VPC …

54df9fe

…documentation

komer3 dismissed rahulait’s stale review via 54df9fe July 10, 2025 18:00

komer3 had a problem deploying to prod July 10, 2025 18:01 — with GitHub Actions Failure

komer3 had a problem deploying to prod July 10, 2025 18:01 — with GitHub Actions Error

rahulait approved these changes Jul 10, 2025

View reviewed changes

Merge branch 'main' into retain-vpc

7a05a08

komer3 temporarily deployed to prod July 10, 2025 18:03 — with GitHub Actions Inactive

komer3 temporarily deployed to prod July 10, 2025 18:04 — with GitHub Actions Inactive

komer3 merged commit cb589b3 into main Jul 10, 2025
14 of 16 checks passed

komer3 deleted the retain-vpc branch July 10, 2025 19:55

[feat] Add VPC and Subnet retention and adoption #789

[feat] Add VPC and Subnet retention and adoption #789

Uh oh!

Conversation

komer3 commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

amold1 Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

komer3 Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scmeggitt commented Jul 7, 2025

Uh oh!

komer3 commented Jul 8, 2025

Uh oh!

AshleyDumaine Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

komer3 Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

komer3 Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

AshleyDumaine Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rahulait left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

komer3 commented Jul 2, 2025 •

edited

Loading

codecov bot commented Jul 2, 2025 •

edited

Loading

komer3 Jul 3, 2025 •

edited

Loading