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

Adds support for Public Access Prevention #10670

Conversation

jackwhelpton
Copy link

No description provided.

@jackwhelpton
Copy link
Author

If this looks good I'll probably need some guidance getting it applied correctly. From a preliminary reading it sounds like I should be modifying this file instead:

https://github.com/GoogleCloudPlatform/magic-modules/blob/master/mmv1/third_party/terraform/resources/resource_storage_bucket.go

and renaming that to resource_storage_bucket.go.erb to ensure it applies only to the beta provider. I'm not totally clear yet why the corresponding test file doesn't seem to exist in the magic-modules repo.

@rileykarson
Copy link
Collaborator

That file's at https://github.com/GoogleCloudPlatform/magic-modules/blob/master/mmv1/third_party/terraform/tests/resource_storage_bucket_test.go!

@jackwhelpton
Copy link
Author

Great, thanks. Do you want me to start on a PR in that magic-modules repo based on what's here? Does it look correct?

@ScottSuarez
Copy link
Collaborator

taking a look now ~ thanks for your patience

Copy link
Collaborator

@ScottSuarez ScottSuarez left a comment

Choose a reason for hiding this comment

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

Only concerns that I have currently as long as tests pass. If you could make a pull request at the upstream repository as mentioned and tag me that would be most appreciated.

Comment on lines +1132 to 1138
cfg := &storage.BucketIamConfiguration{
ForceSendFields: []string{"UniformBucketLevelAccess"},
UniformBucketLevelAccess: &storage.BucketIamConfigurationUniformBucketLevelAccess{
Enabled: d.Get("uniform_bucket_level_access").(bool),
ForceSendFields: []string{"Enabled"},
},
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is changing existing behavior... we should craft the object manually so that we only configure the object with these properties if UniformBucketLevelAccess is present on the config. I recommend checking uniform_bucket_level_access and configuring it appropriately.

Copy link
Author

Choose a reason for hiding this comment

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

Isn't that what the code currently does at L592? expandIamConfiguration is only called if:

	if d.HasChange("uniform_bucket_level_access") || d.HasChange("public_access_prevention") {
		sb.IamConfiguration = expandIamConfiguration(d)
	}

This check was already there, I added the test for public_access_prevention.

The tests look promising locally, although the CI/CD workflow looks like it still needs approval. I'll prepare the upstream changes now.

Copy link
Author

Choose a reason for hiding this comment

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

Raised GoogleCloudPlatform/magic-modules#5519 and tagged you there. Thanks!

@ScottSuarez
Copy link
Collaborator

I've requested the relevant changes and expanded on my requests there... closing this pr as it is superseded by the new one.

@github-actions
Copy link

github-actions bot commented Jan 6, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants