From 032f2f2693c6c75cc4ae0339be805c0bb94fa064 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Wed, 17 Apr 2024 19:49:14 -0400 Subject: [PATCH] fix: update grpc handling of IAM Policy etag to account for base64 encoding (#2499) Etags returned by the JSON api are base64 encoded, and IAM Policy is currently modeled around this for its public api. Update GrpcConversions to base64 decode/encode appropriately. --- .../com/google/cloud/storage/GrpcConversions.java | 11 +++++++++-- .../cloud/storage/GrpcRetryAlgorithmManager.java | 8 ++++++-- .../cloud/storage/conformance/retry/CtxFunctions.java | 4 ++++ .../conformance/retry/ITRetryConformanceTest.java | 8 +------- .../storage/conformance/retry/RpcMethodMappings.java | 3 ++- 5 files changed, 22 insertions(+), 12 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcConversions.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcConversions.java index 28c51873b0..63a33b80a1 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcConversions.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcConversions.java @@ -60,11 +60,13 @@ import com.google.storage.v2.Owner; import com.google.type.Date; import com.google.type.Expr; +import java.nio.charset.StandardCharsets; import java.time.Duration; import java.time.Instant; import java.time.LocalDate; import java.time.OffsetDateTime; import java.time.ZoneOffset; +import java.util.Base64; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -118,6 +120,11 @@ final class GrpcConversions { hierarchicalNamespaceCodec = Codec.of(this::hierarchicalNamespaceEncode, this::hierarchicalNamespaceDecode); + private final Codec byteStringB64StringCodec = + Codec.of( + bs -> Base64.getEncoder().encodeToString(bs.toByteArray()), + s -> ByteString.copyFrom(Base64.getDecoder().decode(s.getBytes(StandardCharsets.UTF_8)))); + @VisibleForTesting final Codec timestampCodec = Codec.of( @@ -1048,7 +1055,7 @@ private NotificationInfo notificationDecode(NotificationConfig from) { private com.google.iam.v1.Policy policyEncode(Policy from) { com.google.iam.v1.Policy.Builder to = com.google.iam.v1.Policy.newBuilder(); - ifNonNull(from.getEtag(), ByteString::copyFromUtf8, to::setEtag); + ifNonNull(from.getEtag(), byteStringB64StringCodec::decode, to::setEtag); ifNonNull(from.getVersion(), to::setVersion); from.getBindingsList().stream().map(bindingCodec::encode).forEach(to::addBindings); return to.build(); @@ -1058,7 +1065,7 @@ private Policy policyDecode(com.google.iam.v1.Policy from) { Policy.Builder to = Policy.newBuilder(); ByteString etag = from.getEtag(); if (!etag.isEmpty()) { - to.setEtag(etag.toStringUtf8()); + to.setEtag(byteStringB64StringCodec.encode(etag)); } to.setVersion(from.getVersion()); List bindingsList = from.getBindingsList(); diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcRetryAlgorithmManager.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcRetryAlgorithmManager.java index 3a97c2e952..1178c7c942 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcRetryAlgorithmManager.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcRetryAlgorithmManager.java @@ -20,6 +20,7 @@ import com.google.iam.v1.GetIamPolicyRequest; import com.google.iam.v1.SetIamPolicyRequest; import com.google.iam.v1.TestIamPermissionsRequest; +import com.google.protobuf.ByteString; import com.google.storage.v2.BidiWriteObjectRequest; import com.google.storage.v2.ComposeObjectRequest; import com.google.storage.v2.CreateBucketRequest; @@ -168,8 +169,11 @@ public ResultRetryAlgorithm getFor(RewriteObjectRequest req) { } public ResultRetryAlgorithm getFor(SetIamPolicyRequest req) { - // TODO: etag - return retryStrategy.getNonidempotentHandler(); + if (req.getPolicy().getEtag().equals(ByteString.empty())) { + return retryStrategy.getNonidempotentHandler(); + } else { + return retryStrategy.getIdempotentHandler(); + } } public ResultRetryAlgorithm getFor(StartResumableWriteRequest req) { diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/CtxFunctions.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/CtxFunctions.java index 7617be79f9..8a415fd8f6 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/CtxFunctions.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/CtxFunctions.java @@ -136,6 +136,10 @@ static final class Local { static final class Rpc { static final CtxFunction createEmptyBlob = (ctx, c) -> ctx.map(state -> state.with(ctx.getStorage().create(state.getBlobInfo()))); + static final CtxFunction bucketIamPolicy = + (ctx, c) -> + ctx.map( + state -> state.with(ctx.getStorage().getIamPolicy(state.getBucket().getName()))); } static final class ResourceSetup { diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/ITRetryConformanceTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/ITRetryConformanceTest.java index 8d0a24a86f..814729f840 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/ITRetryConformanceTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/ITRetryConformanceTest.java @@ -19,7 +19,6 @@ import static com.google.cloud.storage.PackagePrivateMethodWorkarounds.blobCopyWithStorage; import static com.google.cloud.storage.PackagePrivateMethodWorkarounds.bucketCopyWithStorage; import static com.google.cloud.storage.conformance.retry.Ctx.ctx; -import static com.google.cloud.storage.conformance.retry.ITRetryConformanceTest.RetryTestCaseResolver.lift; import static com.google.cloud.storage.conformance.retry.State.empty; import static com.google.common.truth.Truth.assertThat; import static java.util.Objects.requireNonNull; @@ -168,12 +167,7 @@ public ImmutableList parameters() { .setMappings(new RpcMethodMappings()) .setProjectId("conformance-tests") .setHost(testBench.getBaseUri().replaceAll("https?://", "")) - .setTestAllowFilter( - RetryTestCaseResolver.includeAll() - .and( - (lift(trc -> trc.getTransport() == Transport.GRPC) - .and((m, trc) -> m == RpcMethod.storage.buckets.setIamPolicy)) - .negate())) + .setTestAllowFilter(RetryTestCaseResolver.includeAll()) .build(); List retryTestCases; diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java index 3f98a94fa4..a60b1bb984 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java @@ -685,6 +685,7 @@ private static void setIamPolicy(ArrayList a) { a.add( RpcMethodMapping.newBuilder(240, buckets.setIamPolicy) .withApplicable(TestRetryConformance::isPreconditionsProvided) + .withSetup(ResourceSetup.defaultSetup.andThen(Rpc.bucketIamPolicy)) .withTest( (ctx, c) -> ctx.map( @@ -694,7 +695,7 @@ private static void setIamPolicy(ArrayList a) { .setIamPolicy( state.getBucket().getName(), Policy.newBuilder() - .setEtag("h??") + .setEtag(state.getPolicy().getEtag()) .setVersion(3) .setBindings( ImmutableList.of(