Skip to content

Commit 9e59a13

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

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
@@ -30,6 +30,7 @@ import (
3030
"k8s.io/utils/clock"
3131

3232
"github.com/aws/karpenter-provider-aws/pkg/errors"
33+
"github.com/aws/karpenter-provider-aws/pkg/utils"
3334

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

167164
//nolint:gocyclo
168-
func (p *DefaultProvider) amis(ctx context.Context, queries []DescribeImageQuery) (AMIs, error) {
169-
hash, err := hashstructure.Hash(queries, hashstructure.FormatV2, &hashstructure.HashOptions{SlicesAsSets: true})
165+
func (p *DefaultProvider) amis(ctx context.Context, nodeClass *v1.EC2NodeClass) (AMIs, error) {
166+
queries, err := p.DescribeImageQueries(ctx, nodeClass)
170167
if err != nil {
171-
return nil, err
168+
return nil, fmt.Errorf("getting AMI queries, %w", err)
172169
}
173-
if images, ok := p.cache.Get(fmt.Sprintf("%d", hash)); ok {
170+
hash := utils.GetNodeClassHash(nodeClass)
171+
if images, ok := p.cache.Get(hash); ok {
174172
// Ensure what's returned from this function is a deep-copy of AMIs so alterations
175173
// to the data don't affect the original
176174
return append(AMIs{}, images.(AMIs)...), nil
@@ -214,7 +212,7 @@ func (p *DefaultProvider) amis(ctx context.Context, queries []DescribeImageQuery
214212
}
215213
}
216214
}
217-
p.cache.SetDefault(fmt.Sprintf("%d", hash), AMIs(lo.Values(images)))
215+
p.cache.SetDefault(hash, AMIs(lo.Values(images)))
218216
return lo.Values(images), nil
219217
}
220218

‎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
@@ -564,6 +567,219 @@ var _ = Describe("AMIProvider", func() {
564567
}))
565568
})
566569
})
570+
Context("Provider Cache", func() {
571+
It("should resolve AMIs from cache that are filtered by id", func() {
572+
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
573+
{
574+
Name: aws.String(coretest.RandomName()),
575+
ImageId: aws.String("ami-123"),
576+
Architecture: "x86_64",
577+
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
578+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
579+
State: ec2types.ImageStateAvailable,
580+
},
581+
{
582+
Name: aws.String(coretest.RandomName()),
583+
ImageId: aws.String("ami-456"),
584+
Architecture: "arm64",
585+
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
586+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
587+
State: ec2types.ImageStateAvailable,
588+
},
589+
}})
590+
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
591+
{
592+
ID: "ami-123",
593+
},
594+
{
595+
ID: "ami-456",
596+
},
597+
}
598+
_, err := awsEnv.AMIProvider.List(ctx, nodeClass)
599+
Expect(err).To(BeNil())
600+
601+
Expect(awsEnv.AMICache.Items()).To(HaveLen(1))
602+
cachedImages := lo.Values(awsEnv.AMICache.Items())[0].Object.(amifamily.AMIs)
603+
Expect(cachedImages).To(ContainElements(
604+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
605+
"AmiID": Equal("ami-123"),
606+
}),
607+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
608+
"AmiID": Equal("ami-456"),
609+
}),
610+
))
611+
})
612+
It("should resolve AMIs from cache that are filtered by name", func() {
613+
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
614+
{
615+
Name: aws.String("ami-name-1"),
616+
ImageId: aws.String("ami-123"),
617+
Architecture: "x86_64",
618+
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
619+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
620+
State: ec2types.ImageStateAvailable,
621+
},
622+
{
623+
Name: aws.String("ami-name-2"),
624+
ImageId: aws.String("ami-456"),
625+
Architecture: "arm64",
626+
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
627+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
628+
State: ec2types.ImageStateAvailable,
629+
},
630+
}})
631+
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
632+
{
633+
Name: "ami-name-1",
634+
},
635+
{
636+
Name: "ami-name-2",
637+
},
638+
}
639+
_, err := awsEnv.AMIProvider.List(ctx, nodeClass)
640+
Expect(err).To(BeNil())
641+
642+
Expect(awsEnv.AMICache.Items()).To(HaveLen(1))
643+
cachedImages := lo.Values(awsEnv.AMICache.Items())[0].Object.(amifamily.AMIs)
644+
Expect(cachedImages).To(ContainElements(
645+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
646+
"Name": Equal("ami-name-1"),
647+
}),
648+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
649+
"Name": Equal("ami-name-2"),
650+
}),
651+
))
652+
})
653+
It("should resolve AMIs from cache that are filtered by tags", func() {
654+
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
655+
{
656+
Name: aws.String("ami-name-1"),
657+
ImageId: aws.String("ami-123"),
658+
Architecture: "x86_64",
659+
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
660+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
661+
State: ec2types.ImageStateAvailable,
662+
},
663+
{
664+
Name: aws.String("ami-name-2"),
665+
ImageId: aws.String("ami-456"),
666+
Architecture: "arm64",
667+
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
668+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
669+
State: ec2types.ImageStateAvailable,
670+
},
671+
}})
672+
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
673+
{
674+
Tags: map[string]string{"test": "test"},
675+
},
676+
}
677+
_, err := awsEnv.AMIProvider.List(ctx, nodeClass)
678+
Expect(err).To(BeNil())
679+
680+
Expect(awsEnv.AMICache.Items()).To(HaveLen(1))
681+
cachedImages := lo.Values(awsEnv.AMICache.Items())[0].Object.(amifamily.AMIs)
682+
Expect(cachedImages).To(ContainElements(
683+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
684+
"Name": Equal("ami-name-1"),
685+
}),
686+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
687+
"Name": Equal("ami-name-2"),
688+
}),
689+
))
690+
})
691+
It("should correctly disambiguate AND vs OR semantics for tags", func() {
692+
// AND semantics
693+
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
694+
{
695+
Name: aws.String("ami-name-3"),
696+
ImageId: aws.String("ami-789"),
697+
Architecture: "x86_64",
698+
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")}},
699+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
700+
State: ec2types.ImageStateAvailable,
701+
},
702+
}})
703+
nodeClass.Spec.AMIFamily = &v1.AMIFamilyAL2
704+
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
705+
{
706+
Tags: map[string]string{"tag-key-1": "tag-value-1", "tag-key-2": "tag-value-2"},
707+
},
708+
}
709+
ExpectApplied(ctx, env.Client, nodeClass)
710+
amis, err := awsEnv.AMIProvider.List(ctx, nodeClass)
711+
Expect(err).To(BeNil())
712+
713+
Expect(amis).To(ContainElements(
714+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
715+
"Name": Equal("ami-name-3"),
716+
}),
717+
))
718+
719+
// OR semantics
720+
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
721+
{
722+
Name: aws.String("ami-name-1"),
723+
ImageId: aws.String("ami-123"),
724+
Architecture: "x86_64",
725+
Tags: []ec2types.Tag{{Key: aws.String("tag-key-1"), Value: aws.String("tag-value-1")}},
726+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
727+
State: ec2types.ImageStateAvailable,
728+
},
729+
{
730+
Name: aws.String("ami-name-2"),
731+
ImageId: aws.String("ami-456"),
732+
Architecture: "arm64",
733+
Tags: []ec2types.Tag{{Key: aws.String("tag-key-2"), Value: aws.String("tag-value-2")}},
734+
CreationDate: aws.String("2022-08-15T12:00:00Z"),
735+
State: ec2types.ImageStateAvailable,
736+
},
737+
}})
738+
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
739+
{
740+
Tags: map[string]string{"tag-key-1": "tag-value-1"},
741+
},
742+
{
743+
Tags: map[string]string{"tag-key-2": "tag-value-2"},
744+
},
745+
}
746+
ExpectApplied(ctx, env.Client, nodeClass)
747+
amis, err = awsEnv.AMIProvider.List(ctx, nodeClass)
748+
Expect(err).To(BeNil())
749+
750+
Expect(amis).To(ContainElements(
751+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
752+
"Name": Equal("ami-name-1"),
753+
}),
754+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
755+
"Name": Equal("ami-name-2"),
756+
}),
757+
))
758+
759+
cacheItems := awsEnv.AMICache.Items()
760+
Expect(cacheItems).To(HaveLen(2))
761+
cachedImages := make([]amifamily.AMIs, 0, len(cacheItems))
762+
for _, item := range cacheItems {
763+
cachedImages = append(cachedImages, item.Object.(amifamily.AMIs))
764+
}
765+
766+
Expect(cachedImages).To(ConsistOf(
767+
ConsistOf(
768+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
769+
"Name": Equal("ami-name-3"),
770+
}),
771+
),
772+
ConsistOf(
773+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
774+
"Name": Equal("ami-name-1"),
775+
}),
776+
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
777+
"Name": Equal("ami-name-2"),
778+
}),
779+
),
780+
))
781+
})
782+
})
567783
Context("AMI Selectors", func() {
568784
// 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
569785
// https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html#tag-restrictions

0 commit comments

Comments
 (0)