From 47cc587f83cda004b2f78dd09f60bca2d567fe48 Mon Sep 17 00:00:00 2001 From: Amanuel Engeda <74629455+engedaam@users.noreply.github.com> Date: Mon, 13 May 2024 19:01:32 -0500 Subject: [PATCH] chore: Update `associatePublicIPAddress` to not be set in the launch templets (#6159) --- pkg/cache/cache.go | 4 +-- .../launchtemplate/launchtemplate.go | 6 ++-- pkg/providers/subnet/subnet.go | 15 +++++---- pkg/providers/subnet/suite_test.go | 32 ++++++++++++++----- 4 files changed, 37 insertions(+), 20 deletions(-) diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index 796fa1ff9b20..2b7a926db59b 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -32,9 +32,9 @@ const ( // InstanceProfileTTL is the time before we refresh checking instance profile existence at IAM InstanceProfileTTL = 15 * time.Minute // AvailableIPAddressTTL is time to drop AvailableIPAddress data if it is not updated within the TTL - AvailableIPAddressTTL = 2 * time.Minute + AvailableIPAddressTTL = 5 * time.Minute // AvailableIPAddressTTL is time to drop AssociatePublicIPAddressTTL data if it is not updated within the TTL - AssociatePublicIPAddressTTL = 2 * time.Minute + AssociatePublicIPAddressTTL = 5 * time.Minute ) const ( diff --git a/pkg/providers/launchtemplate/launchtemplate.go b/pkg/providers/launchtemplate/launchtemplate.go index b90949530f6f..95f2d068667c 100644 --- a/pkg/providers/launchtemplate/launchtemplate.go +++ b/pkg/providers/launchtemplate/launchtemplate.go @@ -188,15 +188,13 @@ func (p *DefaultProvider) createAMIOptions(ctx context.Context, nodeClass *v1bet } if nodeClass.Spec.AssociatePublicIPAddress != nil { options.AssociatePublicIPAddress = nodeClass.Spec.AssociatePublicIPAddress - } else if ok, err := p.subnetProvider.CheckAnyPublicIPAssociations(ctx, nodeClass); err != nil { - return nil, err - } else if !ok { + } else { // when `AssociatePublicIPAddress` is not specified in the `EC2NodeClass` spec, // 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 of that field to 'false'. // https://github.com/aws/karpenter-provider-aws/issues/3815 - options.AssociatePublicIPAddress = aws.Bool(false) + options.AssociatePublicIPAddress = p.subnetProvider.AssociatePublicIPAddressValue(nodeClass) } return options, nil } diff --git a/pkg/providers/subnet/subnet.go b/pkg/providers/subnet/subnet.go index d67babebb1d9..2b07944037ef 100644 --- a/pkg/providers/subnet/subnet.go +++ b/pkg/providers/subnet/subnet.go @@ -37,7 +37,7 @@ import ( type Provider interface { LivenessProbe(*http.Request) error List(context.Context, *v1beta1.EC2NodeClass) ([]*ec2.Subnet, error) - CheckAnyPublicIPAssociations(context.Context, *v1beta1.EC2NodeClass) (bool, error) + AssociatePublicIPAddressValue( *v1beta1.EC2NodeClass) (*bool) ZonalSubnetsForLaunch(context.Context, *v1beta1.EC2NodeClass, []*cloudprovider.InstanceType, string) (map[string]*Subnet, error) UpdateInflightIPs(*ec2.CreateFleetInput, *ec2.CreateFleetOutput, []*cloudprovider.InstanceType, []*Subnet, string) } @@ -116,14 +116,17 @@ func (p *DefaultProvider) List(ctx context.Context, nodeClass *v1beta1.EC2NodeCl return lo.Values(subnets), nil } -// CheckAnyPublicIPAssociations returns a bool indicating whether all referenced subnets assign public IPv4 addresses to EC2 instances created therein -func (p *DefaultProvider) CheckAnyPublicIPAssociations(ctx context.Context, nodeClass *v1beta1.EC2NodeClass) (bool, error) { +// associatePublicIPAddressValue validates whether we know the association value for all subnets AND +// that all subnets don't have associatePublicIP set. If both of these are true, we set the value explicitly to false +// For more detail see: https://github.com/aws/karpenter-provider-aws/pull/3814 +func (p *DefaultProvider) AssociatePublicIPAddressValue(nodeClass *v1beta1.EC2NodeClass) *bool { for _, subnet := range nodeClass.Status.Subnets { - if subnetAssociatePublicIP, ok := p.associatePublicIPAddressCache.Get(subnet.ID); ok && subnetAssociatePublicIP.(bool) { - return true, nil + subnetAssociatePublicIP, ok := p.associatePublicIPAddressCache.Get(subnet.ID) + if !ok || subnetAssociatePublicIP.(bool) { + return nil } } - return false, nil + return lo.ToPtr(false) } // ZonalSubnetsForLaunch returns a mapping of zone to the subnet with the most available IP addresses and deducts the passed ips from the available count diff --git a/pkg/providers/subnet/suite_test.go b/pkg/providers/subnet/suite_test.go index 4d343bce09b1..f7007fd4e260 100644 --- a/pkg/providers/subnet/suite_test.go +++ b/pkg/providers/subnet/suite_test.go @@ -219,19 +219,20 @@ var _ = Describe("SubnetProvider", func() { }, subnets) }) }) - Context("CheckAnyPublicIPAssociations", func() { - It("should note that no subnets assign a public IPv4 address to EC2 instances on launch", func() { + Context("AssociatePublicIPAddress", func() { + It("should be false when no subnets assign a public IPv4 address to EC2 instances on launch", func() { nodeClass.Spec.SubnetSelectorTerms = []v1beta1.SubnetSelectorTerm{ { ID: "subnet-test1", Tags: map[string]string{"foo": "bar"}, }, } - onlyPrivate, err := awsEnv.SubnetProvider.CheckAnyPublicIPAssociations(ctx, nodeClass) + _, err := awsEnv.SubnetProvider.List(ctx, nodeClass) Expect(err).To(BeNil()) - Expect(onlyPrivate).To(BeFalse()) + associatePublicIP := awsEnv.SubnetProvider.AssociatePublicIPAddressValue(nodeClass) + Expect(lo.FromPtr(associatePublicIP)).To(BeFalse()) }) - It("should note that at least one subnet assigns a public IPv4 address to EC2instances on launch", func() { + It("should be nil when at least one subnet assigns a public IPv4 address to EC2instances on launch", func() { nodeClass.Spec.SubnetSelectorTerms = []v1beta1.SubnetSelectorTerm{ { ID: "subnet-test2", @@ -245,9 +246,24 @@ var _ = Describe("SubnetProvider", func() { } _, err := awsEnv.SubnetProvider.List(ctx, nodeClass) Expect(err).To(BeNil()) - onlyPrivate, err := awsEnv.SubnetProvider.CheckAnyPublicIPAssociations(ctx, nodeClass) - Expect(err).To(BeNil()) - Expect(onlyPrivate).To(BeTrue()) + associatePublicIP := awsEnv.SubnetProvider.AssociatePublicIPAddressValue(nodeClass) + Expect(associatePublicIP).To(BeNil()) + }) + It("should be nil when no subnet data is present in the provider cache", func() { + nodeClass.Spec.SubnetSelectorTerms = []v1beta1.SubnetSelectorTerm{ + { + ID: "subnet-test2", + }, + } + nodeClass.Status.Subnets = []v1beta1.Subnet{ + { + ID: "subnet-test2", + Zone: "test-zone-1b", + }, + } + awsEnv.SubnetCache.Flush() // remove any subnet data that might be in the subnetCache + associatePublicIP := awsEnv.SubnetProvider.AssociatePublicIPAddressValue(nodeClass) + Expect(associatePublicIP).To(BeNil()) }) }) Context("Provider Cache", func() {