feat: add networkInterfaces configuration to launchTemplate #4353
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -235,7 +235,7 @@ func (p *Provider) createLaunchTemplate(ctx context.Context, options *amifamily. | |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| networkInterface := p.generateNetworkInterfaces(options) | ||
| output, err := p.ec2api.CreateLaunchTemplateWithContext(ctx, &ec2.CreateLaunchTemplateInput{ | ||
| LaunchTemplateName: aws.String(launchTemplateName(options)), | ||
| LaunchTemplateData: &ec2.RequestLaunchTemplateData{ | ||
|
|
@@ -275,22 +275,36 @@ func (p *Provider) createLaunchTemplate(ctx context.Context, options *amifamily. | |
| return output.LaunchTemplate, nil | ||
| } | ||
|
|
||
| // generateNetworkInterfaces generates a network interface for the launch template. | ||
| // If all referenced subnets do not assign public IPv4 addresses to EC2 instances therein, we explicitly set | ||
| // AssociatePublicIpAddress to 'false' in the Launch Template, generated based on this configuration struct. | ||
| // This is done to help comply with AWS account policies that require explicitly setting that field to 'false'. | ||
| // https://github.com/aws/karpenter/issues/3815 | ||
| func (p *Provider) generateNetworkInterfaces(options *amifamily.LaunchTemplate) []*ec2.LaunchTemplateInstanceNetworkInterfaceSpecificationRequest { | ||
| if len(options.NetworkInterfaces) == 0 { | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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 |
||
| if options.AssociatePublicIPAddress != nil { | ||
| return []*ec2.LaunchTemplateInstanceNetworkInterfaceSpecificationRequest{ | ||
| { | ||
| AssociatePublicIpAddress: options.AssociatePublicIPAddress, | ||
| DeviceIndex: aws.Int64(0), | ||
| Groups: lo.Map(options.SecurityGroups, func(s v1beta1.SecurityGroup, _ int) *string { return aws.String(s.ID) }), | ||
| }, | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| var networkInterfacesRequest []*ec2.LaunchTemplateInstanceNetworkInterfaceSpecificationRequest | ||
| for _, networkInterface := range options.NetworkInterfaces { | ||
| networkInterfacesRequest = append(networkInterfacesRequest, &ec2.LaunchTemplateInstanceNetworkInterfaceSpecificationRequest{ | ||
| AssociatePublicIpAddress: networkInterface.AssociatePublicIPAddress, | ||
| DeviceIndex: networkInterface.DeviceIndex, | ||
| Description: networkInterface.Description, | ||
| Groups: lo.Map(options.SecurityGroups, func(s v1beta1.SecurityGroup, _ int) *string { return aws.String(s.ID) }), | ||
| }) | ||
| } | ||
| return networkInterfacesRequest | ||
|
|
||
| } | ||
|
|
||
| func (p *Provider) blockDeviceMappings(blockDeviceMappings []*v1beta1.BlockDeviceMapping) []*ec2.LaunchTemplateBlockDeviceMappingRequest { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| /* | ||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package integration_test | ||
|
|
||
| import ( | ||
| "github.com/aws/aws-sdk-go/service/ec2" | ||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
| "github.com/samber/lo" | ||
|
|
||
| "github.com/aws/karpenter-core/pkg/test" | ||
| "github.com/aws/karpenter/pkg/apis/v1beta1" | ||
| ) | ||
|
|
||
| var _ = FDescribe("NetworkInterfaces", func() { | ||
| BeforeEach(func() { | ||
| // Ensure that nodes schedule to private subnets. If a node without a public IP is assigned to a public subnet, | ||
| // and that subnet does not contain any private endpoints to the cluster, the node will be unable to join the | ||
| // cluster. | ||
| nodeClass.Spec.SubnetSelectorTerms = []v1beta1.SubnetSelectorTerm{ | ||
| { | ||
| Tags: map[string]string{ | ||
| "Name": "*Private*", | ||
| "karpenter.sh/discovery": env.ClusterName, | ||
| }, | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
| DescribeTable( | ||
| "should correctly create NetworkInterfaces", | ||
| func(interfaces ...*v1beta1.NetworkInterface) { | ||
| nodeClass.Spec.NetworkInterfaces = interfaces | ||
| pod := test.Pod() | ||
| env.ExpectCreated(pod, nodeClass, nodePool) | ||
| env.EventuallyExpectHealthy(pod) | ||
| env.ExpectCreatedNodeCount("==", 1) | ||
| instance := env.GetInstance(pod.Spec.NodeName) | ||
| for _, interfaceSpec := range interfaces { | ||
|
There was a problem hiding this comment. 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? |
||
| ni, ok := lo.Find(instance.NetworkInterfaces, func(ni *ec2.InstanceNetworkInterface) bool { | ||
| if ni.Description == nil { | ||
| return false | ||
| } | ||
| return *ni.Description == *interfaceSpec.Description | ||
| }) | ||
| Expect(ok).To(BeTrue()) | ||
| Expect(ni.Attachment).To(HaveField("DeviceIndex", HaveValue(Equal(*interfaceSpec.DeviceIndex)))) | ||
| } | ||
|
|
||
| if len(interfaces) == 1 && interfaces[0].AssociatePublicIPAddress != nil { | ||
| if *interfaces[0].AssociatePublicIPAddress { | ||
| Expect(instance.PublicIpAddress).ToNot(BeNil()) | ||
| } else { | ||
| Expect(instance.PublicIpAddress).To(BeNil()) | ||
| } | ||
| } | ||
| }, | ||
| Entry("when a single interface is specified", &v1beta1.NetworkInterface{ | ||
|
There was a problem hiding this comment. 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 |
||
| Description: lo.ToPtr("a test interface"), | ||
| DeviceIndex: lo.ToPtr(int64(0)), | ||
| }), | ||
| Entry("when a single interface is specified with AssociatePublicIPAddress = true", &v1beta1.NetworkInterface{ | ||
| AssociatePublicIPAddress: lo.ToPtr(true), | ||
| Description: lo.ToPtr("a test interface"), | ||
| DeviceIndex: lo.ToPtr(int64(0)), | ||
| }), | ||
| Entry("when a single interface is specified with AssociatePublicIPAddress = false", &v1beta1.NetworkInterface{ | ||
| AssociatePublicIPAddress: lo.ToPtr(false), | ||
| Description: lo.ToPtr("a test interface"), | ||
| DeviceIndex: lo.ToPtr(int64(0)), | ||
| }), | ||
| Entry( | ||
| "when multiple interfaces are specified", | ||
| &v1beta1.NetworkInterface{ | ||
| Description: lo.ToPtr("a test interface"), | ||
| DeviceIndex: lo.ToPtr(int64(0)), | ||
| }, | ||
| &v1beta1.NetworkInterface{ | ||
| Description: lo.ToPtr("another test interface"), | ||
| DeviceIndex: lo.ToPtr(int64(1)), | ||
| }, | ||
| ), | ||
| ) | ||
| }) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
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'll need to double check but I believe the zero value for
AssociatePublicIPAddresswould not be an acceptable default. If it's not specified in theEC2NodeClasswe don't want to specify it in the launch template either that way we don't override the subnets behavior.