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

feat: Extended trusted_entities variable to support multiple types #143

Merged

Conversation

flibustier
Copy link
Contributor

Description

This change allows a better customization for the trusted entities used in the assume role policy

For example :

trusted_entities = [
    {
      type = "AWS",
      identifiers = ["arn:aws:sts::XXXXXXXXXXXX:assumed-role/OktaRoleForXXX/xxxx@xxxx"]
    }
]

Motivation and Context

Currently, trusted_entities input variable only accepts a list of strings which is added to the type Service of the assume role Principal.

For example:

trusted_entities = ["service.amazonaws.com"]

Will be interpreted as :

Principal = {
   ~ Service = "lambda.amazonaws.com" -> [
      + "lambda.amazonaws.com",
      + "service.amazonaws.com",
    ]
}

In the issue #138 I explain that we need a AWS Principal in our assume role (the goal is to execute lambda in local using its assume role).

Breaking Changes

This first version overwrites trusted_entities input variable. It was previously a list of (service only) strings, and now becomes a list of objects consisting of a type string (like Service or AWS) and an identifiers array of strings (like an array of services URL as before or a list of ARNs), which will break previous usages of this variable as a list of strings.

But It could also be moved to another input variable so it doesn’t break the current trusted_entities input variable. The only thing that is missing is a good variable name which reflects the openness of the input variable compared to the current trusted_entities, which actually seems to be a trusted_services_entities.

How Has This Been Tested?

  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed the code in our project and it results in a good terraform plan :
module.lambda.aws_iam_role.lambda[0] will be updated in-place
  ~ resource "aws_iam_role" "lambda" {
      ~ assume_role_policy    = jsonencode(
          ~ {
              ~ Statement = [
                  ~ {
                      ~ Principal = {
                          + AWS     = [
                              + "arn:aws:sts::XXXXXXXXXXX:assumed-role/OktaRoleForXXX/xxx@xxx",
                            ]
                            # (1 unchanged element hidden)
                        }
                        # (3 unchanged elements hidden)
                    },
                ]
                # (1 unchanged element hidden)
            }
        )
        # (7 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

Thank you for your help and your fantastic work on this module 👏

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

I like the proposal and it will be even better if you actually update code in examples/complete to show this for real.

variables.tf Outdated
@@ -437,8 +437,11 @@ variable "attach_policy_statements" {

variable "trusted_entities" {
description = "Lambda Function additional trusted entities for assuming roles (trust relationship)"
type = list(string)
default = []
type = list(object({
Copy link
Member

Choose a reason for hiding this comment

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

Could you change this to list(any) and use try() to make this PR backward compatible and support previous values also.

I think it should be rather straightforward. Let me know if you have questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s a very good idea, I will try this and keep you informed on this, thank you very much !

@flibustier flibustier force-pushed the feat/trusted-entities branch 4 times, most recently from 194c55d to 3123d8e Compare April 17, 2021 18:16
@flibustier flibustier force-pushed the feat/trusted-entities branch from 3123d8e to 0c5ff91 Compare April 17, 2021 18:21
@flibustier
Copy link
Contributor Author

I’ve updated the pull request using try but I’m not very familiar with it so don’t hesitate to give me your feedback/modifications
Thank you !

@antonbabenko antonbabenko changed the title feat: extending trusted_entities variable feat: Extended trusted_entities variable to support multiple types Apr 19, 2021
@antonbabenko antonbabenko merged commit 004f13d into terraform-aws-modules:master Apr 19, 2021
@antonbabenko
Copy link
Member

Thanks @flibustier !

I have updated the code and examples.

v1.47.0 has been just released.

@flibustier flibustier deleted the feat/trusted-entities branch April 19, 2021 12:31
@flibustier
Copy link
Contributor Author

Great job! I successfully tested it in our project!

Thanks so much!! 🎉

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants