From 5a5b6e0e5851e2fc682be58c103442520a7ecb31 Mon Sep 17 00:00:00 2001 From: Tyler Williams Date: Mon, 7 Aug 2023 08:02:10 -0700 Subject: [PATCH] Separate new-style-hash content in DiskCache DiskCache always stores files in /root/{cas/ac}/digestHash. This change keeps the current behavior, but for new style digest functions inserts a directory between /root/ and {cas/ac} to disambiguate the digest function type. This prevents issues that could theoretically happen if there were hash collisions between two digest functions sharing the same cache directory. PiperOrigin-RevId: 554477775 Change-Id: Ibef994e432764c260a3cab821ca6583c231c5b50 (cherry picked from commit f17b280b2c22b51af29953451a64bf9e0f50c19c) Signed-off-by: Brentley Jones --- .../remote/ByteStreamBuildEventArtifactUploader.java | 9 +-------- .../build/lib/remote/ByteStreamUploader.java | 11 ++--------- .../devtools/build/lib/remote/GrpcCacheClient.java | 9 +-------- .../build/lib/remote/disk/DiskCacheClient.java | 12 +++++++++++- .../devtools/build/lib/remote/util/DigestUtil.java | 8 ++++++++ 5 files changed, 23 insertions(+), 26 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java index 78a53b7f4d5825..3a4d7a2b081b7d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.remote; +import static com.google.devtools.build.lib.remote.util.DigestUtil.isOldStyleDigestFunction; import static com.google.devtools.build.lib.remote.util.RxFutures.toCompletable; import static com.google.devtools.build.lib.remote.util.RxFutures.toListenableFuture; import static com.google.devtools.build.lib.remote.util.RxFutures.toSingle; @@ -443,14 +444,6 @@ private static class PathConverterImpl implements PathConverter { this.localPaths = localPaths.build(); } - private static boolean isOldStyleDigestFunction(DigestFunction.Value digestFunction) { - // Old-style digest functions (SHA256, etc) are distinguishable by the length - // of their hash alone and do not require extra specification, but newer - // digest functions (which may have the same length hashes as the older - // functions!) must be explicitly specified in the upload resource name. - return digestFunction.getNumber() <= 7; - } - @Override @Nullable public String apply(Path path) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamUploader.java index 2db0f9b955e96a..0dea65107c670d 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamUploader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamUploader.java @@ -15,6 +15,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.util.concurrent.Futures.immediateVoidFuture; +import static com.google.devtools.build.lib.remote.util.DigestUtil.isOldStyleDigestFunction; import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; import static com.google.devtools.build.lib.remote.util.Utils.waitForBulkTransfer; import static java.lang.String.format; @@ -179,20 +180,12 @@ public ListenableFuture uploadBlobAsync( MoreExecutors.directExecutor()); } - private boolean isOldStyleDigestFunction() { - // Old-style digest functions (SHA256, etc) are distinguishable by the length - // of their hash alone and do not require extra specification, but newer - // digest functions (which may have the same length hashes as the older - // functions!) must be explicitly specified in the upload resource name. - return digestFunction.getNumber() <= 7; - } - private String buildUploadResourceName( String instanceName, UUID uuid, Digest digest, boolean compressed) { String resourceName; - if (isOldStyleDigestFunction()) { + if (isOldStyleDigestFunction(digestFunction)) { String template = compressed ? "uploads/%s/compressed-blobs/zstd/%s/%d" : "uploads/%s/blobs/%s/%d"; resourceName = format(template, uuid, digest.getHash(), digest.getSizeBytes()); diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java index 8c86a31d9bafa8..401c1fedc53518 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcCacheClient.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.remote; import static com.google.common.base.Strings.isNullOrEmpty; +import static com.google.devtools.build.lib.remote.util.DigestUtil.isOldStyleDigestFunction; import build.bazel.remote.execution.v2.ActionCacheGrpc; import build.bazel.remote.execution.v2.ActionCacheGrpc.ActionCacheFutureStub; @@ -354,14 +355,6 @@ private ListenableFuture downloadBlob( MoreExecutors.directExecutor()); } - private static boolean isOldStyleDigestFunction(DigestFunction.Value digestFunction) { - // Old-style digest functions (SHA256, etc) are distinguishable by the length - // of their hash alone and do not require extra specification, but newer - // digest functions (which may have the same length hashes as the older - // functions!) must be explicitly specified in the upload resource name. - return digestFunction.getNumber() <= 7; - } - public static String getResourceName( String instanceName, Digest digest, boolean compressed, DigestFunction.Value digestFunction) { String resourceName = ""; diff --git a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java index 55d6b43581f8dd..1c0777f77bc0de 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java +++ b/src/main/java/com/google/devtools/build/lib/remote/disk/DiskCacheClient.java @@ -13,10 +13,13 @@ // limitations under the License. package com.google.devtools.build.lib.remote.disk; +import static com.google.devtools.build.lib.remote.util.DigestUtil.isOldStyleDigestFunction; + import build.bazel.remote.execution.v2.ActionResult; import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.Directory; import build.bazel.remote.execution.v2.Tree; +import com.google.common.base.Ascii; import com.google.common.collect.ImmutableSet; import com.google.common.io.ByteStreams; import com.google.common.util.concurrent.Futures; @@ -57,10 +60,17 @@ public class DiskCacheClient implements RemoteCacheClient { */ public DiskCacheClient( Path root, boolean verifyDownloads, boolean checkActionResult, DigestUtil digestUtil) { - this.root = root; this.verifyDownloads = verifyDownloads; this.checkActionResult = checkActionResult; this.digestUtil = digestUtil; + + if (isOldStyleDigestFunction(digestUtil.getDigestFunction())) { + this.root = root; + } else { + this.root = + root.getChild( + Ascii.toLowerCase(digestUtil.getDigestFunction().getValueDescriptor().getName())); + } } /** Returns {@code true} if the provided {@code key} is stored in the CAS. */ diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java index 76b258443890ca..1e876d424ce6ff 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java @@ -131,4 +131,12 @@ public static String toString(Digest digest) { public static byte[] toBinaryDigest(Digest digest) { return HashCode.fromString(digest.getHash()).asBytes(); } + + public static boolean isOldStyleDigestFunction(DigestFunction.Value digestFunction) { + // Old-style digest functions (SHA256, etc) are distinguishable by the length + // of their hash alone and do not require extra specification, but newer + // digest functions (which may have the same length hashes as the older + // functions!) must be explicitly specified in the upload resource name. + return digestFunction.getNumber() <= 7; + } }