Skip to content
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

Make zone configuration optional when creating a regional cluster #19

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cluster_regional.tf
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ resource "google_container_cluster" "primary" {
project = "${var.project_id}"

region = "${var.region}"
additional_zones = "${var.zones}"
additional_zones = ["${coalescelist(compact(var.zones), sort(random_shuffle.available_zones.result))}"]

network = "${data.google_compute_network.gke_network.self_link}"
subnetwork = "${data.google_compute_subnetwork.gke_subnetwork.self_link}"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Simple Cluster
# Simple Regional Cluster
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're explicitly calling out that it's a regional example, lets delete examples/simple since that one one was inherently a regional cluster. I like it being simple_regional better... makes it more clear.


This example illustrates how to create a simple cluster.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ provider "google" {
module "gke" {
source = "../../"
project_id = "${var.project_id}"
name = "simple-sample-cluster"
name = "simple-regional-cluster"
regional = true
region = "${var.region}"
network = "${var.network}"
subnetwork = "${var.subnetwork}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,3 @@ output "location_example" {
description = "Cluster location"
value = "${module.gke.location}"
}

output "zones_example" {
description = "List of zones in which the cluster resides"
value = "${module.gke.zones}"
}
File renamed without changes.
1 change: 1 addition & 0 deletions examples/simple_zonal/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ locals {

provider "google" {
credentials = "${file(local.credentials_file_path)}"
region = "${var.region}"
}

module "gke" {
Expand Down
5 changes: 5 additions & 0 deletions main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ data "google_compute_zones" "available" {
region = "${var.region}"
}

resource "random_shuffle" "available_zones" {
input = ["${data.google_compute_zones.available.names}"]
result_count = 3
}

locals {
kubernetes_version = "${var.kubernetes_version != "latest" ? var.kubernetes_version : data.google_container_engine_versions.region.latest_node_version}"
node_version = "${var.node_version != "" ? var.node_version : local.kubernetes_version}"
Expand Down
2 changes: 2 additions & 0 deletions test/integration/gcloud/Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
ruby '2.4.2'

source 'https://rubygems.org/' do
gem 'googleauth'
gem 'google-api-client'
gem 'kitchen-terraform', '~> 3.3'
gem 'kitchen-inspec', :git => 'https://github.com/inspec/kitchen-inspec.git', :ref => '0590f1b'
end
16 changes: 16 additions & 0 deletions test/integration/gcloud/run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,34 @@ function export_vars() {
export TEST_ID="modules_gke_integration_gcloud_${RANDOM}"
export KUBECONFIG="${TEMPDIR}/${CLUSTER_TYPE}/${TEST_ID}.kubeconfig"
if [[ $CLUSTER_TYPE = "regional" ]]; then
if [ -f "./regional_config.sh" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The complex variable export logic defined in this function adds a barrier to troubleshooting tests. If I needed to test out some modifications to the module, run a kitchen converge, and then kitchen verify, I would have to either mangle this script to generate the proper exports, or manually export the variables myself.

In addition, the sourcing of additional scripts means that if I set some variables in config.sh, the zonal_config.sh and regional_config.sh will silently stomp on those variables, which is no fun.

This only affects the tests and not the correctness of the module, so I think that we can merge this. @ryanckoch @Jberlinsky what do you think of filing a ticket/doing a followup pull request that converts the tests to something like what we do for the cloud-datastore module?

Copy link
Contributor Author

@Jberlinsky Jberlinsky Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I like what we're doing in cloud-datastore. I agree that this doesn't affect module correctness, and believe that the correct path is to merge this, and follow on in the future with a test conversion. There are two ways that come to mind as to how we could accomplish this -- I tend to prefer the former:

  • Converge on a singular methodology as we go -- the next person to pick up an adequately significant task affecting tests in this repository would get the "converge on this methodology" ticket; or
  • Add explicit tickets to the backlog to execute a convergence of testing systems for each repository, and prioritize them along with functional requests.

@morgante, would love to hear your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like my reply got lost here.

My preference is that we merge towards the style of testing which @adrienthebo developed (and away from the bash scripts). We can add tickets to the backlog and whoever picks up each module next will also work on the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@morgante Once this is merged in, I'll create tickets. Thanks for weighing in!

source ./regional_config.sh
fi
export CLUSTER_REGIONAL="true"
export CLUSTER_LOCATION="$REGIONAL_LOCATION"
export CLUSTER_NAME="$REGIONAL_CLUSTER_NAME"
export IP_RANGE_PODS="$REGIONAL_IP_RANGE_PODS"
export IP_RANGE_SERVICES="$REGIONAL_IP_RANGE_SERVICES"
else
if [ -f "./zonal_config.sh" ]; then
source ./zonal_config.sh
fi
if [ -z "${ZONE}" ]; then
echo "Can not create a zonal cluster without specifying \$ZONE. Aborting..."
exit 1
fi
export CLUSTER_REGIONAL="false"
export CLUSTER_LOCATION="$ZONAL_LOCATION"
export CLUSTER_NAME="$ZONAL_CLUSTER_NAME"
export IP_RANGE_PODS="$ZONAL_IP_RANGE_PODS"
export IP_RANGE_SERVICES="$ZONAL_IP_RANGE_SERVICES"
fi

if [ "${ZONE}" = "" ] && [ "${ADDITIONAL_ZONES}" = "" ]; then
export ZONES=""
else
export ZONES="\"$ZONE\",$ADDITIONAL_ZONES"
fi
}

# Activate test working directory
Expand Down
2 changes: 1 addition & 1 deletion test/integration/gcloud/sample.sh
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ export CLUSTER_NAME="int-test-cluster-01"
export REGION="us-east4"
export ZONE="us-east4-a"
export ADDITIONAL_ZONES='"us-east4-b","us-east4-c"'
export ZONES="\"$ZONE\",$ADDITIONAL_ZONES"
export KUBERNETES_VERSION="1.10.6-gke.2"
export NODE_POOL_SERVICE_ACCOUNT=""
export REGIONAL_CLUSTER_NAME="int-test-regional-01"
export REGIONAL_LOCATION="$REGION"
export ZONAL_CLUSTER_NAME="int-test-zonal-01"
export ZONAL_LOCATION="$ZONE"
export CLOUDSDK_AUTH_CREDENTIAL_FILE_OVERRIDE=$CREDENTIALS_PATH
export GOOGLE_APPLICATION_CREDENTIALS=$CREDENTIALS_PATH
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.

require_relative '../../../../test/support/google_cloud.rb'

# Test the name output
describe command('terraform output name_example') do
its('stdout.strip') { should eq ENV['CLUSTER_NAME'] }
Expand All @@ -34,7 +36,19 @@

# Test the zones output
describe command('terraform output -json zones_example | jq -cre \'.value\'') do
its('stdout.strip') { should eq '[' + ENV['ZONES'] + ']' }
if ENV['ZONES'] != ''
its('stdout.strip') { should eq '[' + ENV['ZONES'] + ']' }
else
it "should be 3 zones in the region" do
zones = JSON.parse(subject.stdout.strip)
zones.count.should be 3

available_zones = google_compute_service.get_region(ENV['PROJECT_ID'], ENV['REGION']).zones.map { |z| z.split("/").last }
zones.each do |z|
available_zones.should include z
end
end
end
end

# Test the endpoint output
Expand Down
22 changes: 22 additions & 0 deletions test/integration/gcloud/test/support/google_cloud.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Copyright 2018 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

require 'googleauth'
require 'google/apis/compute_v1'

def google_compute_service
Google::Apis::ComputeV1::ComputeService.new.tap do |service|
service.authorization = Google::Auth.get_application_default(['https://www.googleapis.com/auth/cloud-platform'])
end
end
25 changes: 25 additions & 0 deletions test/integration/gcloud/zonal_config.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#!/bin/bash
# Copyright 2018 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.


#################################################################
# PLEASE FILL THE VARIABLES WITH VALID VALUES FOR TESTING #
# DO NOT REMOVE ANY OF THE VARIABLES #
#################################################################

export ZONE="us-east4-a"
export ADDITIONAL_ZONES='"us-east4-b","us-east4-c"'
export ZONAL_LOCATION="$ZONE"