gcp basis #1918

enxebre · 2017-09-15T07:08:20Z

This is a follow up for the work started here #531

It allows to consistently recreate and destroy a functional Tectonic cluster on Google Cloud Compute with three masters, three etcd nodes and TLS support.
It allows to consistently deploy and access a simple app in the form of pod + service + ingress.
It adds basic smoke testing for GCP
It runs with Terraform v0.10.4 and use all the common/cloud abstracted modules in line with the other providers.

coreosbot · 2017-09-15T07:08:22Z

Can one of the admins verify this patch?

coreosbot · 2017-09-15T07:08:22Z

Can one of the admins verify this patch?

coreosbot · 2017-09-15T07:08:22Z

Can one of the admins verify this patch?

s-urbaniak

Looking great so far, thanks! Just a few comments/refactoring suggestions. 👍

s-urbaniak · 2017-09-15T07:32:33Z

modules/google/etcd/dns.tf

@@ -0,0 +1,45 @@
+/*


do you mind to move all dns related bootstrapping to modules/dns? That way we make the DNS story swappable, as this is requested by some users.

Hey @s-urbaniak thanks for the comments! I'm looking at the refactoring stuff but may be better leave it for a follow up PR? This is something functional now and refactoring can lead to difficult to anticipate dependency issues which can affect to other clouds which can lead to other problems out of the scope of having an "alpha" gcp version in place which should be the target here. What do you think?

I'd like to avoid platform skew as much as possible. We loosened constraints in the past for other platforms and it turned out to be a big mistake, especially in the light of alpha platforms. Being an alpha platform we can loosen the constraint on test coverage but I would like to avoid to loosen the constraint on general architectural compliance.

More specifically we are working on enabling optional modules. If we leave GCP behind (as we already did with modules/aws which still has its internal DNS implementation) re-priorization of tasks could slip tech-debt.

tldr: I'd really advocate for having dns logic in modules/dns/gcp. Sorry if this complicates things.

s-urbaniak · 2017-09-15T07:41:05Z

modules/google/etcd/ignition.tf

+  --initial-advertise-peer-urls=${var.tls_enabled ? "https" : "http"}://${var.cluster_name}-etcd-${count.index}.${var.base_domain}:2380 \
+  --listen-client-urls=${var.tls_enabled ? "https" : "http"}://0.0.0.0:2379 \
+  --listen-peer-urls=${var.tls_enabled ? "https" : "http"}://0.0.0.0:2380
+EOF


note to myself: we really need to unify this. We already have done this for the kubelet, but etcd is a complex service and needs one place to be configured. Leave it here for now, we will tackle this as a follow-up.

PS: this is already planned for the next sprint.

s-urbaniak · 2017-09-15T07:41:36Z

modules/google/etcd/outputs.tf

+  value = ["${split(",", length(var.external_endpoints) == 0 ? join(",", google_dns_record_set.etc_a_node.*.name) : join(",", var.external_endpoints))}"]
+}
+
+# vim: ts=2:sw=2:sts=2:et:ai


no strong feelings, but I'd like to keep editor-specific metadata out of the files.

s-urbaniak · 2017-09-15T07:42:07Z

modules/google/master-igm/master.tf

+  base_instance_name = "mstr"
+}
+
+# vim: ts=2:sw=2:sts=2:et:ai


see nit above

s-urbaniak · 2017-09-15T07:42:23Z

modules/google/master-igm/outputs.tf

+  value = ["${google_compute_instance_group_manager.tectonic-master-igm.*.instance_group}"]
+}
+
+# vim: ts=2:sw=2:sts=2:et:ai


s-urbaniak · 2017-09-15T07:46:20Z

modules/google/master-igm/resources/services/init-assets.service

+
+[Install]
+WantedBy=multi-user.target
+RequiredBy=bootkube.service kubelet-env.service


this is a carbon-copy of modules/aws/master-asg/resources/services/init-assets.service. As we use the very same thing here, this becomes a candidate for unification.

Can you please move this to modules/ignition/resources/services and dependency inject it as the other ignition artifacts?

s-urbaniak · 2017-09-15T07:49:12Z

modules/google/network/loadbalancer.tf

+  port_range            = "443"
+}
+
+resource "google_dns_record_set" "api-external" {


same comment as above for etcd. let's bootstrap all dns records in modules/dns/google.

s-urbaniak · 2017-09-15T07:51:04Z

platforms/gcp/gcs.tf

@@ -0,0 +1,42 @@
+/*


just a naming nit/question: in platforms we call it platforms/gcp, in modules we call this modules/google. seems impedance mismatch'y a little bit, no?

coreosbot · 2017-09-19T08:17:54Z

Can one of the admins verify this patch?

coreosbot · 2017-09-19T08:17:54Z

Can one of the admins verify this patch?

coreosbot · 2017-09-19T08:17:54Z

Can one of the admins verify this patch?

s-urbaniak

LGTM on first eye-sight, but referring to @alexsomesan for a final opinion.

alexsomesan

Had a partial look and am putting down some of the comment while I go through the rest of it, so we can speed up the process.

alexsomesan · 2017-09-20T15:40:51Z

Documentation/variables/gcp.md

+|------|-------------|:----:|:-----:|
+| google_managedzone_name | GCP resource name of Cloud DNS ManagedZone - created outside of Tectonic | string | `my-managed-zone` |
+| tectonic_dns_prefix_name | DNS prefix used to construct the console and API server endpoints. | string | `` |
+| tectonic_etcd_nodes_max | Upper limit for auto-scaling, per zone. | string | `2` |


Unless I'm missing something this is not being used anywhere in the current change set.

Also, is this intended to configure autoscaling for etcd nodes? Can't really make that out from the description.

alexsomesan · 2017-09-20T15:43:39Z

Documentation/variables/gcp.md

+| Name | Description | Type | Default |
+|------|-------------|:----:|:-----:|
+| google_managedzone_name | GCP resource name of Cloud DNS ManagedZone - created outside of Tectonic | string | `my-managed-zone` |
+| tectonic_dns_prefix_name | DNS prefix used to construct the console and API server endpoints. | string | `` |


This seems to be a GCP specific setting. It should be infixed with _gcp_ just like the other ones introduced in this file.

alexsomesan · 2017-09-20T15:44:30Z

Documentation/variables/gcp.md

+
+| Name | Description | Type | Default |
+|------|-------------|:----:|:-----:|
+| google_managedzone_name | GCP resource name of Cloud DNS ManagedZone - created outside of Tectonic | string | `my-managed-zone` |


Why doesn't this one have a tectonic_gcp_ prefix?

All installer inputs have to be namespaced (at least) under tectonic_. If it refers to an externally provisioned resource we usually denote that by including something like _external_ or _ext_.

See here, for example: https://github.com/coreos/tectonic-installer/blob/master/platforms/aws/variables.tf#L80

alexsomesan · 2017-09-20T15:49:23Z

Documentation/variables/gcp.md

+| tectonic_gcp_worker_disktype | The type of disk (pd-standard or pd-ssd) for the worker nodes. | string | `pd-standard` |
+| tectonic_gcp_worker_gce_type | Instance size for the worker node(s). Example: `n1-standard-2`. | string | `n1-standard-2` |
+| tectonic_gcp_zones | List of two or more zones to use from specified GCP region. | list | `<list>` |
+| tectonic_masters_max | Upper limit for auto-scaling, per zone. | string | `2` |


_gcp_ platform infix? Same for the one below.

Hey @alexsomesan thanks for pointing that out, just removed the stale variables and fixed namespacing

alexsomesan

OK, I went through the rest of it and left some more comments.

One general question on top: how does SSH-ing into the nodes work? How would the user specify the key? We need that option for debugging the cluster.

alexsomesan · 2017-09-21T16:19:57Z

platforms/gcp/variables.tf

+  description = "The CIDR range to use for the subnetwork for etcd."
+}
+
+variable "tectonic_gcp_network_etcd_loadbalancer_ip" {


Why does this need to be a user-facing input?

stale variable, thanks!

alexsomesan · 2017-09-21T16:20:53Z

platforms/gcp/variables.tf

+  description = "The CIDR range to use for the subnetwork for Workers."
+}
+
+variable "tectonic_gcp_network_etcd_cidr_range" {


Why does this need to be a user-facing input?

same as above

alexsomesan · 2017-09-21T16:21:04Z

platforms/gcp/variables.tf

+  description = "List of two or more zones to use from specified GCP region."
+}
+
+variable "tectonic_gcp_network_masters_cidr_range" {


Why does this need to be a user-facing input?

yep, don't really need to. This also might be taken into consideration on how we handle the zones. I removed the user facing var and hardcoded the value on the module input. In line with my comment above, keeping things simple and explicit for now

alexsomesan · 2017-09-21T16:24:45Z

platforms/gcp/variables.tf

+  description = "The GCP region to use. Some regions only have 2 zones."
+}
+
+variable "tectonic_gcp_zones" {


Could you rather use this here and not have this as an input?
Also we'd rather not have default values here, for the same reason as the region parameter.

Hey @alexsomesan just removed default values. This is related to how we handle the number of instances, the instance group managers here https://github.com/coreos/tectonic-installer/pull/1918/files#diff-00091bc50a0d600c32caba6d18409955R53, how we'll want to handle them once this lands in terraform hashicorp/terraform-provider-google#394 and also to adding autoscaling. I'd rather leave it explicit for now as it makes things more obvious, simple and easy, we can complement it later in conjunction with those other factors. what do you think?

alexsomesan · 2017-09-21T16:26:16Z

platforms/gcp/variables.tf

+
+variable "tectonic_gcp_region" {
+  type        = "string"
+  default     = "us-central1"


We're deliberately not setting any default for regions in any of the other platforms. We want this to be a deliberate input from the user. This is to raise the user's awareness about where their resources are going to be created.

Hey @alexsomesan make sense, just removed the default. Raising a warning here though as it seems we are actually currently setting the default for aws https://github.com/coreos/tectonic-installer/blob/master/platforms/aws/variables.tf#L262

alexsomesan · 2017-09-21T16:43:22Z

tests/smoke/gcp/vars/gcp.tfvars.json

+  "tectonic_gcp_project_id": "project-id",
+  "tectonic_gcp_region": "us-central1",
+  "google_managedzone_name": "managedzone-name",
+  "tectonic_base_domain": "tectonic.dev.coreos.systems",


This domain is already registered in Route53 for the AWS installer.
Does this actually also work here? Does it not need to be registered in Google Cloud NS?

Neither "project-id" or "managedzone-name" exist yet. I set those values as a sample, proper values will need to be set later when hooking into CI system

Got it, thanks for making that clear. Makes sense now.

enxebre · 2017-09-22T10:24:17Z

Hey @alexsomesan I'm relying on gcloud for ssh into the machines for now which handle the ssh keys on demand. https://cloud.google.com/compute/docs/instances/connecting-to-instance#gcetools
i.e gcloud compute ssh [INSTANCE_NAME]

alexsomesan

I think this now looks in shape to get in.
We can address eventual minor issues that come out of testing as they arise.

alexsomesan · 2017-09-25T09:33:11Z

ok to test

alexsomesan · 2017-09-25T14:17:08Z

platforms/gcp/main.tf

+limitations under the License.
+*/
+
+provider "google" {


To keep things aligned with the other platforms and allow the CI tool to work consistently, we should only inject the region via config variables.
credentials and project should come from the corresponding Terraform / SDK environment variables.
(See here and here)

…ap_service_id

alexsomesan · 2017-09-28T12:13:07Z

one test timed out - looks unrelated
we can merge

s-urbaniak suggested changes Sep 15, 2017

View reviewed changes

s-urbaniak mentioned this pull request Sep 18, 2017

WIP: Google Cloud Platform #531

Closed

5 tasks

s-urbaniak added the run-smoke-tests label Sep 19, 2017

s-urbaniak closed this Sep 19, 2017

s-urbaniak reopened this Sep 19, 2017

s-urbaniak previously approved these changes Sep 19, 2017

View reviewed changes

enxebre dismissed s-urbaniak’s stale review via c7d29d0 September 19, 2017 14:57

enxebre force-pushed the gcp-basis branch 5 times, most recently from 6456007 to 8af6968 Compare September 19, 2017 16:55

sym3tri assigned alexsomesan Sep 20, 2017

sym3tri added the needs review label Sep 20, 2017

alexsomesan suggested changes Sep 20, 2017

View reviewed changes

enxebre force-pushed the gcp-basis branch from 57f93b0 to 2e914d4 Compare September 21, 2017 11:16

alexsomesan reviewed Sep 21, 2017

View reviewed changes

alexsomesan previously approved these changes Sep 25, 2017

View reviewed changes

alexsomesan reviewed Sep 25, 2017

View reviewed changes

erjohnso and others added 3 commits September 25, 2017 17:21

platform/gcp: introduce Google Cloud Platform basis

59c75c4

platform/gcp: fix network related issues

810921c

platform/gcp: Adjust tectonic.tf to follow platform/aws

bf2a75f

pst and others added 20 commits September 25, 2017 17:21

platform/gcp: add missing variables

1a9aa4a

platform/gcp: Setup network correctly

3d3f069

platform/gcp: add missing variables

a9c866f

platform/gcp: fix resources being recreated on apply n+1 - one left

4519f20

platform/gcp: using shorter names for IGM VMs

5b6dd5a

platform/gcp: add support for Etcd TLS HA

e5d6ab9

platform/gcp: add calico-network-policy module

645cfc0

platform/gcp: add support for multiple Kube-masters

b5aad49

platform/gcp: use terraform v0.10.4 and Ignition generic module

1216351

platform/gcp: integrate with tls generic module

ad68e76

platform/gcp: add support for basic smoke tests

211045f

platform/gcp: remove unused

e358760

platform/gcp: auto generate docs

c39143d

platform/gcp: rename module google to gcp

a77ee37

platform/gcp: refactor dns stuff into own module

6c3fd65

platform/gcp: refactor init_assets service into ignition module

71b930e

platform/gcp: using installer_kubelet_env_id and ign_k8s_node_bootstr…

0854dd9

…ap_service_id

platform/gcp: remove stale variables and add namespace

06e6a54

platform/gcp: add Readme

1104099

platform/gcp: Removing unnecessary user facing vars

61d982a

enxebre dismissed alexsomesan’s stale review via aa93da8 September 25, 2017 15:43

enxebre force-pushed the gcp-basis branch 4 times, most recently from e31aaf0 to b803373 Compare September 27, 2017 12:59

platform/gcp: injecting provider variables via env

ffb71a0

enxebre force-pushed the gcp-basis branch from b803373 to ffb71a0 Compare September 27, 2017 13:12

alexsomesan approved these changes Sep 27, 2017

View reviewed changes

alexsomesan merged commit 28c95c5 into coreos:master Sep 28, 2017

gcp basis #1918

gcp basis #1918

Conversation

enxebre commented Sep 15, 2017 • edited Loading

coreosbot commented Sep 15, 2017

coreosbot commented Sep 15, 2017

coreosbot commented Sep 15, 2017

s-urbaniak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreosbot commented Sep 19, 2017

coreosbot commented Sep 19, 2017

coreosbot commented Sep 19, 2017

s-urbaniak left a comment

Choose a reason for hiding this comment

alexsomesan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsomesan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented Sep 22, 2017 • edited Loading

alexsomesan left a comment • edited Loading

Choose a reason for hiding this comment

alexsomesan commented Sep 25, 2017

Choose a reason for hiding this comment

alexsomesan commented Sep 28, 2017

enxebre commented Sep 15, 2017 •

edited

Loading

enxebre Sep 15, 2017 •

edited

Loading

enxebre commented Sep 22, 2017 •

edited

Loading

alexsomesan left a comment •

edited

Loading