feat: Allow users to add more Audiences to OpenID Connect

[cabrinha]

PR o'clock

Fixes #1145

Description

I'd like to add more audiences to the OpenID Connect Provider: #1145

Checklist

• README.md has been updated after any changes to variables and outputs. See https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

[github-actions]

Thank you for your contribution!

The CHANGELOG.md file contents are handled by the maintainers during merge. This is to prevent pull request merge conflicts.
Please see the Contributing Guide for additional pull request review items.

Remove any changes to the CHANGELOG.md file and commit them in this pull request.

[cabrinha]

@barryib can I get a review on this one?

[daroga0002]

I have studied #1145 and I understand pain. In shortcut EKS module is working here correctly for China region but 3rd party tooling using IRSA roles is not aware that China has local sts endpoint sts.amazonaws.com.cn. 3rd party tooling using IRSA roles are trying to authorise in sts.amazonaws.com which is not in openid audience so it is not working.

This change is just adding new possible variable which will solve a lot of issues for China (and GOV?) regions.

@antonbabenko I think it is ok to merge 👍

[antonbabenko]

Hi @cabrinha and thanks for the PR!

v17.5.0 has been just released.

[lisfo4ka]

@antonbabenko @daroga0002 @cabrinha folks, maybe it's too late to claim, but I assume that the name of the variable with the client id list should be something other than sts_principal since it can contain not only STS endpoint but any client ID that identifies the application. See an example, in the doc https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_openid_connect_provider
So I'm back to suggest the change from my PR #1557 to have the separate variable for a full list with client ids. What do you think about this?

[daroga0002]

if I understand correctly you want just to change a locals variable name?

I think as this PR was arleady merged and there was created release input variable should stay as is (openid_connect_audiences) but locals probably can be adjusted as this will make sense and will not cause any user facing changes.

[antonbabenko]

@lisfo4ka Isn't sts a required item in the context of this module? I mean, sometimes one may want to have another value but in this module, EKS always expects to have at least one sts... item, right?

[lisfo4ka]

@daroga0002 yes, you're right, I've meant just a local variable renaming.

But I see now that it's really not so important change since for the IRSA option the client id list will contain STS endpoints only. Thanks, @antonbabenko.

So let's leave as it is?

[antonbabenko]

We can rename locals name without any problems. client_id_list as a local name, and value wrapped with distinct() would be great. Please make a PR.

[lisfo4ka]

@antonbabenko @daroga0002 please, find the discussed changes in #1561.
I've tested it with the latest master.

[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.