search

hashicorp / terraform-provider-aws

The AWS Provider enables Terraform to manage AWS resources.
https://registry.terraform.io/providers/hashicorp/aws
Mozilla Public License 2.0
9.85k stars 9.2k forks source link

[Provider]: Implement exclusive relationship management resources #39376

Open jar-b opened 2 months ago

jar-b commented 2 months ago

Description

This meta issue will track the progress of the effort to implement "exclusive relationship management" resources, as described in this proposal (see the rendered version in the contributor guide).

IAM inline policies:

IAM customer managed policies:

Organizations policies:

SSO Admin managed policies:

VPC managed prefix lists:

VPC route tables:

VPC security groups:

References

Relates #39204

Would you like to implement a fix?

None

github-actions[bot] commented 2 months ago

Community Note

Voting for Prioritization

Volunteering to Work on This Issue

lorengordon commented 2 months ago

I love this sooooo much! Here's another related feature request:

lorengordon commented 2 months ago

Routes in a route table is another use case for exclusive management.

lorengordon commented 2 months ago

Commenting here for visibility since the proposal is already approved and merged, but what is the expected config migration path for practitioners? For example, if I am currently using aws_iam_role with the inline_policy argument exactly for the exclusive management feature, how do I move to the new pattern without impacting existing resources?

yermulnik commented 2 months ago

Commenting here for visibility since the proposal is already approved and merged, but what is the expected config migration path for practitioners? For example, if I am currently using aws_iam_role with the inline_policy argument exactly for the exclusive management feature, how do I move to the new pattern without impacting existing resources?

+1 It's been for years that we've been enforcing inline_policy blocks inside aws_iam_role resource to ensure exclusive inline policies management by TF and now we're to migrate (back?) to aws_iam_role_policy with auxiliary aws_iam_role_policies_exclusive which is to manage at least two standalone resources outside aws_iam_role including policies being decoupled from the IAM Role resource to decrease visibility 😢

yermulnik commented 2 months ago

I believe there's a good reasoning behind deprecating inline_policy in favour of when one looks into aws_iam_role resource and sees no inline policies attached because they are managed by at least two other standalone resources and each may be a part of separate .tf file 🤷🏻

lorengordon commented 2 months ago

Commenting here for visibility since the proposal is already approved and merged, but what is the expected config migration path for practitioners? For example, if I am currently using aws_iam_role with the inline_policy argument exactly for the exclusive management feature, how do I move to the new pattern without impacting existing resources?

I went ahead and demoed a quick change to our iam-principals module to see what the migration path looks like currently. I think it needs some work. It's going to be a big lift, without any way to move a resource created by an inline_policy block to the aws_iam_role_policy resource.

PR here: https://github.com/plus3it/terraform-aws-tardigrade-iam-principals/pull/205

To see what I mean:

  1. checkout the default branch for that project
  2. run init/apply in tests/create_roles/prereq
  3. run init/apply in tests/create_roles
  4. checkout the branch feat/new-exclusive-pattern
  5. run plan in tests/create_roles

You'll see that it wants to create the inline role policies again.

Of course, a user could write import blocks to import all the inline policies, but that's a little nuts. It would be amazing if I could write a moved block for the module, where the from is actually the inline_policy block on the aws_iam_role resource, and the to is the new aws_iam_role_policy resource.

Without such a moved feature, I'd have to consider this a breaking change for the module. 😢

@jar-b Thoughts?

lorengordon commented 2 months ago

Also, managed prefix lists and their entries are another resource that could benefit from this new exclusive relationship pattern...

jar-b commented 2 months ago

To migrate an existing inline policy to the standalone resource, an import block (or alternatively a manual import via the Terraform CLI) is required. Unfortunately a moved block cannot be used, as the shape of the two resources (aws_iam_role and aws_iam_role_policy) do not match.

However, because the inline_policy argument is Optional/Computed (meaning removal of this argument will not trigger deletion of inline policy, just change it to track as computed only), this step can be accomplished in a single apply.

Here's a small example (the configuration is hidden by default to improve readability):

Show/Hide Configuration The uncommented configuration below represents the existing state where inline policies are defined directly on the `aws_iam_role` resource. ```terraform terraform { required_providers { aws = { source = "hashicorp/aws" version = "~> 5.0" } } } # Configure the AWS Provider provider "aws" {} data "aws_iam_policy_document" "trust" { statement { actions = ["sts:AssumeRole"] principals { type = "Service" identifiers = ["ec2.amazonaws.com"] } } } data "aws_iam_policy_document" "inline" { statement { actions = ["s3:ListBucket"] resources = [ "arn:aws:s3:::some-bucket-a", "arn:aws:s3:::some-bucket-b", ] } } resource "aws_iam_role" "test" { name = "jb-test-inline-policy-migration" assume_role_policy = data.aws_iam_policy_document.trust.json # Remove this when the new standalone resource is added inline_policy { name = "test-inline" policy = data.aws_iam_policy_document.inline.json } } ### Start of new resources ### # # This can be removed after the apply in which the resource is imported # import { # to = aws_iam_role_policy.test # id = "jb-test-inline-policy-migration:test-inline" # } # # resource "aws_iam_role_policy" "test" { # name = "test-inline" # role = aws_iam_role.test.name # policy = data.aws_iam_policy_document.inline.json # } # # # resource "aws_iam_role_policies_exclusive" "test" { # role_name = aws_iam_role.test.name # policy_names = [ # aws_iam_role_policy.test.name, # ] # } ### End of new resources ### ```

To covert this to use the standalone aws_iam_role_policy resource, do the following:

  1. Remove the inline_policy block.
  2. Uncomment the ### Start of new resources ### section.
  3. terraform apply. There should be 1 resource to import and 1 to add.
% terraform apply -auto-approve
data.aws_iam_policy_document.inline: Reading...
data.aws_iam_policy_document.trust: Reading...
data.aws_iam_policy_document.inline: Read complete after 0s [id=748695472]
data.aws_iam_policy_document.trust: Read complete after 0s [id=2851119427]
aws_iam_role.test: Refreshing state... [id=jb-test-inline-policy-migration]
aws_iam_role_policy.test: Preparing import... [id=jb-test-inline-policy-migration:test-inline]
aws_iam_role_policy.test: Refreshing state... [id=jb-test-inline-policy-migration:test-inline]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # aws_iam_role_policies_exclusive.test will be created
  + resource "aws_iam_role_policies_exclusive" "test" {
      + policy_names = [
          + "test-inline",
        ]
      + role_name    = "jb-test-inline-policy-migration"
    }

  # aws_iam_role_policy.test will be imported
    resource "aws_iam_role_policy" "test" {
        id          = "jb-test-inline-policy-migration:test-inline"
        name        = "test-inline"
        name_prefix = null
        policy      = jsonencode(
            {
                Statement = [
                    {
                        Action   = "s3:ListBucket"
                        Effect   = "Allow"
                        Resource = [
                            "arn:aws:s3:::some-bucket-b",
                            "arn:aws:s3:::some-bucket-a",
                        ]
                    },
                ]
                Version   = "2012-10-17"
            }
        )
        role        = "jb-test-inline-policy-migration"
    }

Plan: 1 to import, 1 to add, 0 to change, 0 to destroy.
aws_iam_role_policy.test: Importing... [id=jb-test-inline-policy-migration:test-inline]
aws_iam_role_policy.test: Import complete [id=jb-test-inline-policy-migration:test-inline]
aws_iam_role_policies_exclusive.test: Creating...
aws_iam_role_policies_exclusive.test: Creation complete after 1s

Apply complete! Resources: 1 imported, 1 added, 0 changed, 0 destroyed.
  1. After a successful apply, the import block can be removed, and there are no planned changes.
% terraform plan
data.aws_iam_policy_document.trust: Reading...
data.aws_iam_policy_document.inline: Reading...
data.aws_iam_policy_document.trust: Read complete after 0s [id=2851119427]
data.aws_iam_policy_document.inline: Read complete after 0s [id=748695472]
aws_iam_role.test: Refreshing state... [id=jb-test-inline-policy-migration]
aws_iam_role_policy.test: Refreshing state... [id=jb-test-inline-policy-migration:test-inline]
aws_iam_role_policies_exclusive.test: Refreshing state...

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration and found no differences, so no changes are needed.
jar-b commented 2 months ago

In theory if all previous versions of the module guarantee the inline policies will already exist, you could publish a version containing the import directive in a non-breaking way. However, a breaking major version change may be preferable to avoid cases where users need to sequence minor versions in such a way to guarantee the import conditions are satisfied.

The terraform-aws-s3-bucket module may provide some precedent for this in how it handled breaking arguments which were previously defined inline on the aws_s3_bucket resource into their own standalone resources between v2 and v3. https://github.com/terraform-aws-modules/terraform-aws-s3-bucket/blob/v4.1.2/UPGRADE-3.0.md

lorengordon commented 2 months ago

In theory if all previous versions of the module guarantee the inline policies will already exist, you could publish a version containing the import directive in a non-breaking way.

I must have missed something... When did it become possible to put import blocks in reusable modules? I've only ever used import blocks as part of root modules...

lorengordon commented 2 months ago

Unfortunately a moved block cannot be used, as the shape of the two resources (aws_iam_role and aws_iam_role_policy) do not match.

Wasn't there a fairly recent change/feature in Terraform core that made it possible to complete a move across resources with different shapes? I know you can migrate from null_resource to terraform_data, for example. See:

jar-b commented 2 months ago

I must have missed something... When did it become possible to put import blocks in reusable modules? I've only ever used import blocks as part of root modules...

🤦 - thanks for the correction. I'm clearly not a module author and will therefore refrain from additional suggestions.

Wasn't there a fairly recent change/feature in Terraform core that made it possible to complete a move across resources with different shapes?

Yes. My assumption about the schema requirements is indeed out of date. However, we are still limited in our ability to support the moved block between these resource types because aws_iam_role_policy is implemented with Terraform Plugin SDK V2, which does not support the MoveResourceState RPC. To support this we'd need to:

While possible, adding support is unfortunately not as simple as implementing the state movement logic.

As an aside, I'm not sure moved is quite correct to model this change. We aren't moving the role resource in the sense that it should no longer be managed, which is the result of a moved operation.

Before creating a new plan for the resource specified in the to field, Terraform checks the state for an existing object at the address specified in the from field. Terraform renames existing objects to the string specified in the to field and then creates a plan. The plan directs Terraform to provision the resource specified in the from field as the resource specified in the to field. As a result, Terraform does not destroy the resource during the Terraform run.

I suspect after a successful apply with a move operation, deletion of the moved block and an import of the aws_iam_role resource would be required to get the "from" resource address back into state. Importing the existing inline policy to the standalone resource may be a more direct path in this situation.

lorengordon commented 2 months ago

As an aside, I'm not sure moved is quite correct to model this change. We aren't moving the role resource in the sense that it should no longer be managed, which is the result of a moved operation.

I think perhaps a further update to moved semantics could be in order, to support this type of shift towards resources that align primarily to a single API action. Previously, many resources would use blocks, like aws_iam_role with its inline_policy, and basically become a resource that manages multiple resources. The last few years, the providers shifted away from that. We also got some great new refactoring tools like moved, import, and removed. But none of them currently support refactoring away from blocks and towards a separate resource.

Note that I was careful in how I phrased the move request previously...

It would be amazing if I could write a moved block for the module, where the from is actually the inline_policy block on the aws_iam_role resource, and the to is the new aws_iam_role_policy resource.

I do not want to move the aws_iam_role resource entirely. Just the inline_policy block that it was managing.

jar-b commented 2 months ago

Makes sense, I'm on the same page now. This sounds like a moved enhancement or a new "copy state" feature which functions similar to moved but preserves the source resource as-is.

Working within the existing language constraints on the provider side, the import block seems the best current migration option. This unfortunately leaves module authors with a decision between a breaking change and retaining a deprecated argument. For our part, we don't intend to fully remove the inline_policy argument in the next major release due to the ubiquity of IAM resources. A relevant section of the original proposal:

Due to the popularity of the resources in this section, argument deprecations are likely to be "soft" deprecations where removal will not happen for several major releases, or until tooling is available to limit the amount of manual changes required to migrate to the preferred pattern. Despite this long removal window, a soft deprecation is still helpful for maintainers to reference when making best practice recommendations to the community.

Hopefully this helps in weighing the available options.

jar-b commented 2 months ago

Also, I've added new items in the issue body for the additional suggested resources. Thanks for those @lorengordon! 👍

lorengordon commented 2 months ago

This sounds like a moved enhancement or a new "copy state" feature which functions similar to moved but preserves the source resource as-is.

Yeah. I'll check core and open a feature/enhancement request, if something doesn't already exist.

lorengordon commented 1 month ago

This sounds like a moved enhancement or a new "copy state" feature which functions similar to moved but preserves the source resource as-is.

Yeah. I'll check core and open a feature/enhancement request, if something doesn't already exist.

Alright, issue opened: https://github.com/hashicorp/terraform/issues/35785.

lusitania commented 1 month ago

aws_ssoadmin_managed_policy_attachment could do with an exclusive sibling as well.

jar-b commented 1 month ago

Thanks @lusitania 👍 - opened #39822 and added it to the parent issue above.

lorengordon commented 1 month ago

Another candidate for exclusive attachments, I think, would be aws_eks_access_policy_association.

mbbush commented 1 month ago

aws_ssoadmin_customer_managed_policy_attachment would be just as useful to have as aws_ssoadmin_managed_policy_attachment

(because sso is cross-account, it differentiates with separate API calls between aws-managed policies that have no account id in their ARN, and customer-managed ones that do)

lorengordon commented 3 weeks ago

An interesting resource to add this exclusive attachment for might be aws_account_region. It would be nice to be able to say, "this account must be enabled for these regions and only these regions; any other enabled regions should be considered drift and disabled".