From 13cd91387dd3d9da08d789b2b4b962d79506d1f0 Mon Sep 17 00:00:00 2001 From: Jonathan Innis Date: Fri, 12 Apr 2024 15:19:00 -0700 Subject: [PATCH] chore: Add an instance profile interface to instance profile provider (#6034) --- pkg/apis/v1beta1/ec2nodeclass.go | 17 ++++++ pkg/controllers/nodeclass/suite_test.go | 4 +- pkg/operator/operator.go | 2 +- .../instanceprofile/instanceprofile.go | 53 ++++++++----------- pkg/test/environment.go | 2 +- 5 files changed, 44 insertions(+), 34 deletions(-) diff --git a/pkg/apis/v1beta1/ec2nodeclass.go b/pkg/apis/v1beta1/ec2nodeclass.go index b5e96ac1dc6b..f9515b5e4d2f 100644 --- a/pkg/apis/v1beta1/ec2nodeclass.go +++ b/pkg/apis/v1beta1/ec2nodeclass.go @@ -21,6 +21,7 @@ import ( "github.com/samber/lo" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + corev1beta1 "sigs.k8s.io/karpenter/pkg/apis/v1beta1" ) // EC2NodeClassSpec is the top level specification for the AWS Karpenter Provider. @@ -343,6 +344,22 @@ func (in *EC2NodeClass) Hash() string { }))) } +func (in *EC2NodeClass) InstanceProfileName(clusterName, region string) string { + return fmt.Sprintf("%s_%d", clusterName, lo.Must(hashstructure.Hash(fmt.Sprintf("%s%s", region, in.Name), hashstructure.FormatV2, nil))) +} + +func (in *EC2NodeClass) InstanceProfileRole() string { + return in.Spec.Role +} + +func (in *EC2NodeClass) InstanceProfileTags(clusterName string) map[string]string { + return lo.Assign(in.Spec.Tags, map[string]string{ + fmt.Sprintf("kubernetes.io/cluster/%s", clusterName): "owned", + corev1beta1.ManagedByAnnotationKey: clusterName, + LabelNodeClass: in.Name, + }) +} + // EC2NodeClassList contains a list of EC2NodeClass // +kubebuilder:object:root=true type EC2NodeClassList struct { diff --git a/pkg/controllers/nodeclass/suite_test.go b/pkg/controllers/nodeclass/suite_test.go index 222fa5b07092..3549130f25f3 100644 --- a/pkg/controllers/nodeclass/suite_test.go +++ b/pkg/controllers/nodeclass/suite_test.go @@ -1015,7 +1015,7 @@ var _ = Describe("NodeClassController", func() { Context("NodeClass Termination", func() { var profileName string BeforeEach(func() { - profileName = awsEnv.InstanceProfileProvider.GetProfileName(ctx, fake.DefaultRegion, nodeClass.Name) + profileName = nodeClass.InstanceProfileName(options.FromContext(ctx).ClusterName, fake.DefaultRegion) }) It("should not delete the NodeClass if launch template deletion fails", func() { launchTemplateName := aws.String(fake.LaunchTemplateName()) @@ -1188,7 +1188,7 @@ var _ = Describe("NodeClassController", func() { Context("Instance Profile Status", func() { var profileName string BeforeEach(func() { - profileName = awsEnv.InstanceProfileProvider.GetProfileName(ctx, fake.DefaultRegion, nodeClass.Name) + profileName = nodeClass.InstanceProfileName(options.FromContext(ctx).ClusterName, fake.DefaultRegion) }) It("should create the instance profile when it doesn't exist", func() { nodeClass.Spec.Role = "test-role" diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index 207de5a8e303..39be55788322 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -134,7 +134,7 @@ func NewOperator(ctx context.Context, operator *operator.Operator) (context.Cont unavailableOfferingsCache := awscache.NewUnavailableOfferings() subnetProvider := subnet.NewDefaultProvider(ec2api, cache.New(awscache.DefaultTTL, awscache.DefaultCleanupInterval)) securityGroupProvider := securitygroup.NewDefaultProvider(ec2api, cache.New(awscache.DefaultTTL, awscache.DefaultCleanupInterval)) - instanceProfileProvider := instanceprofile.NewProvider(*sess.Config.Region, iam.New(sess), cache.New(awscache.InstanceProfileTTL, awscache.DefaultCleanupInterval)) + instanceProfileProvider := instanceprofile.NewDefaultProvider(*sess.Config.Region, iam.New(sess), cache.New(awscache.InstanceProfileTTL, awscache.DefaultCleanupInterval)) pricingProvider := pricing.NewDefaultProvider( ctx, pricing.NewAPI(sess, *sess.Config.Region), diff --git a/pkg/providers/instanceprofile/instanceprofile.go b/pkg/providers/instanceprofile/instanceprofile.go index d9066dc31a46..d8b361a8fd23 100644 --- a/pkg/providers/instanceprofile/instanceprofile.go +++ b/pkg/providers/instanceprofile/instanceprofile.go @@ -21,22 +21,26 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/iam" "github.com/aws/aws-sdk-go/service/iam/iamiface" - "github.com/mitchellh/hashstructure/v2" "github.com/patrickmn/go-cache" "github.com/samber/lo" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" - corev1beta1 "sigs.k8s.io/karpenter/pkg/apis/v1beta1" - - "github.com/aws/karpenter-provider-aws/pkg/apis/v1beta1" awserrors "github.com/aws/karpenter-provider-aws/pkg/errors" "github.com/aws/karpenter-provider-aws/pkg/operator/options" ) +// ResourceOwner is an object that manages an instance profile +type ResourceOwner interface { + GetUID() types.UID + InstanceProfileName(string, string) string + InstanceProfileRole() string + InstanceProfileTags(string) map[string]string +} + type Provider interface { - Create(context.Context, *v1beta1.EC2NodeClass) (string, error) - Delete(context.Context, *v1beta1.EC2NodeClass) error - GetProfileName(ctx context.Context, region, nodeClassName string) string + Create(context.Context, ResourceOwner) (string, error) + Delete(context.Context, ResourceOwner) error } type DefaultProvider struct { @@ -45,7 +49,7 @@ type DefaultProvider struct { cache *cache.Cache } -func NewProvider(region string, iamapi iamiface.IAMAPI, cache *cache.Cache) *DefaultProvider { +func NewDefaultProvider(region string, iamapi iamiface.IAMAPI, cache *cache.Cache) *DefaultProvider { return &DefaultProvider{ region: region, iamapi: iamapi, @@ -53,17 +57,12 @@ func NewProvider(region string, iamapi iamiface.IAMAPI, cache *cache.Cache) *Def } } -func (p *DefaultProvider) Create(ctx context.Context, nodeClass *v1beta1.EC2NodeClass) (string, error) { - tags := lo.Assign(nodeClass.Spec.Tags, map[string]string{ - fmt.Sprintf("kubernetes.io/cluster/%s", options.FromContext(ctx).ClusterName): "owned", - corev1beta1.ManagedByAnnotationKey: options.FromContext(ctx).ClusterName, - v1beta1.LabelNodeClass: nodeClass.Name, - v1.LabelTopologyRegion: p.region, - }) - profileName := p.GetProfileName(ctx, p.region, nodeClass.Name) +func (p *DefaultProvider) Create(ctx context.Context, m ResourceOwner) (string, error) { + profileName := m.InstanceProfileName(options.FromContext(ctx).ClusterName, p.region) + tags := lo.Assign(m.InstanceProfileTags(options.FromContext(ctx).ClusterName), map[string]string{v1.LabelTopologyRegion: p.region}) // An instance profile exists for this NodeClass - if _, ok := p.cache.Get(string(nodeClass.UID)); ok { + if _, ok := p.cache.Get(string(m.GetUID())); ok { return profileName, nil } // Validate if the instance profile exists and has the correct role assigned to it @@ -87,7 +86,7 @@ func (p *DefaultProvider) Create(ctx context.Context, nodeClass *v1beta1.EC2Node // Instance profiles can only have a single role assigned to them so this profile either has 1 or 0 roles // https://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_use_switch-role-ec2_instance-profiles.html if len(instanceProfile.Roles) == 1 { - if aws.StringValue(instanceProfile.Roles[0].RoleName) == nodeClass.Spec.Role { + if aws.StringValue(instanceProfile.Roles[0].RoleName) == m.InstanceProfileRole() { return profileName, nil } if _, err = p.iamapi.RemoveRoleFromInstanceProfileWithContext(ctx, &iam.RemoveRoleFromInstanceProfileInput{ @@ -99,16 +98,16 @@ func (p *DefaultProvider) Create(ctx context.Context, nodeClass *v1beta1.EC2Node } if _, err = p.iamapi.AddRoleToInstanceProfileWithContext(ctx, &iam.AddRoleToInstanceProfileInput{ InstanceProfileName: aws.String(profileName), - RoleName: aws.String(nodeClass.Spec.Role), + RoleName: aws.String(m.InstanceProfileRole()), }); err != nil { - return "", fmt.Errorf("adding role %q to instance profile %q, %w", nodeClass.Spec.Role, profileName, err) + return "", fmt.Errorf("adding role %q to instance profile %q, %w", m.InstanceProfileRole(), profileName, err) } - p.cache.SetDefault(string(nodeClass.UID), nil) - return profileName, nil + p.cache.SetDefault(string(m.GetUID()), nil) + return aws.StringValue(instanceProfile.InstanceProfileName), nil } -func (p *DefaultProvider) Delete(ctx context.Context, nodeClass *v1beta1.EC2NodeClass) error { - profileName := p.GetProfileName(ctx, p.region, nodeClass.Name) +func (p *DefaultProvider) Delete(ctx context.Context, m ResourceOwner) error { + profileName := m.InstanceProfileName(options.FromContext(ctx).ClusterName, p.region) out, err := p.iamapi.GetInstanceProfileWithContext(ctx, &iam.GetInstanceProfileInput{ InstanceProfileName: aws.String(profileName), }) @@ -132,9 +131,3 @@ func (p *DefaultProvider) Delete(ctx context.Context, nodeClass *v1beta1.EC2Node } return nil } - -// GetProfileName gets the string for the profile name based on the cluster name and the NodeClass UUID. -// The length of this string can never exceed the maximum instance profile name limit of 128 characters. -func (p *DefaultProvider) GetProfileName(ctx context.Context, region, nodeClassName string) string { - return fmt.Sprintf("%s_%d", options.FromContext(ctx).ClusterName, lo.Must(hashstructure.Hash(fmt.Sprintf("%s%s", region, nodeClassName), hashstructure.FormatV2, nil))) -} diff --git a/pkg/test/environment.go b/pkg/test/environment.go index cf7ecf4aa06b..0a0b1ca56e51 100644 --- a/pkg/test/environment.go +++ b/pkg/test/environment.go @@ -103,7 +103,7 @@ func NewEnvironment(ctx context.Context, env *coretest.Environment) *Environment subnetProvider := subnet.NewDefaultProvider(ec2api, subnetCache) securityGroupProvider := securitygroup.NewDefaultProvider(ec2api, securityGroupCache) versionProvider := version.NewDefaultProvider(env.KubernetesInterface, kubernetesVersionCache) - instanceProfileProvider := instanceprofile.NewProvider(fake.DefaultRegion, iamapi, instanceProfileCache) + instanceProfileProvider := instanceprofile.NewDefaultProvider(fake.DefaultRegion, iamapi, instanceProfileCache) amiProvider := amifamily.NewDefaultProvider(versionProvider, ssmapi, ec2api, ec2Cache) amiResolver := amifamily.NewResolver(amiProvider) instanceTypesProvider := instancetype.NewDefaultProvider(fake.DefaultRegion, instanceTypeCache, ec2api, subnetProvider, unavailableOfferingsCache, pricingProvider)