search

terraform-google-modules / terraform-example-foundation

Shows how the CFT modules can be composed to build a secure cloud foundation
https://cloud.google.com/architecture/security-foundations
Apache License 2.0
1.21k stars 708 forks source link

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

Closed nbugden closed 3 months ago

nbugden commented 5 months ago

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: https://github.com/terraform-google-modules/terraform-example-foundation/issues/1172

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

daniel-cit commented 4 months ago

/gcbrun

daniel-cit commented 3 months ago

/gcbrun

daniel-cit commented 3 months ago

/gcbrun

daniel-cit commented 3 months ago

@nbugden Could you please check the conflicts? the files were deleted on master.

nbugden commented 3 months ago

@nbugden Could you please check the conflicts? the files were deleted on master.

@daniel-cit I merged in the latest master... ready for /gcbrun again

apeabody commented 3 months ago

/gcbrun

daniel-cit commented 3 months ago

/gcbrun

nbugden commented 3 months ago

@daniel-cit @apeabody need help with /gcbrun please

daniel-cit commented 3 months ago

/gcbrun

nbugden commented 3 months ago

@daniel-cit merged latest master again, needs /gcbrun

apeabody commented 3 months ago

/gcbrun

nbugden commented 3 months ago

One of the CI pipelines failed.

nbugden commented 3 months ago

Need /gcbrun... Merged latest master

daniel-cit commented 3 months ago

/gcbrun

daniel-cit commented 3 months ago

/gcbrun

nbugden commented 3 months ago

both CI pipelines failed once more 😢

apeabody commented 3 months ago

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

Already opened https://github.com/GoogleCloudPlatform/cloud-foundation-toolkit/pull/2371

daniel-cit commented 3 months ago

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

bdashrad commented 3 months ago

we're excited to get this one in!

bdashrad commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

daniel-cit commented 3 months ago

/gcbrun

daniel-cit commented 3 months ago

/gcbrun

nbugden commented 3 months ago

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

daniel-cit commented 3 months ago

/gcbrun

daniel-cit commented 3 months ago

build failing because of missing Access Context Manager Policy ID

1252

daniel-cit commented 3 months ago

/gcbrun

nbugden commented 3 months ago

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

@daniel-cit one of the CI workflows failed... are we still waiting on https://github.com/terraform-google-modules/terraform-example-foundation/pull/1252?

daniel-cit commented 3 months ago

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 commented 3 months ago

/gcbrun

daniel-cit commented 3 months ago

/gcbrun