diff --git a/spinifex/handlers/eks/nodegroup.go b/spinifex/handlers/eks/nodegroup.go index 74681707c..de8a58bc9 100644 --- a/spinifex/handlers/eks/nodegroup.go +++ b/spinifex/handlers/eks/nodegroup.go @@ -20,6 +20,7 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/eks" "github.com/aws/aws-sdk-go/service/iam" + "github.com/mulgadc/bluebottle/pkg/auth" "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" "github.com/mulgadc/spinifex/spinifex/instancetypes" @@ -711,9 +712,14 @@ func gatewayHostIP(gatewayURL string) string { // provider, mirroring the implicit instance profile real EKS creates for a node // role. Idempotent: concurrent worker launches converge on the same profile. func (s *EKSServiceImpl) ensureNodeInstanceProfile(accountID, nodeRoleARN string) (string, error) { - roleName := roleNameFromARN(nodeRoleARN) - if roleName == "" { - return "", fmt.Errorf("node role ARN %q has no role name", nodeRoleARN) + roleAccount, roleName, err := auth.ParseRoleARN(nodeRoleARN) + if err != nil { + return "", fmt.Errorf("node role ARN %q: %w", nodeRoleARN, err) + } + // The profile is created under the caller's account, so a role from another + // account would silently bind a name that account does not own. + if roleAccount != accountID { + return "", fmt.Errorf("node role ARN %q is not in account %s", nodeRoleARN, accountID) } profileName := roleName @@ -766,15 +772,6 @@ func (s *EKSServiceImpl) attachRoleToProfile(accountID, profileName, roleName, p return profileARN, nil } -// roleNameFromARN extracts the role name from an arn:aws:iam:::role/ -// ARN, returning "" when the ARN does not carry the :role/ segment. -func roleNameFromARN(arn string) string { - if _, after, ok := strings.Cut(arn, ":role/"); ok { - return after - } - return "" -} - func (s *EKSServiceImpl) describeNodegroup(ctx context.Context, acctKV jetstream.KeyValue, input *eks.DescribeNodegroupInput) (*eks.DescribeNodegroupOutput, error) { cluster := aws.StringValue(input.ClusterName) ng := aws.StringValue(input.NodegroupName) diff --git a/spinifex/handlers/eks/nodegroup_instance_profile_test.go b/spinifex/handlers/eks/nodegroup_instance_profile_test.go index 99faa8ef9..dfa6bc7fe 100644 --- a/spinifex/handlers/eks/nodegroup_instance_profile_test.go +++ b/spinifex/handlers/eks/nodegroup_instance_profile_test.go @@ -72,14 +72,38 @@ func TestEnsureNodeInstanceProfile_AddRoleLimitExceededIsSuccess(t *testing.T) { assert.NotEmpty(t, arn) } -func TestEnsureNodeInstanceProfile_BadARN(t *testing.T) { - s := &EKSServiceImpl{deps: EKSServiceDeps{IAM: newFakeEnsurer()}} - _, err := s.ensureNodeInstanceProfile("000000000001", "not-an-arn") - require.Error(t, err) +// A path-bearing role ARN names its profile after the final segment only; the +// path is not a legal instance-profile name character. +func TestEnsureNodeInstanceProfile_PathBearingRoleUsesFinalSegment(t *testing.T) { + f := newFakeEnsurer() + s := &EKSServiceImpl{deps: EKSServiceDeps{IAM: f}} + + arn, err := s.ensureNodeInstanceProfile("000000000001", "arn:aws:iam::000000000001:role/team/Worker") + require.NoError(t, err) + assert.Equal(t, "arn:aws:iam::000000000001:instance-profile/Worker", arn) + assert.Len(t, f.Profiles["Worker"].Roles, 1) + assert.Equal(t, "Worker", aws.StringValue(f.Profiles["Worker"].Roles[0].RoleName)) } -func TestRoleNameFromARN(t *testing.T) { - assert.Equal(t, "eks-quickstart-node-role", roleNameFromARN(testNodeRoleARN)) - assert.Empty(t, roleNameFromARN("arn:aws:iam::000000000001:user/bob")) - assert.Empty(t, roleNameFromARN("")) +func TestEnsureNodeInstanceProfile_RejectsInvalidARNs(t *testing.T) { + cases := map[string]string{ + "empty": "", + "not an ARN": "not-an-arn", + "bare role suffix": "garbage:role/Worker", + "non-IAM service": "arn:aws:sts::000000000001:role/Worker", + "non-role resource": "arn:aws:iam::000000000001:user/bob", + "empty role name": "arn:aws:iam::000000000001:role/", + "empty account": "arn:aws:iam:::role/Worker", + "cross-account": "arn:aws:iam::000000000002:role/Worker", + } + for name, arn := range cases { + t.Run(name, func(t *testing.T) { + f := newFakeEnsurer() + s := &EKSServiceImpl{deps: EKSServiceDeps{IAM: f}} + _, err := s.ensureNodeInstanceProfile("000000000001", arn) + require.Error(t, err) + assert.Zero(t, f.CreateInstanceProfileCalls) + assert.Zero(t, f.AddRoleToInstanceProfileCalls) + }) + } }