Skip to content

Commit f23af71

Browse files
engedaamsaurav-agarwalla
engedaam
and
saurav-agarwalla
authored
(Backport v1.4.x)fix: prevent hash collisions while resolving subnets, security groups and AMIs from nodeclass selectors (#8658)
Co-authored-by: Saurav Agarwalla <[email protected]>
1 parent 7735a95 commit f23af71

File tree

12 files changed

+511
-37
lines changed

12 files changed

+511
-37
lines changed

‎pkg/controllers/nodeclass/ami_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -647,7 +647,7 @@ var _ = Describe("NodeClass AMI Status Controller", func() {
647647
awsEnv.Clock.Step(40 * time.Minute)
648648

649649
// Flush Cache
650-
awsEnv.EC2Cache.Flush()
650+
awsEnv.AMICache.Flush()
651651

652652
ExpectObjectReconciled(ctx, env.Client, controller, nodeClass)
653653
nodeClass = ExpectExists(ctx, env.Client, nodeClass)
@@ -746,7 +746,7 @@ var _ = Describe("NodeClass AMI Status Controller", func() {
746746
},
747747
})
748748

749-
awsEnv.EC2Cache.Flush()
749+
awsEnv.AMICache.Flush()
750750

751751
ExpectApplied(ctx, env.Client, nodeClass)
752752
ExpectObjectReconciled(ctx, env.Client, controller, nodeClass)

‎pkg/controllers/providers/ssm/invalidation/controller.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ import (
2929
v1 "github.com/aws/karpenter-provider-aws/pkg/apis/v1"
3030
"github.com/aws/karpenter-provider-aws/pkg/providers/amifamily"
3131
"github.com/aws/karpenter-provider-aws/pkg/providers/ssm"
32+
33+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
34+
"k8s.io/apimachinery/pkg/util/uuid"
3235
)
3336

3437
// The SSM Invalidation controller is responsible for invalidating "latest" SSM parameters when they point to deprecated
@@ -66,6 +69,9 @@ func (c *Controller) Reconcile(ctx context.Context) (reconcile.Result, error) {
6669
amis := []amifamily.AMI{}
6770
for _, nodeClass := range lo.Map(lo.Keys(amiIDsToParameters), func(amiID string, _ int) *v1.EC2NodeClass {
6871
return &v1.EC2NodeClass{
72+
ObjectMeta: metav1.ObjectMeta{
73+
UID: uuid.NewUUID(), // ensures that this doesn't hit the AMI cache.
74+
},
6975
Spec: v1.EC2NodeClassSpec{
7076
AMISelectorTerms: []v1.AMISelectorTerm{{ID: amiID}},
7177
},

‎pkg/fake/types.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525

2626
type MockedFunction[I any, O any] struct {
2727
Output AtomicPtr[O] // Output to return on call to this function
28+
MultiOut AtomicPtrSlice[O]
2829
OutputPages AtomicPtrSlice[O]
2930
CalledWithInput AtomicPtrSlice[I] // Slice used to keep track of passed input to this function
3031
Error AtomicError // Error to return a certain number of times defined by custom error options
@@ -38,6 +39,7 @@ type MockedFunction[I any, O any] struct {
3839
// each other.
3940
func (m *MockedFunction[I, O]) Reset() {
4041
m.Output.Reset()
42+
m.MultiOut.Reset()
4143
m.OutputPages.Reset()
4244
m.CalledWithInput.Reset()
4345
m.Error.Reset()
@@ -60,6 +62,11 @@ func (m *MockedFunction[I, O]) Invoke(input *I, defaultTransformer func(*I) (*O,
6062
m.successfulCalls.Add(1)
6163
return m.Output.Clone(), nil
6264
}
65+
66+
if m.MultiOut.Len() > 0 {
67+
m.successfulCalls.Add(1)
68+
return m.MultiOut.Pop(), nil
69+
}
6370
// This output pages multi-threaded handling isn't perfect
6471
// It will fail if pages are asynchronously requested from the same NextToken
6572
if m.OutputPages.Len() > 0 {

‎pkg/providers/amifamily/ami.go‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
"k8s.io/utils/clock"
3030

3131
"github.com/aws/karpenter-provider-aws/pkg/errors"
32+
"github.com/aws/karpenter-provider-aws/pkg/utils"
3233

3334
v1 "github.com/aws/karpenter-provider-aws/pkg/apis/v1"
3435
sdk "github.com/aws/karpenter-provider-aws/pkg/aws"
@@ -69,11 +70,7 @@ func NewDefaultProvider(clk clock.Clock, versionProvider version.Provider, ssmPr
6970
func (p *DefaultProvider) List(ctx context.Context, nodeClass *v1.EC2NodeClass) (AMIs, error) {
7071
p.Lock()
7172
defer p.Unlock()
72-
queries, err := p.DescribeImageQueries(ctx, nodeClass)
73-
if err != nil {
74-
return nil, fmt.Errorf("getting AMI queries, %w", err)
75-
}
76-
amis, err := p.amis(ctx, queries)
73+
amis, err := p.amis(ctx, nodeClass)
7774
if err != nil {
7875
return nil, err
7976
}
@@ -156,12 +153,13 @@ func (p *DefaultProvider) DescribeImageQueries(ctx context.Context, nodeClass *v
156153
}
157154

158155
//nolint:gocyclo
159-
func (p *DefaultProvider) amis(ctx context.Context, queries []DescribeImageQuery) (AMIs, error) {
160-
hash, err := hashstructure.Hash(queries, hashstructure.FormatV2, &hashstructure.HashOptions{SlicesAsSets: true})
156+
func (p *DefaultProvider) amis(ctx context.Context, nodeClass *v1.EC2NodeClass) (AMIs, error) {
157+
queries, err := p.DescribeImageQueries(ctx, nodeClass)
161158
if err != nil {
162-
return nil, err
159+
return nil, fmt.Errorf("getting AMI queries, %w", err)
163160
}
164-
if images, ok := p.cache.Get(fmt.Sprintf("%d", hash)); ok {
161+
hash := utils.GetNodeClassHash(nodeClass)
162+
if images, ok := p.cache.Get(hash); ok {
165163
// Ensure what's returned from this function is a deep-copy of AMIs so alterations
166164
// to the data don't affect the original
167165
return append(AMIs{}, images.(AMIs)...), nil
@@ -205,7 +203,7 @@ func (p *DefaultProvider) amis(ctx context.Context, queries []DescribeImageQuery
205203
}
206204
}
207205
}
208-
p.cache.SetDefault(fmt.Sprintf("%d", hash), AMIs(lo.Values(images)))
206+
p.cache.SetDefault(hash, AMIs(lo.Values(images)))
209207
return lo.Values(images), nil
210208
}
211209

‎pkg/providers/amifamily/suite_test.go‎

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030

3131
. "github.com/onsi/ginkgo/v2"
3232
. "github.com/onsi/gomega"
33+
"github.com/onsi/gomega/gstruct"
3334
. "sigs.k8s.io/karpenter/pkg/utils/testing"
3435

3536
"github.com/samber/lo"
@@ -45,6 +46,8 @@ import (
4546
"github.com/aws/karpenter-provider-aws/pkg/operator/options"
4647
"github.com/aws/karpenter-provider-aws/pkg/providers/amifamily"
4748
"github.com/aws/karpenter-provider-aws/pkg/test"
49+
50+
. "sigs.k8s.io/karpenter/pkg/test/expectations"
4851
)
4952

5053
var ctx context.Context
@@ -533,6 +536,219 @@ var _ = Describe("AMIProvider", func() {
533536
}))
534537
})
535538
})
539+
Context("Provider Cache", func() {
540+
It("should resolve AMIs from cache that are filtered by id", func() {
541+
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
542+
{
543+
Name: aws.String(coretest.RandomName()),
544+
ImageId: aws.String("ami-123"),
545+
Architecture: "x86_64",
546+
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
547+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
548+
State: ec2types.ImageStateAvailable,
549+
},
550+
{
551+
Name: aws.String(coretest.RandomName()),
552+
ImageId: aws.String("ami-456"),
553+
Architecture: "arm64",
554+
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
555+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
556+
State: ec2types.ImageStateAvailable,
557+
},
558+
}})
559+
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
560+
{
561+
ID: "ami-123",
562+
},
563+
{
564+
ID: "ami-456",
565+
},
566+
}
567+
_, err := awsEnv.AMIProvider.List(ctx, nodeClass)
568+
Expect(err).To(BeNil())
569+
570+
Expect(awsEnv.AMICache.Items()).To(HaveLen(1))
571+
cachedImages := lo.Values(awsEnv.AMICache.Items())[0].Object.(amifamily.AMIs)
572+
Expect(cachedImages).To(ContainElements(
573+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
574+
"AmiID": Equal("ami-123"),
575+
}),
576+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
577+
"AmiID": Equal("ami-456"),
578+
}),
579+
))
580+
})
581+
It("should resolve AMIs from cache that are filtered by name", func() {
582+
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
583+
{
584+
Name: aws.String("ami-name-1"),
585+
ImageId: aws.String("ami-123"),
586+
Architecture: "x86_64",
587+
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
588+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
589+
State: ec2types.ImageStateAvailable,
590+
},
591+
{
592+
Name: aws.String("ami-name-2"),
593+
ImageId: aws.String("ami-456"),
594+
Architecture: "arm64",
595+
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
596+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
597+
State: ec2types.ImageStateAvailable,
598+
},
599+
}})
600+
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
601+
{
602+
Name: "ami-name-1",
603+
},
604+
{
605+
Name: "ami-name-2",
606+
},
607+
}
608+
_, err := awsEnv.AMIProvider.List(ctx, nodeClass)
609+
Expect(err).To(BeNil())
610+
611+
Expect(awsEnv.AMICache.Items()).To(HaveLen(1))
612+
cachedImages := lo.Values(awsEnv.AMICache.Items())[0].Object.(amifamily.AMIs)
613+
Expect(cachedImages).To(ContainElements(
614+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
615+
"Name": Equal("ami-name-1"),
616+
}),
617+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
618+
"Name": Equal("ami-name-2"),
619+
}),
620+
))
621+
})
622+
It("should resolve AMIs from cache that are filtered by tags", func() {
623+
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
624+
{
625+
Name: aws.String("ami-name-1"),
626+
ImageId: aws.String("ami-123"),
627+
Architecture: "x86_64",
628+
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
629+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
630+
State: ec2types.ImageStateAvailable,
631+
},
632+
{
633+
Name: aws.String("ami-name-2"),
634+
ImageId: aws.String("ami-456"),
635+
Architecture: "arm64",
636+
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
637+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
638+
State: ec2types.ImageStateAvailable,
639+
},
640+
}})
641+
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
642+
{
643+
Tags: map[string]string{"test": "test"},
644+
},
645+
}
646+
_, err := awsEnv.AMIProvider.List(ctx, nodeClass)
647+
Expect(err).To(BeNil())
648+
649+
Expect(awsEnv.AMICache.Items()).To(HaveLen(1))
650+
cachedImages := lo.Values(awsEnv.AMICache.Items())[0].Object.(amifamily.AMIs)
651+
Expect(cachedImages).To(ContainElements(
652+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
653+
"Name": Equal("ami-name-1"),
654+
}),
655+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
656+
"Name": Equal("ami-name-2"),
657+
}),
658+
))
659+
})
660+
It("should correctly disambiguate AND vs OR semantics for tags", func() {
661+
// AND semantics
662+
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
663+
{
664+
Name: aws.String("ami-name-3"),
665+
ImageId: aws.String("ami-789"),
666+
Architecture: "x86_64",
667+
Tags: []ec2types.Tag{{Key: aws.String("tag-key-1"), Value: aws.String("tag-value-1")}, {Key: aws.String("tag-key-2"), Value: aws.String("tag-value-2")}},
668+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
669+
State: ec2types.ImageStateAvailable,
670+
},
671+
}})
672+
nodeClass.Spec.AMIFamily = &v1.AMIFamilyAL2
673+
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
674+
{
675+
Tags: map[string]string{"tag-key-1": "tag-value-1", "tag-key-2": "tag-value-2"},
676+
},
677+
}
678+
ExpectApplied(ctx, env.Client, nodeClass)
679+
amis, err := awsEnv.AMIProvider.List(ctx, nodeClass)
680+
Expect(err).To(BeNil())
681+
682+
Expect(amis).To(ContainElements(
683+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
684+
"Name": Equal("ami-name-3"),
685+
}),
686+
))
687+
688+
// OR semantics
689+
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
690+
{
691+
Name: aws.String("ami-name-1"),
692+
ImageId: aws.String("ami-123"),
693+
Architecture: "x86_64",
694+
Tags: []ec2types.Tag{{Key: aws.String("tag-key-1"), Value: aws.String("tag-value-1")}},
695+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
696+
State: ec2types.ImageStateAvailable,
697+
},
698+
{
699+
Name: aws.String("ami-name-2"),
700+
ImageId: aws.String("ami-456"),
701+
Architecture: "arm64",
702+
Tags: []ec2types.Tag{{Key: aws.String("tag-key-2"), Value: aws.String("tag-value-2")}},
703+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
704+
State: ec2types.ImageStateAvailable,
705+
},
706+
}})
707+
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
708+
{
709+
Tags: map[string]string{"tag-key-1": "tag-value-1"},
710+
},
711+
{
712+
Tags: map[string]string{"tag-key-2": "tag-value-2"},
713+
},
714+
}
715+
ExpectApplied(ctx, env.Client, nodeClass)
716+
amis, err = awsEnv.AMIProvider.List(ctx, nodeClass)
717+
Expect(err).To(BeNil())
718+
719+
Expect(amis).To(ContainElements(
720+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
721+
"Name": Equal("ami-name-1"),
722+
}),
723+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
724+
"Name": Equal("ami-name-2"),
725+
}),
726+
))
727+
728+
cacheItems := awsEnv.AMICache.Items()
729+
Expect(cacheItems).To(HaveLen(2))
730+
cachedImages := make([]amifamily.AMIs, 0, len(cacheItems))
731+
for _, item := range cacheItems {
732+
cachedImages = append(cachedImages, item.Object.(amifamily.AMIs))
733+
}
734+
735+
Expect(cachedImages).To(ConsistOf(
736+
ConsistOf(
737+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
738+
"Name": Equal("ami-name-3"),
739+
}),
740+
),
741+
ConsistOf(
742+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
743+
"Name": Equal("ami-name-1"),
744+
}),
745+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
746+
"Name": Equal("ami-name-2"),
747+
}),
748+
),
749+
))
750+
})
751+
})
536752
Context("AMI Selectors", func() {
537753
// When you tag public or shared resources, the tags you assign are available only to your AWS account; no other AWS account will have access to those tags
538754
// https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html#tag-restrictions

0 commit comments

Comments
 (0)