-
Notifications
You must be signed in to change notification settings - Fork 32
[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
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.
// 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: