Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for Node Group placement attributes #65

Closed
wants to merge 19 commits into from

Conversation

reixd
Copy link

@reixd reixd commented Apr 13, 2021

what

  • Add placement variable in order to specify optional placement parameters for the managed nodes (e.g. pin them to a single AZ).
  • When the placement variable is set, the module checks if an availability zone is specified, if so, then only the corresponding subnets are passed to the node_group module.

why

  • I want a managed node pool with additional placement restrictions, e.g. for EBS volume management.

Example

placement      = {
      availability_zone = "eu-west-1a"
}

module "eks_node_group" {
     #[....]
    name = "alice-pool-euw1a"
    placement = var.placement
     #[....]
}

references

@reixd reixd requested review from a team as code owners April 13, 2021 13:43
@reixd reixd requested a review from a team as a code owner April 13, 2021 13:44
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me other than one question.

cc @Nuru, since you typically have strong opinions on the EKS modules.

main.tf Outdated Show resolved Hide resolved
@Gowiem
Copy link
Member

Gowiem commented Apr 14, 2021

/test all

@Gowiem
Copy link
Member

Gowiem commented Apr 16, 2021

/test all

@Gowiem
Copy link
Member

Gowiem commented Apr 16, 2021

@reixd we're getting the following errors from our terratests -- Mind looking into it / giving your thoughts?

--- FAIL: TestExamplesComplete (25.13s)
    apply.go:15: 
        	Error Trace:	apply.go:15
        	            				examples_complete_test.go:82
        	Error:      	Received unexpected error:
        	            	FatalError{Underlying: error while running command: exit status 1; 
        	            	Error: Invalid for_each argument
        	            	
        	            	  on ../../main.tf line 60, in data "aws_subnet" "default":
        	            	  60:   for_each = toset(var.subnet_ids)
        	            	
        	            	The "for_each" value depends on resource attributes that cannot be determined
        	            	until apply, so Terraform cannot predict how many instances will be created.
        	            	To work around this, use the -target argument to first apply only the
        	            	resources that the for_each depends on.
        	            	}
        	Test:       	TestExamplesComplete

@Gowiem
Copy link
Member

Gowiem commented Apr 16, 2021

/test terratest

@reixd
Copy link
Author

reixd commented Apr 22, 2021

/test terratest

@Gowiem
Copy link
Member

Gowiem commented Apr 22, 2021

/test all

@Gowiem
Copy link
Member

Gowiem commented Apr 22, 2021

@reixd seems to be the same error:

--- FAIL: TestExamplesComplete (23.54s)
    apply.go:15: 
        	Error Trace:	apply.go:15
        	            				examples_complete_test.go:82
        	Error:      	Received unexpected error:
        	            	FatalError{Underlying: error while running command: exit status 1; 
        	            	Error: Invalid for_each argument
        	            	
        	            	  on ../../main.tf line 60, in data "aws_subnet" "default":
        	            	  60:   for_each = toset(var.subnet_ids)
        	            	
        	            	The "for_each" value depends on resource attributes that cannot be determined
        	            	until apply, so Terraform cannot predict how many instances will be created.
        	            	To work around this, use the -target argument to first apply only the
        	            	resources that the for_each depends on.
        	            	}
        	Test:       	TestExamplesComplete

@reixd
Copy link
Author

reixd commented Apr 23, 2021

@Gowiem still having this error. I am able to reproduce this error locally, however do not know how to fix it...
Related to hashicorp/terraform#26878
When using the module to install/configure an EKS cluster live it works ¯_(ツ)_/¯

@Gowiem
Copy link
Member

Gowiem commented Apr 26, 2021

@reixd ah interesting. Haven't seen that bug before. I will surface with the contributor team and figure out if we have a way to run tests against 0.14. I don't believe we do, but we should so we'll move this forward one way or another.

@Gowiem
Copy link
Member

Gowiem commented Apr 27, 2021

/test terratest

@Gowiem
Copy link
Member

Gowiem commented Apr 27, 2021

@reixd when you use it locally, what version of terraform are you using? If this introduces an issue which breaks compatibility with 0.13 then we'll need to assess if we want to move it forward or not.

@Gowiem Gowiem added terraform/0.13 Module requires Terraform 0.13 or later and removed terraform/0.14 labels Apr 27, 2021
@Gowiem
Copy link
Member

Gowiem commented Apr 27, 2021

/test terratest

@Gowiem
Copy link
Member

Gowiem commented Apr 27, 2021

@reixd can you relax the required_version for this module to < 0.15 and we'll give this a test run on 0.14?

@reixd
Copy link
Author

reixd commented Apr 27, 2021

@reixd when you use it locally, what version of terraform are you using?

I do use the latest 0.14 version, however for another reason: optinal variables.

@reixd
Copy link
Author

reixd commented Apr 27, 2021

@reixd can you relax the required_version for this module to < 0.15 and we'll give this a test run on 0.14?

Wilco

@Gowiem
Copy link
Member

Gowiem commented Apr 27, 2021

/test all

@Gowiem Gowiem added terraform/0.14 and removed terraform/0.13 Module requires Terraform 0.13 or later labels Apr 27, 2021
@Gowiem
Copy link
Member

Gowiem commented Apr 27, 2021

/test all

@Gowiem
Copy link
Member

Gowiem commented Apr 27, 2021

@reixd our unit tests don't like you:

Run # Some legacy support is on 0.11 branches and we determine the Terraform version based on the target branch name
++ cut -d/ -f1
+ VERSION=master
+ [[ master != \0\.\1\1 ]]
+ TF12=0.12.30
++ terraform-0.13 version --json
++ jq -r .terraform_version
+ TF13=0.13.6
++ terraform-0.14 version --json
++ jq -r .terraform_version
+ TF14=0.14.8
++ head -1
+++ jq -r '.required_core[]'
+++ terraform-config-inspect --json .
++ vert -s '>= 0.14.10, < 0.15.0' 0.12.30 0.13.6 0.14.8
+ VERSION=
+ [[ -n '' ]]
Error: Process completed with exit code 1.

That's because you updated to put the minimum version at 0.14.10, which it doesn't seem we're using yet... Can you update to what it was previously but just bump the maximum version to < 0.15

@reixd
Copy link
Author

reixd commented Apr 28, 2021

@reixd our unit tests don't like you:

Well if so, I do not like the tests either xD

@reixd
Copy link
Author

reixd commented Apr 28, 2021

Using TF 0.13 and changing the subnet IDs to the private ones:

-  subnet_ids         = module.subnets.public_subnet_ids
+  subnet_ids         = module.subnets.private_subnet_cidrs

I get the following error:

                Error Trace:    apply.go:15
                                                        examples_complete_test.go:82
                Error:          Received unexpected error:
                                FatalError{Underlying: error while running command: exit status 1; 
                                Error: InvalidSubnetID.NotFound: The subnet ID '172.16.0.0/19' does not exist
                                        status code: 400, request id: d5c860fc-ee36-48b2-90c9-cf0908789928
                                
                                  on ../../main.tf line 59, in data "aws_subnet" "default":
                                  59: data "aws_subnet" "default"

Maybe there is a problem with the public subnets?

Rainer 'rei' Schuth and others added 2 commits April 28, 2021 16:35
@mergify
Copy link

mergify bot commented May 13, 2021

This pull request is now in conflict. Could you fix it @reixd? 🙏

@mergify
Copy link

mergify bot commented Jun 15, 2021

This pull request is now in conflict. Could you fix it @reixd? 🙏

Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bridgecrew has found 0 infrastructure configuration error in this PR ⬇️

Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bridgecrew has found 0 infrastructure configuration error in this PR ⬇️

@mergify
Copy link

mergify bot commented Aug 21, 2021

This pull request is now in conflict. Could you fix it @reixd? 🙏

@Nuru Nuru mentioned this pull request Aug 29, 2021
@Nuru Nuru closed this in #84 Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants