r/aws_eks_identity_provider_config - Adds a new resource for EKS OIDC Identity Provider Config

[willvrny]

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #17607

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSEksIdentityProviderConfig_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSEksIdentityProviderConfig_basic -timeout 120m
=== RUN   TestAccAWSEksIdentityProviderConfig_basic
=== PAUSE TestAccAWSEksIdentityProviderConfig_basic
=== CONT  TestAccAWSEksIdentityProviderConfig_basic
--- PASS: TestAccAWSEksIdentityProviderConfig_basic (3542.20s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	3544.777s

$ make testacc TESTARGS='-run=TestAccAWSEksIdentityProviderConfig_disappears'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSEksIdentityProviderConfig_disappears -timeout 120m
=== RUN   TestAccAWSEksIdentityProviderConfig_disappears
=== PAUSE TestAccAWSEksIdentityProviderConfig_disappears
=== CONT  TestAccAWSEksIdentityProviderConfig_disappears
--- PASS: TestAccAWSEksIdentityProviderConfig_disappears (3554.95s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	3558.570s

$ make testacc TESTARGS='-run=TestAccAWSEksIdentityProviderConfig_Oidc_Group'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSEksIdentityProviderConfig_Oidc_Group -timeout 120m
=== RUN   TestAccAWSEksIdentityProviderConfig_Oidc_Group
=== PAUSE TestAccAWSEksIdentityProviderConfig_Oidc_Group
=== CONT  TestAccAWSEksIdentityProviderConfig_Oidc_Group
--- PASS: TestAccAWSEksIdentityProviderConfig_Oidc_Group (2975.67s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	2978.136s

$ make testacc TESTARGS='-run=TestAccAWSEksIdentityProviderConfig_Oidc_Username'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSEksIdentityProviderConfig_Oidc_Username -timeout 120m
=== RUN   TestAccAWSEksIdentityProviderConfig_Oidc_Username
=== PAUSE TestAccAWSEksIdentityProviderConfig_Oidc_Username
=== CONT  TestAccAWSEksIdentityProviderConfig_Oidc_Username
--- PASS: TestAccAWSEksIdentityProviderConfig_Oidc_Username (3748.44s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	3751.742s

$ make testacc TESTARGS='-run=TestAccAWSEksIdentityProviderConfig_Oidc_RequiredClaims'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSEksIdentityProviderConfig_Oidc_RequiredClaims -timeout 120m
=== RUN   TestAccAWSEksIdentityProviderConfig_Oidc_RequiredClaims
=== PAUSE TestAccAWSEksIdentityProviderConfig_Oidc_RequiredClaims
=== CONT  TestAccAWSEksIdentityProviderConfig_Oidc_RequiredClaims
--- PASS: TestAccAWSEksIdentityProviderConfig_Oidc_RequiredClaims (3588.45s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	3591.398s

$ make testacc TESTARGS='-run=TestAccAWSEksIdentityProviderConfig_Tags'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSEksIdentityProviderConfig_Tags -timeout 120m
=== RUN   TestAccAWSEksIdentityProviderConfig_Tags
=== PAUSE TestAccAWSEksIdentityProviderConfig_Tags
=== CONT  TestAccAWSEksIdentityProviderConfig_Tags
--- PASS: TestAccAWSEksIdentityProviderConfig_Tags (3701.77s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	3704.366s

Questions:

1. I've not done a test per attribute as I'm seeing a single test take as long as 3832.718s. Is this okay or would you prefer a single test per attribute? I think the length is due to spinning up an EKS cluster and then I imagine the identity provider config requires rolling the master nodes.
2. The AWS SDK put the name of the identity provider inside the OIDC struct. I've mirrored this in the resource but I'm not sure if it would be more consistent to move it out and have it as a top level field in the resource like other similar resources?
3. I'm having to check an exception message for a particular case. This feels brittle but I can see this convention used in other areas. Is this okay?
4. I've tried to use the newer funcs that allow for passing in a context e.g. CreateContext to avoid using deprecated code. In the tests I've just used context.Background(). I haven't written too much Go so I'm not too sure if this is correct?
5. I've left the commits separate but I'm happy to rebase into a single commit?

[github-actions]

Thank you for your contribution! 🚀

Please note that typically Go dependency changes are handled in this repository by dependabot or the maintainers. This is to prevent pull request merge conflicts and further delay reviews of contributions. Remove any changes to the go.mod or go.sum files and commit them into this pull request.

Additional details:

• Check open pull requests with the dependencies label to view other dependency updates.
• If this pull request includes an update the AWS Go SDK (or any other dependency) version, only updates submitted via dependabot will be merged. This pull request will need to remove these changes and will need to be rebased after the existing dependency update via dependabot has been merged for this pull request to be reviewed.
• If this pull request is for supporting a new AWS service:
  • Ensure the new AWS service changes are following the Contributing Guide section on new services, in particular that the dependency addition and initial provider support are in a separate pull request from other changes (e.g. new resources). Contributions not following this item will not be reviewed until the changes are split.
  • If this pull request is already a separate pull request from the above item, you can ignore this message.

[github-actions]

Welcome @willvrny 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

[xabinapal]

Is there anything missing on this PR prior to removing the draft flag? I'd be happy to help if necessary, this would be a great feature to have!

[willvrny]

Hi @xabinapal. I was just hoping to get some clarification on my questions above. Aside from any changes from those answers I think functionality wise it's ready.

[jfdoube]

Any plan to release this feature soon ?

[ewbankkit]

@willvrny Thanks for the contribution 🎉 👏.
In response to your questions above:

1. The acceptance tests you have are sufficient given the length of time required to create all the pre-requisite resources
2. We always prefer to mimic the AWS API structure, so lets leave the IdP name where it is
3. Yes, checking AWS error message text is an unfortunate fact of life...
4. It looks like we prefer context.TODO() in the acceptance tests:

   terraform-provider-aws/aws/resource_aws_eks_addon_test.go

   Lines 87 to 93 in b063e4f

   	func TestAccAWSEksAddon_basic(t *testing.T) {
   	var addon eks.Addon
   	rName := acctest.RandomWithPrefix("tf-acc-test")
   	clusterResourceName := "aws_eks_cluster.test"
   	addonResourceName := "aws_eks_addon.test"
   	addonName := "vpc-cni"
   	ctx := context.TODO()

5. Don't worry about rebasing

[willvrny]

Thank you @ewbankkit. I've made the change to use context.TODO().

[alexandr-its]

Any plan to merge this feature? It will be great if I can use this feature

[ewbankkit]

@willvrny Once you remove [WIP] I can start review and get this one merged.

[willvrny]

@ewbankkit Done. Thank you.

[ewbankkit]

LGTM 🚀.

Commercial

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSEksIdentityProviderConfig_'     
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSEksIdentityProviderConfig_ -timeout 180m
=== RUN   TestAccAWSEksIdentityProviderConfig_basic
=== PAUSE TestAccAWSEksIdentityProviderConfig_basic
=== RUN   TestAccAWSEksIdentityProviderConfig_disappears
=== PAUSE TestAccAWSEksIdentityProviderConfig_disappears
=== RUN   TestAccAWSEksIdentityProviderConfig_AllOidcOptions
=== PAUSE TestAccAWSEksIdentityProviderConfig_AllOidcOptions
=== RUN   TestAccAWSEksIdentityProviderConfig_Tags
=== PAUSE TestAccAWSEksIdentityProviderConfig_Tags
=== CONT  TestAccAWSEksIdentityProviderConfig_basic
=== CONT  TestAccAWSEksIdentityProviderConfig_Tags
=== CONT  TestAccAWSEksIdentityProviderConfig_AllOidcOptions
=== CONT  TestAccAWSEksIdentityProviderConfig_disappears
--- PASS: TestAccAWSEksIdentityProviderConfig_AllOidcOptions (2208.99s)
--- PASS: TestAccAWSEksIdentityProviderConfig_disappears (2226.30s)
--- PASS: TestAccAWSEksIdentityProviderConfig_basic (2226.77s)
=== CONT  TestAccAWSEksIdentityProviderConfig_Tags        
--- PASS: TestAccAWSEksIdentityProviderConfig_Tags (2320.02s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	2323.155s

GovCloud

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSEksIdentityProviderConfig_' 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSEksIdentityProviderConfig_ -timeout 180m
=== RUN   TestAccAWSEksIdentityProviderConfig_basic
=== PAUSE TestAccAWSEksIdentityProviderConfig_basic
=== RUN   TestAccAWSEksIdentityProviderConfig_disappears
=== PAUSE TestAccAWSEksIdentityProviderConfig_disappears
=== RUN   TestAccAWSEksIdentityProviderConfig_AllOidcOptions
=== PAUSE TestAccAWSEksIdentityProviderConfig_AllOidcOptions
=== RUN   TestAccAWSEksIdentityProviderConfig_Tags
=== PAUSE TestAccAWSEksIdentityProviderConfig_Tags
=== CONT  TestAccAWSEksIdentityProviderConfig_basic
=== CONT  TestAccAWSEksIdentityProviderConfig_Tags
=== CONT  TestAccAWSEksIdentityProviderConfig_disappears
=== CONT  TestAccAWSEksIdentityProviderConfig_AllOidcOptions
--- PASS: TestAccAWSEksIdentityProviderConfig_basic (2123.50s)
--- PASS: TestAccAWSEksIdentityProviderConfig_disappears (2169.43s)
--- PASS: TestAccAWSEksIdentityProviderConfig_AllOidcOptions (2259.97s)
--- PASS: TestAccAWSEksIdentityProviderConfig_Tags (2292.37s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	2295.438s

[ewbankkit]

@willvrny Thanks for the contribution 🎉 👏.
In order to get this merged quicker I went ahead and made a few minor additions:

• add arn attribute
• tags can be updated
• add tags_all attribute

[willvrny]

Ah thank you.

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