-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
New Resource and Data Source: aws_eks_cluster #4749
Conversation
Also includes EKS Getting Started Guide
Wow this is the fastest I've seen an AWS service integrated into the terraform provider. Great work and thanks for the prompt update! |
FYI I'm just waiting for the official EKS User Guide to be published so I can verify some pieces of the getting started guide. Some pieces might have changed between preview and general availability. |
…rror found during acceptance testing
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.
Few comments, but nothing major, overall good job, LGTM!
}, | ||
}, | ||
}, | ||
"created_at": { |
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.
Is this field actually useful for anyone/anything? Not a big deal if not, but I thought historically we avoided encoding fields which aren't useful from user's perspective to the schema.
// InvalidParameterException: The provided role doesn't have the Amazon EKS Managed Policies associated with it. Please ensure the following policies [arn:aws:iam::aws:policy/AmazonEKSClusterPolicy, arn:aws:iam::aws:policy/AmazonEKSServicePolicy] are attached | ||
if isAWSErr(err, eks.ErrCodeInvalidParameterException, "The provided role doesn't have the Amazon EKS Managed Policies associated with it") { | ||
return resource.RetryableError(err) | ||
} |
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.
😭
} | ||
// Sometimes the EKS API returns the ResourceNotFound error in this form: | ||
// ClientException: No cluster found for name: tf-acc-test-0o1f8 | ||
if isAWSErr(err, eks.ErrCodeClientException, "No cluster found for name:") { |
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.
😭
aws/resource_aws_eks_cluster_test.go
Outdated
return nil | ||
} | ||
|
||
func TestAccAWSEksCluster_Basic(t *testing.T) { |
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.
Nitpick, but AFAIK all our tests use _basic
, not _Basic
.
|
||
conn := testAccProvider.Meta().(*AWSClient).eksconn | ||
|
||
// Handle eventual consistency |
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.
😭
manage or retrieve data from other AWS services. It is also possible to create | ||
these policies with the [`aws_iam_policy_document` data source](/docs/providers/aws/d/iam_policy_document.html) | ||
|
||
For the latest required policy, see the EKS User Guide. |
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.
Nitpick, but link to the EKS User Guide may be useful here.
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.
Ha! Great catch. I was waiting for the official URL. I'll add it.
locals { | ||
kubeconfig = <<KUBECONFIG | ||
|
||
|
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.
Are the two empty lines desired here?
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.
When I was developing the original demo configuration, the extra lines were helpful for distinguishing the "config" output from the "TF" output in the terminal. I don't think they'll hurt anything being in here still.
to allow worker nodes to join the cluster. It is also possible to create | ||
these policies with the [`aws_iam_policy_document` data source](/docs/providers/aws/d/iam_policy_document.html) | ||
|
||
For the latest required policy, see the EKS User Guide. |
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.
Likewise, link to the user guide would be nice to have.
data "aws_ami" "eks-worker" { | ||
filter { | ||
name = "name" | ||
values = ["eks-worker-*"] |
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.
Just curious - this is version agnostic intentionally? i.e. is the data source always going to fetch a valid AMI regardless of the K8S version (currently 1.10)?
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.
I think we can address this at a later time by updating the various pieces that might be version specific. 👍 Currently the guide is version agnostic.
|
||
While managing the underlying Kubernetes cluster configuration is beyond the | ||
scope of this guide, we provide an example of how to apply the required | ||
Kubernetes `ConfigMap` via `kubectl` below for completeness. |
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.
A link to Config Map would be nice to have here.
https://cloud.google.com/kubernetes-engine/docs/concepts/configmap
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.
http://kubernetes.io/docs/user-guide/configmap/ would be better IMO
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.
Awesome! Will add.
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.
FYI we'll update the link to the one from @errm post-merge 👍
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! |
Closes #2473
Changes proposed in this pull request:
aws_eks_cluster
aws_eks_cluster
Output from acceptance testing: