Skip to content
This repository has been archived by the owner on May 7, 2021. It is now read-only.

platform/api/aws: add additional checks in ensureInstanceProfile #1058

Closed
wants to merge 1 commit into from
Closed

platform/api/aws: add additional checks in ensureInstanceProfile #1058

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
59 changes: 48 additions & 11 deletions platform/api/aws/iam.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,50 @@ const (
// AmazonS3RReadOnlyAccess permissions policy applied to allow fetches
// of S3 objects that are not owned by the root account.
func (a *API) ensureInstanceProfile(name string) error {
_, err := a.iam.GetInstanceProfile(&iam.GetInstanceProfileInput{
profileExists := false
policy := "AmazonS3ReadOnlyAccess"

profile, err := a.iam.GetInstanceProfile(&iam.GetInstanceProfileInput{
InstanceProfileName: &name,
})
if err == nil {
return nil
}
if awserr, ok := err.(awserr.Error); !ok || awserr.Code() != "NoSuchEntity" {
if awsErr, ok := err.(awserr.Error); !ok || awsErr.Code() != "NoSuchEntity" {
return fmt.Errorf("getting instance profile %q: %v", name, err)
} else if err == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just fold back this whole block with the rest of the logic below and just be more idempotent? E.g.:

- CreateInstanceProfile, ignore EntityAlreadyExists
- CreateRole, ignore EntityAlreadyExists
- PutRolePolicy (seems like that API is already idempotent)
- AddRoleToInstanceProfile, ignore EntityAlreadyExists

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the approach, the main deviation from what you have listed is AddRoleToInstanceProfile, in testing this is actually not returning an EntityAlreadyExists but rather a LimitExceeded because the default limit for InstanceSessionsPerInstanceProfile is 1 (essentially each instance profile can only have one role).

I'm not sure I feel that error is sufficient towards ensuring that the correct Role is attached to the InstanceProfile. Perhaps I'll just add a check there in addition to ignoring the EntityAlreadyExists errors on CreateInstanceProfile & CreateRole

profileExists = true
// check if a role of the same name already exists and is attached
// to the instance profile
for _, role := range profile.InstanceProfile.Roles {
if role.RoleName != nil && *role.RoleName == name {
if role.AssumeRolePolicyDocument != nil && *role.AssumeRolePolicyDocument != ec2AssumeRolePolicy {
// the role exists but is missing the assume role policy
// present this as an error to the user requiring manual intervention
return fmt.Errorf("role %q exists but is not configured properly, manual intervention required", name)
}
// validate that the role has the correct policy
_, err := a.iam.GetRolePolicy(&iam.GetRolePolicyInput{
PolicyName: aws.String(policy),
RoleName: aws.String(name),
})
if awserr, ok := err.(awserr.Error); !ok || awserr.Code() != "NoSuchEntity" {
return fmt.Errorf("getting role policy: %v", err)
} else if awserr.Code() == "NoSuchEntity" {
// the role does not contain the AmazonS3ReadOnlyAccess policy
// attempt to add it to allow the existing instance prrofile
// to be re-used
_, err = a.iam.PutRolePolicy(&iam.PutRolePolicyInput{
PolicyName: &policy,
PolicyDocument: aws.String(s3ReadOnlyAccess),
RoleName: &name,
})
if err != nil {
return fmt.Errorf("adding %q policy to role %q: %v", policy, name, err)
}
}
// the role exists with the correct policy and is attached to the instance profile
// re-use the existing instance profile
return nil
}
}
}

_, err = a.iam.CreateRole(&iam.CreateRoleInput{
Expand All @@ -77,7 +113,6 @@ func (a *API) ensureInstanceProfile(name string) error {
if err != nil {
return fmt.Errorf("creating role %q: %v", name, err)
}
policy := "AmazonS3ReadOnlyAccess"
_, err = a.iam.PutRolePolicy(&iam.PutRolePolicyInput{
PolicyName: &policy,
PolicyDocument: aws.String(s3ReadOnlyAccess),
Expand All @@ -87,11 +122,13 @@ func (a *API) ensureInstanceProfile(name string) error {
return fmt.Errorf("adding %q policy to role %q: %v", policy, name, err)
}

_, err = a.iam.CreateInstanceProfile(&iam.CreateInstanceProfileInput{
InstanceProfileName: &name,
})
if err != nil {
return fmt.Errorf("creating instance profile %q: %v", name, err)
if !profileExists {
_, err = a.iam.CreateInstanceProfile(&iam.CreateInstanceProfileInput{
InstanceProfileName: &name,
})
if err != nil {
return fmt.Errorf("creating instance profile %q: %v", name, err)
}
}

_, err = a.iam.AddRoleToInstanceProfile(&iam.AddRoleToInstanceProfileInput{
Expand Down