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.

Sign up for GitHub

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

Jump to bottom

feat(storage): public access prevention #7423

Merged
merged 4 commits into from Jun 28, 2021
Merged

feat(storage): public access prevention #7423

merged 4 commits into from Jun 28, 2021

Conversation

frankyn
Copy link
Member

@frankyn frankyn commented Dec 14, 2020 •
edited

Samples for Public Access Prevention.

Blocked until PR (googleapis/java-storage#636) is released.

@snippet-bot
Copy link

snippet-bot bot commented Dec 14, 2020 •
edited

Here is the summary of changes.

You are about to add 3 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 14, 2020
@frankyn frankyn force-pushed the publicaccessprevention branch 5 times, most recently from 1bfc1c5 to 9287c98 Compare December 14, 2020 07:33
@frankyn frankyn requested a review from tritone December 14, 2020 07:33
@frankyn frankyn changed the title Publicaccessprevention feat(storage): public access prevention Dec 14, 2020
@frankyn frankyn added the status: blocked Resolving the issue is dependent on other work. label Dec 14, 2020
@frankyn frankyn marked this pull request as ready for review December 14, 2020 19:37
@frankyn frankyn requested a review from a team as a code owner December 14, 2020 19:37
tritone
tritone reviewed Dec 14, 2020
View reviewed changes
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Looks good!

...mples/src/main/java/com/google/cloud/examples/storage/buckets/GetPublicAccessPrevention.java Outdated
Storage storage = StorageOptions.newBuilder().setProjectId(projectId).build().getService();
Bucket bucket =
storage.get(
bucketName, Storage.BucketGetOption.fields(Storage.BucketField.IAMCONFIGURATION));
Copy link
Contributor

Choose a reason for hiding this comment

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

This field should be present regardless (unless something in Java specifically works differently).

Copy link
Member Author

Choose a reason for hiding this comment

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

Testing, I thought only a subset of fields were selected in the Java client. I'll confirm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Field is always present! My mistake and fixed.

...loud-examples/src/test/java/com/google/cloud/examples/storage/snippets/ITBucketSnippets.java Outdated
public void testSetPublicAccessPreventionEnforced() {
SetPublicAccessPreventionEnforced.setPublicAccessPreventionEnforced(PROJECT_ID, BUCKET);
assertEquals(
storage.get(BUCKET).getIamConfiguration().getPublicAccessPrevention(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this re-fetch from GCS?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it does.

shaffeeullah
shaffeeullah reviewed Dec 15, 2020
View reviewed changes
...loud-examples/src/test/java/com/google/cloud/examples/storage/snippets/ITBucketSnippets.java
}

@Test
public void testSetPublicAccessPreventionUnspecified() {
Copy link

Choose a reason for hiding this comment

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

Should these tests be verifying public access prevention without the API call to getPublicAccessPrevention? This is testing the setPublicAccessPreventionUnspecified function, but also relies on the functionality of getPublicAccessPrevention. Not sure what practices are on how small a unit should be for each unit test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point, I'll decouple.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah for these tests, I keep them simple for samples. This case is covered now by library integration tests.

@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Dec 15, 2020
BenWhitehead
BenWhitehead approved these changes Mar 1, 2021
View reviewed changes
Copy link
Contributor

@BenWhitehead BenWhitehead left a comment

Choose a reason for hiding this comment

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

Couple small questions, but otherwise LGTM

...loud-examples/src/test/java/com/google/cloud/examples/storage/snippets/ITBucketSnippets.java Outdated
assertEquals(
storage.get(BUCKET).getIamConfiguration().getPublicAccessPrevention(),
BucketInfo.PublicAccessPrevention.ENFORCED);
storage
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this call chain setting the Bucket back to UNSPECIFIED should be in a finally block so that it will execute even if the above sample run or assertion fails for any reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed.

...loud-examples/src/test/java/com/google/cloud/examples/storage/snippets/ITBucketSnippets.java Outdated

@Test
public void testSetPublicAccessPreventionEnforced() {
SetPublicAccessPreventionEnforced.setPublicAccessPreventionEnforced(PROJECT_ID, BUCKET);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call out here why we don't need to first set the bucket to UNSPECIFIED before this line? (In the test case for unspecified it does set enforced before setting UNSPACIFIED)

Copy link
Member Author

Choose a reason for hiding this comment

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

attempted to address.

@shaffeeullah
Copy link

@frankyn & @BenWhitehead ,

We would like to merge these samples next week after the client library has been released. Does that work for you?

@shaffeeullah
Copy link

Public access prevention rollout has been delayed due to a bug surfaced during Googler preview. I will keep this PR updated as I learn new release timeline details.

@shaffeeullah
Copy link

@BenWhitehead These samples can now be merged and released.

@BenWhitehead BenWhitehead merged commit 7b19e26 into master Jun 28, 2021
@BenWhitehead BenWhitehead deleted the publicaccessprevention branch June 28, 2021 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenWhitehead BenWhitehead BenWhitehead approved these changes

@shaffeeullah shaffeeullah shaffeeullah approved these changes

@tritone tritone Awaiting requested review from tritone

@JesseLovelace JesseLovelace Awaiting requested review from JesseLovelace

Assignees
No one assigned
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement. status: blocked Resolving the issue is dependent on other work.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@frankyn @shaffeeullah @BenWhitehead @tritone