-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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: Add new addon policy for AWS load balancer controller to IRSA role #189
feat: Add new addon policy for AWS load balancer controller to IRSA role #189
Conversation
condition { | ||
test = "StringEquals" | ||
variable = "iam:AWSServiceName" | ||
values = ["elasticloadbalancing.${local.partition}"] |
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.
Is it better to use aws_service
data source instead of generating values manually? It is not in the documentation for some reason, yet. I am trying to think about a use case for it and whether we should be using it in some time?
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.
(oof, looks like I had an error here). I tried that new data source but I don't think in this scenario it will help. using the dns_name
attribute it gives back a region specific endpoint but for IAM permissions I think we want global like https://github.com/kubernetes-sigs/aws-load-balancer-controller/blob/694a0b14184e388806f9f34be0dd9075aa8fb0a7/docs/install/iam_policy.json#L12
at least for this instance I don't think its warranted yet, I'm sure there are other areas that it could be useful
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 see. We would need to use dns_prefix
("amazonaws.com"
) but there is only reverse_dns_prefix
("com.amazonaws"
) among attributes.
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.
@@ -73,7 +73,8 @@ resource "aws_iam_role" "this" { | |||
} | |||
|
|||
resource "aws_iam_role_policy_attachment" "custom" { | |||
for_each = var.create_role ? toset(var.role_policy_arns) : [] | |||
for_each = toset([for arn in var.role_policy_arns : arn if var.create_role]) |
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.
Cool! assuming it works 😵💫
Is this ready for merge, too? |
yes, we should be all set |
## [4.13.0](v4.12.0...v4.13.0) (2022-02-17) ### Features * Add new addon policy for AWS load balancer controller to IRSA role ([#189](#189)) ([e2ce5c9](e2ce5c9))
This PR is included in version 4.13.0 🎉 |
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
Motivation and Context
Breaking Changes
How Has This Been Tested?
examples/*
projects