Skip to content

Commit 07ed933

Browse files
saurav-agarwallaengedaam
saurav-agarwalla
authored and
engedaam
committed
fix: prevent hash collisions while resolving subnets, security groups and AMIs from nodeclass selectors
1 parent afe3eba commit 07ed933

File tree

13 files changed

+512
-38
lines changed

13 files changed

+512
-38
lines changed

‎hack/toolchain.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ tools() {
1919
go install github.com/google/ko@latest
2020
go install github.com/mikefarah/yq/v4@latest
2121
go install github.com/norwoodj/helm-docs/cmd/helm-docs@latest
22-
go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
22+
go install sigs.k8s.io/controller-runtime/tools/setup-envtest@b9bccfd419149d26d14130887a5e5819e4a3b2be
2323
go install sigs.k8s.io/controller-tools/cmd/controller-gen@latest
2424
go install github.com/sigstore/cosign/v2/cmd/cosign@latest
2525
go install -tags extended github.com/gohugoio/[email protected]

‎pkg/controllers/nodeclass/ami_test.go‎

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

653653
// Flush Cache
654-
awsEnv.EC2Cache.Flush()
654+
awsEnv.AMICache.Flush()
655655

656656
ExpectObjectReconciled(ctx, env.Client, controller, nodeClass)
657657
nodeClass = ExpectExists(ctx, env.Client, nodeClass)
@@ -750,7 +750,7 @@ var _ = Describe("NodeClass AMI Status Controller", func() {
750750
},
751751
})
752752

753-
awsEnv.EC2Cache.Flush()
753+
awsEnv.AMICache.Flush()
754754

755755
ExpectApplied(ctx, env.Client, nodeClass)
756756
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) (reconciler.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
@@ -31,6 +31,7 @@ import (
3131

3232
. "github.com/onsi/ginkgo/v2"
3333
. "github.com/onsi/gomega"
34+
"github.com/onsi/gomega/gstruct"
3435
. "sigs.k8s.io/karpenter/pkg/utils/testing"
3536

3637
"github.com/samber/lo"
@@ -51,6 +52,8 @@ import (
5152
"github.com/aws/karpenter-provider-aws/pkg/operator/options"
5253
"github.com/aws/karpenter-provider-aws/pkg/providers/amifamily"
5354
"github.com/aws/karpenter-provider-aws/pkg/test"
55+
56+
. "sigs.k8s.io/karpenter/pkg/test/expectations"
5457
)
5558

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

0 commit comments

Comments
 (0)