Skip to content

feat: implement GrpcStorageImpl#deleteDefaultAcl #1807

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

Merged
merged 1 commit into from
Dec 15, 2022
Merged

feat: implement GrpcStorageImpl#deleteDefaultAcl #1807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion google-cloud-storage/src/main/java/com/google/cloud/storage/Bucket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,7 +1266,7 @@ public Acl getDefaultAcl(Entity entity) {
* @return {@code true} if the ACL was deleted, {@code false} if it was not found
* @throws StorageException upon failure
*/
@TransportCompatibility({Transport.HTTP})
@TransportCompatibility({Transport.HTTP, Transport.GRPC})
public boolean deleteDefaultAcl(Entity entity) {
return storage.deleteDefaultAcl(getName(), entity);
}
Expand Down
86 changes: 63 additions & 23 deletions google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -932,7 +932,40 @@ public Acl getDefaultAcl(String bucket, Entity entity) {

@Override
public boolean deleteDefaultAcl(String bucket, Entity entity) {
return throwNotYetImplemented(fmtMethodName("deleteDefaultAcl", String.class, Entity.class));
try {
com.google.storage.v2.Bucket resp = getBucketDefaultAcls(bucket);
String encode = codecs.entity().encode(entity);

Predicate<ObjectAccessControl> entityPredicate = objectAclEntityOrAltEq(encode);

List<ObjectAccessControl> currentDefaultAcls = resp.getDefaultObjectAclList();
ImmutableList<ObjectAccessControl> newDefaultAcls =
currentDefaultAcls.stream()
.filter(entityPredicate.negate())
.collect(ImmutableList.toImmutableList());
if (newDefaultAcls.equals(currentDefaultAcls)) {
// we didn't actually filter anything out, no need to send an RPC, simply return false
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment into the Storage interface javadocs that calls out this particular case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What should we add to the javadocs?

This is sort of specific to the fact that we're emulating what was previously done via a single blind RPC.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like I glossed over the need to make a request to get Acl List first; Why didn't you call the RPC to delete Acl instead of querying existing bucketdefaultAcl first?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are patching the full default acl list of the bucket.

  1. We get the full list
  2. filter out any existing acl for the specified entity
  3. If there is no change, quickly return (this line)
  4. Otherwise, create an update bucket request
  5. Send update bucket request
  6. return true if the acl is no longer in the buckets default acl from the response

}
long metageneration = resp.getMetageneration();

UpdateBucketRequest req = createUpdateRequest(bucket, newDefaultAcls, metageneration);

com.google.storage.v2.Bucket updateResult = updateBucket(req);
// read the response to ensure there is no longer an acl for the specified entity
Optional<ObjectAccessControl> first =
updateResult.getDefaultObjectAclList().stream().filter(entityPredicate).findFirst();
return !first.isPresent();
} catch (NotFoundException e) {
// HttpStorageRpc returns false if the bucket doesn't exist :(
return false;
} catch (StorageException se) {
if (se.getCode() == 404) {
return false;
} else {
throw se;
}
}
}

@Override
Expand All @@ -949,36 +982,17 @@ public Acl updateDefaultAcl(String bucket, Acl acl) {

Predicate<ObjectAccessControl> entityPredicate = objectAclEntityOrAltEq(entity);

ImmutableList<ObjectAccessControl> collect =
ImmutableList<ObjectAccessControl> newDefaultAcls =
Streams.concat(
resp.getDefaultObjectAclList().stream().filter(entityPredicate.negate()),
Stream.of(encode))
.collect(ImmutableList.toImmutableList());

com.google.storage.v2.Bucket update =
com.google.storage.v2.Bucket.newBuilder()
.setName(bucketNameCodec.encode(bucket))
.addAllDefaultObjectAcl(collect)
.build();
Opts<BucketTargetOpt> opts =
Opts.from(
UnifiedOpts.fields(ImmutableSet.of(BucketField.DEFAULT_OBJECT_ACL)),
UnifiedOpts.metagenerationMatch(resp.getMetageneration()));
UpdateBucketRequest req =
opts.updateBucketsRequest()
.apply(UpdateBucketRequest.newBuilder())
.setBucket(update)
.build();
createUpdateRequest(bucket, newDefaultAcls, resp.getMetageneration());

GrpcCallContext grpcCallContext = GrpcCallContext.createDefault();
com.google.storage.v2.Bucket updateResult =
Retrying.run(
getOptions(),
retryAlgorithmManager.getFor(req),
() -> storageClient.updateBucketCallable().call(req, grpcCallContext),
Decoder.identity());
com.google.storage.v2.Bucket updateResult = updateBucket(req);

//noinspection DataFlowIssue
Optional<Acl> first =
updateResult.getDefaultObjectAclList().stream()
.filter(entityPredicate)
Expand Down Expand Up @@ -1488,4 +1502,30 @@ private com.google.storage.v2.Bucket getBucketDefaultAcls(String bucketName) {
() -> storageClient.getBucketCallable().call(req, grpcCallContext),
Decoder.identity());
}

private com.google.storage.v2.Bucket updateBucket(UpdateBucketRequest req) {
GrpcCallContext grpcCallContext = GrpcCallContext.createDefault();
return Retrying.run(
getOptions(),
retryAlgorithmManager.getFor(req),
() -> storageClient.updateBucketCallable().call(req, grpcCallContext),
Decoder.identity());
}

private static UpdateBucketRequest createUpdateRequest(
String bucket, ImmutableList<ObjectAccessControl> newDefaultAcls, long metageneration) {
com.google.storage.v2.Bucket update =
com.google.storage.v2.Bucket.newBuilder()
.setName(bucketNameCodec.encode(bucket))
.addAllDefaultObjectAcl(newDefaultAcls)
.build();
Opts<BucketTargetOpt> opts =
Opts.from(
UnifiedOpts.fields(ImmutableSet.of(BucketField.DEFAULT_OBJECT_ACL)),
UnifiedOpts.metagenerationMatch(metageneration));
return opts.updateBucketsRequest()
.apply(UpdateBucketRequest.newBuilder())
.setBucket(update)
.build();
}
}
2 changes: 1 addition & 1 deletion google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3511,7 +3511,7 @@ PostPolicyV4 generateSignedPostPolicyV4(
* @return {@code true} if the ACL was deleted, {@code false} if it was not found
* @throws StorageException upon failure
*/
@TransportCompatibility({Transport.HTTP})
@TransportCompatibility({Transport.HTTP, Transport.GRPC})
boolean deleteDefaultAcl(String bucket, Entity entity);

/**
Expand Down
92 changes: 59 additions & 33 deletions google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITAccessTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.function.Predicate;
Expand Down Expand Up @@ -286,40 +287,65 @@ public void bucket_defaultAcl_update_bucket404() {
}

@Test
@CrossRun.Ignore(transports = Transport.GRPC)
public void testBucketDefaultAcl() {
// TODO: break this test up into each of the respective scenarios
// 2. Delete a default ACL for a specific entity
// 4. Update default ACL to change role of a specific entity

// according to https://cloud.google.com/storage/docs/access-control/lists#default
// it can take up to 30 seconds for default acl updates to propagate
// Since this test is performing so many mutations to default acls there are several calls
// that are otherwise non-idempotent wrapped with retries.
assertNull(
retry429s(
() -> storage.getDefaultAcl(bucket.getName(), User.ofAllAuthenticatedUsers()),
storage));
assertFalse(
retry429s(
() -> storage.deleteDefaultAcl(bucket.getName(), User.ofAllAuthenticatedUsers()),
storage));
Acl acl = Acl.of(User.ofAllAuthenticatedUsers(), Role.READER);
assertNotNull(retry429s(() -> storage.createDefaultAcl(bucket.getName(), acl), storage));
Acl updatedAcl =
retry429s(
() ->
storage.updateDefaultAcl(
bucket.getName(), acl.toBuilder().setRole(Role.OWNER).build()),
storage);
assertEquals(Role.OWNER, updatedAcl.getRole());
Set<Acl> acls = new HashSet<>(storage.listDefaultAcls(bucket.getName()));
assertTrue(acls.contains(updatedAcl));
assertTrue(
public void bucket_defaultAcl_delete() throws Exception {
BucketInfo bucketInfo = BucketInfo.newBuilder(generator.randomBucketName()).build();
try (TemporaryBucket tempB =
TemporaryBucket.newBuilder().setBucketInfo(bucketInfo).setStorage(storage).build()) {
BucketInfo bucket = tempB.getBucket();

List<Acl> defaultAcls = bucket.getDefaultAcl();
assertThat(defaultAcls).isNotEmpty();

Predicate<Acl> isProjectEditor = hasProjectRole(ProjectRole.VIEWERS);

//noinspection OptionalGetWithoutIsPresent
Acl projectViewerAsReader =
defaultAcls.stream().filter(hasRole(Role.READER).and(isProjectEditor)).findFirst().get();

Entity entity = projectViewerAsReader.getEntity();

boolean actual = retry429s(() -> storage.deleteDefaultAcl(bucket.getName(), entity), storage);

assertThat(actual).isTrue();

Bucket bucketUpdated =
storage.get(bucket.getName(), BucketGetOption.fields(BucketField.values()));
assertThat(bucketUpdated.getMetageneration()).isNotEqualTo(bucket.getMetageneration());

// etags change when deletes happen, drop before our comparison
List<Acl> expectedAcls =
dropEtags(
bucket.getDefaultAcl().stream()
.filter(isProjectEditor.negate())
.collect(Collectors.toList()));
List<Acl> actualAcls = dropEtags(bucketUpdated.getDefaultAcl());
assertThat(actualAcls).containsAtLeastElementsIn(expectedAcls);
Optional<Entity> search =
actualAcls.stream().map(Acl::getEntity).filter(e -> e.equals(entity)).findAny();
assertThat(search.isPresent()).isFalse();
}
}

@Test
public void bucket_defaultAcl_delete_bucket404() {
boolean actual =
retry429s(
() -> storage.deleteDefaultAcl(bucket.getName(), User.ofAllAuthenticatedUsers()),
storage));
assertNull(storage.getDefaultAcl(bucket.getName(), User.ofAllAuthenticatedUsers()));
() -> storage.deleteDefaultAcl(bucket.getName() + "x", User.ofAllUsers()), storage);

assertThat(actual).isEqualTo(false);
}

@Test
public void bucket_defaultAcl_delete_noExistingAcl() throws Exception {
BucketInfo bucketInfo = BucketInfo.newBuilder(generator.randomBucketName()).build();
try (TemporaryBucket tempB =
TemporaryBucket.newBuilder().setBucketInfo(bucketInfo).setStorage(storage).build()) {
BucketInfo bucket = tempB.getBucket();
boolean actual =
retry429s(() -> storage.deleteDefaultAcl(bucket.getName(), User.ofAllUsers()), storage);

assertThat(actual).isEqualTo(false);
}
}

@Test
Expand Down