Add eks addon resource, data_source #16972

juozasget · 2021-01-05T11:09:34Z

This is my first contribution - feedback is very much welcome.

Thank you!

UPDATED & removed WIP mention

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

Release note for CHANGELOG:

data-source/aws_eks_addon: new data-source
resource/aws_eks_addon: new resource

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSEksAddon*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSEksAddon* -timeout 120m
=== RUN   TestAccAWSEksAddonDataSource_basic
=== PAUSE TestAccAWSEksAddonDataSource_basic
=== RUN   TestAccAWSEksAddon_basic
=== PAUSE TestAccAWSEksAddon_basic
=== RUN   TestAccAWSEksAddon_disappears
=== PAUSE TestAccAWSEksAddon_disappears
=== RUN   TestAccAWSEksAddon_disappears_Cluster
=== PAUSE TestAccAWSEksAddon_disappears_Cluster
=== RUN   TestAccAWSEksAddons_AddonVersion
=== PAUSE TestAccAWSEksAddons_AddonVersion
=== RUN   TestAccAWSEksAddons_ResolveConflicts
=== PAUSE TestAccAWSEksAddons_ResolveConflicts
=== RUN   TestAccAWSEksAddons_ServiceAccountRoleArn
=== PAUSE TestAccAWSEksAddons_ServiceAccountRoleArn
=== RUN   TestAccAWSEksAddons_Tags
=== PAUSE TestAccAWSEksAddons_Tags
=== CONT  TestAccAWSEksAddonDataSource_basic
=== CONT  TestAccAWSEksAddons_ResolveConflicts
=== CONT  TestAccAWSEksAddon_disappears_Cluster
=== CONT  TestAccAWSEksAddons_AddonVersion
=== CONT  TestAccAWSEksAddon_basic
=== CONT  TestAccAWSEksAddon_disappears
=== CONT  TestAccAWSEksAddons_ServiceAccountRoleArn
=== CONT  TestAccAWSEksAddons_Tags
--- PASS: TestAccAWSEksAddons_AddonVersion (848.97s)
--- PASS: TestAccAWSEksAddon_basic (889.61s)
--- PASS: TestAccAWSEksAddonDataSource_basic (938.24s)
--- PASS: TestAccAWSEksAddons_ServiceAccountRoleArn (977.66s)
--- PASS: TestAccAWSEksAddon_disappears (981.65s)
--- PASS: TestAccAWSEksAddon_disappears_Cluster (986.26s)
--- PASS: TestAccAWSEksAddons_ResolveConflicts (1118.40s)
--- PASS: TestAccAWSEksAddons_Tags (1412.16s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1413.873s
...

github-actions

Welcome @juozasget 👋

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! 😃

aws/resource_aws_eks_addon.go

- Add validation func for resolve_conflicts - Add idempotency token for create and update requests

juozasget · 2021-01-11T12:55:21Z

@shuheiktgw @bflad Thank you very much for your initial comments.

I have updated original post with newly added ACC test outputs along with other missing things. I think the PR is now in good shape for a review. Things I'd like to point out:

Issue Support Managing EKS Cluster Add-Ons #16542 is only for resource, and this PR adds a data_source also. Let me know if you think it's not needed at all or if it should be moved and not done in scope of this PR.
There are some interesting interactions to consider when creating EKS Addon, when unmanaged version exists or when updating the addon and there are conflicts. Would definitely like some feedback on this part and if we should make some decision for the user and handle it or we should just throw an error and let the user handle it (current implementation).

I also left some TODO items, which would be nice to do/improve. I will try to work on them while waiting for feedback. 🔧
Thank you for waiting!

juozasget · 2021-01-30T21:55:22Z

Hello. I've addressed the TODO things and improved the PR, mainly in these areas:

Added waiter package and moved relevant code there.
Fixed error handling according to newly released guidelines.
Improved error handling in the area I mentioned when add-on creation/update fails with information/warning for user.

I have been marinating this for 2-3 weeks, because after these improvements broke test case for TestAccAWSEksAddons_Tags. I've been pulling my hair out trying to find what could have caused this. I believe I've finally found it. Here are the details:

I added aws_eks_addon to aws_eks_cluster deps:
resource.AddTestSweepers("aws_eks_cluster", &resource.Sweeper{ Name: "aws_eks_cluster", F: testSweepEksClusters, Dependencies: []string{ "aws_eks_fargate_profile", "aws_eks_fargate_node_group", }, })

This caused the eks add-on deletion api to be called, before eks cluster deletion. Previously it would just delete the eks cluster. I believe that there might be some weird bug that will cause eks add-on to get stuck in DELETING state if the eks add-on had it's TAG values modified. This only happens if tag values were modified/set.

Create EKS addon cia CLI with some tags.

Modify the tags/put new tags on the created EKS addon via CLI.

Delete the EKS addon.

Result: addon is stuck in DELETING state for 24+ hours. Example of 1 hours DELETION attached.

Opened question in AWS forum. Sadly I don't think there is much chance of getting an answer...
https://forums.aws.amazon.com/thread.jspa?threadID=334229&tstart=0

I've since removed it from sweeper dependencies and all test are passing. Not sure what to do about this.

…hanges()

NeckBeardPrince · 2021-03-04T23:27:48Z

Any chance we can review this PR again? Please.

DonBower · 2021-03-19T16:47:43Z

is this going to be merged?

williamcodes · 2021-03-29T14:21:56Z

Can we get this merged in? It's impossible to configure a fully functioning EKS cluster with managed nodes using only terraform right now because you can't install the VPC CNI addon.

Who needs to code review this? @shuheiktgw @bflad

gdavison

Thanks for the PR, @juozasget. I've made some updates

Acceptance test results

--- PASS: TestAccAWSEksAddon_basic (772.00s)
--- PASS: TestAccAWSEksAddon_ServiceAccountRoleArn (830.59s)
--- PASS: TestAccAWSEksAddonDataSource_basic (857.39s)
--- PASS: TestAccAWSEksAddon_Tags (885.01s)
--- PASS: TestAccAWSEksAddon_disappears_Cluster (891.55s)
--- PASS: TestAccAWSEksAddon_disappears (946.00s)
--- PASS: TestAccAWSEksAddon_ResolveConflicts (976.91s)
--- PASS: TestAccAWSEksAddon_AddonVersion (989.17s)

gdavison · 2021-04-12T18:30:13Z

aws/data_source_aws_eks_addon.go

+			"addon_name": {
+				Type:         schema.TypeString,
+				Required:     true,
+				ForceNew:     true,


ForceNew isn't needed on data sources

gdavison · 2021-04-12T18:37:49Z

aws/data_source_aws_eks_addon.go

+
+func dataSourceAwsEksAddon() *schema.Resource {
+	return &schema.Resource{
+		ReadContext: dataSourceAwsEksAddonRead,


We're still working out the interactions between timeouts handled by the Terraform Context and the AWS API. To work around the interactions, we use the <operation>WithoutTimeout functions instead of <operation>Context functions.

Suggested change

ReadContext: dataSourceAwsEksAddonRead,

ReadWithoutTimeout: dataSourceAwsEksAddonRead,

gdavison · 2021-04-12T18:56:49Z

aws/data_source_aws_eks_addon_test.go

+		PreCheck:          func() { testAccPreCheck(t); testAccPreCheckAWSEks(t); testAccPreCheckAWSEksAddon(t) },
+		ProviderFactories: testAccProviderFactories,


We've added a parameter to handle acceptance test errors in a cleaner manner: https://github.com/hashicorp/terraform-provider-aws/blob/main/docs/contributing/running-and-writing-acceptance-tests.md#errorchecks

Suggested change

PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSEks(t); testAccPreCheckAWSEksAddon(t) },

ProviderFactories: testAccProviderFactories,

PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSEks(t); testAccPreCheckAWSEksAddon(t) },

ErrorCheck: testAccErrorCheck(t, eks.EndpointsID),

ProviderFactories: testAccProviderFactories,

gdavison · 2021-04-12T18:58:39Z

website/docs/r/eks_addon.html.markdown

+
+## Example Usage
+
+```hcl


We prefer using terraform as the language name now

Suggested change

```hcl

```terraform

gdavison · 2021-04-12T19:15:23Z

aws/resource_aws_eks_addon_test.go

+		PreCheck:          func() { testAccPreCheck(t); testAccPreCheckAWSEks(t); testAccPreCheckAWSEksAddon(t) },
+		ProviderFactories: testAccProviderFactories,


We've added a parameter to handle acceptance test errors in a cleaner manner: https://github.com/hashicorp/terraform-provider-aws/blob/main/docs/contributing/running-and-writing-acceptance-tests.md#errorchecks

Suggested change

PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSEks(t); testAccPreCheckAWSEksAddon(t) },

ProviderFactories: testAccProviderFactories,

PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSEks(t); testAccPreCheckAWSEksAddon(t) },

ErrorCheck: testAccErrorCheck(t, eks.EndpointsID),

ProviderFactories: testAccProviderFactories,

ghost · 2021-04-16T16:35:28Z

This has been released in version 3.37.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2021-05-13T17:10:12Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

juozasget requested a review from a team as a code owner January 5, 2021 11:09

ghost added size/XL Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. service/eks Issues and PRs that pertain to the eks service. labels Jan 5, 2021

github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Jan 5, 2021

github-actions bot reviewed Jan 5, 2021

View reviewed changes

shuheiktgw reviewed Jan 5, 2021

View reviewed changes

aws/resource_aws_eks_addon.go Outdated Show resolved Hide resolved

shuheiktgw reviewed Jan 5, 2021

View reviewed changes

aws/resource_aws_eks_addon.go Show resolved Hide resolved

shuheiktgw reviewed Jan 5, 2021

View reviewed changes

aws/resource_aws_eks_addon.go Outdated Show resolved Hide resolved

juozasget added 5 commits January 5, 2021 21:33

Add eks addon resource, data_source

b1f3b57

Unify erro message, UpdateID actually not used

9179b04

Address feedback

e3efd14

- Add validation func for resolve_conflicts - Add idempotency token for create and update requests

Add missing attributes: status, created_at, modified_at

0edeeb1

Add fixes, ACC tests (passing), documentation

a9b9c61

juozasget requested review from bflad and removed request for a team January 11, 2021 13:00

juozasget added 2 commits January 12, 2021 22:14

Fix import grouping, typo and remove unnecessary IAM Role attachement

b1f833c

Add waiter package, improve error handling, other fixes

2582f98

Base automatically changed from master to main January 23, 2021 01:00

Remove eks addon from eks cluster sweeper dependencies

34be4e2

Combine multiple ResourceData.HasChange() calls to single single HasC…

0d232c2

…hanges()

billinghamj mentioned this pull request Feb 20, 2021

Support Managing EKS Cluster Add-Ons #16542

Closed

NeckBeardPrince mentioned this pull request Mar 4, 2021

EKS Managed Cluster Addons terraform-aws-modules/terraform-aws-eks#1137

Closed

4 tasks

gdavison self-assigned this Apr 12, 2021

This was referenced Apr 12, 2021

Consolidate EKS update waiter function #18805

Closed

Improve EKS cluster name validation #18806

Closed

gdavison approved these changes Apr 13, 2021

View reviewed changes

gdavison added a commit that referenced this pull request Apr 13, 2021

Merge pull request #16972 from juozasget/eks-cluster-addons

d03950a

gdavison merged commit 171a9da into hashicorp:main Apr 13, 2021

github-actions bot added this to the v3.37.0 milestone Apr 13, 2021

gdavison mentioned this pull request Apr 15, 2021

resource/aws_eks_addon: Add default tags support #18871

Merged

ghost locked as resolved and limited conversation to collaborators May 13, 2021

breathingdust removed the needs-triage Waiting for first response or review from a maintainer. label Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add eks addon resource, data_source #16972

Add eks addon resource, data_source #16972

juozasget commented Jan 5, 2021 •

edited

Loading

github-actions bot left a comment

juozasget commented Jan 11, 2021

juozasget commented Jan 30, 2021 •

edited

Loading

NeckBeardPrince commented Mar 4, 2021

DonBower commented Mar 19, 2021

williamcodes commented Mar 29, 2021

gdavison left a comment

gdavison Apr 12, 2021

gdavison Apr 12, 2021

gdavison Apr 12, 2021

gdavison Apr 12, 2021

gdavison Apr 12, 2021

ghost commented Apr 16, 2021

ghost commented May 13, 2021

	ReadContext: dataSourceAwsEksAddonRead,
	ReadWithoutTimeout: dataSourceAwsEksAddonRead,

		PreCheck: func() { testAccPreCheck(t); testAccPreCheckAWSEks(t); testAccPreCheckAWSEksAddon(t) },
		ProviderFactories: testAccProviderFactories,

Add eks addon resource, data_source #16972

Add eks addon resource, data_source #16972

Conversation

juozasget commented Jan 5, 2021 • edited Loading

Community Note

github-actions bot left a comment

Choose a reason for hiding this comment

juozasget commented Jan 11, 2021

juozasget commented Jan 30, 2021 • edited Loading

NeckBeardPrince commented Mar 4, 2021

DonBower commented Mar 19, 2021

williamcodes commented Mar 29, 2021

gdavison left a comment

Choose a reason for hiding this comment

gdavison Apr 12, 2021

Choose a reason for hiding this comment

gdavison Apr 12, 2021

Choose a reason for hiding this comment

gdavison Apr 12, 2021

Choose a reason for hiding this comment

gdavison Apr 12, 2021

Choose a reason for hiding this comment

gdavison Apr 12, 2021

Choose a reason for hiding this comment

ghost commented Apr 16, 2021

ghost commented May 13, 2021

juozasget commented Jan 5, 2021 •

edited

Loading

juozasget commented Jan 30, 2021 •

edited

Loading