Add the OIDC provider ARN to outputs

[eytanhanig]

PR o'clock

Description

Please explain the changes you made here and link to any relevant issues.

This PR adds the OICD Provider ARN to outputs when enable_irsa = true. This needed when creating IAM policy documents/roles for Service Accounts, an example of which can be seen here.

Note: The output is conditionally generated only when enable_irsa = true. When enable_irsa = false no output will be given, which is preferable to it returning a bad value, such as the empty string, which could then lead to misconfigured downstream resources.

Checklist

• Change added to CHANGELOG.md. All changes must be added and breaking changes and highlighted
• CI tests are passing
• README.md has been updated after any changes to variables and outputs. See https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

[jonathancolby-olx]

hi. did you see this ? #651

I personally like your usage of var.enable_irsa better. i can close mine.

what do you think about naming the output cluster_oidc_connect_provider_arn to make it appear next to cluster_oidc_issuer_url

[dpiddockcmp]

This isn't returning just the ARN. It's returning a list of aws_iam_openid_connect_provider outputs.

For example:

oidc = [
  {
    "arn" = "arn:aws:iam::111111111111:oidc-provider/oidc.eks.us-west-2.amazonaws.com/id/518F3C3FCEEB9B22064F6EE41A9BDC39"
    "client_id_list" = [
      "sts.amazonaws.com",
    ]
    "id" = "arn:aws:iam::111111111111:oidc-provider/oidc.eks.us-west-2.amazonaws.com/id/518F3C3FCEEB9B22064F6EE41A9BDC39"
    "thumbprint_list" = [
      "9e99a48a9960b14926bb7f3b02e22da2b0ab7280",
    ]
    "url" = "oidc.eks.us-west-2.amazonaws.com/id/518F3C3FCEEB9B22064F6EE41A9BDC39"
  },
]

Suggested change

	value = var.enable_irsa ? aws_iam_openid_connect_provider.oidc_provider : null
	value = var.enable_irsa ? aws_iam_openid_connect_provider.oidc_provider[0].arn : null

Tried with 0.12.9 and 0.12.18. Appears to work correctly. Having indexes in outputs scare me a little due to historical issues but it seems the lazy evaluation of the ternary operator is working now. Also tried manually deleting the resource, and running destroy on an empty state with enable_irsa = true.

Setting an output to null causes it to not be listed in the state's output. This may cause issues for people using the terraform_remote_state data source to chain configs together.

[eytanhanig]

Good catch and suggested change, I'll push an update. I'm also not a fan of how using the ternary operator plus count forces you to refer to an index and have been pondering whether switching to a for_each makes more sense, however the place to make that switch is where the resource is being created. Do you think that is the smarter way to do it?

Setting up terraform_remote_state will work regardless of whether enable_irsa is true or false, however anything referencing this output will fail when it is set to false. I believe that this is desirable behavior since an early Terraform error message is preferable to a resource being created with bad data downstream. Say for example that we use the empty string when enable_irsa = false and the user attempts to deploy a Kubernetes manifest with this value. The user would likely see no errors in Terraform and end up wasting a bunch of time trying to debug it as a Kubernetes problem.

[eytanhanig]

hi. did you see this ? #651

I personally like your usage of var.enable_irsa better. i can close mine.

@ jonathancolby-olx Apparently I missed it. I'm OK closing my PR as well. Let me know which you'd prefer.

what do you think about naming the output cluster_oidc_connect_provider_arn to make it appear next to cluster_oidc_issuer_url

I've been pondering the same thing. The issue is that nearly every output starting with cluster_ - including cluster_oidc_issuer_url - is referring to attributes associated with and output from the actual aws_eks_cluster resource. It ultimately comes down to two things:

1. Is the fact that it's related to the cluster implicit given what this module is?
2. If it isn't implicit, then would we rather have clarity (starts with cluster_), or precision (leave as above since it's not output from the aws_eks_cluster resource)?

[jonathancolby-olx]

hi. did you see this ? #651
I personally like your usage of var.enable_irsa better. i can close mine.

@ jonathancolby-olx Apparently I missed it. I'm OK closing my PR as well. Let me know which you'd prefer.

@eytanhanig - Let's go with yours. Your case for using conditional with enable_irsa is completely valid. I will close mine.

what do you think about naming the output cluster_oidc_connect_provider_arn to make it appear next to cluster_oidc_issuer_url

I've been pondering the same thing. The issue is that nearly every output starting with cluster_ - including cluster_oidc_issuer_url - is referring to attributes associated with and output from the actual aws_eks_cluster resource. It ultimately comes down to two things:

1. Is the fact that it's related to the cluster implicit given what this module is?
2. If it isn't implicit, then would we rather have clarity (starts with cluster_), or precision (leave as above since it's not output from the aws_eks_cluster resource)?

@eytanhanig - after thinking about this, I also tend to agree that the cluster_ prefix is redundant. I'm OK with how you did it too.

[max-rocket-internet]

NICE. Thanks @eytanhanig

[github-actions]

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.