From 6b90e4e72a6becc1f053d52057721a8626830536 Mon Sep 17 00:00:00 2001 From: Jason Deal Date: Thu, 15 Feb 2024 16:07:13 -0800 Subject: [PATCH] chore: respect AssociatePublicIPAddress with EFA (#5666) --- .../launchtemplate/launchtemplate.go | 3 + pkg/providers/launchtemplate/suite_test.go | 69 +++++++------------ .../en/preview/concepts/nodeclasses.md | 47 +++++++------ 3 files changed, 50 insertions(+), 69 deletions(-) diff --git a/pkg/providers/launchtemplate/launchtemplate.go b/pkg/providers/launchtemplate/launchtemplate.go index c91f2bca3e09..ae935db60bbf 100644 --- a/pkg/providers/launchtemplate/launchtemplate.go +++ b/pkg/providers/launchtemplate/launchtemplate.go @@ -291,6 +291,9 @@ func (p *Provider) generateNetworkInterfaces(options *amifamily.LaunchTemplate) DeviceIndex: lo.ToPtr(lo.Ternary[int64](i == 0, 0, 1)), InterfaceType: lo.ToPtr(ec2.NetworkInterfaceTypeEfa), Groups: lo.Map(options.SecurityGroups, func(s v1beta1.SecurityGroup, _ int) *string { return aws.String(s.ID) }), + // Instances launched with multiple pre-configured network interfaces cannot set AssociatePublicIPAddress to true. This is an EC2 limitation. However, this does not apply for instances + // with a single EFA network interface, and we should support those use cases. Launch failures with multiple enis should be considered user misconfiguration. + AssociatePublicIpAddress: options.AssociatePublicIPAddress, } }) } diff --git a/pkg/providers/launchtemplate/suite_test.go b/pkg/providers/launchtemplate/suite_test.go index aaf548a780ac..a48084cae712 100644 --- a/pkg/providers/launchtemplate/suite_test.go +++ b/pkg/providers/launchtemplate/suite_test.go @@ -1560,7 +1560,7 @@ var _ = Describe("LaunchTemplates", func() { Expect(*input.LaunchTemplateData.ImageId).To(ContainSubstring("test-ami")) }) }) - Context("Subnet-based Launch Template Configration", func() { + Context("Public IP Association", func() { It("should explicitly set 'AssociatePublicIPAddress' to false in the Launch Template", func() { nodeClass.Spec.SubnetSelectorTerms = []v1beta1.SubnetSelectorTerm{ {Tags: map[string]string{"Name": "test-subnet-1"}}, @@ -1573,52 +1573,10 @@ var _ = Describe("LaunchTemplates", func() { input := awsEnv.EC2API.CalledWithCreateLaunchTemplateInput.Pop() Expect(*input.LaunchTemplateData.NetworkInterfaces[0].AssociatePublicIpAddress).To(BeFalse()) }) - - It("should overwrite 'AssociatePublicIPAddress' to true when specified by user in the EC2NodeClass", func() { - nodeClass.Spec.SubnetSelectorTerms = []v1beta1.SubnetSelectorTerm{ - {Tags: map[string]string{"Name": "test-subnet-1"}}, - {Tags: map[string]string{"Name": "test-subnet-3"}}, - } - nodeClass.Spec.AssociatePublicIPAddress = lo.ToPtr(true) - - ExpectApplied(ctx, env.Client, nodePool, nodeClass) - pod := coretest.UnschedulablePod() - ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, prov, pod) - ExpectScheduled(ctx, env.Client, pod) - input := awsEnv.EC2API.CalledWithCreateLaunchTemplateInput.Pop() - Expect(*input.LaunchTemplateData.NetworkInterfaces[0].AssociatePublicIpAddress).To(BeTrue()) - }) - - It("should set 'AssociatePublicIPAddress' to false when specified by user in the EC2NodeClass", func() { - nodeClass.Spec.SubnetSelectorTerms = []v1beta1.SubnetSelectorTerm{ - {Tags: map[string]string{"Name": "test-subnet-1"}}, - {Tags: map[string]string{"Name": "test-subnet-3"}}, - } - nodeClass.Spec.AssociatePublicIPAddress = lo.ToPtr(false) - - ExpectApplied(ctx, env.Client, nodePool, nodeClass) - pod := coretest.UnschedulablePod() - ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, prov, pod) - ExpectScheduled(ctx, env.Client, pod) - input := awsEnv.EC2API.CalledWithCreateLaunchTemplateInput.Pop() - Expect(*input.LaunchTemplateData.NetworkInterfaces[0].AssociatePublicIpAddress).To(BeFalse()) - }) - - It("should set 'AssociatePublicIPAddress' to false when not specified by the user in the EC2NodeClass using private subnet", func() { + It("should not explicitly set 'AssociatePublicIPAddress' when the subnets are configured to assign public IPv4 addresses", func() { nodeClass.Spec.SubnetSelectorTerms = []v1beta1.SubnetSelectorTerm{ - {Tags: map[string]string{"Name": "test-subnet-1"}}, - {Tags: map[string]string{"Name": "test-subnet-3"}}, + {Tags: map[string]string{"Name": "test-subnet-2"}}, } - - ExpectApplied(ctx, env.Client, nodePool, nodeClass) - pod := coretest.UnschedulablePod() - ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, prov, pod) - ExpectScheduled(ctx, env.Client, pod) - input := awsEnv.EC2API.CalledWithCreateLaunchTemplateInput.Pop() - Expect(*input.LaunchTemplateData.NetworkInterfaces[0].AssociatePublicIpAddress).To(BeFalse()) - }) - It("should not explicitly set 'AssociatePublicIPAddress' when the subnets are configured to assign public IPv4 addresses", func() { - nodeClass.Spec.SubnetSelectorTerms = []v1beta1.SubnetSelectorTerm{{Tags: map[string]string{"Name": "test-subnet-2"}}} ExpectApplied(ctx, env.Client, nodePool, nodeClass) pod := coretest.UnschedulablePod() ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, prov, pod) @@ -1626,6 +1584,27 @@ var _ = Describe("LaunchTemplates", func() { input := awsEnv.EC2API.CalledWithCreateLaunchTemplateInput.Pop() Expect(len(input.LaunchTemplateData.NetworkInterfaces)).To(BeNumerically("==", 0)) }) + DescribeTable( + "should set 'AssociatePublicIPAddress' based on EC2NodeClass", + func(setValue, expectedValue, isEFA bool) { + nodeClass.Spec.AssociatePublicIPAddress = lo.ToPtr(setValue) + ExpectApplied(ctx, env.Client, nodePool, nodeClass) + pod := coretest.UnschedulablePod(lo.Ternary(isEFA, coretest.PodOptions{ + ResourceRequirements: v1.ResourceRequirements{ + Requests: v1.ResourceList{v1beta1.ResourceEFA: resource.MustParse("2")}, + Limits: v1.ResourceList{v1beta1.ResourceEFA: resource.MustParse("2")}, + }, + }, coretest.PodOptions{})) + ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, prov, pod) + ExpectScheduled(ctx, env.Client, pod) + input := awsEnv.EC2API.CalledWithCreateLaunchTemplateInput.Pop() + Expect(*input.LaunchTemplateData.NetworkInterfaces[0].AssociatePublicIpAddress).To(Equal(expectedValue)) + }, + Entry("AssociatePublicIPAddress is true", true, true, false), + Entry("AssociatePublicIPAddress is false", false, false, false), + Entry("AssociatePublicIPAddress is true (EFA)", true, true, true), + Entry("AssociatePublicIPAddress is false (EFA)", false, false, true), + ) }) Context("Kubelet Args", func() { It("should specify the --dns-cluster-ip flag when clusterDNSIP is set", func() { diff --git a/website/content/en/preview/concepts/nodeclasses.md b/website/content/en/preview/concepts/nodeclasses.md index a5208f7e82ae..d5e164a5b364 100644 --- a/website/content/en/preview/concepts/nodeclasses.md +++ b/website/content/en/preview/concepts/nodeclasses.md @@ -35,7 +35,7 @@ spec: # Each term in the array of subnetSelectorTerms is ORed together # Within a single term, all conditions are ANDed subnetSelectorTerms: - # Select on any subnet that has the "karpenter.sh/discovery: ${CLUSTER_NAME}" + # Select on any subnet that has the "karpenter.sh/discovery: ${CLUSTER_NAME}" # AND the "environment: test" tag OR any subnet with ID "subnet-09fa4a0a8f233a921" - tags: karpenter.sh/discovery: "${CLUSTER_NAME}" @@ -46,8 +46,8 @@ spec: # Each term in the array of securityGroupSelectorTerms is ORed together # Within a single term, all conditions are ANDed securityGroupSelectorTerms: - # Select on any security group that has both the "karpenter.sh/discovery: ${CLUSTER_NAME}" tag - # AND the "environment: test" tag OR any security group with the "my-security-group" name + # Select on any security group that has both the "karpenter.sh/discovery: ${CLUSTER_NAME}" tag + # AND the "environment: test" tag OR any security group with the "my-security-group" name # OR any security group with ID "sg-063d7acfb4b06c82c" - tags: karpenter.sh/discovery: "${CLUSTER_NAME}" @@ -70,15 +70,15 @@ spec: # Each term in the array of amiSelectorTerms is ORed together # Within a single term, all conditions are ANDed amiSelectorTerms: - # Select on any AMI that has both the "karpenter.sh/discovery: ${CLUSTER_NAME}" tag - # AND the "environment: test" tag OR any AMI with the "my-ami" name + # Select on any AMI that has both the "karpenter.sh/discovery: ${CLUSTER_NAME}" tag + # AND the "environment: test" tag OR any AMI with the "my-ami" name # OR any AMI with ID "ami-123" - tags: karpenter.sh/discovery: "${CLUSTER_NAME}" environment: test - name: my-ami - id: ami-123 - + # Optional, use instance-store volumes for node ephemeral-storage instanceStorePolicy: RAID0 @@ -252,7 +252,7 @@ This selection logic is modeled as terms, where each term contains multiple cond ```yaml subnetSelectorTerms: - # Select on any subnet that has the "karpenter.sh/discovery: ${CLUSTER_NAME}" + # Select on any subnet that has the "karpenter.sh/discovery: ${CLUSTER_NAME}" # AND the "environment: test" tag OR any subnet with ID "subnet-09fa4a0a8f233a921" - tags: karpenter.sh/discovery: "${CLUSTER_NAME}" @@ -320,8 +320,8 @@ This selection logic is modeled as terms, where each term contains multiple cond ```yaml securityGroupSelectorTerms: - # Select on any security group that has both the "karpenter.sh/discovery: ${CLUSTER_NAME}" tag - # AND the "environment: test" tag OR any security group with the "my-security-group" name + # Select on any security group that has both the "karpenter.sh/discovery: ${CLUSTER_NAME}" tag + # AND the "environment: test" tag OR any security group with the "my-security-group" name # OR any security group with ID "sg-063d7acfb4b06c82c" - tags: karpenter.sh/discovery: "${CLUSTER_NAME}" @@ -407,8 +407,8 @@ This selection logic is modeled as terms, where each term contains multiple cond ```yaml amiSelectorTerms: - # Select on any AMI that has both the "karpenter.sh/discovery: ${CLUSTER_NAME}" tag - # AND the "environment: test" tag OR any AMI with the "my-ami" name + # Select on any AMI that has both the "karpenter.sh/discovery: ${CLUSTER_NAME}" tag + # AND the "environment: test" tag OR any AMI with the "my-ami" name # OR any AMI with ID "ami-123" - tags: karpenter.sh/discovery: "${CLUSTER_NAME}" @@ -633,25 +633,25 @@ The `instanceStorePolicy` field controls how [instance-store](https://docs.aws.a If you intend to use these volumes for faster node ephemeral-storage, set `instanceStorePolicy` to `RAID0`: -```yaml -spec: +```yaml +spec: instanceStorePolicy: RAID0 ``` -This will set the allocatable ephemeral-storage of each node to the total size of the instance-store volume(s). +This will set the allocatable ephemeral-storage of each node to the total size of the instance-store volume(s). -The disks must be formatted & mounted in a RAID0 and be the underlying filesystem for the Kubelet & Containerd. Instructions for each AMI family are listed below: +The disks must be formatted & mounted in a RAID0 and be the underlying filesystem for the Kubelet & Containerd. Instructions for each AMI family are listed below: -#### AL2 +#### AL2 -On AL2, Karpenter automatically configures the disks through an additional boostrap argument (`--local-disks raid0`). The device name is `/dev/md/0` and its mount point is `/mnt/k8s-disks/0`. You should ensure any additional disk setup does not interfere with these. +On AL2, Karpenter automatically configures the disks through an additional boostrap argument (`--local-disks raid0`). The device name is `/dev/md/0` and its mount point is `/mnt/k8s-disks/0`. You should ensure any additional disk setup does not interfere with these. -#### Others +#### Others -For all other AMI families, you must configure the disks yourself. Check out the [`setup-local-disks`](https://github.com/awslabs/amazon-eks-ami/blob/master/files/bin/setup-local-disks) script in [amazon-eks-ami](https://github.com/awslabs/amazon-eks-ami) to see how this is done for AL2. +For all other AMI families, you must configure the disks yourself. Check out the [`setup-local-disks`](https://github.com/awslabs/amazon-eks-ami/blob/master/files/bin/setup-local-disks) script in [amazon-eks-ami](https://github.com/awslabs/amazon-eks-ami) to see how this is done for AL2. -{{% alert title="Tip" color="secondary" %}} -Since the Kubelet & Containerd will be using the instance-store filesystem, you may consider using a more minimal root volume size. +{{% alert title="Tip" color="secondary" %}} +Since the Kubelet & Containerd will be using the instance-store filesystem, you may consider using a more minimal root volume size. {{% /alert %}} ## spec.userData @@ -872,9 +872,8 @@ spec: A boolean field that controls whether instances created by Karpenter for this EC2NodeClass will have an associated public IP address. This overrides the `MapPublicIpOnLaunch` setting applied to the subnet the node is launched in. If this field is not set, the `MapPublicIpOnLaunch` field will be respected. {{% alert title="Note" color="warning" %}} -If a `NodeClaim` requests `vpc.amazonaws.com/efa` resources, the `associatePublicIPAddress` field is ignored. -A public IP address may only be associated with a node at launch if a single network interface is configured. -This is inherently incompatible with instances configured for EFA workloads since Karpenter will configure an EFA for each network card on the instance. +If a `NodeClaim` requests `vpc.amazonaws.com/efa` resources, `spec.associatePublicIPAddress` is respected. However, if this `NodeClaim` requests **multiple** EFA resources and the value for `spec.associatePublicIPAddress` is true, the instance will fail to launch. This is due to an EC2 restriction which +requires that the field is only set to true when configuring an instance with a single ENI at launch. When using this field, it is advised that users segregate their EFA workload to use a separate `NodePool` / `EC2NodeClass` pair. {{% /alert %}} ## status.subnets