-
Notifications
You must be signed in to change notification settings - Fork 28
[feat] Add support for new network interfaces #821
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6366173
to
a740457
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #821 +/- ##
==========================================
+ Coverage 63.44% 63.46% +0.01%
==========================================
Files 71 71
Lines 7414 7833 +419
==========================================
+ Hits 4704 4971 +267
- Misses 2435 2570 +135
- Partials 275 292 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4c2d4ea
to
4197d22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for Linode's new network interfaces feature (currently in beta), introducing a new network interface configuration approach while maintaining compatibility with existing legacy interfaces. The key changes include adding a new linodeInterfaces
field and interfaceGeneration
parameter to control which interface generation is used.
Key changes:
- Adds
linodeInterfaces
field toLinodeMachine
/LinodeMachineTemplate
CRDs for the new network interface generation - Introduces
interfaceGeneration
field to control which interface type to use (defaults to "legacy_config") - Updates controller logic to handle both legacy and new interface configurations with proper validation
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
api/v1alpha2/linodemachine_types.go | Adds new LinodeInterface types and interfaceGeneration field to support beta network interfaces |
internal/controller/linodemachine_controller_helpers.go | Implements dual interface support logic with generation-aware configuration |
internal/controller/linodemachine_controller.go | Updates reconciliation logic to handle new interface generation and firewall management |
internal/webhook/v1alpha2/linodemachine_webhook.go | Adds validation rules preventing incompatible combinations of new interfaces with legacy features |
config/crd/bases/*.yaml | Updates CRD definitions to include new interface fields and validation |
templates/infra/linodeMachineTemplate.yaml | Adds commented examples and warnings about new interface usage |
clients/clients.go | Adds LinodeInterfacesClient interface for new API methods |
mock/client.go | Adds mock implementations for new interface-related API calls |
Comments suppressed due to low confidence (2)
internal/controller/linodemachine_controller_helpers.go:768
- Missing assignment for the Address field. The code sets NAT1To1Address to "auto" but doesn't set Address, which is required according to the struct definition.
}
internal/controller/linodemachine_controller_helpers.go:277
- The buildInstanceAddrs function has been modified to call handleVlanIps, but this introduces code duplication. The VLAN IP handling logic appears twice - once in the new handleVlanIps function and once inline in the existing code. Consider consolidating this logic.
}
883ef4d
to
1e2504d
Compare
1e2504d
to
dffc41e
Compare
dffc41e
to
3405cf9
Compare
3405cf9
to
fd939a4
Compare
fd939a4
to
5166c9a
Compare
be8b0d2
to
265d4f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good to me! Once linode interfaces become GA (out of beta), we should create a flavor for it and add e2e tests.
@@ -125,6 +130,13 @@ type LinodeMachineSpec struct { | |||
// For more information, see https://techdocs.akamai.com/cloud-computing/docs/automatically-configure-networking | |||
// Defaults to true. | |||
NetworkHelper *bool `json:"networkHelper,omitempty"` | |||
|
|||
// InterfaceGeneration is the generation of the interface to use for the cluster's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm struggling to parse that. Should that be "...for the cluster's nodes IF interface / linodeInterface are not specified for a LinodeMachine"? If not, I'm not sure what it means. But then again, I don't really know what I'm talking about in this domain...
Feel free to resolve this comment into oblivion.
NOTES:
What this PR does / why we need it: Linode Interfaces is in beta. This adds support so customers opted into the beta can make use of the feature in CAPL.
This adds 2 new fields to the
LinodeMachine
/LinodeMachineTemplate
CRD:interfaceGeneration
which defaults tolegacy_config
if omitted (to not break existing users), unlesslinodeInterfaces
is defined, in which case it will default tolinode
(new interfaces). This field allows the user to omit bothinterfaces
andlinodeInterfaces
while allowing the LM controller to know which generation to use for the LM's auto-provisioned interfaces.linodeInterfaces
(various webhook validation rules have been added since this does not play well with certain settings like network helper, legacy interfaces, and private IPs)Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer: If you want to test this locally you need to opt in to the Linode Interfaces beta.
TODOs:
Testing
PREREQUISITE: Opt in to the new network interfaces beta: https://cloud.linode.com/betas. Make sure once you do that you can list the "Linode Interfaces" capability via the linode-CLI:
You'll also want to disable the default network helper in your account settings.
make local-release
make local-deploy
clusterctl generate cluster test-cluster --kubernetes-version v1.31.8 --infrastructure local-linode:v0.0.0 > test-cluster.yaml
LinodeMachineTemplate
for test-cluster-md-0:with
(If you want to do this for the control-plane LMT, you will need the changes in #825 so the default cluster NB doesn't need private IPs which are not supported by the new interfaces. With this you only need to add
interfaceGeneration: linode
into the LMT spec for both the control plane and workers and can exclude defininginterfaces
/linodeInterfaces
)5.
kubectl apply -f test-cluster.yaml
6. Verify the cluster eventually provisions successfully: