fix!: use bootstrap.outputs.common_config as default region #1181

nbugden · 2024-04-03T18:58:11Z

This PR adds some additional outputs bootstrap.outputs.common_config and uses these values by default. This fixes the issue of resources being deployed in us-central1 and us-west1 which was caused by module defaults (set in variables.tf) not being overridden when modules invoked. It also maintains the ability to override the defaults using the auto.tfvars should that be desired.

Issue(s) Resolved:
#1172

BEGIN_COMMIT_OVERRIDE
fix: use bootstrap.outputs.common_config as default region (#1181)
END_COMMIT_OVERRIDE

fmichaelobrien

/lgtm

fmichaelobrien

The same fix will be needed in the locals default for 3-networks-hub-and-spoke

https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/3-networks-hub-and-spoke/envs/shared/main.tf#L22

nbugden · 2024-04-11T16:58:42Z

The same fix will be needed in the locals default for 3-networks-hub-and-spoke

https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/3-networks-hub-and-spoke/envs/shared/main.tf#L22

Good catch @fmichaelobrien ! Fixed in this commit: 7c4409f

fmichaelobrien

/lgtm

nbugden · 2024-04-17T18:06:01Z

@rjerrems @eeaton @gtsorbo can one of you folks take a look. PR needs /gcbrun and an approving review.

nbugden · 2024-04-24T12:38:36Z

@rjerrems @eeaton @gtsorbo can one of you folks take a look. PR needs /gcbrun and an approving review.

Nudge, can I get a review. If there is something else required before review, please let me know.

gtsorbo

Thanks for your contribution! Could you re-source the network module to the published version now that v9.1.0 is available?

3-networks-dual-svpc/modules/base_shared_vpc/private_service_connect.tf

3-networks-dual-svpc/modules/restricted_shared_vpc/private_service_connect.tf

daniel-cit

requested changes are related to the integration tests and deployment flows (helper, TFE)

3-networks-dual-svpc/envs/production/main.tf

0-bootstrap/outputs.tf

1-org/envs/shared/log_sinks.tf

3-networks-dual-svpc/envs/development/remote.tf

0-bootstrap/variables.tf

nbugden · 2024-04-26T18:33:57Z

@gtsorbo @daniel-cit I believe I've addressed everything in your change requests. Can you review.

3-networks-dual-svpc/envs/development/main.tf

daniel-cit · 2024-04-26T18:43:38Z

@nbugden the PR should also be renamed to fix!: use bootstrap.outputs.common_config as default region since this is a braking change (subnetworks need to be recreated).

daniel-cit · 2024-04-26T18:48:51Z

@nbugden is the documentation still reflected in the code based on the changes in the subnetwork ?
https://cloud.google.com/architecture/security-foundations/networking#ip-address-allocation

nbugden · 2024-04-26T19:08:06Z

@nbugden the PR should also be renamed to fix!: use bootstrap.outputs.common_config as default region since this is a braking change (subnetworks need to be recreated).

Done

@nbugden is the documentation still reflected in the code based on the changes in the subnetwork ? https://cloud.google.com/architecture/security-foundations/networking#ip-address-allocation

@daniel-cit Yes, the changes to the subnets are still inline with the documentation. The doc you shared states region1 and region2. It does not make reference to specific data centers (us-west1 or us-central1). This PR does not change the CIDRs, just the locations in which they are provisioned.

3-networks-dual-svpc/envs/production/remote.tf.cloud.example

daniel-cit · 2024-04-26T19:25:32Z

/gcbrun

daniel-cit · 2024-05-22T17:50:52Z

/gcbrun

nbugden · 2024-05-22T20:17:53Z

both CI pipelines failed once more 😢

apeabody · 2024-05-22T20:57:47Z

@daniel-cit - Error 429: Existing sinks count: 201 is greater than the limit: 200.

Already opened GoogleCloudPlatform/cloud-foundation-toolkit#2371

daniel-cit · 2024-05-23T14:08:15Z

It is better to wait for #1251 to be merged before trying again

bdashrad · 2024-05-23T19:42:42Z

we're excited to get this one in!

bdashrad · 2024-05-23T21:29:04Z

Using the same value for location here in storage_options and for location here in project_options results in no way to override values to match the previous defaults, which results in the log storage bucket in 1-org module.logs_export.module.destination_storage[0].google_storage_bucket.bucket needing to be replaced.

If I don't set any values, these are the plan changes:

  # module.logs_export.module.destination_storage[0].google_storage_bucket.bucket must be replaced
-/+ resource "google_storage_bucket" "bucket" {
      - default_event_based_hold    = false -> null
      ~ effective_labels            = {} -> (known after apply)
      - enable_object_retention     = false -> null
      ~ id                          = "bkt-prj-c-logging-hcia-org-logs-h6nd" -> (known after apply)
      - labels                      = {} -> null
      ~ location                    = "US" -> "US-EAST1" # forces replacement
        name                        = "bkt-prj-c-logging-1234-org-logs-5678"
      ~ project_number              = 123456789101 -> (known after apply)
      - requester_pays              = false -> null
      ~ rpo                         = "DEFAULT" -> (known after apply)
      ~ self_link                   = "https://www.googleapis.com/storage/v1/b/bkt-prj-c-logging-1234-org-logs-5678" -> (known after apply)
      ~ terraform_labels            = {} -> (known after apply)
      ~ url                         = "gs://bkt-prj-c-logging-1234-org-logs-5678" -> (known after apply)
        # (5 unchanged attributes hidden)

      - soft_delete_policy {
          - effective_time             = "2024-05-08T14:43:32.090Z" -> null
          - retention_duration_seconds = 604800 -> null
        }

        # (2 unchanged blocks hidden)
    }

And if I set log_export_storage_location = "US" we get this, which isn't valid, since the linked dataset and the log sync don't support a multi-region location (e.g. US) like the bucket

  # module.logs_export.module.destination_aggregated_logs[0].google_logging_linked_dataset.linked_dataset[0] must be replaced
-/+ resource "google_logging_linked_dataset" "linked_dataset" {
      ~ bucket          = "AggregatedLogs" # forces replacement -> (known after apply) # forces replacement
      ~ create_time     = "2024-05-08T14:45:06.171132597Z" -> (known after apply)
      ~ id              = "projects/prj-c-logging-hcia/locations/us-east1/buckets/AggregatedLogs/links/ds_c_prj_aggregated_logs_analytics" -> (known after apply)
      ~ lifecycle_state = "ACTIVE" -> (known after apply)
      ~ location        = "us-east1" -> "US" # forces replacement
      ~ name            = "projects/prj-c-logging-hcia/locations/us-east1/buckets/AggregatedLogs/links/ds_c_prj_aggregated_logs_analytics" -> (known after apply)
        # (3 unchanged attributes hidden)

      - bigquery_dataset {
          - dataset_id = "bigquery.googleapis.com/projects/854274779945/datasets/ds_c_prj_aggregated_logs_analytics" -> null
        }
    }

  # module.logs_export.module.destination_aggregated_logs[0].google_logging_project_bucket_config.bucket must be replaced
-/+ resource "google_logging_project_bucket_config" "bucket" {
      + description      = (known after apply)
      ~ id               = "projects/prj-c-logging-hcia/locations/us-east1/buckets/AggregatedLogs" -> (known after apply)
      ~ lifecycle_state  = "ACTIVE" -> (known after apply)
      ~ location         = "us-east1" -> "US" # forces replacement
      - locked           = false -> null
      ~ name             = "projects/prj-c-logging-hcia/locations/us-east1/buckets/AggregatedLogs" -> (known after apply)
        # (4 unchanged attributes hidden)
    }

  # module.logs_export.module.internal_project_log_export[0].google_logging_project_sink.sink[0] will be updated in-place
  ~ resource "google_logging_project_sink" "sink" {
      ~ destination            = "logging.googleapis.com/projects/prj-c-logging-hcia/locations/us-east1/buckets/AggregatedLogs" -> "logging.googleapis.com/projects/prj-c-logging-hcia/locations/US/buckets/AggregatedLogs"
        id                     = "projects/prj-c-logging-hcia/sinks/sk-c-logging-prj-la"
        name                   = "sk-c-logging-prj-la"
        # (4 unchanged attributes hidden)
    }

which results in these errors:

│ Error: googleapi: Error 400: Sink.destination uses not supported location: US, badRequest
│
│   with module.logs_export.module.internal_project_log_export[0].google_logging_project_sink.sink[0],
│   on .terraform/modules/logs_export.internal_project_log_export/main.tf line 41, in resource "google_logging_project_sink" "sink":
│   41: resource "google_logging_project_sink" "sink" {
│
╵
╷
│ Error: Error creating Bucket: googleapi: Error 404: Cloud Logging does not support location: US
│
│   with module.logs_export.module.destination_aggregated_logs[0].google_logging_project_bucket_config.bucket,
│   on .terraform/modules/logs_export.destination_aggregated_logs/modules/logbucket/main.tf line 36, in resource "google_logging_project_bucket_config" "bucket":
│   36: resource "google_logging_project_bucket_config" "bucket" {

If we want to support multi-region buckets, like the old default, we should change the storage_options.location value.

daniel-cit · 2024-05-23T22:48:07Z

Using the same value for location here in storage_options and for location here in project_options results in no way to override values to match the previous defaults, which results in the log storage bucket in 1-org module.logs_export.module.destination_storage[0].google_storage_bucket.bucket needing to be replaced.

If I don't set any values, these are the plan changes:

  # module.logs_export.module.destination_storage[0].google_storage_bucket.bucket must be replaced
-/+ resource "google_storage_bucket" "bucket" {
      - default_event_based_hold    = false -> null
      ~ effective_labels            = {} -> (known after apply)
      - enable_object_retention     = false -> null
      ~ id                          = "bkt-prj-c-logging-hcia-org-logs-h6nd" -> (known after apply)
      - labels                      = {} -> null
      ~ location                    = "US" -> "US-EAST1" # forces replacement
        name                        = "bkt-prj-c-logging-1234-org-logs-5678"
      ~ project_number              = 123456789101 -> (known after apply)
      - requester_pays              = false -> null
      ~ rpo                         = "DEFAULT" -> (known after apply)
      ~ self_link                   = "https://www.googleapis.com/storage/v1/b/bkt-prj-c-logging-1234-org-logs-5678" -> (known after apply)
      ~ terraform_labels            = {} -> (known after apply)
      ~ url                         = "gs://bkt-prj-c-logging-1234-org-logs-5678" -> (known after apply)
        # (5 unchanged attributes hidden)

      - soft_delete_policy {
          - effective_time             = "2024-05-08T14:43:32.090Z" -> null
          - retention_duration_seconds = 604800 -> null
        }

        # (2 unchanged blocks hidden)
    }

And if I set log_export_storage_location = "US" we get this, which isn't valid, since the linked dataset and the log sync don't support a multi-region location (e.g. US) like the bucket

  # module.logs_export.module.destination_aggregated_logs[0].google_logging_linked_dataset.linked_dataset[0] must be replaced
-/+ resource "google_logging_linked_dataset" "linked_dataset" {
      ~ bucket          = "AggregatedLogs" # forces replacement -> (known after apply) # forces replacement
      ~ create_time     = "2024-05-08T14:45:06.171132597Z" -> (known after apply)
      ~ id              = "projects/prj-c-logging-hcia/locations/us-east1/buckets/AggregatedLogs/links/ds_c_prj_aggregated_logs_analytics" -> (known after apply)
      ~ lifecycle_state = "ACTIVE" -> (known after apply)
      ~ location        = "us-east1" -> "US" # forces replacement
      ~ name            = "projects/prj-c-logging-hcia/locations/us-east1/buckets/AggregatedLogs/links/ds_c_prj_aggregated_logs_analytics" -> (known after apply)
        # (3 unchanged attributes hidden)

      - bigquery_dataset {
          - dataset_id = "bigquery.googleapis.com/projects/854274779945/datasets/ds_c_prj_aggregated_logs_analytics" -> null
        }
    }

  # module.logs_export.module.destination_aggregated_logs[0].google_logging_project_bucket_config.bucket must be replaced
-/+ resource "google_logging_project_bucket_config" "bucket" {
      + description      = (known after apply)
      ~ id               = "projects/prj-c-logging-hcia/locations/us-east1/buckets/AggregatedLogs" -> (known after apply)
      ~ lifecycle_state  = "ACTIVE" -> (known after apply)
      ~ location         = "us-east1" -> "US" # forces replacement
      - locked           = false -> null
      ~ name             = "projects/prj-c-logging-hcia/locations/us-east1/buckets/AggregatedLogs" -> (known after apply)
        # (4 unchanged attributes hidden)
    }

  # module.logs_export.module.internal_project_log_export[0].google_logging_project_sink.sink[0] will be updated in-place
  ~ resource "google_logging_project_sink" "sink" {
      ~ destination            = "logging.googleapis.com/projects/prj-c-logging-hcia/locations/us-east1/buckets/AggregatedLogs" -> "logging.googleapis.com/projects/prj-c-logging-hcia/locations/US/buckets/AggregatedLogs"
        id                     = "projects/prj-c-logging-hcia/sinks/sk-c-logging-prj-la"
        name                   = "sk-c-logging-prj-la"
        # (4 unchanged attributes hidden)
    }

which results in these errors:

│ Error: googleapi: Error 400: Sink.destination uses not supported location: US, badRequest
│
│   with module.logs_export.module.internal_project_log_export[0].google_logging_project_sink.sink[0],
│   on .terraform/modules/logs_export.internal_project_log_export/main.tf line 41, in resource "google_logging_project_sink" "sink":
│   41: resource "google_logging_project_sink" "sink" {
│
╵
╷
│ Error: Error creating Bucket: googleapi: Error 404: Cloud Logging does not support location: US
│
│   with module.logs_export.module.destination_aggregated_logs[0].google_logging_project_bucket_config.bucket,
│   on .terraform/modules/logs_export.destination_aggregated_logs/modules/logbucket/main.tf line 36, in resource "google_logging_project_bucket_config" "bucket":
│   36: resource "google_logging_project_bucket_config" "bucket" {

If we want to support multi-region buckets, like the old default, we should change the storage_options.location value.

@bdashrad Good catch.

the project_options.location

https://github.com/nbugden/terraform-example-foundation/blob/fix/bootstrap-default-region/1-org/envs/shared/log_sinks.tf#L82

location                   = coalesce(var.log_export_storage_location, local.default_region)

should still be

location                   = local.default_region

since it already reads from the remote state.

And

log_export_storage_location     = "US"
billing_export_dataset_location = "US"

Should also be added to the terraform.example.tfvars file

https://github.com/nbugden/terraform-example-foundation/blob/fix/bootstrap-default-region/1-org/envs/shared/terraform.example.tfvars

nbugden · 2024-05-24T00:15:49Z

thanks @bdashrad and @daniel-cit
changes have been made and I merged the latest master as well

daniel-cit · 2024-05-24T00:18:00Z

/gcbrun

daniel-cit

LGTM

daniel-cit · 2024-05-24T12:59:51Z

/gcbrun

nbugden · 2024-05-24T14:03:33Z

@daniel-cit ci failed again, can you /gcbrun once more please

daniel-cit · 2024-05-24T14:39:37Z

/gcbrun

daniel-cit · 2024-05-24T18:28:25Z

build failing because of missing Access Context Manager Policy ID
#1252

daniel-cit · 2024-05-24T22:22:29Z

/gcbrun

nbugden · 2024-05-27T13:26:28Z

build failing because of missing Access Context Manager Policy ID #1252

@daniel-cit one of the CI workflows failed... are we still waiting on #1252?

daniel-cit · 2024-05-28T13:25:21Z

Test failing with

Step #12 - "create-shared": TestShared 2024-05-28T12:24:31Z command.go:100: Running command gcloud with args [access-context-manager policies list --organization REDACTED --filter parent:organizations/REDACTED --quiet --format json]
Step #12 - "create-shared": TestShared 2024-05-28T12:24:32Z command.go:185: WARNING: The following filter keys were not present in any resource : parent
Step #12 - "create-shared": TestShared 2024-05-28T12:24:32Z command.go:185: []
Step #12 - "create-shared":     shared_test.go:45: 
Step #12 - "create-shared":         	Error Trace:	/workspace/test/integration/shared/shared_test.go:45
Step #12 - "create-shared":         	Error:      	Should NOT be empty, but was 
Step #12 - "create-shared":         	Test:       	TestShared
Step #12 - "create-shared":         	Messages:   	Access Context Manager Policy ID must be configured in the organization for the test to proceed.
Step #12 - "create-shared": --- FAIL: TestShared (5.70s)

daniel-cit · 2024-05-28T13:31:55Z

/gcbrun

daniel-cit · 2024-05-28T19:26:56Z

/gcbrun

Grant is OOO for the month, but we've had additional reviews since the initial requested changes

nbugden requested review from rjerrems, gtsorbo, eeaton and a team as code owners April 3, 2024 18:58

nbugden force-pushed the fix/bootstrap-default-region branch from e0e224c to 3833e0b Compare April 3, 2024 19:21

fmichaelobrien mentioned this pull request Apr 3, 2024

Location set in bootstrap is ignored - resources still deployed in us-central1 and us-west1 #1172

Closed

fmichaelobrien approved these changes Apr 4, 2024

View reviewed changes

nbugden force-pushed the fix/bootstrap-default-region branch from eca67c3 to a0397ec Compare April 9, 2024 16:35

fmichaelobrien reviewed Apr 11, 2024

View reviewed changes

nbugden force-pushed the fix/bootstrap-default-region branch from a0397ec to 7c4409f Compare April 11, 2024 16:57

fmichaelobrien approved these changes Apr 11, 2024

View reviewed changes

nbugden force-pushed the fix/bootstrap-default-region branch 5 times, most recently from 1e0f446 to 332b503 Compare April 26, 2024 16:07

gtsorbo previously requested changes Apr 26, 2024

View reviewed changes

3-networks-dual-svpc/modules/base_shared_vpc/private_service_connect.tf Outdated Show resolved Hide resolved

3-networks-dual-svpc/modules/restricted_shared_vpc/private_service_connect.tf Outdated Show resolved Hide resolved

daniel-cit requested changes Apr 26, 2024

View reviewed changes

nbugden force-pushed the fix/bootstrap-default-region branch from b8a8241 to b8e66b6 Compare April 26, 2024 17:24

daniel-cit reviewed Apr 26, 2024

View reviewed changes

3-networks-dual-svpc/envs/development/main.tf Outdated Show resolved Hide resolved

nbugden changed the title ~~fix: use bootstrap.outputs.common_config as default region~~ fix!: use bootstrap.outputs.common_config as default region Apr 26, 2024

daniel-cit reviewed Apr 26, 2024

View reviewed changes

3-networks-dual-svpc/envs/production/remote.tf.cloud.example Show resolved Hide resolved

nbugden and others added 2 commits May 23, 2024 20:14

fix: multi-region buckets

93dfa01

Merge branch 'master' into fix/bootstrap-default-region

1cd5696

daniel-cit approved these changes May 24, 2024

View reviewed changes

Merge branch 'master' into fix/bootstrap-default-region

298dc9b

Merge branch 'master' into fix/bootstrap-default-region

13c9019

Merge branch 'master' into fix/bootstrap-default-region

feb5914

Merge branch 'master' into fix/bootstrap-default-region

ae67e70

daniel-cit requested a review from sleighton2022 as a code owner May 28, 2024 19:26

eeaton approved these changes May 29, 2024

View reviewed changes

eeaton merged commit 105fe52 into terraform-google-modules:master May 29, 2024
7 checks passed

release-please bot mentioned this pull request May 29, 2024

chore(master): release 4.1.0 #1074

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: use bootstrap.outputs.common_config as default region #1181

fix!: use bootstrap.outputs.common_config as default region #1181

nbugden commented Apr 3, 2024 •

edited by apeabody

Loading

fmichaelobrien left a comment

fmichaelobrien left a comment

nbugden commented Apr 11, 2024 •

edited

Loading

fmichaelobrien left a comment

nbugden commented Apr 17, 2024

nbugden commented Apr 24, 2024

gtsorbo left a comment

daniel-cit left a comment

nbugden commented Apr 26, 2024

daniel-cit commented Apr 26, 2024 •

edited

Loading

daniel-cit commented Apr 26, 2024

nbugden commented Apr 26, 2024 •

edited

Loading

daniel-cit commented Apr 26, 2024

daniel-cit commented May 22, 2024

nbugden commented May 22, 2024

apeabody commented May 22, 2024

daniel-cit commented May 23, 2024

bdashrad commented May 23, 2024

bdashrad commented May 23, 2024

daniel-cit commented May 23, 2024

nbugden commented May 24, 2024

daniel-cit commented May 24, 2024

daniel-cit left a comment

daniel-cit commented May 24, 2024

nbugden commented May 24, 2024

daniel-cit commented May 24, 2024

daniel-cit commented May 24, 2024

daniel-cit commented May 24, 2024

nbugden commented May 27, 2024

daniel-cit commented May 28, 2024

daniel-cit commented May 28, 2024

daniel-cit commented May 28, 2024

fix!: use bootstrap.outputs.common_config as default region #1181

fix!: use bootstrap.outputs.common_config as default region #1181

Conversation

nbugden commented Apr 3, 2024 • edited by apeabody Loading

fmichaelobrien left a comment

Choose a reason for hiding this comment

fmichaelobrien left a comment

Choose a reason for hiding this comment

nbugden commented Apr 11, 2024 • edited Loading

fmichaelobrien left a comment

Choose a reason for hiding this comment

nbugden commented Apr 17, 2024

nbugden commented Apr 24, 2024

gtsorbo left a comment

Choose a reason for hiding this comment

daniel-cit left a comment

Choose a reason for hiding this comment

nbugden commented Apr 26, 2024

daniel-cit commented Apr 26, 2024 • edited Loading

daniel-cit commented Apr 26, 2024

nbugden commented Apr 26, 2024 • edited Loading

daniel-cit commented Apr 26, 2024

daniel-cit commented May 22, 2024

nbugden commented May 22, 2024

apeabody commented May 22, 2024

daniel-cit commented May 23, 2024

bdashrad commented May 23, 2024

bdashrad commented May 23, 2024

daniel-cit commented May 23, 2024

nbugden commented May 24, 2024

daniel-cit commented May 24, 2024

daniel-cit left a comment

Choose a reason for hiding this comment

daniel-cit commented May 24, 2024

nbugden commented May 24, 2024

daniel-cit commented May 24, 2024

daniel-cit commented May 24, 2024

daniel-cit commented May 24, 2024

nbugden commented May 27, 2024

daniel-cit commented May 28, 2024

daniel-cit commented May 28, 2024

daniel-cit commented May 28, 2024

nbugden commented Apr 3, 2024 •

edited by apeabody

Loading

nbugden commented Apr 11, 2024 •

edited

Loading

daniel-cit commented Apr 26, 2024 •

edited

Loading

nbugden commented Apr 26, 2024 •

edited

Loading