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

…coding 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 · 5b9675e · 5b9675e
1 parent 03e6c1a
commit 5b9675e
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 13 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,13 +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(RetryTestCaseResolver.scenarioIdIs(2))
-                                  .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(