fix: update grpc handling of IAM Policy etag to account for base64 en…

…coding (#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.
googleapis · Apr 17, 2024 · 032f2f2 · 032f2f2
1 parent 09043c5
commit 032f2f2
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 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
@@ -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<ByteString, String> byteStringB64StringCodec =
+      Codec.of(
+          bs -> Base64.getEncoder().encodeToString(bs.toByteArray()),
+          s -> ByteString.copyFrom(Base64.getDecoder().decode(s.getBytes(StandardCharsets.UTF_8))));
+
   @VisibleForTesting
   final Codec<OffsetDateTime, Timestamp> 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<com.google.iam.v1.Binding> 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
@@ -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/...-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/CtxFunctions.java b/...-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/...rage/src/test/java/com/google/cloud/storage/conformance/retry/ITRetryConformanceTest.java b/...rage/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<RetryTestCase> retryTestCases;

diff --git a/...d-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java b/...d-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java
@@ -685,6 +685,7 @@ private static void setIamPolicy(ArrayList<RpcMethodMapping> 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<RpcMethodMapping> a) {
                                         .setIamPolicy(
                                             state.getBucket().getName(),
                                             Policy.newBuilder()
-                                                .setEtag("h??")
+                                                .setEtag(state.getPolicy().getEtag())
                                                 .setVersion(3)
                                                 .setBindings(
                                                     ImmutableList.of(