-
Notifications
You must be signed in to change notification settings - Fork 282
Conversation
A reminder of a few items we discussed during the team sync today: I know @yorinasub17 had to spend some time on the following for K8S on AWS, so look into if you need to do the same for GCP:
Also, add to the docs a comment on why we have several cluster modules rather than 1 due to Terraform 0.11 limitations.
Unfortunately, the Terraform |
@brikis98 thanks Jim 👍 |
I'm curious if we need to support both I expect that we could defer zonal support until |
modules/gke-cluster/main.tf
Outdated
Get available container engine versions | ||
*****************************************/ | ||
data "google_container_engine_versions" "region" { | ||
zone = "${data.google_compute_zones.available.names[0]}" |
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 regional cluster doesn't necessarily support the same version(s) as any/all of its corresponding zones. We'll need to supply the region
parameter when working with regional clusters.
modules/gke-cluster/main.tf
Outdated
project = "${local.network_project_id}" | ||
} | ||
|
||
resource "random_shuffle" "available_zones" { |
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 for context, the reason this / the corresponding datasource are useful (https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/3f7527e583ffa07e6a06250844e07c38556a4488/cluster_regional.tf#L29) is because it lets you explicitly define which zones in the region your regional / multi-zonal (afaik regional supersedes multi-zonal) cluster resides in.
Co-Authored-By: robmorgan <[email protected]>
Remove extraneous line.
Add an initial node count to gke-cluster, explain why it's necessary.
[WIP] Add Terratest of the first example
…vate clusters. Minor wording changes.
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 realised I had some changes pending in a review pass I thought I'd submitted.
modules/gke-cluster/outputs.tf
Outdated
value = "${var.master_authorized_networks_config}" | ||
} | ||
|
||
output "kubernetes_dashboard_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.
This seems like an output we don't need, since the user will have supplied this variable anyways. Or is that standard convention?
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.
yeah can probably go, I must have copied it from the other repos.
modules/gke-cluster/outputs.tf
Outdated
value = "${google_container_cluster.cluster.min_master_version}" | ||
} | ||
|
||
output "logging_service" { |
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.
Same as dashboard - these are always user supplied (logging, monitoring).
value = "${google_container_cluster.cluster.region}" | ||
} | ||
|
||
output "endpoint" { |
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.
Can you add the outputs from https://github.com/gruntwork-io/terraform-google-gke/pull/10/files#diff-c49d0ae81c71a97455954c7c48bc0eca without the base64decode
step? I think they'll be necessary when integrating Tiller.
modules/gke-cluster/main.tf
Outdated
} | ||
} | ||
|
||
// TODO |
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.
This datasource is present at the bottom of the file.
# part of Create. This leaves us in our desired state- with a cluster master | ||
# with no node pools. | ||
remove_default_node_pool = true | ||
|
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.
To make it clear both lines are connected
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 terraform fmt
is causing me problems here at the moment
Co-Authored-By: rileykarson <[email protected]>
Co-Authored-By: robmorgan <[email protected]>
Integrate GKE cluster admin recommendations, clean up module
Add READMEs for gke-cluster, regional examples
…vate clusters. Minor wording changes.
Co-Authored-By: rileykarson <[email protected]>
add working circleci config
[WIP] Better cluster tests
Hi @rileykarson I'm going to merge this as is. I thought about removing the cluster cert & key outputs, but figured we can do this in the upcoming Helm work. I'll also change all of the other PR bases to |
merge ci changes from upstream
I intend to use this PR to discuss and design the initial module structure for the GKE package. The goal for release v0.0.1 is to write a Terraform module(s) with tests and examples that can create a GKE cluster.
Proposed Structure
The proposed directory layout is as follows:
Design Decisions
Open Questions
Should we have a parent moduleTerraform modules don't supportgke-cluster
that conditionally invokes submodules depending on the desired parameters?count
.Notes
Tasks
Terratest