feat!: allow specifying multiple OIDC issuer URLs #63

philchristensen · 2024-09-24T19:44:44Z

what

Allow specifying multiple OIDC issuer URLs to create a role to be used on two EKS clusters in the same account.

why

I needed this, and I couldn't figure out a way to do it with this module, which I've really liked using.

BREAKING CHANGE

This changes the eks_cluster_oidc_issuer_url variable to eks_cluster_oidc_issuer_urls enabling module consumers to supply multiple OIDC Issuer URLs. If you are migrating to this version and using the previous eks_cluster_oidc_issuer_url variable, please update your usage of this module to pass your single URL like so:

module "example" {
  source = "cloudposse/eks-iam-role/aws"
  # ... 
  
  eks_cluster_oidc_issuer_urls = ["<INSERT YOUR OICD ISSUE URL HERE>"]
}

philchristensen · 2024-09-24T19:59:24Z

Sorry, didn't mean to post this quite so soon. Couple more fixes on the way.

philchristensen · 2024-09-24T20:53:46Z

Okay, this seems to be working for me. I'm open to any improvements, of course.

Gowiem

Instead of just introducing a new variable for backwards compatibility, I'm more of the mind that we create the breaking change and update eks_cluster_oidc_issuer_url to eks_cluster_oidc_issuer_urls and we'll rev the major version. Keeps things simple and this is why we have versioning.

Mind updating?

…er_urls

Gowiem · 2024-09-26T18:44:22Z

/terratest

Gowiem

@philchristensen 3 things:

Please run terraform format locally.
Run make init and make readme and then commit the result.
We need to update tests due to the below issue. Please check that out in examples/complete and fix the issue there and then re-request review from me.

Thanks!

=== NAME  TestExamplesComplete
    destroy.go:11: 
        	Error Trace:	/go/pkg/mod/github.com/gruntwork-io/terratest@v0.41.16/modules/terraform/destroy.go:11
        	            				/__w/terraform-aws-eks-iam-role/terraform-aws-eks-iam-role/test/src/examples_complete_test.go:15
        	            				/usr/local/go/src/runtime/panic.go:523
        	            				/usr/local/go/src/testing/testing.go:999
        	            				/go/pkg/mod/github.com/gruntwork-io/terratest@v0.41.16/modules/terraform/apply.go:15
        	            				/__w/terraform-aws-eks-iam-role/terraform-aws-eks-iam-role/test/src/examples_complete_test.go:46
        	Error:      	Received unexpected error:
        	            	FatalError{Underlying: error while running command: exit status 1; ╷
        	            	│ Error: Missing required argument
        	            	│ 
        	            	│   on main.tf line 13, in module "autoscaler_role":
        	            	│   13: module "autoscaler_role" {
        	            	│ 
        	            	│ The argument "eks_cluster_oidc_issuer_urls" is required, but no definition
        	            	│ was found.
        	            	╵
        	            	╷
        	            	│ Error: Unsupported argument
        	            	│ 
        	            	│   on main.tf line 23, in module "autoscaler_role":
        	            	│   23:   eks_cluster_oidc_issuer_url = "https://oidc.eks.us-west-2.amazonaws.com/id/FEDCBA9876543210FEDCBA9876543210"
        	            	│ 
        	            	│ An argument named "eks_cluster_oidc_issuer_url" is not expected here. Did
        	            	│ you mean "eks_cluster_oidc_issuer_urls"?
        	            	╵}
        	Test:       	TestExamplesComplete

philchristensen · 2024-09-26T20:15:03Z

🤦🏽 whoops. thanks for the review! BTW, is /terratest something I can run locally?

Gowiem · 2024-09-26T20:43:25Z

/terratest

Gowiem

@philchristensen tests are failing again (format on examples) + bats tests. Check them out + fix + re-request review -- Thanks!

philchristensen · 2024-09-26T20:51:32Z

I guess I never realized terraform fmt wasn't recursive. I think I got the last of them 🤞🏽

Gowiem · 2024-09-26T20:52:53Z

/terratest

Gowiem · 2024-09-26T21:01:52Z

@philchristensen I'm getting a 2nd set of eyes on this before we do the major rev -- we'll hopefully be able to merge soon 👍

GabisCampana · 2024-09-27T13:49:48Z

@Nuru

Nuru · 2024-09-27T19:32:14Z

Sorry, @philchristensen, but I completely disagree with @Gowiem. I do not want a breaking change because of the upstream effects. This module is used by other Cloud Posse modules, which would have to be modified, and all for no benefit to them.

We do not have this feature already because we create a separate role for each cluster rather than trying to reuse roles across clusters. Have you considered that approach? Because given the limitation on the size of the IAM trust policy, you can add only 4 OIDC providers with the default size limit and 8 with the max size limit. Alternately, you can use Pod Identities (very much supported by AWS, but not currently supported by Cloud Posse) which only need a single trust policy for all clusters.

Also terraform fmt -recursive . formats all the .tf files in the current directory and its sub-directories.

mergify · 2024-09-27T19:33:16Z

Important

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

Nuru

See other comments. I'm generally opposed to adding this feature, and recommend either creating a separate role per cluster or using Pod Identities which do not need separate trust policies per cluster. However, even if we want this feature, I believe this code is wrong.

Nuru · 2024-09-27T19:45:00Z

main.tf

+ dynamic "condition" {
+ for_each = local.eks_cluster_oidc_issuers
+ content {
+ test = "StringLike"
+ values = formatlist("system:serviceaccount:%s", local.service_account_namespace_name_list)
+ variable = format("%s:sub", condition.value)
+ }
 }
- condition {
- test = "StringEquals"
- values = ["sts.amazonaws.com"]
- variable = format("%s:aud", local.eks_cluster_oidc_issuer)
+
+ dynamic "condition" {
+ for_each = local.eks_cluster_oidc_issuers
+ content {
+ test = "StringEquals"
+ values = ["sts.amazonaws.com"]
+ variable = format("%s:aud", condition.value)
+ }
 }


I'm leaning against adding this feature altogether, but if we do add it, I'm pretty sure this is wrong. By adding a separate condition for each OIDC issuer, you are requiring all the ODICs to validate the role, when only one will. See Condition Logic for details.

I believe the correct solution is to loop over statement rather than conditions, and have on statement for each OIDC provider.

philchristensen · 2024-09-27T19:54:29Z

I know the policy limit you're referring to; this is actually a common problem for me. I support large Kubernetes for large organizations and it's common for me to have multiple clusters (say, per team or delivery area) in a single account.

To greatly reduce the size of the trust policy, I don't add IAM restrictions to the roles by namespace (only by cluster). Instead I use Kyverno on the cluster to validate whether a provided IAM role is allowed for a given namespace.

I need this capability quite substantially, so I'm glad to make the other changes @Nuru mentioned in hopes that we can come to some agreement on this feature. Thanks again for the help on this PR.

philchristensen · 2024-09-27T19:55:54Z

I'll also confirm whether I can use Pod Identities for this purpose instead. I thought I had confirmed this already but I really can't remember for sure...

feat: allow specifying multiple OIDC issuer URLs

fab9e18

philchristensen requested review from a team as code owners September 24, 2024 19:44

philchristensen requested review from Gowiem and RoseSecurity September 24, 2024 19:44

mergify bot added the triage Needs triage label Sep 24, 2024

fix: use correct condition convention for multiple OIDC URLs

91918aa

Gowiem requested changes Sep 26, 2024

View reviewed changes

feat!: replace eks_cluster_oidc_issuer_url with eks_cluster_oidc_issu…

3721845

…er_urls

philchristensen requested a review from Gowiem September 26, 2024 15:45

Gowiem changed the title ~~feat: allow specifying multiple OIDC issuer URLs~~ feat!: allow specifying multiple OIDC issuer URLs Sep 26, 2024

Gowiem added the major Breaking changes (or first stable release) label Sep 26, 2024

Gowiem requested changes Sep 26, 2024

View reviewed changes

philchristensen added 3 commits September 26, 2024 16:13

chore: fix formatting

0ae3ea6

test: use new param

f7e81d9

docs: updated README

4344d8e

philchristensen requested a review from Gowiem September 26, 2024 20:15

Gowiem requested changes Sep 26, 2024

View reviewed changes

chore: fix formatting

024d0df

philchristensen requested a review from Gowiem September 26, 2024 20:51

Gowiem requested a review from Nuru September 27, 2024 15:27

Nuru added do not merge Do not merge this PR, doing so would cause problems needs-cloudposse Needs Cloud Posse assistance and removed triage Needs triage labels Sep 27, 2024

Nuru requested changes Sep 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: allow specifying multiple OIDC issuer URLs #63

feat!: allow specifying multiple OIDC issuer URLs #63

philchristensen commented Sep 24, 2024 •

edited by Gowiem

Loading

philchristensen commented Sep 24, 2024

philchristensen commented Sep 24, 2024

Gowiem left a comment

Gowiem commented Sep 26, 2024

Gowiem left a comment

philchristensen commented Sep 26, 2024 •

edited

Loading

Gowiem commented Sep 26, 2024

Gowiem left a comment

philchristensen commented Sep 26, 2024

Gowiem commented Sep 26, 2024

Gowiem commented Sep 26, 2024

GabisCampana commented Sep 27, 2024

Nuru commented Sep 27, 2024

mergify bot commented Sep 27, 2024

Cloud Posse Engineering Team Review Required

Nuru left a comment

Nuru Sep 27, 2024

philchristensen commented Sep 27, 2024

philchristensen commented Sep 27, 2024

feat!: allow specifying multiple OIDC issuer URLs #63

Are you sure you want to change the base?

feat!: allow specifying multiple OIDC issuer URLs #63

Conversation

philchristensen commented Sep 24, 2024 • edited by Gowiem Loading

what

why

BREAKING CHANGE

philchristensen commented Sep 24, 2024

philchristensen commented Sep 24, 2024

Gowiem left a comment

Choose a reason for hiding this comment

Gowiem commented Sep 26, 2024

Gowiem left a comment

Choose a reason for hiding this comment

philchristensen commented Sep 26, 2024 • edited Loading

Gowiem commented Sep 26, 2024

Gowiem left a comment

Choose a reason for hiding this comment

philchristensen commented Sep 26, 2024

Gowiem commented Sep 26, 2024

Gowiem commented Sep 26, 2024

GabisCampana commented Sep 27, 2024

Nuru commented Sep 27, 2024

mergify bot commented Sep 27, 2024

Cloud Posse Engineering Team Review Required

Nuru left a comment

Choose a reason for hiding this comment

Nuru Sep 27, 2024

Choose a reason for hiding this comment

philchristensen commented Sep 27, 2024

philchristensen commented Sep 27, 2024

philchristensen commented Sep 24, 2024 •

edited by Gowiem

Loading

philchristensen commented Sep 26, 2024 •

edited

Loading