fix: (storage) added validation for cors parameters method, origin and response_header cannot be empty #14482

gurusai-voleti · 2025-07-09T13:50:09Z

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.
Fixes: hashicorp/terraform-provider-google#15280

storage: added validation for cors parameters method, origin and response_header cannot be empty

modular-magician · 2025-07-09T14:01:51Z

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 ( 1 file changed, 18 insertions(+))
google-beta provider: Diff ( 1 file changed, 18 insertions(+))

modular-magician · 2025-07-09T15:29:04Z

Tests analytics

Total tests: 123
Passed tests: 112
Skipped tests: 10
Affected tests: 1

Click here to see the affected service packages

#### 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

Get to know how VCR tests work

modular-magician · 2025-07-09T15:29:26Z

🔴 Tests failed during RECORDING mode:
TestAccStorageBucket_cors [Error message] [Debug log]

🔴 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 · 2025-07-11T11:14:12Z

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 ( 1 file changed, 32 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 1 file changed, 32 insertions(+), 2 deletions(-))

modular-magician · 2025-07-11T11:26:53Z

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 ( 1 file changed, 31 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 1 file changed, 31 insertions(+), 2 deletions(-))

modular-magician · 2025-07-11T11:28:01Z

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 ( 1 file changed, 31 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 1 file changed, 31 insertions(+), 2 deletions(-))

modular-magician · 2025-07-11T12:40:47Z

Tests analytics

Total tests: 123
Passed tests: 113
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

modular-magician · 2025-07-11T12:53:36Z

Tests analytics

Total tests: 123
Passed tests: 113
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

modular-magician · 2025-07-11T12:55:32Z

Tests analytics

Total tests: 123
Passed tests: 113
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

modular-magician · 2025-07-11T14:53:25Z

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 ( 1 file changed, 30 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 1 file changed, 30 insertions(+), 2 deletions(-))

modular-magician · 2025-07-11T16:20:20Z

Tests analytics

Total tests: 123
Passed tests: 113
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

modular-magician · 2025-07-16T09:01:59Z

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 ( 1 file changed, 30 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 1 file changed, 30 insertions(+), 2 deletions(-))

modular-magician · 2025-07-16T10:11:26Z

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 ( 2 files changed, 61 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 2 files changed, 61 insertions(+), 4 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_storage_bucket (604 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_storage_bucket" "primary" {
  cors {
    send_max_age_seconds_if_zero = # value needed
  }
}

modular-magician · 2025-07-16T10:29:28Z

Tests analytics

Total tests: 123
Passed tests: 113
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

modular-magician · 2025-07-16T10:39:48Z

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, 67 insertions(+), 9 deletions(-))
google-beta provider: Diff ( 3 files changed, 67 insertions(+), 9 deletions(-))

modular-magician · 2025-07-16T11:23:29Z

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, 70 insertions(+), 10 deletions(-))
google-beta provider: Diff ( 3 files changed, 70 insertions(+), 10 deletions(-))

modular-magician · 2025-07-16T11:38:58Z

Tests analytics

Total tests: 123
Passed tests: 113
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

modular-magician · 2025-07-16T12:07:20Z

Tests analytics

Total tests: 123
Passed tests: 112
Skipped tests: 10
Affected tests: 1

Click here to see the affected service packages

#### 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

Get to know how VCR tests work

modular-magician · 2025-07-16T12:08:10Z

🟢 Tests passed during RECORDING mode:
TestAccStorageBucket_cors [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

🟢 All tests passed!

View the build log or the debug log for each test

github-actions · 2025-07-17T07:31:32Z

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

Googlers: For automatic test runs see go/terraform-auto-test-runs.

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

kautikdk

While I agree that validation logic will work in this case, can we not fix the issue with diffSupressFunc? If that works, we don't have to go through separate customize diff and validation logic from client side.

If CustomizeDiff is the only solution, I would recommend adding unit and acceptance tests for the same. Current tests are essentially not validating the issue scenario rather they just ensure working of the new field.

kautikdk · 2025-07-17T09:24:07Z

mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.tmpl

@@ -2208,8 +2256,10 @@ func setStorageBucket(d *schema.ResourceData, config *transport_tpg.Config, res
 	if err := d.Set("location", res.Location); err != nil {
 		return fmt.Errorf("Error setting location: %s", err)
 	}
-	if err := d.Set("cors", flattenCors(res.Cors)); err != nil {
-		return fmt.Errorf("Error setting cors: %s", err)
+	if res.Cors != nil {


In case of nil value of CORS, can we set empty empty list similar to lifecycle rules?

added empty list

modular-magician · 2025-07-18T09:11:14Z

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, 70 insertions(+), 9 deletions(-))
google-beta provider: Diff ( 3 files changed, 70 insertions(+), 9 deletions(-))

modular-magician · 2025-07-18T10:38:52Z

Tests analytics

Total tests: 123
Passed tests: 113
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

github-actions · 2025-07-22T09:45:48Z

@melinath This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

melinath · 2025-07-22T17:26:02Z

mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.tmpl

+func corsValidation(_ context.Context, d *schema.ResourceDiff, meta interface{}) error {
+	cors, corsOk := d.GetOk("cors")
+	if corsOk && len(cors.([]interface{})) == 0 {
+		return fmt.Errorf("cors cannot be empty")
+	}
+	for _, corsRule := range cors.([]interface{}) {
+		if corsRule == nil {
+			return fmt.Errorf("cors cannot be empty")
+		}
+		rule := corsRule.(map[string]interface{})
+		maxAgeSeconds := rule["max_age_seconds"].(int)
+		sendMaxAge := rule["send_max_age_seconds_if_zero"].(bool)
+		// if send_max_age_seconds_if_zero is true, max_age_seconds can be zero
+		if sendMaxAge || maxAgeSeconds > 0 {
+			continue
+		}
+
+		method := rule["method"].([]interface{})
+		responseHeader := rule["response_header"].([]interface{})
+		origin := rule["origin"].([]interface{})
+
+		// check if all cors attributes provided have values
+		if !(len(method) > 0 || len(origin) > 0 || len(responseHeader) > 0) {
+			return fmt.Errorf("cors attributes cannot be empty or use send_max_age_seconds_if_zero flag to send max_age_seconds as zero")
+		}
+	}
+	return nil
+}


Just want to verify: are the error cases here things that would otherwise be API errors? Otherwise this would be a breaking change.

these are not replication API errors even API accepts empty values for cors field but in API response we wont get anything back for cors field, so validating here to ensure values are provided and not empty

If it's valid from the API's perspective, we can't add stricter client-side validation once the field in question is introduced. Since cors is already released, adding validation to it would be a breaking change.

melinath

I think CustomizeDiff is necessary since this needs to take multiple fields into account. +1 to this point - please add unit tests.

gurusai-voleti · 2025-07-23T04:52:45Z

List types cannot be handled by diffsuppress func as per terraform SDK its not recommended approach

modular-magician · 2025-07-23T05:15:52Z

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, 70 insertions(+), 9 deletions(-))
google-beta provider: Diff ( 3 files changed, 70 insertions(+), 9 deletions(-))

modular-magician · 2025-07-23T06:43:21Z

Tests analytics

Total tests: 123
Passed tests: 113
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

melinath

Marking as changes requested due to the feedback above.

Just to clarify, I don't think this is correct - diffsuppress definitely works for lists - but diffsuppress can't handle validation across multiple fields.

gurusai-voleti · 2025-07-23T17:06:54Z

sorry it supports, I tried diffsuppress first but seems not feasible for this solution to handle multiple fields, customize diff seems a better option here as multiple fields needed for validation

melinath

removing from my queue for now.

gurusai-voleti · 2025-07-24T06:03:32Z

Diffsuppress only provides way to show diff or not in tf plan but it will not to throw any error, here we need to validate the data and let customer/user know about the data validation error, so customDiff seems a feasible solution than diffsuppress, please correct me if I am wrong

modular-magician · 2025-07-24T08:53:01Z

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, 247 insertions(+), 9 deletions(-))
google-beta provider: Diff ( 3 files changed, 247 insertions(+), 9 deletions(-))

modular-magician · 2025-07-24T08:55:39Z

Tests analytics

Total tests: 0
Passed tests: 0
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

🔴 Errors occurred during REPLAYING mode. Please fix them to complete your PR.

View the build log

modular-magician · 2025-07-24T09:36:50Z

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, 249 insertions(+), 9 deletions(-))
google-beta provider: Diff ( 3 files changed, 249 insertions(+), 9 deletions(-))

modular-magician · 2025-07-24T11:03:46Z

Tests analytics

Total tests: 123
Passed tests: 113
Skipped tests: 10
Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

melinath · 2025-07-24T15:38:42Z

yes, that's also correct. Single-field validation would use validation functions, not diff suppress. 🤦

storage add validation for cors

c2e3dff

modular-magician added the service/storage label Jul 9, 2025

modular-magician requested a review from kautikdk July 9, 2025 14:01

updates

4e5717a

gurusai-voleti added 2 commits July 11, 2025 11:15

update

b1b1779

update

b02d3ff

update

0b2ef5f

Merge branch 'GoogleCloudPlatform:main' into cors_validation

eb62dbc

update

655cfb6

update

5777c3b

expandcors update

0c9872e

gurusai-voleti marked this pull request as ready for review July 17, 2025 07:30

github-actions bot requested a review from melinath July 17, 2025 07:31

kautikdk reviewed Jul 17, 2025

View reviewed changes

address review comments

e87c973

modular-magician requested a review from kautikdk July 18, 2025 09:11

melinath reviewed Jul 22, 2025

View reviewed changes

melinath requested changes Jul 22, 2025

View reviewed changes

github-actions bot requested a review from melinath July 23, 2025 04:53

Merge branch 'GoogleCloudPlatform:main' into cors_validation

1fbe924

added unit test cases

ca15984

melinath requested changes Jul 23, 2025

View reviewed changes

github-actions bot requested a review from melinath July 23, 2025 17:08

melinath requested changes Jul 23, 2025

View reviewed changes

github-actions bot requested a review from melinath July 24, 2025 06:04

update

7019940

update

a3b2b50

fix: (storage) added validation for cors parameters method, origin and response_header cannot be empty #14482

Are you sure you want to change the base?

fix: (storage) added validation for cors parameters method, origin and response_header cannot be empty #14482

Conversation

gurusai-voleti commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

modular-magician commented Jul 9, 2025

Diff report

Uh oh!

modular-magician commented Jul 9, 2025

Tests analytics

Uh oh!

modular-magician commented Jul 9, 2025

Uh oh!

modular-magician commented Jul 11, 2025

Diff report

Uh oh!

modular-magician commented Jul 11, 2025

Diff report

Uh oh!

modular-magician commented Jul 11, 2025

Diff report

Uh oh!

modular-magician commented Jul 11, 2025

Tests analytics

Uh oh!

modular-magician commented Jul 11, 2025

Tests analytics

Uh oh!

modular-magician commented Jul 11, 2025

Tests analytics

Uh oh!

modular-magician commented Jul 11, 2025

Diff report

Uh oh!

modular-magician commented Jul 11, 2025

Tests analytics

Uh oh!

modular-magician commented Jul 16, 2025

Diff report

Uh oh!

modular-magician commented Jul 16, 2025

Diff report

Missing test report

Uh oh!

modular-magician commented Jul 16, 2025

Tests analytics

Uh oh!

modular-magician commented Jul 16, 2025

Diff report

Uh oh!

modular-magician commented Jul 16, 2025

Diff report

Uh oh!

modular-magician commented Jul 16, 2025

Tests analytics

Uh oh!

modular-magician commented Jul 16, 2025

Tests analytics

Uh oh!

modular-magician commented Jul 16, 2025

Uh oh!

github-actions bot commented Jul 17, 2025

Uh oh!

kautikdk left a comment

Choose a reason for hiding this comment

Uh oh!

kautikdk Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

gurusai-voleti Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

modular-magician commented Jul 18, 2025

Diff report

Uh oh!

modular-magician commented Jul 18, 2025

Tests analytics

Uh oh!

gurusai-voleti commented Jul 9, 2025 •

edited

Loading

gurusai-voleti commented Jul 23, 2025 •

edited

Loading

gurusai-voleti commented Jul 23, 2025 •

edited

Loading

gurusai-voleti commented Jul 24, 2025 •

edited

Loading