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: Add: output external_dns_policy_arn to module iam-role-for-service-accounts-eks #484

Closed
wants to merge 6 commits into from
Closed

feat: Add: output external_dns_policy_arn to module iam-role-for-service-accounts-eks #484

wants to merge 6 commits into from

Conversation

Diaa-Hassan
Copy link

Description

This pull request adds a new output for the external_dns_policy_arn. This output provides the ARN of the IAM policy for external-dns. This information can be useful for other parts of the codebase that need to reference this policy.

Motivation and Context

This can help in indexing and using this arn by other modules

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@Diaa-Hassan Diaa-Hassan changed the title Update: Module/iam-role-for-service-accounts-eksUpdate output to contain the external_dns_policy_arn feat: Add: output external_dns_policy_arn to module iam-role-for-service-accounts-eks May 8, 2024
@bryantbiggs
Copy link
Member

This information can be useful for other parts of the codebase that need to reference this policy

Such as? This sounds like you are leaking details across boundaries which will lead to conflicts or surprises

@Diaa-Hassan
Copy link
Author

@bryantbiggs
how would this lead to conflicts and surprises ??!!!

resource "aws_iam_role_policy_attachment" "example" {
  for_each   = module.cluster.eks_managed_node_groups
  policy_arn = module.eks_external_dns_iam.external_dns_policy_arn
  role       = each.value.iam_role_name
}

how can the policy be attached to any role if the policy name and arn not known ???
the policy generated has by default the prefix AmazonEKS_External_DNS_Policy- and followed by a random number that can't be retrieved unless there is an output of this generated policy arn

@bryantbiggs
Copy link
Member

well:

  1. You don't attach the external DNS policy to the node IAM role, thats an anti-pattern
  2. If you rely on the node IAM role for providing the permissions, other applications will inherit those permissions. If you later remove this from the node IAM role and use it only on the external DNS chart (namespace/service account), then you could break those other applications that had inherited permissions that were not known

In short, you use the IAM role that is generated from this module, not its policy

@bryantbiggs bryantbiggs closed this May 8, 2024
@Diaa-Hassan Diaa-Hassan deleted the update-external-dns-policy-arn-output branch May 8, 2024 15:42
Copy link

github-actions bot commented Jun 8, 2024

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 Jun 8, 2024
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