feat: add networkInterfaces configuration to launchTemplate #4353

myaser · 2023-07-29T21:34:28Z

Fixes #2026

Description
a reduced version of #3819,
Add a networkInterfaces struct to the AWSNodeTemplate CRD, and use the parameters to create AWS launch templates.

How was this change tested?
unit and integration tests were created; also tested manually on a test cluster

Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: #
No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

netlify · 2023-07-29T21:34:44Z

✅ Deploy Preview for karpenter-docs-prod ready!

Name	Link
🔨 Latest commit	`3d378e3`
🔍 Latest deploy log	https://app.netlify.com/sites/karpenter-docs-prod/deploys/653aaca8262aaa000863e87d
😎 Deploy Preview	https://deploy-preview-4353--karpenter-docs-prod.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

aws/karpenter-provider-aws#4353

github-actions · 2023-08-13T12:03:39Z

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

bwagner5 · 2023-08-15T21:35:40Z

I've taken a first pass and then got side tracked by some other issues. I'll pull this down tomorrow and take it for a test drive.

pkg/providers/launchtemplate/launchtemplate.go

bwagner5 · 2023-08-17T20:41:52Z

I've tested this out on my side and verified everything is updating properly with the LaunchTemplate, instance, and drift when changing between params within NetworkInterfaces!

Nice! Glad we can get some functionality in so that we can build on this base!

Remaining items are just to fix the DeviceIndex and Description passing to the NetworkInterfaces and that should also fix the test that is failing.

myaser · 2023-08-18T05:23:04Z

I've tested this out on my side and verified everything is updating properly with the LaunchTemplate, instance, and drift when changing between params within NetworkInterfaces!

Nice! Glad we can get some functionality in so that we can build on this base!

Remaining items are just to fix the DeviceIndex and Description passing to the NetworkInterfaces and that should also fix the test that is failing.

great to hear 🎉
I have added the missing fields

test/suites/integration/networkinterfaces_test.go

bwagner5

/karpenter snapshot

github-actions · 2023-08-21T01:32:27Z

Snapshot successfully published to oci://public.ecr.aws/karpenter/karpenter:v0-a2843091fafed84c6e3557d460954c7148e2a1ae. Find the image tag and installation instructions at https://gallery.ecr.aws/karpenter/karpenter/

bwagner5

/karpenter snapshot

github-actions · 2023-08-21T17:23:57Z

Snapshot successfully published to oci://public.ecr.aws/karpenter/karpenter:v0-986c25ea65854d41f518c7527a1fe93e832b39c5. Find the image tag and installation instructions at https://gallery.ecr.aws/karpenter/karpenter/

bwagner5 · 2023-08-21T20:15:12Z

Looks like the network interfaces e2e are still failing:

2023-08-21T19:16:12.6161277Z �[38;5;9m• [FAILED] [51.744 seconds]�[0m
2023-08-21T19:16:12.6161643Z NetworkInterfaces
2023-08-21T19:16:12.6162973Z �[38;5;243m/home/runner/work/karpenter/karpenter/test/suites/integration/networkinterfaces_test.go:32�[0m
2023-08-21T19:16:12.6163764Z   �[38;5;9m�[1m[It] should create a node with more than one NetworkInterface�[0m
2023-08-21T19:16:12.6164422Z   �[38;5;243m/home/runner/work/karpenter/karpenter/test/suites/integration/networkinterfaces_test.go:124�[0m
2023-08-21T19:16:12.6165081Z �[38;5;243m------------------------------�[0m
2023-08-21T19:16:12.6165298Z 
2023-08-21T19:16:12.6165493Z �[38;5;9m�[1mSummarizing 2 Failures:�[0m
2023-08-21T19:16:12.6166305Z   �[38;5;9m[FAIL]�[0m �[0mNetworkInterfaces �[38;5;9m�[1m[It] should use the specified NetworkInterface�[0m
2023-08-21T19:16:12.6167362Z   �[38;5;243m/home/runner/work/karpenter/karpenter/test/suites/integration/networkinterfaces_test.go:121�[0m
2023-08-21T19:16:12.6168145Z   �[38;5;9m[FAIL]�[0m �[0mNetworkInterfaces �[38;5;9m�[1m[It] should create a node with more than one NetworkInterface�[0m
2023-08-21T19:16:12.6168893Z   �[38;5;243m/home/runner/work/karpenter/karpenter/test/suites/integration/networkinterfaces_test.go:155�[0m
2023-08-21T19:16:12.6169225Z 
2023-08-21T19:16:12.6169482Z �[38;5;9m�[1mRan 91 of 93 Specs in 5523.602 seconds�[0m
2023-08-21T19:16:12.6170324Z �[38;5;9m�[1mFAIL!�[0m -- �[38;5;10m�[1m89 Passed�[0m | �[38;5;9m�[1m2 Failed�[0m | �[38;5;11m�[1m0 Pending�[0m | �[38;5;14m�[1m2 Skipped�[0m

  [FAILED] Expected
      <*string | 0xc00141d648>: a test network interface
  to equal
      <string>: a test network interface
  In [It] at: /home/runner/work/karpenter/karpenter/test/suites/integration/networkinterfaces_test.go:121 @ 08/21/23 19:15:08.693
  < Exit [It] should use the specified NetworkInterface -

  [FAILED] Expected
      <*string | 0xc001279b28>: a test network interface
  to equal
      <string>: a test network interface
  In [It] at: /home/runner/work/karpenter/karpenter/test/suites/integration/networkinterfaces_test.go:155 @ 08/21/23 19:16:00.53
  < Exit [It] should create a node with more than one NetworkInterface -

test/suites/integration/networkinterfaces_test.go

myaser · 2023-08-22T09:07:34Z

Looks like the network interfaces e2e are still failing:

Sorry about that; I did not manage to run the e2e tests myself because of a different environment setup. Previously I had to hack around it and introduce changes to Makefile and test runner, which soon became outdated with upstream changes.

I hope the last commits have fixed it

bwagner5 · 2023-08-22T22:56:25Z

Looks like the network interfaces e2e are still failing:

Sorry about that; I did not manage to run the e2e tests myself because of a different environment setup. Previously I had to hack around it and introduce changes to Makefile and test runner, which soon became outdated with upstream changes.

I hope the last commits have fixed it

No worries! I'll kick it off!

bwagner5

/karpenter snapshot

github-actions · 2023-08-22T23:02:48Z

Snapshot successfully published to oci://public.ecr.aws/karpenter/karpenter:v0-a5558ac80077f5c1dd6856945b73731dfe66904a. Find the image tag and installation instructions at https://gallery.ecr.aws/karpenter/karpenter/

test/suites/integration/networkinterfaces_test.go

bwagner5

/karpenter snapshot

github-actions · 2023-08-30T14:23:09Z

Snapshot successfully published to oci://public.ecr.aws/karpenter/karpenter:v0-902336244f2750618749aab26e9f6cf5eba811c3. Find the image tag and installation instructions at https://gallery.ecr.aws/karpenter/karpenter/

bwagner5 · 2023-08-30T18:06:19Z

Ok, looks like the non-Integ e2e tests failed due to some transient infra issue. But the Integ tests now only have 2 failures, looking at those now:

Summarizing 2 Failures:
  [FAIL] NetworkInterfaces [It] should create a default NetworkInterface if none specified, with no public IP auto assignment
  /home/runner/work/karpenter/karpenter/test/suites/integration/networkinterfaces_test.go:50
  [FAIL] Scheduling [It] should provision three nodes for a zonal topology spread
  /home/runner/work/karpenter/karpenter/test/suites/integration/scheduling_test.go:325

bwagner5

/karpenter snapshot

jmdeal

/karpenter snapshot

github-actions · 2023-10-26T16:59:14Z

Snapshot successfully published to oci://071440425669.dkr.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter:v0-6771deec5719c4b408781a8e1445bc455d731a1b.

jmdeal

/karpenter snapshot

github-actions · 2023-10-26T17:16:02Z

Snapshot successfully published to oci://071440425669.dkr.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter:v0-3ecd2d2efc197672c14dfdc58f666dcb852903be.

…implified

jmdeal

/karpenter snapshot

github-actions · 2023-10-26T18:46:25Z

Snapshot successfully published to oci://071440425669.dkr.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter:v0-3d378e3739908c63104376fc94f8ee28f2305c35.

jmdeal · 2023-10-26T18:55:25Z

pkg/providers/launchtemplate/launchtemplate.go

-				Groups:                   lo.Map(options.SecurityGroups, func(s v1beta1.SecurityGroup, _ int) *string { return aws.String(s.ID) }),
-			},
+func (p *Provider) generateNetworkInterfaces(options *amifamily.LaunchTemplate) []*ec2.LaunchTemplateInstanceNetworkInterfaceSpecificationRequest {
+	if len(options.NetworkInterfaces) == 0 {


I'm thinking we should drop this behavior now, looking at the quoted issue it looks like the desired long term solution was adding these explicit fields. Thoughts @jonathan-innis?

I could see us marking this behavior as deprecated and then choosing to drop this behavior entirely at v1. We can add this to the v1 laundry list that is getting tracked in this issue: #4993

jonathan-innis

We should consider adding an example to the examples/v1beta1 directory as well when we add the interfaces to the API

jonathan-innis · 2023-10-26T22:19:44Z

pkg/apis/v1beta1/ec2nodeclass.go

+	AssociatePublicIPAddress *bool `json:"associatePublicIPAddress,omitempty"`
+
+	// A description for the network interface.
+	Description *string `json:"description,omitempty"`


How necessary is the description here? This feels ancillary and probably something that we can wait to add until users ask for it

It's not really necessary, its used in tests to grab the correct network interface but that can be done with the device index.

jonathan-innis · 2023-10-26T22:20:38Z

pkg/apis/v1beta1/ec2nodeclass.go

@@ -312,6 +315,18 @@ type BlockDevice struct {
 	VolumeType *string `json:"volumeType,omitempty"`
 }

+type NetworkInterface struct {


Is there a reason that all of these values need to be pointers? I think an empty value is probably equal to not set? Typically, you need pointers when an empty value is still a valid value, but I'm not sure that that's the case for any of these fields

I'll need to double check but I believe the zero value for AssociatePublicIPAddress would not be an acceptable default. If it's not specified in the EC2NodeClass we don't want to specify it in the launch template either that way we don't override the subnets behavior.

jonathan-innis · 2023-10-31T19:34:44Z

pkg/providers/launchtemplate/launchtemplate.go

-				Groups:                   lo.Map(options.SecurityGroups, func(s v1beta1.SecurityGroup, _ int) *string { return aws.String(s.ID) }),
-			},
+func (p *Provider) generateNetworkInterfaces(options *amifamily.LaunchTemplate) []*ec2.LaunchTemplateInstanceNetworkInterfaceSpecificationRequest {
+	if len(options.NetworkInterfaces) == 0 {


I could see us marking this behavior as deprecated and then choosing to drop this behavior entirely at v1. We can add this to the v1 laundry list that is getting tracked in this issue: #4993

jonathan-innis · 2023-10-31T19:36:51Z

test/suites/beta/integration/networkinterfaces_test.go

+			env.EventuallyExpectHealthy(pod)
+			env.ExpectCreatedNodeCount("==", 1)
+			instance := env.GetInstance(pod.Spec.NodeName)
+			for _, interfaceSpec := range interfaces {


I think that we have a function that grabs the interfaces from the instance? Can we leverage this function rather than grabbing the instance and doing things with checking the fields?

I'll take a look 👍

jonathan-innis · 2023-10-31T19:37:59Z

test/suites/beta/integration/networkinterfaces_test.go

+				}
+			}
+		},
+		Entry("when a single interface is specified", &v1beta1.NetworkInterface{


Each of these tests adds time. I like our bias for code coverage, but I think that we might be able to get away with testing the most complex scenario in the E2E test environment, condensing this down to a single test and then testing a bunch of different edge-case scenarios within the functional/unit testing

jonathan-innis · 2023-10-31T19:38:21Z

website/content/en/preview/concepts/nodeclasses.md

  # optional, configures IMDS for the instance
  metadataOptions:
    httpEndpoint: enabled
    httpProtocolIPv6: disabled
    httpPutResponseHopLimit: 2
    httpTokens: required

+  # optional, configure network interfaces for the instance


This is missing the networkInterfaces header

github-actions · 2023-11-15T12:04:40Z

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

jmdeal · 2023-11-20T18:55:06Z

Removing lifecycle/stale, this PR is still under consideration. There's been some discussion on whether Karpenter is the ideal place for this configuration surface to live, I plan on trying to move on this soon.

garvinp-stripe · 2023-11-21T20:40:11Z

Can you provide some update on the current thought?

jmdeal · 2023-11-22T22:03:55Z

We're hesitant to expand the EC2NodeClass to allow for configuring multiple network interfaces if it isn't strictly necessary. There were two separate use cases in the original issue: configuring the AssignPublicIpAddress field, and configuring multiple network interfaces for EFA support. AssignPublicIpAddress can only be used if you're configuring a single network interface; you can't assign a public IP in this manner to an instance launched with multiple network interfaces. As for EFA, we currently have a PR out which enables launching EFA instances without the need for explicit network interface configuration. The path forward here will likely be scoping this PR back to only include an AssignPublicIpAddress field on the EC2NodeClass rather than a NetworkInterfaces field.

garvinp-stripe · 2023-12-05T18:35:53Z

We might be the odd man out however we do leverage attaching multiple network interfaces to our instances outside of Public IP and EFA, we use a custom CNI that expects a secondary ENI. This PR solves this however we could get around this issue.

One thing I am curious about Karpenter's position is that the AWS provider variant of Karpenter essentially replaces ASGs and Launch Templates in AWS for us however there is a feature disparity between ASGs and Karpenter node management. Its true we may not need to match the functionalities however in my opinion the bias should be towards closing the disparity rather than away. This isn't specific to network interface support but rather as a whole how does/ should Karpenter think about what features it should be keeping from the components it is replacing vs dropping.

jmdeal · 2023-12-11T23:13:22Z

@garvinp-stripe are you able to elaborate about your use-case, preferably in an issue where it's easier to track? We're certainly not against bringing Karpenter closer to feature parity with launch templates, but we want to drive those decisions by user need rather than adding features because they are in launch templates. In this case, the problems outlined in the original issue either didn't need the full scope or were solved through other mechanisms. If you have a use-case for this we're more than happy to continue the discussion.

myaser · 2023-12-18T09:22:32Z

The path forward here will likely be scoping this PR back to only include an AssignPublicIpAddress field on the EC2NodeClass rather than a NetworkInterfaces field.

@jmdeal

ok, I can work on that. but I will do a separate PR.

jmdeal · 2023-12-18T22:19:24Z

Sounds good! I'll go ahead and close this PR out then.

myaser requested a review from a team as a code owner July 29, 2023 21:34

myaser requested a review from bwagner5 July 29, 2023 21:34

myaser added a commit to zalando-incubator/kubernetes-on-aws that referenced this pull request Jul 31, 2023

Update karpenter image

6dcc784

aws/karpenter-provider-aws#4353

github-actions bot added lifecycle/stale and removed lifecycle/stale labels Aug 13, 2023

myaser force-pushed the networkinterfaces-simplified branch from 6cc4b49 to c12aaaf Compare August 16, 2023 09:58

bwagner5 reviewed Aug 17, 2023

View reviewed changes

pkg/providers/launchtemplate/launchtemplate.go Outdated Show resolved Hide resolved

bwagner5 reviewed Aug 18, 2023

View reviewed changes

bwagner5 reviewed Aug 21, 2023

View reviewed changes

test/suites/integration/networkinterfaces_test.go Outdated Show resolved Hide resolved

test/suites/integration/networkinterfaces_test.go Outdated Show resolved Hide resolved

test/suites/integration/networkinterfaces_test.go Outdated Show resolved Hide resolved

myaser force-pushed the networkinterfaces-simplified branch from 5155663 to a5558ac Compare August 22, 2023 09:03

bwagner5 reviewed Aug 22, 2023

View reviewed changes

bwagner5 reviewed Aug 24, 2023

View reviewed changes

test/suites/integration/networkinterfaces_test.go Outdated Show resolved Hide resolved

bwagner5 reviewed Aug 30, 2023

View reviewed changes

bwagner5 force-pushed the networkinterfaces-simplified branch from 9023362 to 4dfdbc1 Compare September 6, 2023 17:02

bwagner5 reviewed Sep 6, 2023

View reviewed changes

jmdeal added 3 commits October 26, 2023 09:39

v1beta1 only + integration test fixes

d37cd44

fix merge errors

9f5a0df

Merge branch 'main' into networkinterfaces-simplified

6771dee

jmdeal reviewed Oct 26, 2023

View reviewed changes

formatting fix

3ecd2d2

jmdeal reviewed Oct 26, 2023

View reviewed changes

jmdeal force-pushed the networkinterfaces-simplified branch from 3009c77 to 8340b00 Compare October 26, 2023 17:54

update docs

44d2973

jmdeal force-pushed the networkinterfaces-simplified branch from 8340b00 to 44d2973 Compare October 26, 2023 17:55

jmdeal added 2 commits October 26, 2023 11:13

remove focus from e2e test

9be6ada

Merge remote-tracking branch 'upstream/main' into networkinterfaces-s…

3d378e3

…implified

jmdeal reviewed Oct 26, 2023

View reviewed changes

jonathan-innis reviewed Oct 31, 2023

View reviewed changes

github-actions bot added the lifecycle/stale label Nov 15, 2023

jmdeal removed the lifecycle/stale label Nov 20, 2023

mikkeloscar mentioned this pull request Dec 15, 2023

Update to Karpenter v0.31.3 zalando-incubator/kubernetes-on-aws#6561

Merged

jmdeal closed this Dec 18, 2023

lli-hiya mentioned this pull request May 2, 2024

Network Interface ipv6 support #6135

Open

feat: add networkInterfaces configuration to launchTemplate #4353

feat: add networkInterfaces configuration to launchTemplate #4353

Conversation

myaser commented Jul 29, 2023

netlify bot commented Jul 29, 2023 • edited Loading

✅ Deploy Preview for karpenter-docs-prod ready!

github-actions bot commented Aug 13, 2023

bwagner5 commented Aug 15, 2023

bwagner5 commented Aug 17, 2023

myaser commented Aug 18, 2023

bwagner5 left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 21, 2023

bwagner5 left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 21, 2023

bwagner5 commented Aug 21, 2023 • edited Loading

myaser commented Aug 22, 2023 • edited Loading

bwagner5 commented Aug 22, 2023

bwagner5 left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 22, 2023

bwagner5 left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 30, 2023

bwagner5 commented Aug 30, 2023

bwagner5 left a comment

Choose a reason for hiding this comment

jmdeal left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 26, 2023

jmdeal left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 26, 2023

jmdeal left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-innis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2023

jmdeal commented Nov 20, 2023

garvinp-stripe commented Nov 21, 2023

jmdeal commented Nov 22, 2023 • edited Loading

garvinp-stripe commented Dec 5, 2023

jmdeal commented Dec 11, 2023

myaser commented Dec 18, 2023

jmdeal commented Dec 18, 2023

netlify bot commented Jul 29, 2023 •

edited

Loading

bwagner5 commented Aug 21, 2023 •

edited

Loading

myaser commented Aug 22, 2023 •

edited

Loading

jmdeal commented Nov 22, 2023 •

edited

Loading