-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Enhancement/eks adding service ipv4 range #15518
Enhancement/eks adding service ipv4 range #15518
Conversation
reran acceptance test (with newly added acceptance test and latest commit that sets minItems on list:
|
@anGie44 We're in need of this functionality too. The PR is looking good so far. Does anything else need to happen before it can be merged and released? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @philnichol, thank you for this PR! Off to a really great start 👍 A couple comments and suggestions to help address the current acctest failure and request for test coverage. If you have any questions or concerns, please reach out!
Co-authored-by: angie pinilla <[email protected]>
Co-authored-by: angie pinilla <[email protected]>
Co-authored-by: angie pinilla <[email protected]>
Co-authored-by: angie pinilla <[email protected]>
@anGie44 thanks for the feedback! All great points! I think I've addressed your comments but let me know if I've missed anything.
|
@anGie44 Thanks a lot for that initial review. Everything is looking good here. Do you have some time to give this another pass? |
we also in need for this functionality, please release asap. tks |
We need this functionality as well. How long will this take to be released? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @philnichol (and those following this feature 😄 )-- apologies for the delay here ! a couple final notes before we can merge this in 👍
@@ -113,6 +114,28 @@ func resourceAwsEksCluster() *schema.Resource { | |||
}, | |||
}, | |||
}, | |||
|
|||
"kubernetes_network_config": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @philnichol -- apologies for the delay ! i missed to note in my previous review that we should have parity with the matching data-source now that this field has been added in the resource so if you don't mind adding a similar block in data_source_aws_eks_cluster
would be greatly appreciated (as well add a line in the data-source test to check for the field against the resource i.e. using resource.TestCheckResourceAttrPair()
) ! Should be a quick addition with the corresponding fields set to Computed
. if you're crunched for time, we're also happy to help out 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @anGie44 , no worries at all. Happy to have a crack at the data_source, but if it takes a while I might well ask for your help since I don't want to hold people up 😄. Before making any changes I seem to be getting the following error on all datasource tests, but can't see what I'm doing wrong, any ideas? I get the same (resourceNotFound) regardless of which datasource test I run it seems.
TF_ACC=1 go test ./aws -v -count 1 -parallel 10 -run=TestAccAWSEksClusterDataSource_basic
=== RUN TestAccAWSEksClusterDataSource_basic
=== PAUSE TestAccAWSEksClusterDataSource_basic
=== CONT TestAccAWSEksClusterDataSource_basic
data_source_aws_eks_cluster_test.go:18: Step 1/1 error: terraform failed: exit status 1
stderr:
Error: error reading EKS Cluster (tf-acc-test-gs09q): ResourceNotFoundException: No cluster found for name: tf-acc-test-gs09q.
{
RespMetadata: {
StatusCode: 404,
RequestID: "91f46140-c965-4387-bddf-9834ad7beb69"
},
Message_: "No cluster found for name: tf-acc-test-gs09q."
}
--- FAIL: TestAccAWSEksClusterDataSource_basic (5.87s)
FAIL
FAIL github.com/terraform-providers/terraform-provider-aws/aws 5.909s
FAIL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh which version of Terraform do you have locally @philnichol ? I've come across these type of errors when using the family of 0.13
versions. I'd recommendv.0.12.29
or 0.14+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 sorry I didn't try that before, works now, will aim to address this and the documentation comment shortly, thanks @anGie44 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no worries - i don't think we have that behavior documented unfortunately ..sounds great 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anGie44 I've pushed my changes to the data source, let me know if there are any issues. Acceptance tests ran here:
$ TF_ACC=1 go test ./aws -v -count 1 -parallel 10 -run=TestAccAWSEksClusterDataSource_basic -timeout 60m
=== RUN TestAccAWSEksClusterDataSource_basic
=== PAUSE TestAccAWSEksClusterDataSource_basic
=== CONT TestAccAWSEksClusterDataSource_basic
--- PASS: TestAccAWSEksClusterDataSource_basic (998.81s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 998.846s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @philnichol -- test looks good on my end as well 👍 i'm running the resource acceptance tests as well in the background (I don't expect any changes)..and will comment back w/the PR approval
@@ -156,6 +156,7 @@ The following arguments are supported: | |||
* `vpc_config` - (Required) Nested argument for the VPC associated with your cluster. Amazon EKS VPC resources have specific requirements to work properly with Kubernetes. For more information, see [Cluster VPC Considerations](https://docs.aws.amazon.com/eks/latest/userguide/network_reqs.html) and [Cluster Security Group Considerations](https://docs.aws.amazon.com/eks/latest/userguide/sec-group-reqs.html) in the Amazon EKS User Guide. Configuration detailed below. | |||
* `enabled_cluster_log_types` - (Optional) A list of the desired control plane logging to enable. For more information, see [Amazon EKS Control Plane Logging](https://docs.aws.amazon.com/eks/latest/userguide/control-plane-logs.html) | |||
* `encryption_config` - (Optional) Configuration block with encryption configuration for the cluster. Only available on Kubernetes 1.13 and above clusters created after March 6, 2020. Detailed below. | |||
* `kubernetes_network_config` - (Optional) Configuration block with kubernetes network configuration for the cluster. Detailed below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also add a note here that if this block is removed from the configuration though, its essentially a no-op in the prior state so we can't do drift detection unless it's configured
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good shout, have done this, let me know if the phrasing is alright.
Co-authored-by: angie pinilla <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for this feature @philnichol 🚀
Output of acceptance tests:
--- PASS: TestAccAWSEksClusterAuthDataSource_basic (17.33s)
--- PASS: TestAccAWSEksCluster_EncryptionConfig (859.07s)
--- PASS: TestAccAWSEksCluster_Logging (895.26s)
--- PASS: TestAccAWSEksCluster_basic (919.82s)
--- PASS: TestAccAWSEksCluster_Tags (948.84s)
--- PASS: TestAccAWSEksClusterDataSource_basic (969.99s)
--- PASS: TestAccAWSEksCluster_VpcConfig_SecurityGroupIds (999.36s)
--- PASS: TestAccAWSEksCluster_VpcConfig_PublicAccessCidrs (1377.22s)
--- PASS: TestAccAWSEksCluster_VpcConfig_EndpointPublicAccess (1632.31s)
--- PASS: TestAccAWSEksCluster_NetworkConfig_ServiceIpv4Cidr (2127.25s)
--- PASS: TestAccAWSEksCluster_VpcConfig_EndpointPrivateAccess (2502.05s)
--- PASS: TestAccAWSEksCluster_Version (2609.50s)
This has been released in version 3.19.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! |
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! |
Community Note
Closes #15505
Release note for CHANGELOG:
Output from acceptance testing:
Adds a kubernetes_network_configuration block, currently the only field in the block is service_ipv4_cidr, which sets the IP range allocated to k8s services within a cluster