Skip to content
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.

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

New Resource: aws_iam_service_linked_role #2985

Merged
merged 1 commit into from
Apr 11, 2018
Merged

New Resource: aws_iam_service_linked_role #2985

merged 1 commit into from
Apr 11, 2018

Conversation

robinjoseph08
Copy link
Contributor

@robinjoseph08 robinjoseph08 commented Jan 14, 2018

Fixes #921

TF_ACC=1 go test ./aws -v -run=TestAccAwsIamServiceLinkedRole -timeout 120m
=== RUN   TestAccAwsIamServiceLinkedRole
--- PASS: TestAccAwsIamServiceLinkedRole (22.98s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       23.015s

@Ninir Ninir added enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. service/iam Issues and PRs that pertain to the iam service. labels Jan 15, 2018
@erikbor
Copy link

erikbor commented Feb 5, 2018

We've encountered the same issue with AWS swapping to service roles -- could this get merged soon? Thanks @robinjoseph08 @apparentlymart !

@roman-cnd
Copy link

This is required to setup spot instances fleet for batch...

@bflad bflad added this to the v1.12.0 milestone Feb 21, 2018
@bflad bflad self-requested a review February 21, 2018 04:53

_, err := conn.CreateServiceLinkedRole(params)

if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest checking to see if the role already exists before creating it or handling the conflict error with something like

	d.SetId(serviceName)
	if _, err := conn.CreateServiceLinkedRole(params); err != nil {
		aerr, ok := err.(awserr.Error)
		if ok && strings.Contains(aerr.Message(), "already taken") {
			if rerr := resourceAwsIamServiceLinkedRoleRead(d, meta); err != nil {
				return rerr
			}
		}
		if d.Id() == "" || !ok {
			return fmt.Errorf("Error creating service-linked role with name %s", serviceName)
		}
	}

	return nil

Otherwise, this will not apply cleanly on an account where the role already exist and is being used to scale production resources.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead just import that role since you can only have one global IAM service linked role?

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @robinjoseph08 thanks for this contribution and sorry for the delays in getting it reviewed. 😅 Can you please see the feedback below and let us know if you have any questions or do not have time to complete them? Thank you!

}

if resp == nil || len(resp.Roles) == 0 {
d.SetId("")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add logging before this? log.Printf("[WARN] IAM service linked role %s not found, removing from state", d.Id())

return fmt.Errorf("Error creating service-linked role with name %s", serviceName)
}

d.SetId(serviceName)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should set this to the role ARN so we can utilize GetRole instead of the unsafe ListRoles (you can have multiple service linked roles for the same service when created with CustomSuffix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bflad Not sure if I'm just missing something, but it doesn't look like you can use the ARN with GetRole, just the role name. Should I use the role name as the Id? That doesn't seem right either, but that is what aws_iam_role does. Should I do what you mention for the DeleteServiceLinkedRole call and just parse the role name out of the ARN?

return nil
}

role := resp.Roles[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Services can have multiple linked roles with custom suffixes, we should switch the read logic to GetRole 👍

name := d.Get("name").(string)

params := &iam.DeleteServiceLinkedRoleInput{
RoleName: aws.String(name),
Copy link
Contributor

Choose a reason for hiding this comment

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

When d.SetId() is updated, d.Id() should contain the ARN we can parse for the name. 👍

deletionTaskId := *resp.DeletionTaskId

stateConf := &resource.StateChangeConf{
Pending: []string{"IN_PROGRESS", "NOT_STARTED"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick: can you please switch these to the SDK constants? e.g. iam. DeletionTaskStatusTypeInProgress


resp, err := conn.ListRoles(params)

if err != nil || resp == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic will currently ignore errors like PermissionDenied. It should return all errors except on isAWSErr(err, iam.ErrCodeNoSuchEntityException, "")

}

conn := testAccProvider.Meta().(*AWSClient).iamconn
service_name := rs.Primary.ID
Copy link
Contributor

Choose a reason for hiding this comment

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

Same deal here with the ARN as above 👍


The following arguments are supported:

* `aws_service_name` - (Required, Forces new resource) The AWS service to which this role is attached. You use a string similar to a URL but without the `http://` in front. For example: `elasticbeanstalk.amazonaws.com`
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably link to documentation about where to find these names here. 😄

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 couldn't seem to find a list of all of the service domains, but the closest thing I found was this. Do you think it's sufficient?

"github.com/hashicorp/terraform/terraform"
)

func TestAccAwsIamServiceLinkedRole(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename this to TestAccAWSIAMServiceLinkedRole_basic. It should also have a test step for testing import (I'll note below)

Config: fmt.Sprintf(testAccAwsIamServiceLinkedRoleConfig, "elasticbeanstalk.amazonaws.com"),
Check: testAccCheckAwsIamServiceLinkedRoleExists("aws_iam_service_linked_role.test"),
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing import TestStep:

{
  ResourceName:      "aws_iam_service_linked_role.test",
  ImportState:       true,
  ImportStateVerify: true,
},

@bflad bflad modified the milestones: v1.12.0, v1.13.0 Mar 23, 2018
@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 23, 2018
@bflad bflad modified the milestones: v1.13.0, v1.14.0 Mar 29, 2018
@bflad bflad modified the milestones: v1.14.0, v1.15.0 Apr 6, 2018
@bflad
Copy link
Contributor

bflad commented Apr 6, 2018

@robinjoseph08 can you take a look at the feedback above please or let us know if you cannot work on this?

@robinjoseph08
Copy link
Contributor Author

Thanks for the feedback all! I'll make the changes this weekend.

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 7, 2018
@robinjoseph08
Copy link
Contributor Author

@bflad updated with your requested changes, but I had a few questions to make sure I'm doing it as you expected. Let me know if there's anything else!

resp, err := conn.CreateServiceLinkedRole(params)

if err != nil {
return fmt.Errorf("Error creating service-linked role with name %s", serviceName)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should return the error messages here and on deletion 😄 I will fix post-merge.

@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Apr 11, 2018
@bflad bflad merged commit 35e7f93 into hashicorp:master Apr 11, 2018
bflad added a commit that referenced this pull request Apr 11, 2018
@bflad
Copy link
Contributor

bflad commented Apr 18, 2018

This has been released in version 1.15.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 6, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. service/iam Issues and PRs that pertain to the iam service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for service-linked roles
7 participants