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

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

Merged

Conversation

nbugden
Copy link
Contributor

@nbugden nbugden commented Apr 3, 2024

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

Copy link
Contributor

@fmichaelobrien fmichaelobrien left a comment

Choose a reason for hiding this comment

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

/lgtm

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

@fmichaelobrien fmichaelobrien left a comment

Choose a reason for hiding this comment

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

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

nbugden commented Apr 11, 2024

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

Copy link
Contributor

@fmichaelobrien fmichaelobrien left a comment

Choose a reason for hiding this comment

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

/lgtm

@nbugden
Copy link
Contributor Author

nbugden commented Apr 17, 2024

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

@nbugden
Copy link
Contributor Author

nbugden commented Apr 24, 2024

@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.

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

@gtsorbo gtsorbo left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@daniel-cit daniel-cit left a comment

Choose a reason for hiding this comment

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

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

3-networks-dual-svpc/envs/production/main.tf Outdated Show resolved Hide resolved
0-bootstrap/outputs.tf Show resolved Hide resolved
1-org/envs/shared/log_sinks.tf Outdated Show resolved Hide resolved
0-bootstrap/variables.tf Show resolved Hide resolved
@nbugden nbugden force-pushed the fix/bootstrap-default-region branch from b8a8241 to b8e66b6 Compare April 26, 2024 17:24
@nbugden
Copy link
Contributor Author

nbugden commented Apr 26, 2024

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

@daniel-cit
Copy link
Contributor

daniel-cit commented Apr 26, 2024

@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
Copy link
Contributor

@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 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
@nbugden
Copy link
Contributor Author

nbugden commented Apr 26, 2024

@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.

@daniel-cit
Copy link
Contributor

/gcbrun

@daniel-cit
Copy link
Contributor

/gcbrun

@nbugden
Copy link
Contributor Author

nbugden commented May 22, 2024

both CI pipelines failed once more 😢

@apeabody
Copy link
Contributor

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

Already opened GoogleCloudPlatform/cloud-foundation-toolkit#2371

@daniel-cit
Copy link
Contributor

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

@bdashrad
Copy link
Contributor

we're excited to get this one in!

@bdashrad
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

nbugden commented May 24, 2024

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

@daniel-cit
Copy link
Contributor

/gcbrun

Copy link
Contributor

@daniel-cit daniel-cit left a comment

Choose a reason for hiding this comment

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

LGTM

@daniel-cit
Copy link
Contributor

/gcbrun

@nbugden
Copy link
Contributor Author

nbugden commented May 24, 2024

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

@daniel-cit
Copy link
Contributor

/gcbrun

@daniel-cit
Copy link
Contributor

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

@daniel-cit
Copy link
Contributor

/gcbrun

@nbugden
Copy link
Contributor Author

nbugden commented May 27, 2024

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
Copy link
Contributor

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
Copy link
Contributor

/gcbrun

@daniel-cit
Copy link
Contributor

/gcbrun

@eeaton eeaton dismissed gtsorbo’s stale review May 29, 2024 13:58

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

@eeaton eeaton merged commit 105fe52 into terraform-google-modules:master May 29, 2024
7 checks passed
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.

7 participants