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

[Provider]: Implement exclusive relationship management resources #39376

Open
6 of 13 tasks
jar-b opened this issue Sep 18, 2024 · 23 comments
Open
6 of 13 tasks

[Provider]: Implement exclusive relationship management resources #39376

jar-b opened this issue Sep 18, 2024 · 23 comments

Comments

@jar-b
Copy link
Member

jar-b commented Sep 18, 2024

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

Copy link

Community Note

Voting for Prioritization

  • Please vote on this issue by adding a 👍 reaction to the original post to help the community and maintainers prioritize this request.
  • Please see our prioritization guide for information on how we prioritize.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

Volunteering to Work on This Issue

  • If you are interested in working on this issue, please leave a comment.
  • If this would be your first contribution, please review the contribution guide.

@lorengordon
Copy link
Contributor

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

@lorengordon
Copy link
Contributor

lorengordon commented Sep 19, 2024

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

@lorengordon
Copy link
Contributor

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
Copy link

yermulnik commented Sep 19, 2024

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
Copy link

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
Copy link
Contributor

lorengordon commented Sep 20, 2024

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: plus3it/terraform-aws-tardigrade-iam-principals#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
Copy link
Contributor

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

@jar-b
Copy link
Member Author

jar-b commented Sep 20, 2024

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 {
  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
Copy link
Member Author

jar-b commented Sep 20, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

jar-b commented Sep 23, 2024

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:

  • Migrate aws_iam_role_policy to Terraform Plugin Framework
  • Migrate at least part of the aws_iam_role schema to Terraform Plugin Framework (for populating the SourceSchema)
  • Implement a StateMover between aws_iam_role and aws_iam_role_policy

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
Copy link
Contributor

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
Copy link
Member Author

jar-b commented Sep 24, 2024

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
Copy link
Member Author

jar-b commented Sep 24, 2024

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

@lorengordon
Copy link
Contributor

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
Copy link
Contributor

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: hashicorp/terraform#35785.

@lusitania
Copy link

aws_ssoadmin_managed_policy_attachment could do with an exclusive sibling as well.

@jar-b
Copy link
Member Author

jar-b commented Oct 21, 2024

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

@lorengordon
Copy link
Contributor

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

@mbbush
Copy link
Contributor

mbbush commented Oct 24, 2024

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
Copy link
Contributor

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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants