Add MaxSesessionDuration to iam_role for federated users #3977

stephencoe · 2018-03-29T12:16:19Z

Description

Adding max_session_duration to iam_role fixes #3976

Changes

bump aws provider
add max_session_duration

stephencoe · 2018-03-29T12:46:33Z

this is my first PR into TF, I am unsure why CI failed 😕

bflad · 2018-03-29T12:48:12Z

My recommendation is to split the SDK update into it's own PR first (generally we merge these really fast)

bflad · 2018-03-29T12:52:16Z

vendor/vendor.json

@@ -39,884 +39,892 @@
 			"revisionTime": "2017-07-27T15:54:43Z"
 		},
 		{
-			"checksumSHA1": "y1bmVkrw6lKwgq8z+S3GzO1F5JE=",
+			"checksumSHA1": "N+8JQQqczoq0UfwXrcVqqRDcb5E=",


Did you accidentally govendor the root? I think it'll complain about that. See any of the recent Deps: Bump aws-sdk-go PRs for how it's done (we're hoping to automate this shortly)

thanks, I will split that out

* master: bumping to 1.13.23

loivis · 2018-03-30T08:14:27Z

aws/resource_aws_iam_role.go

+				Optional: true,
+				Default:  3600,
+				ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
+					value := v.(int64)


how about replace custom function with existing validation.IntBetween() instead?

As noted above, this should be updated to ValidateFunc: validation.IntBetween(3600, 43200)

bflad

Hi @stephencoe! 👋 Thanks for submitting this! Can you please see the feedback below and let us know if you have any questions or do not have time to implement them?

bflad · 2018-04-02T13:35:26Z

aws/resource_aws_iam_role.go

+	if d.HasChange("max_session_duration") {
+		roleMaxDurationInput := &iam.UpdateRoleInput{
+			RoleName:           aws.String(d.Id()),
+			MaxSessionDuration: aws.Int64(d.Get("assume_role_policy").(int64)),


This looks like it has two issues:

Copy-paste typo of which attribute to grab, should be "max_session_duration"

schema.TypeInt returns a regular int, so I believe this needs to be aws.Int64(int64(d.Get("max_session_duration").(int)))

bflad · 2018-04-02T13:38:49Z

aws/resource_aws_iam_role.go

+		}
+		_, err := iamconn.UpdateRole(roleMaxDurationInput)
+		if err != nil {
+			if iamerr, ok := err.(awserr.Error); ok && iamerr.Code() == "NoSuchEntity" {


Two notes here:

This can be simplified and use the SDK provided constant with if isAWSErr(err, iam.ErrCodeNoSuchEntityException, "") {

The error message needs to be updated

bflad · 2018-04-02T13:39:41Z

aws/resource_aws_iam_role_test.go

@@ -158,6 +158,40 @@ func TestAccAWSIAMRole_force_detach_policies(t *testing.T) {
 	})
 }

+func TestAccAWSIAMRole_testMaxSessionDuration(t *testing.T) {


You can drop the second test before MaxSessionDuration

bflad · 2018-04-02T13:41:58Z

aws/resource_aws_iam_role_test.go

@@ -261,6 +295,39 @@ func testAccAddAwsIAMRolePolicy(n string) resource.TestCheckFunc {
 	}
 }

+func testMaxSessionDuration(rName string) string {


The signature here should be:

func testAccCheckIAMRoleConfig_MaxSessionDuration(rName string, maxSessionDuration int) string {

Then accept the parameter in the HCL configuration:

max_session_duration = %d

So we can remove the other two test configurations.

bflad · 2018-04-02T13:44:53Z

website/docs/r/iam_role.html.markdown

@@ -49,6 +49,8 @@ The following arguments are supported:
  See [IAM Identifiers](https://docs.aws.amazon.com/IAM/latest/UserGuide/Using_Identifiers.html) for more information.
 * `description` - (Optional) The description of the role.

+* `max_session_duration` - (Optional) Max duration of a federated role 


This should probably note that the duration is in seconds and defaults to 3600. The SDK documentation sums it up pretty well:

The maximum session duration (in seconds) that you want to set for the specified role. If you do not specify a value for this setting, the default maximum of one hour is applied. This setting can have a value from 1 hour to 12 hours.

bflad · 2018-04-02T13:45:58Z

aws/resource_aws_iam_role.go

+				Optional: true,
+				Default:  3600,
+				ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
+					value := v.(int64)


As noted above, this should be updated to ValidateFunc: validation.IntBetween(3600, 43200)

bflad · 2018-04-02T13:47:24Z

aws/resource_aws_iam_role_test.go

+resource "aws_iam_role" "role" {
+  name   = "test-role-%s"
+	path = "/"
+	max_session_duration = 46800


When the tests are updated, this should test the hard edges of the validation at 43201 and 3599.

bflad · 2018-04-02T13:50:30Z

aws/resource_aws_iam_role_test.go

+				),
+			},
+			{
+				Config: testMaxSessionDuration_too_long(rName),


We expect these configurations to fail, which can be accomplished with:

{ Config: testAccCheckIAMRoleConfig_MaxSessionDuration(rName, 43201), ExpectError: regexp.MustCompile(`error message here`), },

jonotto · 2018-04-02T17:15:13Z

This will also fix duplicate enhancement request #3969

stephencoe · 2018-04-04T11:21:14Z

@bflad thanks for the feedback, sorry its not been updated sooner :)

bflad · 2018-04-04T21:24:40Z

aws/resource_aws_iam_role.go

@@ -137,6 +145,10 @@ func resourceAwsIamRoleCreate(d *schema.ResourceData, meta interface{}) error {
 		request.Description = aws.String(v.(string))
 	}

+	if v, ok := d.GetOk("max_session_duration"); ok {
+		request.MaxSessionDuration = aws.Int64(v.(int64))


=== RUN TestAccAWSIAMRole_MaxSessionDuration panic: interface conversion: interface {} is int, not int64 goroutine 227 [running]: github.com/terraform-providers/terraform-provider-aws/aws.resourceAwsIamRoleCreate(0xc4202c3810, 0x3542900, 0xc4208a0500, 0xc4202c3810, 0x0) /Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_iam_role.go:149 +0x6da

I will fix this on merge.

bflad · 2018-04-04T21:28:41Z

aws/resource_aws_iam_role_test.go

+			{
+				Config: testAccCheckIAMRoleConfig_MaxSessionDuration(rName, 3700),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSRoleExists("aws_iam_role.test", &conf),


--- FAIL: TestAccAWSIAMRole_MaxSessionDuration (6.39s) testing.go:518: Step 0 error: Check failed: Check 1/2 error: Not found: aws_iam_role.test

The resource is named role in the test configuration. I will fix this on merge.

bflad

Fixed up the panic and acceptance testing in my branch which I'll use to merge keeping your commits. Thanks for the contribution! 🚀

make testacc TEST=./aws TESTARGS='-run=TestAccAWSIAMRole'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIAMRole -timeout 120m
=== RUN   TestAccAWSIAMRolePolicy_importBasic
--- PASS: TestAccAWSIAMRolePolicy_importBasic (10.90s)
=== RUN   TestAccAWSIAMRole_importBasic
--- PASS: TestAccAWSIAMRole_importBasic (8.02s)
=== RUN   TestAccAWSIAMRolePolicy_basic
--- PASS: TestAccAWSIAMRolePolicy_basic (15.84s)
=== RUN   TestAccAWSIAMRolePolicy_namePrefix
--- PASS: TestAccAWSIAMRolePolicy_namePrefix (8.09s)
=== RUN   TestAccAWSIAMRolePolicy_generatedName
--- PASS: TestAccAWSIAMRolePolicy_generatedName (8.36s)
=== RUN   TestAccAWSIAMRolePolicy_invalidJSON
--- PASS: TestAccAWSIAMRolePolicy_invalidJSON (1.24s)
=== RUN   TestAccAWSIAMRole_basic
--- PASS: TestAccAWSIAMRole_basic (8.96s)
=== RUN   TestAccAWSIAMRole_basicWithDescription
--- PASS: TestAccAWSIAMRole_basicWithDescription (19.16s)
=== RUN   TestAccAWSIAMRole_namePrefix
--- PASS: TestAccAWSIAMRole_namePrefix (7.15s)
=== RUN   TestAccAWSIAMRole_testNameChange
--- PASS: TestAccAWSIAMRole_testNameChange (14.99s)
=== RUN   TestAccAWSIAMRole_badJSON
--- PASS: TestAccAWSIAMRole_badJSON (1.55s)
=== RUN   TestAccAWSIAMRole_force_detach_policies
--- PASS: TestAccAWSIAMRole_force_detach_policies (7.85s)
=== RUN   TestAccAWSIAMRole_MaxSessionDuration
--- PASS: TestAccAWSIAMRole_MaxSessionDuration (13.85s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	126.014s

bflad · 2018-04-06T19:03:58Z

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

ghost · 2020-04-06T17:56:54Z

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!

adding mfa session duration

1759f45

ghost added size/XXL Managed by automation to categorize the size of a PR. dependencies Used to indicate dependency changes. labels Mar 29, 2018

stephencoe changed the title ~~Iam role maxsesessionduration for federated users~~ Add MaxSesessionDuration to iam_role for federated users Mar 29, 2018

ghost added size/XXL Managed by automation to categorize the size of a PR. dependencies Used to indicate dependency changes. labels Mar 29, 2018

bflad reviewed Mar 29, 2018

View reviewed changes

updating documentation

df55b62

stephencoe force-pushed the iam_role_mfa_duration branch from 4d6ec2e to df55b62 Compare March 29, 2018 13:08

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. dependencies Used to indicate dependency changes. labels Mar 29, 2018

Merge branch 'master' into iam_role_mfa_duration

1e5f4a3

* master: bumping to 1.13.23

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 29, 2018

loivis reviewed Mar 30, 2018

View reviewed changes

bflad added service/iam Issues and PRs that pertain to the iam service. enhancement Requests to existing resources that expand the functionality or scope. labels Apr 2, 2018

bflad suggested changes Apr 2, 2018

View reviewed changes

stephencoe added 3 commits April 4, 2018 12:18

fixing resource

edb698e

fixing test

862f400

updating documentation

f97095a

ghost added size/M Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Apr 4, 2018

update regex error

3ce0379

ghost added the size/M Managed by automation to categorize the size of a PR. label Apr 4, 2018

issyl0 mentioned this pull request Apr 4, 2018

Increase the assume-role MFA expiry time to 8 hours (28800 seconds) alphagov/govuk-aws#533

Merged

bflad reviewed Apr 4, 2018

View reviewed changes

bflad added this to the v1.14.0 milestone Apr 4, 2018

bflad approved these changes Apr 4, 2018

View reviewed changes

bflad merged commit 06c4a51 into hashicorp:master Apr 4, 2018

bflad added a commit that referenced this pull request Apr 4, 2018

Update CHANGELOG for #3977

cbe07c0

ghost locked and limited conversation to collaborators Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MaxSesessionDuration to iam_role for federated users #3977

Add MaxSesessionDuration to iam_role for federated users #3977

stephencoe commented Mar 29, 2018

stephencoe commented Mar 29, 2018

bflad commented Mar 29, 2018

bflad Mar 29, 2018

stephencoe Mar 29, 2018

loivis Mar 30, 2018

bflad Apr 2, 2018

bflad left a comment

bflad Apr 2, 2018

bflad Apr 2, 2018

bflad Apr 2, 2018

bflad Apr 2, 2018

bflad Apr 2, 2018

bflad Apr 2, 2018

bflad Apr 2, 2018

bflad Apr 2, 2018

jonotto commented Apr 2, 2018

stephencoe commented Apr 4, 2018

bflad Apr 4, 2018

bflad Apr 4, 2018

bflad left a comment

bflad commented Apr 6, 2018

ghost commented Apr 6, 2020

Add MaxSesessionDuration to iam_role for federated users #3977

Add MaxSesessionDuration to iam_role for federated users #3977

Conversation

stephencoe commented Mar 29, 2018

Description

Changes

stephencoe commented Mar 29, 2018

bflad commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonotto commented Apr 2, 2018

stephencoe commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

bflad commented Apr 6, 2018

ghost commented Apr 6, 2020