aws-ia / terraform-aws-mwaa

Terraform module for Amazon MWAA(Apache Airflow)
https://registry.terraform.io/modules/aws-ia/mwaa/
Apache License 2.0
40 stars 53 forks source link

iam_role_additional_policies and external IAM Roles #30

Open ZhijieWang opened 1 year ago

ZhijieWang commented 1 year ago

When bringing external iam role with below config

  create_iam_role       = false
  execution_role_arn    = data.aws_iam_role.mwaa.arn
  iam_role_additional_policies = []

TF throws below error

│ Error: Invalid object key
│ 
│   on .terraform/modules/mwaa/locals.tf line 14, in locals:
│   14:   iam_role_additional_policies = { for k, v in toset(concat([var.iam_role_additional_policies])) : k => v if var.execution_role_arn != null }
│ 

Upon verification, terraform-aws-eks uses a similar pattern, but with different variable types

iam_role_additional_policies in var should be map(string) rather than list(string)

Also, the if conditional should not be checking external role, it should be checking create_iam_role

The concact should enclose var.iam_role_additional_policies with []. Detail see below screenshot


> { for k, v in toset(concat([[]])) : k => v if "asdf" != null }
╷
│ Error: Invalid object key
│ 
│   on <console-input> line 1:
│   (source code not available)
│ 
│ The key expression produced an invalid result: string required.
╵

> { for k, v in toset(concat([[]])) : k => v if null != null }
{}

> { for k, v in toset(concat([])) : k => v if "asdf" != null }
{}

if needed, we can discuss about the detail using aws internal channels.

vara-bonthu commented 1 year ago

@ZhijieWang Thanks for raising this issue. Would you be able to raise a PR with the solution you mentioned?

SamuZad commented 1 year ago

I ended up creating a pull request for this.

I implemented the proposed solution- the the chain-wrapping of toset(), concat() etc were unnecessary in the end, since if the data type is map(string), you can iterate through it painlessly

rito-sixt commented 1 year ago

Would be good to merge this PR soon. Currently, it seems there is no way to grant additional permissions to the IAM role, since iam_role_additional_policies has this bug and we cannot add additional policies if create_iam_role=true

rhyek commented 8 months ago

What is the intended way to add policies, currently (v0.0.5)?

data "aws_iam_policy" "secretsmanager_read_write" {
  arn = "arn:aws:iam::aws:policy/SecretsManagerReadWrite"
}

module "mwaa" {
  ...
  create_iam_role = true # optional, added for clarity
  iam_role_additional_policies = {
    "does_this_key_have_any_significance" = data.aws_iam_policy.secretsmanager_read_write.arn
  }
}

I that correct?

rito-sixt commented 8 months ago

What is the intended way to add policies, currently (v0.0.5)?

data "aws_iam_policy" "secretsmanager_read_write" {
  arn = "arn:aws:iam::aws:policy/SecretsManagerReadWrite"
}

module "mwaa" {
  ...
  create_iam_role = true # optional, added for clarity
  iam_role_additional_policies = {
    "does_this_key_have_any_significance" = data.aws_iam_policy.secretsmanager_read_write.arn
  }
}

I that correct?

Yes, looks correct and how we have been doing it as well. The key does not have much significance, apart from that it has to be unique, I think.