-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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 support for GKE (Google Container Engine) #2357
Conversation
Good stuff, I have wanted Kubernetes support for a long time. Here are some thoughts though: What's the benefit of using GKE to create the cluster instead of writing a tf config to do it? Then it would support update. Do you plan to use Terraform to deploy container workloads on the cluster after it is deployed? |
TF config for what exactly? API does not have edit endpoint, which means I can't edit any of the fields, e.g. I can't scale up/down
We can do that, but that would likely be a separate provider/resource using Kubernetes API, not GKE. Either way, we have to build the "Kubernetes API" somehow first (i.e. the underlying instances), before we start deploying things onto it, so I think GKE resource is valid. I only intend to cover GKE itself (i.e. the underlying infrastructure exposing Kubernetes API endpoint) in this PR. |
Correct me if I'm wrong but I thought GKE is just a quick way of getting some nodes up with the right software on them, but you can do that in Terraform using startup-script metadata (or maybe a baked image) and put it in terraform-community-modules. If you want to resize it, you can do so easily by changing the count attribute in Terraform. For users accustomed to the Google Developers Console or gcloud cmdline tool, there is obvious benefit in having an API endpoint that deploys the cluster in one step, but I don't see the benefit for a Terraform user. Is there anything the GKE API does that can't be done by a Terraform module? |
@sparkprime GKE is basically managed Kubernetes cluster - i.e. the obvious benefit of using GKE over building it via remote-exec provisioners / baked images etc. is less steps, less code, less work that needs to be done. The customer/user is different between GKE and custom-built Kubernetes cluster, GKE is something to start with for either demoing or dev env, nobody would probably run production workload (at least not in this stage today). My understanding is that it's supposed to be like AppEngine - i.e. you can build that platform & manage it yourself, but some people just like to delegate the maintenance problems to Google.
How do you know these two groups of users are separate? e.g. why should anyone who uses Terraform not use Console and visa-versa? |
Summarizing our conversation on IRC: There is value in both supporting Kubernetes directly in Terraform (as has already been done e.g. https://github.com/bobtfish/terraform-aws-coreos-kubernates-cluster and http://thepracticalsysadmin.com:8080/create-a-kubernetes-cluster-on-aws-and-coreos-with-terraform/ ) and supporting GKE as a specific resource as you're doing here. Any future Kubernetes provider ought to be configurable to target either a GKE deployment or a custom-built Kubernetes cluster running on any other cloud provider (including GCE). So I'll review this PR properly when I get some time |
There's also https://github.com/kelseyhightower/kubestack existing work. |
b01360e
to
04ba285
Compare
🔔 Ping @sparkprime - this is now ready for review. |
GKE reached beta today. Apparently, update function is also available, but only for changing the Kubernetes version: https://cloud.google.com/container-engine/reference/rest/v1/projects.zones.clusters/update |
Hello, |
Gonna review this week, while at a workshop so in dribs & drabs probably. |
}, | ||
|
||
"master_auth": &schema.Schema{ | ||
Type: schema.TypeSet, |
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.
Isn't the code simpler if you use a list (of length 1) 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.
I will see, but it just feels much more natural to use TypeSet
for these things. If you're actually right, then I believe the schema.Schema
needs much better way of defining singletons.
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.
Changed to TypeList
, which makes Schema.Set
returning 0
unnecessary and the casting is also slightly more readable (although it's arguable what's better here):
v.(*schema.TypeSet).List()
=> v.([]interface{})
Looking at https://cloud.google.com/container-engine/reference/rest/v1/projects.zones.clusters It looks like the v1 library version has a lot of trivial differences. Judging by that, it's probably worth avoiding merging a v1beta1 Terraform resource as we'll have to change it significantly quite soon after (breaking compatibility). How do you feel about updating this code to v1? |
User: masterAuth["username"].(string), | ||
}, | ||
Name: clusterName, | ||
NumNodes: int64(d.Get("number_of_nodes").(int)), |
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.
attribute does not match API
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 can change it to num_nodes
... although this is on the edge of readability to me (it doesn't feel natural when you're reading it), yet it's still acceptable.
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.
Changed in v1
to initial_node_count
according to new API naming convention.
That was my idea since that release became available :) I may have some time later today or by the end of this week to update it. Bear with me. |
28aaf10
to
64940be
Compare
b45522c
to
2f859d8
Compare
@sparkprime I have updated everything to v1 + see my comments above. Will you give it another look, please? |
@radeksimko Just some quick feedback: Tried this PR today, only used the basic functionality to create a cluster in an existing project using basic auth for the Kubernetes master and everything worked as advertised. Especially like the outputs, |
@simonvanderveldt Glad to hear that. Regarding exported arguments, I was thinking whether there would be any interest in base64-decoding auth key & certificates... Not sure what would be the use-case and whether the tool/resource taking this as input would more likely need base64-encoded strings or raw strings. Also it should be possible to take |
That's an interesting point, as we wanted to automatically access the Kubernetes API after we created the cluster to bootstrap the "contents" of the cluster with a replication controller. I'm not sure if it would be the right way for this module to export these values or that you'd simply have to define them as variables and use them in the resources that need them. Does that make sense?
Hadn't looked at this today, but you're correct. Seems like currently there is no way for GKE to directly create a new instance group or use an existing one. |
cluster.NodeConfig.MachineType = v.(string) | ||
} | ||
|
||
if v, ok = nodeConfig["disk_size_in_gb"]; ok { |
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.
should just be disk_size_gb
It's google style guide to suffix the type onto things, so you see things like timeout_ms and so on.
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
Basically LGTM though :) |
LGTM, will merge now |
Add support for GKE (Google Container Engine)
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Status
google_container_engine
google_container_engine
Test plan
Caveats
There's currently no API available for modifying existing cluster, therefore all attributes have to be
ForceNew=true
and the whole cluster has to be recreated all over again for any change.I believe updates will come soon, since GKE is still considered as beta.