Skip to content

Introduce separate networking submodule#25

Merged
bobbyiliev merged 4 commits intomainfrom
introduce-networking-module
Mar 14, 2025
Merged

Introduce separate networking submodule#25
bobbyiliev merged 4 commits intomainfrom
introduce-networking-module

Conversation

@bobbyiliev
Copy link
Copy Markdown
Collaborator

@bobbyiliev bobbyiliev commented Mar 13, 2025

As we discussed a few weeks ago, introducing a new networking module rather than handling all of the network resources within the other modules.

Fixes #16

While working on this I also removed the providers definition from the root module similar to what we did for the AWS module to allow users to handle that configuration on their end. See individual commits for those changes.

@bobbyiliev bobbyiliev requested a review from jubrad March 13, 2025 16:01
@bobbyiliev bobbyiliev changed the title Introduce separate networking module Introduce separate networking submodule Mar 13, 2025
@bobbyiliev bobbyiliev requested a review from kay-kim March 13, 2025 16:45
Copy link
Copy Markdown
Contributor

@kay-kim kay-kim left a comment

Choose a reason for hiding this comment

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

will make changes to the docs about providers.

Comment thread examples/simple/main.tf
google = google
kubernetes = kubernetes
helm = helm
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Below in the output section, should we add the output "network" { section?

Copy link
Copy Markdown
Contributor

@jubrad jubrad left a comment

Choose a reason for hiding this comment

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

One questions, otherwise looks good

Comment thread modules/gke/main.tf Outdated
google_service_networking_connection.private_vpc_connection,
google_compute_subnetwork.subnet,
google_compute_route.default_route
var.network_dependency # This ensures the network is created first
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't the gke module dependency on the networking dependency also ensure the network is created first?

I'm guessing you wouldn't have done this if it did, but that's weird.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point, just tested this without the extra redundancy of the network_dependency var and it works well as well.

create_before_destroy = true
}

deletion_policy = "ABANDON"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we abandon here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We initially added that as we’ve hit an issue in CI where the VPC peering gets deleted before its dependent resources (like Cloud SQL or GKE), which causes the destroy to fail.

Comment thread main.tf

module "gke" {
source = "./modules/gke"
module "networking" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we add the data "google_client_config" ... (from providers.tf) to main.tf?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Seems like we always had it there and it was just duplicated in the prociders.tf file as well:

data "google_client_config" "current" {}

@bobbyiliev bobbyiliev merged commit 7840cc5 into main Mar 14, 2025
2 checks passed
@bobbyiliev bobbyiliev deleted the introduce-networking-module branch March 14, 2025 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Abstract networking resources in a separate submodule

3 participants