-
Notifications
You must be signed in to change notification settings - Fork 816
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
Add AKS, GKE and Helm terraform modules #756
Add AKS, GKE and Helm terraform modules #756
Conversation
Build Succeeded 👏 Build Id: e059442b-f8db-49dc-9d0a-85375bc3f713 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@@ -0,0 +1,115 @@ | |||
# Copyright 2019 Google LLC All Rights Reserved. |
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.
Have you thought about breaking this into multiple files to separate inputs / resources / outputs as per https://www.terraform-best-practices.com/code-structure#getting-started-with-structuring-of-terraform-configurations?
Thanks for the update. I will break down tf config files and update this PR. Also I want to create prod and dev configurations, |
d84ff9a
to
9259d65
Compare
Add more natural variables and outputs which make me able to have one helm terraform configuration used by both GKE and AKS. |
Build Succeeded 👏 Build Id: 96342f3f-2da8-45ee-82a7-f3c13d7cf1d3 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
9259d65
to
7c875c1
Compare
Build Succeeded 👏 Build Id: 6a485ddd-6005-43bb-b9d9-536801f6e562 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
adfd47f
to
b39e682
Compare
Build Succeeded 👏 Build Id: a785dc23-5214-4af4-9738-697f23d63ad9 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: ab16cc8a-8a86-4cfc-be9c-d57c4aa1ef43 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
b39e682
to
d05469c
Compare
Build Succeeded 👏 Build Id: b355cd8d-1ee2-4abe-83df-c06deb43b176 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
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 for the high latency on reviews. I had a few more comments, but overal lgtm.
@@ -12,10 +12,10 @@ description: > | |||
- Terraform v0.11.13 | |||
- [Helm](https://docs.helm.sh/helm/) package manager 2.10.0+ | |||
- Access to Google Cloud Kubernetes Engine | |||
- `gcloud` utility installed | |||
- `gcloud` or `az` utility installed |
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.
The above line says "Google Cloud Kubernetes Engine" so I think this could either be written as:
"Access to the the Kubernetes hosting provider you are using (e.g. gcloud
or az
utility installed)"
or
"Access to Google Cloud Kubernetes Engine (gcloud
utilitity installed) or Azure Kubernetes Service (az
utility installed)".
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 reworded this lines and split specific requirements for subsequent sections.
@@ -12,10 +12,10 @@ description: > | |||
- Terraform v0.11.13 | |||
- [Helm](https://docs.helm.sh/helm/) package manager 2.10.0+ | |||
- Access to Google Cloud Kubernetes Engine |
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.
Regarding the following comment, I'm not sure what this line is supposed to mean. Does this mean that you have a project and the api enabled?
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.
Yes, that would be much better description, I will split this into two parts, one for GKE and one for AKS
build/modules/gke/cluster.tf
Outdated
password = "${var.password}" | ||
} | ||
enable_legacy_abac = "${lookup(var.cluster, "legacyAbac")}" | ||
node_pool = [ |
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.
After reading the docs about how terraform handles updates to node pools, I think it would be better to make each node pool a separate resource.
From https://www.terraform.io/docs/providers/google/r/container_cluster.html#node_pool:
Warning: node pools defined inside a cluster can't be changed (or added/removed) after cluster creation without deleting and recreating the entire cluster. Unless you absolutely need the ability to say "these are the only node pools associated with this cluster", use the google_container_node_pool resource instead of this property.
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.
That would make cluster creation a bit longer, about 10 minutes compared to 3 minutes now.
But regarding this warning it seems to be inevitable.
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.
Note that after this change I faced issue with cluster starts upgrading after I attach 2 additional node_pools to it.
hashicorp/terraform-provider-google#3385
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.
Fixed the issue as proposed in link I posted above with:
remove_default_node_pool = true
And now we have 3 node pools and helm would install Agones right after creation of all node pools.
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 agree that cluster creation is much slower. It's really annoying that the terraform implementation for GKE requires jumping through these hoops. :/
} | ||
} | ||
|
||
/* |
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.
Delete these commented out lines?
d05469c
to
6f30051
Compare
Build Succeeded 👏 Build Id: c6d188a5-9f31-46ad-ba10-fe121055dbb2 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
6f30051
to
fc6f1ee
Compare
Build Succeeded 👏 Build Id: d1d2f628-a5a8-407c-b311-2608faa54ebf The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
fc6f1ee
to
753e133
Compare
Build Succeeded 👏 Build Id: 659a23b9-d648-4ddf-b9d0-9ee7cf73dc81 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@roberthbailey, thanks for the review. Applied all your comments, in a separate PR I am going to remove old not module version of tf config which is left in terraform.mk. |
Thanks! @markmandel / @jkowalski - can one of you take a quick look for approval? |
Add configuration for deploying cluster and installing Agones on AKS.
753e133
to
6291452
Compare
Build Succeeded 👏 Build Id: f7fcaf20-0ffd-4aa4-b076-6aff2d01cfe0 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
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.
Did a quick look through - LGTM!
Build Succeeded 👏 Build Id: f6d74c56-a29d-4ca0-acd3-994efddf2444 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Add configuration for deploying cluster and installing Agones on AKS.
Next step would be to move common modules into
/build/modules/
from/build
and changeterraform.mk
make targets to support this. As you can see there is a small difference betweenhelm.tf
for GKE and AKS. So need to create common output to combine them into one helm.tf file.Tested that this configuration works on Azure free account. AWS does not support EKS on free tier.
For #657 .