fix: added gcs name validation

[nbugden]

Fixes hashicorp/terraform-provider-google#17831

Release Note Template for Downstream PRs (will be copied)

storage: added validation to `name` field in `google_storage_bucket` resource

[github-actions]

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@zli82016, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

[zli82016]

/gcbrun

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 3 files changed, 80 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 3 files changed, 80 insertions(+), 4 deletions(-))

[daniel-cit]

is this the same validation

https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.erb#L581C1-L583C3

that was implented in #6250 ?

[nbugden]

@daniel-cit there is some overlap. The differences with #6250 include:

• The original validations DON'T run when terraform plan is run. They are only run when attempting to create the bucket as they are part of the resourceStorageBucketCreate function.
• The original validations are also incomplete as they do not apply all of the rules from the docs

IMO it would make sense to keep the validations in one place. I tried to follow the convention of the other validations which were in the verify package. I'd be happy to remove the changes from #6250 to deduplicate validations if that's preferred.

[modular-magician]

Tests analytics

Total tests: 3572
Passed tests: 3204
Skipped tests: 368
Affected tests: 0

Click here to see the affected service packages

all service packages are affected

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log

[nbugden]

@daniel-cit just pushed 8b1cbd0 which removes the CheckGCSName function in favor of the validations proposed in this PR.

[daniel-cit]

@daniel-cit just pushed 8b1cbd0 which removes the CheckGCSName function in favor of the validations proposed in this PR.

Maybe it would be good to also cover the same test cases from the old validation code

https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/tpgresource/utils_test.go#L1262C1-L1285C36

to ensure that the changes are compatible.

[daniel-cit]

@nbugden
what would happen if the name of the bucket uses a random resource to compose the name like

resource "random_string" "suffix" {
  length  = 64
  numeric = true
  lower   = false
  upper   = false
  special = false
}

resource "google_storage_bucket" "after_plan" {
  name          = "plan-${random_string.suffix.result}"
  location      = "US"
}

the value random_string.suffix.result will only be known at apply(create) time.

in this case, will the resource delay validation to the apply phase ?

Will the new validation be used in all the execution flows: read, create, update, and destroy?

Using the current version of the resource, the execution will fail on the apply phase

Plan: 2 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

random_string.suffix: Creating...
random_string.suffix: Creation complete after 0s [id=9204249628951144225952644330474297575131248816450437079420460938]
google_storage_bucket.after_plan: Creating...
╷
│ Error: error: bucket name validation failed plan-9204249628951144225952644330474297575131248816450437079420460938
│ 
│   with google_storage_bucket.after_plan,
│   on bucket.tf line 9, in resource "google_storage_bucket" "after_plan":
│    9: resource "google_storage_bucket" "after_plan" {
│ 

however, since the validation is only on the create function for the current resource, it is possible to fix this error reducing the length of the random_string

resource "random_string" "suffix" {
  length  = 34
  numeric = true
  lower   = false
  upper   = false
  special = false
}

  # random_string.suffix must be replaced
-/+ resource "random_string" "suffix" {
      ~ id          = "9204249628951144225952644330474297575131248816450437079420460938" -> (known after apply)
      ~ length      = 64 -> 34 # forces replacement
      ~ result      = "9204249628951144225952644330474297575131248816450437079420460938" -> (known after apply)
        # (9 unchanged attributes hidden)
    }

Plan: 2 to add, 0 to change, 1 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

random_string.suffix: Destroying... [id=9204249628951144225952644330474297575131248816450437079420460938]
random_string.suffix: Destruction complete after 0s
random_string.suffix: Creating...
random_string.suffix: Creation complete after 0s [id=2439292337254968112779191630127567]
google_storage_bucket.after_plan: Creating...
google_storage_bucket.after_plan: Creation complete after 2s [id=plan-2439292337254968112779191630127567]

Apply complete! Resources: 2 added, 0 changed, 1 destroyed.

will this behavior be preserved with the new validation ?

Sorry asking all these questions but I was affect by the initial version of the validation and I would like this change to be a smooth one.

[nbugden]

Maybe it would be good to also cover the same test cases from the old validation code
https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/tpgresource/utils_test.go#L1262C1-L1285C36
to ensure that the changes are compatible.

@daniel-cit b3f3d9e adds the tests for parity from CheckGCSName

[nbugden]

what would happen if the name of the bucket uses a random resource to compose the name

@daniel-cit in that case since the name cannot be known prior to apply the validation is not run during the plan (see below)

Terraform Config

resource "random_string" "suffix" {
  length  = 64
  numeric = true
  lower   = false
  upper   = false
  special = false
}

resource "google_storage_bucket" "after_plan" {
  name     = "plan-${random_string.suffix.result}"
  location = "US"
}

Terraform Plan

Terraform Command

TF_CLI_CONFIG_FILE="$HOME/terraform/magic-modules-dev-override.tfrc" terraform plan -target='google_storage_bucket.after_plan'

Output

│ Warning: Provider development overrides are in effect
│ 
│ The following provider
│ development overrides are
│ set in the CLI
│ configuration:
│  - hashicorp/google in /Users/nathan.bugden/go/bin
│ 
│ The behavior may
│ therefore not match any
│ released version of the
│ provider and applying
│ changes may cause the
│ state to become
│ incompatible with
│ published releases.
╵

Terraform used the selected
providers to generate the
following execution plan.
Resource actions are
indicated with the
following symbols:
  + create

Terraform will perform the following actions:

  # google_storage_bucket.after_plan will be created
  + resource "google_storage_bucket" "after_plan" {
      + effective_labels            = (known after apply)
      + force_destroy               = false
      + id                          = (known after apply)
      + location                    = "US"
      + name                        = (known after apply)
      + project                     = (known after apply)
      + project_number              = (known after apply)
      + public_access_prevention    = (known after apply)
      + rpo                         = (known after apply)
      + self_link                   = (known after apply)
      + storage_class               = "STANDARD"
      + terraform_labels            = (known after apply)
      + uniform_bucket_level_access = (known after apply)
      + url                         = (known after apply)
    }

  # random_string.suffix will be created
  + resource "random_string" "suffix" {
      + id          = (known after apply)
      + length      = 64
      + lower       = false
      + min_lower   = 0
      + min_numeric = 0
      + min_special = 0
      + min_upper   = 0
      + number      = true
      + numeric     = true
      + result      = (known after apply)
      + special     = false
      + upper       = false
    }

Plan: 2 to add, 0 to change, 0 to destroy.

Terraform Apply

When the same configuration is applied, the validation is run prior to creating the bucket.

Terraform Command

TF_CLI_CONFIG_FILE="$HOME/terraform/magic-modules-dev-override.tfrc" terraform apply -target='google_storage_bucket.after_plan'

Terraform Output

│ Warning: Provider development overrides are in effect
│ 
│ The following provider
│ development overrides are
│ set in the CLI
│ configuration:
│  - hashicorp/google in /Users/nathan.bugden/go/bin
│ 
│ The behavior may
│ therefore not match any
│ released version of the
│ provider and applying
│ changes may cause the
│ state to become
│ incompatible with
│ published releases.
╵

Terraform used the selected
providers to generate the
following execution plan.
Resource actions are
indicated with the
following symbols:
  + create

Terraform will perform the following actions:

  # google_storage_bucket.after_plan will be created
  + resource "google_storage_bucket" "after_plan" {
      + effective_labels            = (known after apply)
      + force_destroy               = false
      + id                          = (known after apply)
      + location                    = "US"
      + name                        = (known after apply)
      + project                     = (known after apply)
      + project_number              = (known after apply)
      + public_access_prevention    = (known after apply)
      + rpo                         = (known after apply)
      + self_link                   = (known after apply)
      + storage_class               = "STANDARD"
      + terraform_labels            = (known after apply)
      + uniform_bucket_level_access = (known after apply)
      + url                         = (known after apply)
    }

  # random_string.suffix will be created
  + resource "random_string" "suffix" {
      + id          = (known after apply)
      + length      = 64
      + lower       = false
      + min_lower   = 0
      + min_numeric = 0
      + min_special = 0
      + min_upper   = 0
      + number      = true
      + numeric     = true
      + result      = (known after apply)
      + special     = false
      + upper       = false
    }

Plan: 2 to add, 0 to change, 0 to destroy.
╷
│ Warning: Resource targeting is in effect
│ 
│ You are creating a plan
│ with the -target option,
│ which means that the
│ result of this plan may
│ not represent all of the
│ changes requested by the
│ current configuration.
│ 
│ The -target option is not
│ for routine use, and is
│ provided only for
│ exceptional situations
│ such as recovering from
│ errors or mistakes, or
│ when Terraform
│ specifically suggests to
│ use it as part of an
│ error message.
╵

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

random_string.suffix: Creating...
random_string.suffix: Creation complete after 0s [id=0808056569454331236561064499984914160477239699147199885142821831]
╷
│ Warning: Applied changes may be incomplete
│ 
│ The plan was created with
│ the -target option in
│ effect, so some changes
│ requested in the
│ configuration may have
│ been ignored and the
│ output values may not be
│ fully updated. Run the
│ following command to
│ verify that no other
│ changes are pending:
│     terraform plan
│ 
│ Note that the -target
│ option is not suitable
│ for routine use, and is
│ provided only for
│ exceptional situations
│ such as recovering from
│ errors or mistakes, or
│ when Terraform
│ specifically suggests to
│ use it as part of an
│ error message.
╵
╷
│ Error: "plan-0808056569454331236561064499984914160477239699147199885142821831" name value must contain 3-63 characters. Names containing dots can contain up to 222 characters, but each dot-separated component can be no longer than 63 characters
│ 
│   with google_storage_bucket.after_plan,
│   on test.tf line 10, in resource "google_storage_bucket" "after_plan":
│   10:   name     = "plan-${random_string.suffix.result}"

[nbugden]

however, since the validation is only on the create function for the current resource, it is possible to fix this error reducing the length of the random_string
since the validation is only on the create function for the current resource, it is possible to fix this error reducing the length of the random_string

@daniel-cit yes, updating the length of the suffix will fix the error as you mentioned. See output below.

Updating the Random Suffix Length

Updated Terraform Config

resource "random_string" "suffix" {
  length  = 34
  numeric = true
  lower   = false
  upper   = false
  special = false
}

Terraform Command

TF_CLI_CONFIG_FILE="$HOME/terraform/magic-modules-dev-override.tfrc" terraform apply -target='google_storage_bucket.after_plan'

Terraform Output

Terraform used the selected
providers to generate the
following execution plan.
Resource actions are
indicated with the
following symbols:
  + create
-/+ destroy and then create replacement

Terraform will perform the following actions:

  # google_storage_bucket.after_plan will be created
  + resource "google_storage_bucket" "after_plan" {
      + effective_labels            = (known after apply)
      + force_destroy               = false
      + id                          = (known after apply)
      + location                    = "US"
      + name                        = (known after apply)
      + project                     = "prj-c-scc-5xv2"
      + project_number              = (known after apply)
      + public_access_prevention    = (known after apply)
      + rpo                         = (known after apply)
      + self_link                   = (known after apply)
      + storage_class               = "STANDARD"
      + terraform_labels            = (known after apply)
      + uniform_bucket_level_access = true
      + url                         = (known after apply)
    }

  # random_string.suffix must be replaced
-/+ resource "random_string" "suffix" {
      ~ id          = "4459658187874002848905409602733573523031843089159360765646519166" -> (known after apply)
      ~ length      = 64 -> 34 # forces replacement
      ~ result      = "4459658187874002848905409602733573523031843089159360765646519166" -> (known after apply)
        # (9 unchanged attributes hidden)
    }

Plan: 2 to add, 0 to change, 1 to destroy.
╷
│ Warning: Resource targeting is in effect
│ 
│ You are creating a plan
│ with the -target option,
│ which means that the
│ result of this plan may
│ not represent all of the
│ changes requested by the
│ current configuration.
│ 
│ The -target option is not
│ for routine use, and is
│ provided only for
│ exceptional situations
│ such as recovering from
│ errors or mistakes, or
│ when Terraform
│ specifically suggests to
│ use it as part of an
│ error message.
╵

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

random_string.suffix: Destroying... [id=4459658187874002848905409602733573523031843089159360765646519166]
random_string.suffix: Destruction complete after 0s
random_string.suffix: Creating...
random_string.suffix: Creation complete after 0s [id=4905591332324978645088960881522615]
google_storage_bucket.after_plan: Creating...
google_storage_bucket.after_plan: Creation complete after 2s [id=plan-4905591332324978645088960881522615]
╷
│ Warning: Applied changes may be incomplete
│ 
│ The plan was created with
│ the -target option in
│ effect, so some changes
│ requested in the
│ configuration may have
│ been ignored and the
│ output values may not be
│ fully updated. Run the
│ following command to
│ verify that no other
│ changes are pending:
│     terraform plan
│ 
│ Note that the -target
│ option is not suitable
│ for routine use, and is
│ provided only for
│ exceptional situations
│ such as recovering from
│ errors or mistakes, or
│ when Terraform
│ specifically suggests to
│ use it as part of an
│ error message.
╵

Apply complete! Resources: 2 added, 0 changed, 1 destroyed.

[nbugden]

Sorry asking all these questions but I was affect by the initial version of the validation and I would like this change to be a smooth one.

@daniel-cit no problem! They are great questions. I agree it's better to have a smooth transition

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 94 insertions(+), 74 deletions(-))
google-beta provider: Diff ( 5 files changed, 94 insertions(+), 74 deletions(-))

[modular-magician]

Tests analytics

Total tests: 3576
Passed tests: 3207
Skipped tests: 368
Affected tests: 1

Click here to see the affected service packages

all service packages are affected

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccDataprocClusterIamPolicy

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$
View the build log or the debug log for each test

[modular-magician]

Tests analytics

Total tests: 3577
Passed tests: 3208
Skipped tests: 368
Affected tests: 1

Click here to see the affected service packages

all service packages are affected

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccDataSourceGoogleServiceAccountIdToken_impersonation

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataSourceGoogleServiceAccountIdToken_impersonation[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 94 insertions(+), 74 deletions(-))
google-beta provider: Diff ( 5 files changed, 94 insertions(+), 74 deletions(-))

[modular-magician]

Tests analytics

Total tests: 3577
Passed tests: 3209
Skipped tests: 368
Affected tests: 0

Click here to see the affected service packages

all service packages are affected

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log

[zli82016]

LGTM. Thanks.

[zli82016]

@daniel-cit, do you have any other comments?

[daniel-cit]

@daniel-cit, do you have any other comments?

@zli82016 No comments, LGTM