Feature/103 submodules #116

taylorludwig · 2019-11-25T22:18:16Z

Fixes #103

Splits up functionality into vpc, subnets, and routes submodules.
Changes routes input to be a map from a list to support for_each resource creation.
Updates ci/docker commands to use version 0.6.0 of developer tools image to fix errors when running generate_docs with complex types.

taylorludwig · 2019-11-25T22:56:47Z

Looks like lint test is failing on lint because of

terraform_validate ./modules/fabric-net-firewall

Error: "allow": only one of `allow,deny` can be specified, but `allow,deny` were specified.

on main.tf line 112, in resource "google_compute_firewall" "custom":
112: resource "google_compute_firewall" "custom" {

(something not touched in this PR)

Also a bit strange - I don't get that error locally even when using the same developer-tools:0.6.0 version that cloud build is using.

morgante · 2019-11-25T23:10:22Z

@taylorludwig This might be an issue with automatically pulling in the latest Terraform provider. Maybe try adding a version constraint on the module? https://www.terraform.io/docs/configuration/providers.html#provider-versions

CHANGELOG.md

morgante · 2019-11-25T23:11:35Z

README.md

-    routes = [
-        {
-            name                   = "egress-internet"
+    routes = {


I don't think we need to change this into a map. We can still maintain the desired behavior by constructing the map (for for_each) internally within the module.

sure, makes sense.

Ill create a local var map with the original default name as the key.

morgante · 2019-11-25T23:13:07Z

modules/routes/README.md

+- Routes within vpc network.
+- Optionally deletes the default internet gateway routes.
+
+## Compatibility


We don't need compatibility warnings on each submodule.

main.tf

modules/routes/main.tf

morgante · 2019-11-25T23:21:15Z

modules/subnets/outputs.tf

+  description = "The subnet resources"
+}
+
+output "subnets_names" {


Let's remove all these unnecessary outputs since we're not directly outputting the resources directly. We can keep them on the root module though.

morgante · 2019-11-25T23:21:51Z

modules/subnets/variables.tf

+ */
+
+variable "project_id" {
+  description = "The ID of the project where this VPC will be created"


Please clean up variable descriptions for submodules. In this case, this module doesn't create a VPC at all.

morgante · 2019-11-25T23:23:00Z

modules/vpc/main.tf

+	Shared VPC
+ *****************************************/
+resource "google_compute_shared_vpc_host_project" "shared_vpc_host" {
+  count      = var.shared_vpc_host == "true" ? 1 : 0


Suggested change

count = var.shared_vpc_host == "true" ? 1 : 0

count = var.shared_vpc_host ? 1 : 0

morgante · 2019-11-25T23:23:59Z

modules/vpc/outputs.tf

+  description = "The URI of the VPC being created"
+}
+
+output "svpc_host_project_id" {


This could just be project_id, no? (We can/should make it depend on the shared_vpc_host_project resource though.)

Yeah seems logical. Updated

Look like it still needs to be updated?

morgante · 2019-11-25T23:24:26Z

modules/vpc/variables.tf

+}
+
+variable "shared_vpc_host" {
+  type        = string


Let's take this opportunity to switch to boolean.

morgante · 2019-11-25T23:25:44Z

modules/fabric-net-firewall/versions.tf

@@ -17,3 +17,7 @@
 terraform {
  required_version = "~> 0.12.0"
 }
+
+provider "google" {


This should be a required version constraint, not a direct module invocation.

…validate" This reverts commit 3a3a408.

Co-Authored-By: Morgante Pell <[email protected]>

taylorludwig · 2019-11-25T23:42:23Z

@morgante yep - the lack of provider version in the modules was the problem. I've added that to the main and submodules and linting is passing.

Working on the other comments now.

…ards compatability don't break

taylorludwig · 2019-11-26T01:32:46Z

@morgante I believe I addressed all your comments. Tests are all passing again now as well. Thanks!

morgante · 2019-11-26T18:14:31Z

modules/routes/variables.tf

+  description = "The name of the network where routes will be created"
+}
+
+variable "network" {


Isn't this redundant with network_name? Don't think we should need both.

This just followed the original, single module, functionality. network and subnet resources were added to the depends_on black of the routes to force the depedency.

But passing in just the network_name from the output of the vpc submodule should force that dependency anyways. Ill remove network

morgante · 2019-11-26T18:14:37Z

modules/routes/variables.tf

+  default     = null
+}
+
+variable "subnets" {


We shouldn't need this.

Its only passed into the depends_on block, functionality that was there before.

Ill remove it and see what happens. I'm not entirely sure if the subnets really do need to be fully created before the routes are created.

If it's an issue, we could possibly use an output from the subnets module to pass into the network.

At least one of the tests fail if we remove this.

When creating a route with a next_hop_ip with an ip inside a subnet it'll error out with the following when the route tries to get created before the subnet is finished.

Error: Error creating Route: googleapi: Error 400: Invalid value for field 'resource.nextHopIp': '10.10.40.10'. 10.10.40.10 must lie within the address spaces of (). multi-vpc-a1-02 does not own any address space. Please create a subnetwork first., invalid

Got it. In that case, how about we follow the convention we started with network peering and add an explicit module_depends_on variable?

terraform-google-network/modules/network-peering/variables.tf

Line 45 in 8368373

variable "module_depends_on" {

modules/subnets/README.md

modules/subnets/variables.tf

morgante · 2019-11-26T18:16:23Z

modules/vpc/outputs.tf

+  description = "The URI of the VPC being created"
+}
+
+output "svpc_host_project_id" {


Look like it still needs to be updated?

morgante · 2019-11-26T18:17:07Z

outputs.tf

  description = "The name of the VPC being created"
 }

 output "network_self_link" {
-  value       = google_compute_network.network.self_link
+  value       = module.vpc.network_self_link
  description = "The URI of the VPC being created"
 }

 output "svpc_host_project_id" {


Let's just call this project_id.

Do you still want it only outputted if shared_vpc_host == true ?

The original logic was svpc_host_project_id would only be outputted if you enabled shared vpc, otherwise it was "".
But outputting just project_id seems like you'd still expect a value even with shared vpc off

Yes, let's output it in both cases.

…work name

…migration

aaron-lane

One observation after looking at our current documentation.

modules/subnets/variables.tf

taylorludwig added 10 commits November 25, 2019 11:03

fix makefile

dedcf4c

move vpc creation to its own module

0a135c0

move subnets to own module

987b4e4

move routes to its own submodule

28a26a2

use for_each for route creation

211a516

update developer tools version to fix generate doc errors

2dd1fad

generate docs

6e0faef

add readme to submodules

2c8ed09

formatting fix

f566110

changelog

f3e5e99

taylorludwig requested a review from morgante November 25, 2019 22:18

taylorludwig requested review from andreyk-code and Jeanno as code owners November 25, 2019 22:18

taylorludwig requested a review from a team November 25, 2019 22:18

taylorludwig requested a review from a team as a code owner November 25, 2019 22:18

lock provider for google_project_services removal error

605269a

specify provider versions for each module for terraform init/validate

3a3a408

morgante suggested changes Nov 25, 2019

View reviewed changes

morgante reviewed Nov 25, 2019

View reviewed changes

taylorludwig and others added 3 commits November 25, 2019 15:32

Revert "specify provider versions for each module for terraform init/…

fec8285

…validate" This reverts commit 3a3a408.

required provider versions on modules

560bbb2

Update CHANGELOG.md

fc02b37

Co-Authored-By: Morgante Pell <[email protected]>

taylorludwig added 5 commits November 25, 2019 16:10

remove compatability from submodules

188bf1c

update variable descriptions

2831941

switch routes back to list type with map generated within module

8649e43

default rotue values to null so google_compute_route defaults are used

d567d89

update output names per pr

b5b9ad8

taylorludwig added 6 commits November 25, 2019 16:35

switch shared_vpc_host to bool

e95a6b7

simpler way to return project id for shared vpc

a351412

update changelog and readme for list structure

4146283

fix terraform fmt issues

c9a6090

name isn't required anymore

db0448e

default output needs to be to match original value so tests and backw…

84119a8

…ards compatability don't break

taylorludwig requested a review from morgante November 26, 2019 01:33

morgante suggested changes Nov 26, 2019

View reviewed changes

taylorludwig added 7 commits November 26, 2019 10:51

fix readme examples

babfafa

remove unused leftover var

6b10555

remove network var for route depedency. Its inherit by passing in net…

f0443f7

…work name

remove description

f86ad43

rename svpc_project_id to project_id

2f6aee7

use a module_depends_on list instead of using explicit subnet

6e7ffd1

lint/validate fixes

da5372d

taylorludwig requested a review from morgante November 26, 2019 20:06

taylorludwig added 5 commits November 27, 2019 10:19

initial migrate.py from gke module

7ec22d8

update migrate.py to work with google-network resources and for_each …

99a3819

…migration

remove old sh migrate script

69615da

update readme for new py migrate script

2d4027a

python format

3afde82

aaron-lane suggested changes Nov 29, 2019

View reviewed changes

modules/subnets/variables.tf Show resolved Hide resolved

update migration doc to mention issue with single count mvs

9342a26

morgante requested a review from aaron-lane December 6, 2019 23:29

morgante approved these changes Dec 6, 2019

View reviewed changes

morgante merged commit b60f0ab into terraform-google-modules:release/2.0 Dec 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/103 submodules #116

Feature/103 submodules #116

taylorludwig commented Nov 25, 2019

taylorludwig commented Nov 25, 2019 •

edited

Loading

morgante commented Nov 25, 2019

morgante Nov 25, 2019

taylorludwig Nov 25, 2019

morgante Nov 25, 2019

morgante Nov 25, 2019

morgante Nov 25, 2019

morgante Nov 25, 2019

morgante Nov 25, 2019

taylorludwig Nov 26, 2019

morgante Nov 26, 2019

morgante Nov 25, 2019

morgante Nov 25, 2019

taylorludwig commented Nov 25, 2019

taylorludwig commented Nov 26, 2019

morgante Nov 26, 2019

taylorludwig Nov 26, 2019

morgante Nov 26, 2019

taylorludwig Nov 26, 2019

morgante Nov 26, 2019

taylorludwig Nov 26, 2019

morgante Nov 26, 2019

morgante Nov 26, 2019

morgante Nov 26, 2019

taylorludwig Nov 26, 2019

morgante Nov 26, 2019

aaron-lane left a comment

	count = var.shared_vpc_host == "true" ? 1 : 0
	count = var.shared_vpc_host ? 1 : 0

Feature/103 submodules #116

Feature/103 submodules #116

Conversation

taylorludwig commented Nov 25, 2019

taylorludwig commented Nov 25, 2019 • edited Loading

morgante commented Nov 25, 2019

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

taylorludwig commented Nov 25, 2019

taylorludwig commented Nov 26, 2019

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

aaron-lane left a comment

Choose a reason for hiding this comment

taylorludwig commented Nov 25, 2019 •

edited

Loading