-
-
Notifications
You must be signed in to change notification settings - Fork 555
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: Explicit task_exec_secret_arns
#245
feat: Explicit task_exec_secret_arns
#245
Conversation
explicit_task_exec_secret_arns
task_exec_secret_arns
task_exec_secret_arns
task_exec_secret_arns
task_exec_secret_arns
task_exec_secret_arns
modules/service/main.tf
Outdated
@@ -28,7 +28,7 @@ locals { | |||
create_service = var.create && var.create_service | |||
|
|||
container_definitions_secrets = flatten([for k, v in module.container_definition : v.container_definition.secrets]) | |||
task_exec_secret_arns = var.explicit_task_exec_secret_arns ? [for v in local.container_definitions_secrets : v.valueFrom] : var.task_exec_secret_arns | |||
task_exec_secret_arns = var.explicit_task_exec_secret_arns ? [for v in local.container_definitions_secrets : v.valueFrom] : var.task_exec_secret_arns |
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.
highly unlikely that we support this change - this makes the assumption that users only create container definitions through the service module which is not true #147
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.
@bryantbiggs Thank you for pointing me to this issue. If I understand correctly, there appears to be an issue when a container_definition
is built from the service
module. To test this, I changed the example/complete
by adding another element to the container_definitions
with the container_definition
module, which includes a secrets
block. Here is a part of the plan that appears to be fetching the correct secrets ARNs for all containers in the container_definitions
block, regardless of whether they are defined in the "service" module or via container_definition
.
+ {
+ Action = "secretsmanager:GetSecretValue"
+ Effect = "Allow"
+ Resource = [
+ "arn:POSTGRES_PASSWORD", // Defined using container_definition
+ "arn:BAR",
]
+ Sid = "GetSecrets"
},
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 attempted to mimic a more realistic example by retrieving the secret ARN from the secrets-manager
module and referencing it inside the secrets
block as follows:
module "secrets_manager" {
source = "terraform-aws-modules/secrets-manager/aws"
version = "~> 1.3"
name_prefix = "POSTGRES_PASSWORD"
secret_string = "secret"
}
module "postgres" {
source = "../../modules/container-definition"
name = "postgres"
image = "postgres:latest"
secrets = [{
name = "POSTGRES_PASSWORD"
valueFrom = module.secrets_manager.secret_arn
}]
}
However, I got the following error:
│ on ../../modules/service/main.tf line 534, in module "container_definition":
│ 534: for_each = { for k, v in var.container_definitions : k => v if local.create_task_definition && try(v.create, true) }
│ ├────────────────
│ │ local.create_task_definition is true
│ │ var.container_definitions will be known only after apply
│
│ The "for_each" map includes keys derived from resource attributes that cannot be determined until apply, and so Terraform cannot determine the full set of keys that will identify
│ the instances of this resource.
To be fair, I received the same error trying this out on master
, so it appears unrelated to the changes in this PR.
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'm not following - there is an open issue that is currently being resolved in the next major version of the module. However, that conflicts with this change here
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.
Got it; I believe I got lost in the thread a little, and I did not realize there was currently some work in progress in this regard; I apologize for that. About:
However, that conflicts with this change here
In that case, I believe I could close this PR and revisit it once the fix for #147 has been merged, if it makes sense at that point. Thank you for your comments.
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. |
Description
This PR introduces a flag called
explicit_task_exec_secret_arns
to change the default behavior oftask_exec_secret_arns
so rather than defaulting to["arn:aws:secretsmanager:*:*:secret:*"]
it collects all secrets ARNs defined inside thecontainer_definitions
block.Motivation and Context
This PR is related to the open issue #244. The primary motivation for this change is to limit the scope of the
task_exec
policy by passing an explicit list of secret ARNs specified in thecontainer_definitions
block.Breaking Changes
N/A
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectspre-commit run -a
on my pull request