-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
aws_iam _role upgrade to framework and inline policy change #35634
base: main
Are you sure you want to change the base?
aws_iam _role upgrade to framework and inline policy change #35634
Conversation
Hey @teddylear 👋 Thanks for taking the time to raise this, and apologies for missing your pings on the original issue. I brought this up internally as well, to try to get a bit more traction 🙂 |
@justinretzolk Hey! No worries and thanks for the reply. If there's anything I need to add to change please let me know or if one of the maintainers needs to edit the branch directly that works as well. This is my first time doing a migration from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this, @teddylear. I have a number of comments on the PR. The biggest is that the change from inline_policy
to inline_policies
should be in a separate PR, and also still needs to support inline_policy
because of backwards compatibility guarantees.
"arn": schema.StringAttribute{ | ||
CustomType: fwtypes.ARNType, | ||
Computed: true, | ||
Optional: true, | ||
PlanModifiers: []planmodifier.String{ | ||
stringplanmodifier.UseStateForUnknown(), | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arn
parameter should be Computed
-only, without Optional
. This can also be achieved using framework.ARNAttributeComputedOnly
"arn": schema.StringAttribute{ | |
CustomType: fwtypes.ARNType, | |
Computed: true, | |
Optional: true, | |
PlanModifiers: []planmodifier.String{ | |
stringplanmodifier.UseStateForUnknown(), | |
}, | |
}, | |
names.AttrARN: framework.ARNAttributeComputedOnly(), |
TagsAll types.Map `tfsdk:"tags_all"` | ||
} | ||
|
||
func oldSDKRoleSchema() schema.Schema { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use the schema version instead of "old", e.g.
func oldSDKRoleSchema() schema.Schema { | |
func roleSchemaV0() schema.Schema { |
}, | ||
"inline_policy": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the backwards compatibility requirements for the provider, the inline_policy
parameter cannot be removed until the next major version. The map structure that you've used for inline_policies
is more user-friendly, I think, so it's definitely worth adding, and deprecating inline_policy
. However, it would be better to make a separate PR for that change.
// Some partitions (e.g. ISO) may not support tag-on-create. | ||
partition := meta.(*conns.AWSClient).Partition | ||
if input.Tags != nil && errs.IsUnsupportedOperationInPartitionError(partition, err) { | ||
input.Tags = nil | ||
|
||
output, err = retryCreateRole(ctx, conn, input) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should still be supported. Not all AWS partitions support tag-on-create
if v, ok := d.GetOk(names.AttrTags); (!ok || len(v.(map[string]interface{})) == 0) && errs.IsUnsupportedOperationInPartitionError(partition, err) { | ||
return append(diags, resourceRoleRead(ctx, d, meta)...) | ||
} | ||
// if v, ok := d.GetOk(names.AttrTags); (!ok || len(v.(map[string]interface{})) == 0) && errs.IsUnsupportedOperationInPartitionError(conn.PartitionID, err) { | ||
// return append(diags, resourceRoleRead(ctx, d, meta)...) | ||
// } | ||
|
||
if err != nil { | ||
return sdkdiag.AppendErrorf(diags, "setting IAM Role (%s) tags: %s", d.Id(), err) | ||
resp.Diagnostics.AddError( | ||
create.ProblemStandardMessage(names.IAM, create.ErrActionCreating, fmt.Sprintf("%s tags", ResNameRole), name, nil), | ||
err.Error(), | ||
) | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to invert the logic here, so that if len(plan.Tags.Elements()) == 0 && errs.IsUnsupportedOperationInPartitionError(conn.PartitionID, err)
is true, execution falls through to the next part of the function where plan
is populated.
} | ||
|
||
func deleteRole(ctx context.Context, conn *iam.Client, roleName string, forceDetach, hasInline, hasManaged bool) error { | ||
func DeleteRole(ctx context.Context, conn *iam.Client, roleName string, forceDetach, hasInline, hasManaged bool) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be exported
func DeleteRole(ctx context.Context, conn *iam.Client, roleName string, forceDetach, hasInline, hasManaged bool) error { | |
func deleteRole(ctx context.Context, conn *iam.Client, roleName string, forceDetach, hasInline, hasManaged bool) error { |
@@ -42,6 +43,8 @@ func TestAccIAMRole_basic(t *testing.T) { | |||
testAccCheckRoleExists(ctx, resourceName, &conf), | |||
resource.TestCheckResourceAttr(resourceName, "path", "/"), | |||
resource.TestCheckResourceAttrSet(resourceName, "create_date"), | |||
resource.TestCheckResourceAttrSet(resourceName, "unique_id"), | |||
resource.TestCheckResourceAttrSet(resourceName, "arn"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you're adding a test for arn
. However, you can check the specific value, e.g.
resource.TestCheckResourceAttrSet(resourceName, "arn"), | |
acctest.CheckResourceAttrGlobalARN(resourceName, "arn", "iam", fmt.Sprintf("role/%s", rName)), | |
, |
// NOTE: Have to update docs to reflect this because giant issue | ||
// Can't not use PlanOnly anymore with terraform-plugin-testing | ||
// https://github.com/hashicorp/terraform-plugin-testing/issues/256 | ||
ConfigPlanChecks: resource.ConfigPlanChecks{ | ||
PreApply: []plancheck.PlanCheck{ | ||
plancheck.ExpectEmptyPlan(), | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the problem you encountered with PlanOnly
? We haven't seen a problem using it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Linked issue above describes what is happening, it was a Failed to marshal state to json:
error when upgrading no matter what I did. The upstream terraform-plugin-testing
plugin has updated documentation to use this instead.
// // https://github.com/hashicorp/terraform-provider-aws/issues/23288 | ||
// // https://github.com/hashicorp/terraform-provider-aws/issues/28833 | ||
func TestAccIAMRole_diffsNoCondition(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// // https://github.com/hashicorp/terraform-provider-aws/issues/23288 | |
// // https://github.com/hashicorp/terraform-provider-aws/issues/28833 | |
func TestAccIAMRole_diffsNoCondition(t *testing.T) { | |
// https://github.com/hashicorp/terraform-provider-aws/issues/23288 | |
// https://github.com/hashicorp/terraform-provider-aws/issues/28833 | |
func TestAccIAMRole_diffsNoCondition(t *testing.T) { |
// Test empty value | ||
{ | ||
Config: testAccRoleConfig_permissionsBoundary(rName, ""), | ||
ExpectError: regexache.MustCompile(`Value "" cannot be parsed as an ARN.`), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't change the acceptable values here until the next major version. permissions_boundary
will have to accept an empty string and treat it as no value. Validation should only check for a valid ARN if it's not an empty string
Description
Upgrades
aws_iam_role
toterraform-plugin-framework
and resolve issue withinline_policy
being a set insdkv2
to amap
in theterraform-plugin-framework
to resolve linked issue below with inline policy plans and fully deleting then recrating policies on changes instead of just updating in place and then removing policies that are no longer present. Using terraform from that issue with base terraform update here in new version of provider:And the plan to add second bucket in inline policy produces this plan which is close to original one in ticket:
Adding a third bucket produces this plan which is even more user friendly:
Please let me know if there's any other test or anything else I should be adding here. I tried to document as many behavior changes as possible in the existing tests. There are some import changes which are best noted in the tests and code itself. Also please let me know if I should squash commits.
Relations
Closes #22336
Output from Acceptance Testing
Added some new tests to confirm some validation functionality as well as upgrade from previous version of prodiver. Some existing tests had to be changed as result of migration to
terraform-plugin-framework
. I tried to document them as best as possible inrole_test.go
file.