Skip to content

fix: prevent hash collisions while resolving subnets, security groups and AMIs from nodeclass selectors #8632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 18, 2025
Merged

fix: prevent hash collisions while resolving subnets, security groups and AMIs from nodeclass selectors #8632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19e7e9a
ci: Fix failing AMI tests after 1.34 upgrade
saurav-agarwalla Oct 16, 2025
53d5d12
fix: prevent hash collisions while resolving subnets, security groups…
saurav-agarwalla Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/controllers/nodeclass/ami_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ var _ = Describe("NodeClass AMI Status Controller", func() {
awsEnv.Clock.Step(40 * time.Minute)

// Flush Cache
awsEnv.EC2Cache.Flush()
awsEnv.AMICache.Flush()

ExpectObjectReconciled(ctx, env.Client, controller, nodeClass)
nodeClass = ExpectExists(ctx, env.Client, nodeClass)
Expand Down Expand Up @@ -750,7 +750,7 @@ var _ = Describe("NodeClass AMI Status Controller", func() {
},
})

awsEnv.EC2Cache.Flush()
awsEnv.AMICache.Flush()

ExpectApplied(ctx, env.Client, nodeClass)
ExpectObjectReconciled(ctx, env.Client, controller, nodeClass)
Expand Down
6 changes: 6 additions & 0 deletions pkg/controllers/providers/ssm/invalidation/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ import (
v1 "github.com/aws/karpenter-provider-aws/pkg/apis/v1"
"github.com/aws/karpenter-provider-aws/pkg/providers/amifamily"
"github.com/aws/karpenter-provider-aws/pkg/providers/ssm"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/uuid"
)

// The SSM Invalidation controller is responsible for invalidating "latest" SSM parameters when they point to deprecated
Expand Down Expand Up @@ -66,6 +69,9 @@ func (c *Controller) Reconcile(ctx context.Context) (reconciler.Result, error) {
amis := []amifamily.AMI{}
for _, nodeClass := range lo.Map(lo.Keys(amiIDsToParameters), func(amiID string, _ int) *v1.EC2NodeClass {
return &v1.EC2NodeClass{
ObjectMeta: metav1.ObjectMeta{
UID: uuid.NewUUID(), // ensures that this doesn't hit the AMI cache.
},
Spec: v1.EC2NodeClassSpec{
AMISelectorTerms: []v1.AMISelectorTerm{{ID: amiID}},
},
Expand Down
7 changes: 7 additions & 0 deletions pkg/fake/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (

type MockedFunction[I any, O any] struct {
Output AtomicPtr[O] // Output to return on call to this function
MultiOut AtomicPtrSlice[O]
OutputPages AtomicPtrSlice[O]
CalledWithInput AtomicPtrSlice[I] // Slice used to keep track of passed input to this function
Error AtomicError // Error to return a certain number of times defined by custom error options
Expand All @@ -38,6 +39,7 @@ type MockedFunction[I any, O any] struct {
// each other.
func (m *MockedFunction[I, O]) Reset() {
m.Output.Reset()
m.MultiOut.Reset()
m.OutputPages.Reset()
m.CalledWithInput.Reset()
m.Error.Reset()
Expand All @@ -60,6 +62,11 @@ func (m *MockedFunction[I, O]) Invoke(input *I, defaultTransformer func(*I) (*O,
m.successfulCalls.Add(1)
return m.Output.Clone(), nil
}

if m.MultiOut.Len() > 0 {
m.successfulCalls.Add(1)
return m.MultiOut.Pop(), nil
}
// This output pages multi-threaded handling isn't perfect
// It will fail if pages are asynchronously requested from the same NextToken
if m.OutputPages.Len() > 0 {
Expand Down
18 changes: 8 additions & 10 deletions pkg/providers/amifamily/ami.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"k8s.io/utils/clock"

"github.com/aws/karpenter-provider-aws/pkg/errors"
"github.com/aws/karpenter-provider-aws/pkg/utils"

v1 "github.com/aws/karpenter-provider-aws/pkg/apis/v1"
sdk "github.com/aws/karpenter-provider-aws/pkg/aws"
Expand Down Expand Up @@ -70,11 +71,7 @@ func NewDefaultProvider(clk clock.Clock, versionProvider version.Provider, ssmPr
func (p *DefaultProvider) List(ctx context.Context, nodeClass *v1.EC2NodeClass) (AMIs, error) {
p.Lock()
defer p.Unlock()
queries, err := p.DescribeImageQueries(ctx, nodeClass)
if err != nil {
return nil, fmt.Errorf("getting AMI queries, %w", err)
}
amis, err := p.amis(ctx, queries)
amis, err := p.amis(ctx, nodeClass)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -165,12 +162,13 @@ func (p *DefaultProvider) DescribeImageQueries(ctx context.Context, nodeClass *v
}

//nolint:gocyclo
func (p *DefaultProvider) amis(ctx context.Context, queries []DescribeImageQuery) (AMIs, error) {
hash, err := hashstructure.Hash(queries, hashstructure.FormatV2, &hashstructure.HashOptions{SlicesAsSets: true})
func (p *DefaultProvider) amis(ctx context.Context, nodeClass *v1.EC2NodeClass) (AMIs, error) {
queries, err := p.DescribeImageQueries(ctx, nodeClass)
if err != nil {
return nil, err
return nil, fmt.Errorf("getting AMI queries, %w", err)
}
if images, ok := p.cache.Get(fmt.Sprintf("%d", hash)); ok {
hash := utils.GetNodeClassHash(nodeClass)
if images, ok := p.cache.Get(hash); ok {
// Ensure what's returned from this function is a deep-copy of AMIs so alterations
// to the data don't affect the original
return append(AMIs{}, images.(AMIs)...), nil
Expand Down Expand Up @@ -214,7 +212,7 @@ func (p *DefaultProvider) amis(ctx context.Context, queries []DescribeImageQuery
}
}
}
p.cache.SetDefault(fmt.Sprintf("%d", hash), AMIs(lo.Values(images)))
p.cache.SetDefault(hash, AMIs(lo.Values(images)))
return lo.Values(images), nil
}

Expand Down
216 changes: 216 additions & 0 deletions pkg/providers/amifamily/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/onsi/gomega/gstruct"
. "sigs.k8s.io/karpenter/pkg/utils/testing"

"github.com/samber/lo"
Expand All @@ -51,6 +52,8 @@ import (
"github.com/aws/karpenter-provider-aws/pkg/operator/options"
"github.com/aws/karpenter-provider-aws/pkg/providers/amifamily"
"github.com/aws/karpenter-provider-aws/pkg/test"

. "sigs.k8s.io/karpenter/pkg/test/expectations"
)

var ctx context.Context
Expand Down Expand Up @@ -579,6 +582,219 @@ var _ = Describe("AMIProvider", func() {
}))
})
})
Context("Provider Cache", func() {
It("should resolve AMIs from cache that are filtered by id", func() {
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
{
Name: aws.String(coretest.RandomName()),
ImageId: aws.String("ami-123"),
Architecture: "x86_64",
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
CreationDate: aws.String("2022-08-15T12:00:00Z"),
State: ec2types.ImageStateAvailable,
},
{
Name: aws.String(coretest.RandomName()),
ImageId: aws.String("ami-456"),
Architecture: "arm64",
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
CreationDate: aws.String("2022-08-15T12:00:00Z"),
State: ec2types.ImageStateAvailable,
},
}})
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
{
ID: "ami-123",
},
{
ID: "ami-456",
},
}
_, err := awsEnv.AMIProvider.List(ctx, nodeClass)
Expect(err).To(BeNil())

Expect(awsEnv.AMICache.Items()).To(HaveLen(1))
cachedImages := lo.Values(awsEnv.AMICache.Items())[0].Object.(amifamily.AMIs)
Expect(cachedImages).To(ContainElements(
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
"AmiID": Equal("ami-123"),
}),
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
"AmiID": Equal("ami-456"),
}),
))
})
It("should resolve AMIs from cache that are filtered by name", func() {
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
{
Name: aws.String("ami-name-1"),
ImageId: aws.String("ami-123"),
Architecture: "x86_64",
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
CreationDate: aws.String("2022-08-15T12:00:00Z"),
State: ec2types.ImageStateAvailable,
},
{
Name: aws.String("ami-name-2"),
ImageId: aws.String("ami-456"),
Architecture: "arm64",
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
CreationDate: aws.String("2022-08-15T12:00:00Z"),
State: ec2types.ImageStateAvailable,
},
}})
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
{
Name: "ami-name-1",
},
{
Name: "ami-name-2",
},
}
_, err := awsEnv.AMIProvider.List(ctx, nodeClass)
Expect(err).To(BeNil())

Expect(awsEnv.AMICache.Items()).To(HaveLen(1))
cachedImages := lo.Values(awsEnv.AMICache.Items())[0].Object.(amifamily.AMIs)
Expect(cachedImages).To(ContainElements(
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
"Name": Equal("ami-name-1"),
}),
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
"Name": Equal("ami-name-2"),
}),
))
})
It("should resolve AMIs from cache that are filtered by tags", func() {
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
{
Name: aws.String("ami-name-1"),
ImageId: aws.String("ami-123"),
Architecture: "x86_64",
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
CreationDate: aws.String("2022-08-15T12:00:00Z"),
State: ec2types.ImageStateAvailable,
},
{
Name: aws.String("ami-name-2"),
ImageId: aws.String("ami-456"),
Architecture: "arm64",
Tags: []ec2types.Tag{{Key: lo.ToPtr("test"), Value: lo.ToPtr("test")}},
CreationDate: aws.String("2022-08-15T12:00:00Z"),
State: ec2types.ImageStateAvailable,
},
}})
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
{
Tags: map[string]string{"test": "test"},
},
}
_, err := awsEnv.AMIProvider.List(ctx, nodeClass)
Expect(err).To(BeNil())

Expect(awsEnv.AMICache.Items()).To(HaveLen(1))
cachedImages := lo.Values(awsEnv.AMICache.Items())[0].Object.(amifamily.AMIs)
Expect(cachedImages).To(ContainElements(
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
"Name": Equal("ami-name-1"),
}),
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
"Name": Equal("ami-name-2"),
}),
))
})
It("should correctly disambiguate AND vs OR semantics for tags", func() {
// AND semantics
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
{
Name: aws.String("ami-name-3"),
ImageId: aws.String("ami-789"),
Architecture: "x86_64",
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")}},
CreationDate: aws.String("2022-08-15T12:00:00Z"),
State: ec2types.ImageStateAvailable,
},
}})
nodeClass.Spec.AMIFamily = &v1.AMIFamilyAL2
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
{
Tags: map[string]string{"tag-key-1": "tag-value-1", "tag-key-2": "tag-value-2"},
},
}
ExpectApplied(ctx, env.Client, nodeClass)
amis, err := awsEnv.AMIProvider.List(ctx, nodeClass)
Expect(err).To(BeNil())

Expect(amis).To(ContainElements(
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
"Name": Equal("ami-name-3"),
}),
))

// OR semantics
awsEnv.EC2API.DescribeImagesOutput.Set(&ec2.DescribeImagesOutput{Images: []ec2types.Image{
{
Name: aws.String("ami-name-1"),
ImageId: aws.String("ami-123"),
Architecture: "x86_64",
Tags: []ec2types.Tag{{Key: aws.String("tag-key-1"), Value: aws.String("tag-value-1")}},
CreationDate: aws.String("2022-08-15T12:00:00Z"),
State: ec2types.ImageStateAvailable,
},
{
Name: aws.String("ami-name-2"),
ImageId: aws.String("ami-456"),
Architecture: "arm64",
Tags: []ec2types.Tag{{Key: aws.String("tag-key-2"), Value: aws.String("tag-value-2")}},
CreationDate: aws.String("2022-08-15T12:00:00Z"),
State: ec2types.ImageStateAvailable,
},
}})
nodeClass.Spec.AMISelectorTerms = []v1.AMISelectorTerm{
{
Tags: map[string]string{"tag-key-1": "tag-value-1"},
},
{
Tags: map[string]string{"tag-key-2": "tag-value-2"},
},
}
ExpectApplied(ctx, env.Client, nodeClass)
amis, err = awsEnv.AMIProvider.List(ctx, nodeClass)
Expect(err).To(BeNil())

Expect(amis).To(ContainElements(
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
"Name": Equal("ami-name-1"),
}),
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
"Name": Equal("ami-name-2"),
}),
))

cacheItems := awsEnv.AMICache.Items()
Expect(cacheItems).To(HaveLen(2))
cachedImages := make([]amifamily.AMIs, 0, len(cacheItems))
for _, item := range cacheItems {
cachedImages = append(cachedImages, item.Object.(amifamily.AMIs))
}

Expect(cachedImages).To(ConsistOf(
ConsistOf(
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
"Name": Equal("ami-name-3"),
}),
),
ConsistOf(
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
"Name": Equal("ami-name-1"),
}),
gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{
"Name": Equal("ami-name-2"),
}),
),
))
})
})
Context("AMI Selectors", func() {
// 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
// https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html#tag-restrictions
Expand Down
Loading