Enable specifying master_authorized_networks_config #10

pratikmallya · 2018-09-19T06:15:50Z

Fixes #7

pratikmallya · 2018-09-19T06:31:22Z

Not sure if testing described in the README is expected to work; this is what I get on my machine

$ make -s
Running shellcheck

In ./test/make.sh line 91:
  for j in `for i in $(find . -type f | grep \.tf$) ; do dirname $i ; done | sort -u` ; do
           ^-- SC2006: Use $(..) instead of legacy `..`.
                                                                 ^-- SC2086: Double quote to prevent globbing and word splitting.

Running flake8
./helpers/combine_docfiles.py:31:80: E501 line too long (107 > 79 characters)
./helpers/combine_docfiles.py:32:80: E501 line too long (99 > 79 characters)
./helpers/combine_docfiles.py:35:3: E111 indentation is not a multiple of four
./helpers/combine_docfiles.py:41:80: E501 line too long (80 > 79 characters)
Running go fmt and go vet
Running terraform validate
Running hadolint on Dockerfiles
Checking for required files
Testing the validity of the header check
..
----------------------------------------------------------------------
Ran 2 tests in 0.024s

OK
Checking file headers
4 files have incorrect boilerplate headers:
test/integration/tmp/regional/main.tf
test/integration/tmp/regional/outputs.tf
test/integration/tmp/zonal/main.tf
test/integration/tmp/zonal/outputs.tf
make: *** [check_headers] Error 1

lilithmooncohen · 2018-09-19T15:08:42Z

The make -s tests currently fail when the the test/integration/tmp directory has already been generated. The tests that matter most in this instance are the make test_integration tests. I'll submit a separate PR for better clarification in the README.

lilithmooncohen

Looking good, but currently failing on:

-----> Starting Kitchen (v1.23.2)
$$$$$$ Running command `terraform version`
       Terraform v0.11.8
       + provider.google (unversioned)
       + provider.kubernetes v1.2.0
       + provider.null v1.0.0

$$$$$$ Terraform v0.11.8 is supported
-----> Converging <default-local>...
$$$$$$ Running command `terraform workspace select kitchen-terraform-default-local`
$$$$$$ Running command `terraform get -update /Users/ryankoch/Git/github/terraform-google-modules/terraform-google-kubernetes-engine/test/integration/tmp/regional`
       - module.gke
         Updating source "../../../../"
$$$$$$ Running command `terraform validate -check-variables=true    /Users/ryankoch/Git/github/terraform-google-modules/terraform-google-kubernetes-engine/test/integration/tmp/regional`

       Error: module.gke.output.master_authorized_networks_config: local.master_authorized_networks_config: no local value of this name has been declared



       Error: module.gke.local.cluster_master_authorized_networks_config: local.cluster_master_authorized_networks_config: local.cluster_type_output_cluster_master_authorized_networks_config: no local value of this name has been declared

morgante

Thanks so much for the PR. Let's just make sure tests pass and this can get merged in.

morgante · 2018-09-19T16:21:52Z

outputs.tf

@@ -50,6 +50,11 @@ output "min_master_version" {
  value       = "${local.cluster_min_master_version}"
 }

+output "master_authorized_networks_config" {
+  description = "Networks from which access to master is permitted"
+  value       = "${local.master_authorized_networks_config}"


@pratikmallya Looks like this references the wrong value. Think you meant to reference this: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/pull/10/files#diff-7a370d8342e7203b805911c92454f0f4R76

Ah, thanks @morgante ! Fixed

pratikmallya · 2018-09-19T16:32:29Z

@ryanckoch there was a typo which has been fixed. Could you take a look?

lilithmooncohen · 2018-09-19T17:07:04Z

Looking good, just currently hitting the following when master_authorized_networks_config is not set:

       Error: Error running plan: 1 error(s) occurred:

       * module.gke.local.cluster_type_output_master_authorized_networks_config: local.cluster_type_output_master_authorized_networks_config: Resource 'google_container_cluster.primary' does not have attribute 'master_authorized_networks_config' for variable 'google_container_cluster.primary.*.master_authorized_networks_config'

This is a similar issue to what we've seen in on another module: terraform-google-modules/terraform-google-network#5

This seems to be caused by the provider ignoring the empty master_authorized_networks_config = "${var.master_authorized_networks_config}" in the google_container_cluster resource (as it should). It in turn doesn't create that attribute on the resource in the terraform state file.

The best way we've come up to get around this in this case, is just pass the variable to the output. Would also in this case negate the need to set the cluster_type_output_master_authorized_networks_config local variable.

pratikmallya · 2018-09-19T17:16:33Z

Updated

morgante · 2018-09-19T17:20:15Z

Could we add a test for this to the integration tests?

pratikmallya · 2018-09-19T17:24:12Z

I'll need some help setting up the test locally. I get this:

$ make test_integration
./test/integration/gcloud/run.sh regional
./test/integration/gcloud/run.sh: line 54: config.sh: No such file or directory

lilithmooncohen · 2018-09-19T17:29:22Z

Tests are all passing now with master_authorized_networks_config disabled.

lilithmooncohen · 2018-09-19T17:31:12Z

Do cp test/integration/gcloud/sample.sh test/integration/gcloud/config.sh
and modify the first block of variables to fit your environment.

morgante · 2018-09-19T17:32:24Z

@ryanckoch Can we integration test setup directions to the readme?

lilithmooncohen · 2018-09-19T17:33:05Z

There is a sample, it just needs documented a bit better.
#12

qvallance · 2018-09-20T16:53:37Z

I tackled this a little while ago and found the formatting required for the variable to be a bit tricky (at least, how I got it to work), so I ended up including a full example in my variable description to help users.

variable "master_authorized_networks_config" {
  type        = "list"
  description = <<EOF
  The desired configuration options for master authorized networks. Omit the nested cidr_blocks attribute to disallow external access (except the cluster node IPs, which GKE automatically whitelists)

  ### example format ###
  default = [{
    cidr_blocks = [{
      cidr_block   = "10.0.0.0/8"
      display_name = "example_network"
    }],
  }]

  EOF
  default = []
}

pratikmallya · 2018-09-21T00:14:19Z

@qvallance I like the idea of being more explicit; this is kinda of a downstream module though (the variable is a pass-through input to the google_container_cluster module) so I'm not sure if we want to be that descriptive.

qvallance · 2018-09-21T00:55:50Z

@pratikmallya I generally agree that downstream variables don’t need to be that descriptive. I just found the required pass through format for this particularly variable not obvious from the documentation.

pratikmallya · 2018-09-21T01:10:22Z

Ah, I see what you're saying. It doesn't match what the original variable looks like.... thanks @qvallance !

pratikmallya · 2018-09-21T15:03:08Z

Need more help trying to run the tests locally. Stuck on this issue with the terraform driver now:

$ make test_integration
./test/integration/gcloud/run.sh regional

Creating main.tf file
Creating outputs.tf file
Using public_suffix 3.0.3
Using addressable 2.5.2
Using aws-sigv4 1.0.3
Using jmespath 1.4.0
Using aws-sdk-core 2.11.134
Using aws-sdk-resources 2.11.134
Using aws-sdk 2.11.134
Using concurrent-ruby 1.0.5
Using multipart-post 2.0.0
Using faraday 0.15.3
Using unf_ext 0.0.7.5
Using unf 0.1.4
Using domain_name 0.5.20180417
Using http-cookie 1.0.3
Using faraday-cookie_jar 0.0.6
Using timeliness 0.3.8
Using ms_rest 0.7.2
Using ms_rest_azure 0.11.0
Using azure_graph_rbac 0.17.0
Using azure_mgmt_resources 0.17.1
Using builder 3.2.3
Using bundler 1.16.0
Using coderay 1.1.2
Using declarative 0.0.10
Using declarative-option 0.1.0
Using diff-lcs 1.3
Using excon 0.62.0
Using multi_json 1.13.1
Using docker-api 1.34.2
Using dry-configurable 0.7.0
Using dry-container 0.6.0
Using dry-core 0.4.7
Using dry-equalizer 0.2.1
Using dry-inflector 0.1.2
Using dry-logic 0.4.2
Using dry-types 0.13.2
Using dry-validation 0.12.2
Using erubis 2.7.0
Using faraday_middleware 0.12.2
Using ffi 1.9.25
Using jwt 2.1.0
Using memoist 0.16.0
Using os 1.0.0
Using signet 0.9.2
Using googleauth 0.6.6
Using httpclient 2.8.3
Using mime-types-data 3.2018.0812
Using mime-types 3.2.2
Using uber 0.1.0
Using representable 3.0.4
Using retriable 3.1.2
Using google-api-client 0.19.8
Using gssapi 1.2.0
Using gyoku 1.3.1
Using hashie 3.6.0
Using htmlentities 4.3.4
Using inifile 3.0.0
Using json 2.1.0
Using method_source 0.9.0
Using mixlib-log 2.0.4
Using parallel 1.12.1
Using parslet 1.8.2
Using pry 0.11.3
Using rspec-support 3.8.0
Using rspec-core 3.8.0
Using rspec-expectations 3.8.1
Using rspec-mocks 3.8.0
Using rspec 3.8.0
Using rspec-its 1.2.0
Using rubyzip 1.2.2
Using semverse 2.0.0
Using sslshake 1.2.0
Using thor 0.20.0
Using tomlrb 1.2.7
Using mixlib-shellout 2.4.0
Using net-ssh 4.2.0
Using net-scp 1.2.1
Using little-plugger 1.1.4
Using logging 2.2.2
Using nori 2.6.0
Using rubyntlm 0.6.2
Using winrm 2.2.3
Using winrm-fs 1.3.0
Using train 1.4.37
Using inspec 2.2.112
Using mixlib-versioning 1.2.2
Using mixlib-install 3.11.5
Using net-ssh-gateway 1.3.0
Using winrm-elevated 1.1.0
Using test-kitchen 1.23.2
Using kitchen-inspec 0.23.1 from https://github.com/inspec/kitchen-inspec.git (at 0590f1b@0590f1b)
Using kitchen-terraform 3.3.1
Bundle complete! 3 Gemfile dependencies, 92 gems now installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.
Ignoring ffi-1.9.25 because its extensions are not built.  Try: gem pristine ffi --version 1.9.25
Ignoring ffi-yajl-2.3.1 because its extensions are not built.  Try: gem pristine ffi-yajl --version 2.3.1
Ignoring nokogiri-1.6.8 because its extensions are not built.  Try: gem pristine nokogiri --version 1.6.8
Ignoring yajl-ruby-1.4.0 because its extensions are not built.  Try: gem pristine yajl-ruby --version 1.4.0
-----> Starting Kitchen (v1.13.2)
>>>>>> ------Exception-------
>>>>>> Class: Kitchen::ClientError
>>>>>> Message: Could not load the 'terraform' driver from the load path. Please ensure that your driver is installed as a gem or included in your Gemfile if using Bundler.
>>>>>> ----------------------
>>>>>> Please see .kitchen/logs/kitchen.log for more details
>>>>>> Also try running `kitchen diagnose --all` for configuration

make: *** [regional_test_integration] Error 20

Even though I do have terraform installed. Ideas on how to fix this?

pratikmallya · 2018-09-21T15:56:38Z

@morgante without giving clear instructions on how to run the integration tests, its hard for me to make the changes that you have requested. I'm unfortunately not very familiar with ruby/chef/kitchen and have not been able to figure how to fix this error

morgante · 2018-09-21T19:07:53Z

variables.tf

@@ -65,6 +65,12 @@ variable "node_version" {
  default     = ""
 }

+variable "master_authorized_networks_config" {
+  description = "The desired configuration options for master authorized networks. Omit the nested cidr_blocks attribute to disallow external access (except the cluster node IPs, which GKE automatically whitelists)."


@pratikmallya Could we expand this to include an example config, like here: #10 (comment)

morgante · 2018-09-21T19:09:13Z

@pratikmallya Understood, I'm okay with merging this without tests assuming our existing tests pass.

If you address this final comment we can merge: #10 (review)

Thanks for your work on this and patience.

@qvallance

Many thanks to @qvallance for this tip!

pratikmallya · 2018-09-22T01:40:49Z

@morgante @ryanckoch updated the variable description

pratikmallya · 2018-09-24T19:37:44Z

@morgante ping

morgante

LGTM

pratikmallya force-pushed the add_master_auth_network branch from e0ac559 to 8037d88 Compare September 19, 2018 06:29

morgante requested a review from lilithmooncohen September 19, 2018 13:49

lilithmooncohen self-assigned this Sep 19, 2018

lilithmooncohen suggested changes Sep 19, 2018

View reviewed changes

lilithmooncohen removed their assignment Sep 19, 2018

morgante suggested changes Sep 19, 2018

View reviewed changes

pratikmallya force-pushed the add_master_auth_network branch from 8037d88 to 0d214aa Compare September 19, 2018 16:31

pratikmallya force-pushed the add_master_auth_network branch 2 times, most recently from e789827 to c0b223a Compare September 19, 2018 16:35

lilithmooncohen self-assigned this Sep 19, 2018

pratikmallya force-pushed the add_master_auth_network branch from 365c6fe to 03091f4 Compare September 19, 2018 17:20

Enable specifying master_authorized_networks_config

368dc4d

pratikmallya force-pushed the add_master_auth_network branch from 03091f4 to 368dc4d Compare September 21, 2018 15:28

morgante suggested changes Sep 21, 2018

View reviewed changes

Use a more helpful variable description

5fd3e12

Many thanks to @qvallance for this tip!

pratikmallya force-pushed the add_master_auth_network branch from 94c55c7 to 5fd3e12 Compare September 22, 2018 01:40

morgante approved these changes Sep 26, 2018

View reviewed changes

morgante merged commit 05e2634 into terraform-google-modules:master Sep 26, 2018

pratikmallya deleted the add_master_auth_network branch September 26, 2018 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable specifying master_authorized_networks_config #10

Enable specifying master_authorized_networks_config #10

pratikmallya commented Sep 19, 2018 •

edited by morgante

pratikmallya commented Sep 19, 2018

lilithmooncohen commented Sep 19, 2018

lilithmooncohen left a comment

morgante left a comment

morgante Sep 19, 2018

pratikmallya Sep 19, 2018

pratikmallya commented Sep 19, 2018

lilithmooncohen commented Sep 19, 2018 •

edited

pratikmallya commented Sep 19, 2018

morgante commented Sep 19, 2018

pratikmallya commented Sep 19, 2018

lilithmooncohen commented Sep 19, 2018

lilithmooncohen commented Sep 19, 2018

morgante commented Sep 19, 2018 •

edited

lilithmooncohen commented Sep 19, 2018

qvallance commented Sep 20, 2018

pratikmallya commented Sep 21, 2018

qvallance commented Sep 21, 2018

pratikmallya commented Sep 21, 2018

pratikmallya commented Sep 21, 2018

pratikmallya commented Sep 21, 2018

morgante Sep 21, 2018

morgante commented Sep 21, 2018

pratikmallya commented Sep 22, 2018

pratikmallya commented Sep 24, 2018

morgante left a comment

Enable specifying master_authorized_networks_config #10

Enable specifying master_authorized_networks_config #10

Conversation

pratikmallya commented Sep 19, 2018 • edited by morgante

pratikmallya commented Sep 19, 2018

lilithmooncohen commented Sep 19, 2018

lilithmooncohen left a comment

Choose a reason for hiding this comment

morgante left a comment

Choose a reason for hiding this comment

morgante Sep 19, 2018

Choose a reason for hiding this comment

pratikmallya Sep 19, 2018

Choose a reason for hiding this comment

pratikmallya commented Sep 19, 2018

lilithmooncohen commented Sep 19, 2018 • edited

pratikmallya commented Sep 19, 2018

morgante commented Sep 19, 2018

pratikmallya commented Sep 19, 2018

lilithmooncohen commented Sep 19, 2018

lilithmooncohen commented Sep 19, 2018

morgante commented Sep 19, 2018 • edited

lilithmooncohen commented Sep 19, 2018

qvallance commented Sep 20, 2018

pratikmallya commented Sep 21, 2018

qvallance commented Sep 21, 2018

pratikmallya commented Sep 21, 2018

pratikmallya commented Sep 21, 2018

pratikmallya commented Sep 21, 2018

morgante Sep 21, 2018

Choose a reason for hiding this comment

morgante commented Sep 21, 2018

pratikmallya commented Sep 22, 2018

pratikmallya commented Sep 24, 2018

morgante left a comment

Choose a reason for hiding this comment

pratikmallya commented Sep 19, 2018 •

edited by morgante

lilithmooncohen commented Sep 19, 2018 •

edited

morgante commented Sep 19, 2018 •

edited