From ed82b98eed714a2ab5bd4dd9d12415039c3a8a9e Mon Sep 17 00:00:00 2001 From: Mislav Mandaric Date: Mon, 14 Oct 2024 13:24:53 +0200 Subject: [PATCH 1/3] Set oldest_content_accepted for remote downloader requests without the checksum --- .../build/lib/remote/downloader/BUILD | 1 + .../downloader/GrpcRemoteDownloader.java | 21 +++++++- .../build/lib/remote/downloader/BUILD | 1 + .../downloader/GrpcRemoteDownloaderTest.java | 51 +++++++++++++++++-- 4 files changed, 68 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD index dccb139bc94499..2e933587858cfd 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD @@ -27,6 +27,7 @@ java_library( "//third_party:guava", "//third_party:jsr305", "//third_party/grpc-java:grpc-jar", + "@protobuf//:protobuf_java_util", "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_grpc", "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_proto", "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto", diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java index 8cf643b9662105..39bea0760e885f 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java @@ -38,6 +38,7 @@ import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; import com.google.devtools.build.lib.remote.util.Utils; import com.google.devtools.build.lib.vfs.Path; +import com.google.protobuf.util.Timestamps; import io.grpc.CallCredentials; import io.grpc.Channel; import io.grpc.StatusRuntimeException; @@ -45,6 +46,8 @@ import java.io.OutputStream; import java.net.URISyntaxException; import java.net.URL; +import java.time.Clock; +import java.time.Duration; import java.util.List; import java.util.Map; import java.util.Optional; @@ -90,6 +93,8 @@ public class GrpcRemoteDownloader implements AutoCloseable, Downloader { // value when both are present. private static final String QUALIFIER_HTTP_HEADER_URL_PREFIX = "http_header_url:"; + private Clock clock = Clock.systemUTC(); + public GrpcRemoteDownloader( String buildRequestId, String commandId, @@ -148,7 +153,8 @@ public void download( canonicalId, digestFunction, headers, - credentials); + credentials, + clock); try { FetchBlobResponse response = retrier.execute( @@ -199,7 +205,8 @@ static FetchBlobRequest newFetchBlobRequest( String canonicalId, DigestFunction.Value digestFunction, Map> headers, - Credentials credentials) + Credentials credentials, + Clock clock) throws IOException { FetchBlobRequest.Builder requestBuilder = FetchBlobRequest.newBuilder() @@ -235,6 +242,11 @@ static FetchBlobRequest newFetchBlobRequest( .setName(QUALIFIER_CHECKSUM_SRI) .setValue(checksum.get().toSubresourceIntegrity()) .build()); + } else { + // If no checksum is provided, never accept cached content. + // Timestamp is offset by an hour to account for clock skew. + Clock c = Clock.offset(clock, Duration.ofHours(1)); + requestBuilder.setOldestContentAccepted(Timestamps.fromMillis(c.millis())); } if (!Strings.isNullOrEmpty(canonicalId)) { @@ -278,4 +290,9 @@ private OutputStream newOutputStream(Path destination, Optional checks public ReferenceCountedChannel getChannel() { return channel; } + + @VisibleForTesting + public void setClock(Clock clock) { + this.clock = clock; + } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD index 6f45cf407df4c9..ccf930e2504ea5 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD @@ -44,6 +44,7 @@ java_library( "//third_party:truth", "//third_party/grpc-java:grpc-jar", "@protobuf//:protobuf_java", + "@protobuf//:protobuf_java_util", "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_grpc", "@remoteapis//:build_bazel_remote_asset_v1_remote_asset_java_proto", "@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto", diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java index 1e2ba06d11c93c..ddcd2ddb0f2f2d 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java @@ -61,6 +61,8 @@ import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.common.options.Options; import com.google.protobuf.ByteString; +import com.google.protobuf.Timestamp; +import com.google.protobuf.util.Timestamps; import io.grpc.CallCredentials; import io.grpc.ManagedChannel; import io.grpc.Server; @@ -73,6 +75,9 @@ import java.io.InputStream; import java.net.URI; import java.net.URL; +import java.time.Clock; +import java.time.Instant; +import java.time.ZoneId; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -90,6 +95,10 @@ @RunWith(JUnit4.class) public class GrpcRemoteDownloaderTest { + private static final String now = "2024-10-10T00:00:00Z"; + private static final String oneHourFromNow = "2024-10-10T01:00:00Z"; + private static final Clock clock = Clock.fixed(Instant.parse(now), ZoneId.of("UTC")); + private static final DigestUtil DIGEST_UTIL = new DigestUtil(SyscallCache.NO_CACHE, DigestHashFunction.SHA256); @@ -158,7 +167,7 @@ public int maxConcurrency() { return 100; } }); - return new GrpcRemoteDownloader( + GrpcRemoteDownloader downloader = new GrpcRemoteDownloader( "none", "none", channel.retain(), @@ -169,6 +178,8 @@ public int maxConcurrency() { remoteOptions, /* verboseFailures= */ false, fallbackDownloader); + downloader.setClock(clock); + return downloader; } private static byte[] downloadBlob( @@ -202,6 +213,7 @@ private static byte[] downloadBlob( public void testDownload() throws Exception { final byte[] content = "example content".getBytes(UTF_8); final Digest contentDigest = DIGEST_UTIL.compute(content); + final Timestamp timestamp = Timestamps.parse(oneHourFromNow); serviceRegistry.addService( new FetchImplBase() { @@ -212,6 +224,7 @@ public void fetchBlob( .isEqualTo( FetchBlobRequest.newBuilder() .setDigestFunction(DIGEST_UTIL.getDigestFunction()) + .setOldestContentAccepted(timestamp) .addUris("http://example.com/content.txt") .build()); responseObserver.onNext( @@ -366,7 +379,8 @@ public void testFetchBlobRequest() throws Exception { ImmutableMap.of( "Authorization", ImmutableList.of("Basic Zm9vOmJhcg=="), "X-Custom-Token", ImmutableList.of("foo", "bar")), - StaticCredentials.EMPTY); + StaticCredentials.EMPTY, + clock); assertThat(request) .isEqualTo( @@ -415,7 +429,8 @@ public void testFetchBlobRequest_withCredentialsPropagation() throws Exception { "canonical ID", DIGEST_UTIL.getDigestFunction(), ImmutableMap.of(), - credentials); + credentials, + clock); assertThat(request) .isEqualTo( @@ -458,7 +473,8 @@ public void testFetchBlobRequest_withoutCredentialsPropagation() throws Exceptio "canonical ID", DIGEST_UTIL.getDigestFunction(), ImmutableMap.of(), - credentials); + credentials, + clock); assertThat(request) .isEqualTo( @@ -474,4 +490,31 @@ public void testFetchBlobRequest_withoutCredentialsPropagation() throws Exceptio Qualifier.newBuilder().setName("bazel.canonical_id").setValue("canonical ID")) .build()); } + + @Test + public void testFetchBlobRequest_withoutChecksum() throws Exception { + FetchBlobRequest request = + GrpcRemoteDownloader.newFetchBlobRequest( + "instance name", + false, + ImmutableList.of( + new URL("http://example.com/")), + Optional.empty(), + "canonical ID", + DIGEST_UTIL.getDigestFunction(), + ImmutableMap.of(), + StaticCredentials.EMPTY, + clock); + + assertThat(request) + .isEqualTo( + FetchBlobRequest.newBuilder() + .setInstanceName("instance name") + .setDigestFunction(DIGEST_UTIL.getDigestFunction()) + .setOldestContentAccepted(Timestamps.parse(oneHourFromNow)) + .addUris("http://example.com/") + .addQualifiers( + Qualifier.newBuilder().setName("bazel.canonical_id").setValue("canonical ID")) + .build()); + } } From 9fe89688fce43b1d1278dfeda0b671281ea0ee92 Mon Sep 17 00:00:00 2001 From: Mislav Mandaric Date: Mon, 14 Oct 2024 16:24:18 +0200 Subject: [PATCH 2/3] Replace usage of java.time.Clock with com.google.devtools.build.lib.clock.Clock --- .../build/lib/remote/downloader/BUILD | 1 + .../downloader/GrpcRemoteDownloader.java | 19 +++-------- .../build/lib/remote/downloader/BUILD | 1 + .../downloader/GrpcRemoteDownloaderTest.java | 34 +++++++------------ 4 files changed, 20 insertions(+), 35 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD index 2e933587858cfd..16c71bda340efc 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/BUILD @@ -16,6 +16,7 @@ java_library( srcs = glob(["*.java"]), deps = [ "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", + "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/remote:ReferenceCountedChannel", "//src/main/java/com/google/devtools/build/lib/remote:Retrier", diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java index 39bea0760e885f..c76d85946a4344 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java @@ -28,6 +28,7 @@ import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.bazel.repository.downloader.Downloader; import com.google.devtools.build.lib.bazel.repository.downloader.HashOutputStream; +import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.remote.ReferenceCountedChannel; @@ -46,7 +47,6 @@ import java.io.OutputStream; import java.net.URISyntaxException; import java.net.URL; -import java.time.Clock; import java.time.Duration; import java.util.List; import java.util.Map; @@ -93,8 +93,6 @@ public class GrpcRemoteDownloader implements AutoCloseable, Downloader { // value when both are present. private static final String QUALIFIER_HTTP_HEADER_URL_PREFIX = "http_header_url:"; - private Clock clock = Clock.systemUTC(); - public GrpcRemoteDownloader( String buildRequestId, String commandId, @@ -153,8 +151,7 @@ public void download( canonicalId, digestFunction, headers, - credentials, - clock); + credentials); try { FetchBlobResponse response = retrier.execute( @@ -205,8 +202,7 @@ static FetchBlobRequest newFetchBlobRequest( String canonicalId, DigestFunction.Value digestFunction, Map> headers, - Credentials credentials, - Clock clock) + Credentials credentials) throws IOException { FetchBlobRequest.Builder requestBuilder = FetchBlobRequest.newBuilder() @@ -245,8 +241,8 @@ static FetchBlobRequest newFetchBlobRequest( } else { // If no checksum is provided, never accept cached content. // Timestamp is offset by an hour to account for clock skew. - Clock c = Clock.offset(clock, Duration.ofHours(1)); - requestBuilder.setOldestContentAccepted(Timestamps.fromMillis(c.millis())); + long milis = BlazeClock.instance().currentTimeMillis() + Duration.ofHours(1).toMillis(); + requestBuilder.setOldestContentAccepted(Timestamps.fromMillis(milis)); } if (!Strings.isNullOrEmpty(canonicalId)) { @@ -290,9 +286,4 @@ private OutputStream newOutputStream(Path destination, Optional checks public ReferenceCountedChannel getChannel() { return channel; } - - @VisibleForTesting - public void setClock(Clock clock) { - this.clock = clock; - } } diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD index ccf930e2504ea5..d5cc6da5733946 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD @@ -24,6 +24,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/authandtls", "//src/main/java/com/google/devtools/build/lib/bazel/repository/cache", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", + "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/remote", "//src/main/java/com/google/devtools/build/lib/remote/common", diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java index ddcd2ddb0f2f2d..69aa0143304432 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java @@ -42,6 +42,7 @@ import com.google.devtools.build.lib.bazel.repository.downloader.Checksum; import com.google.devtools.build.lib.bazel.repository.downloader.Downloader; import com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException; +import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.remote.ChannelConnectionWithServerCapabilitiesFactory; import com.google.devtools.build.lib.remote.ReferenceCountedChannel; @@ -54,6 +55,7 @@ import com.google.devtools.build.lib.remote.util.InMemoryCacheClient; import com.google.devtools.build.lib.remote.util.TestUtils; import com.google.devtools.build.lib.remote.util.TracingMetadataUtils; +import com.google.devtools.build.lib.testutil.ManualClock; import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -61,7 +63,6 @@ import com.google.devtools.build.lib.vfs.SyscallCache; import com.google.devtools.common.options.Options; import com.google.protobuf.ByteString; -import com.google.protobuf.Timestamp; import com.google.protobuf.util.Timestamps; import io.grpc.CallCredentials; import io.grpc.ManagedChannel; @@ -75,9 +76,7 @@ import java.io.InputStream; import java.net.URI; import java.net.URL; -import java.time.Clock; -import java.time.Instant; -import java.time.ZoneId; +import java.time.Duration; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -95,9 +94,7 @@ @RunWith(JUnit4.class) public class GrpcRemoteDownloaderTest { - private static final String now = "2024-10-10T00:00:00Z"; - private static final String oneHourFromNow = "2024-10-10T01:00:00Z"; - private static final Clock clock = Clock.fixed(Instant.parse(now), ZoneId.of("UTC")); + private static final ManualClock clock = new ManualClock(); private static final DigestUtil DIGEST_UTIL = new DigestUtil(SyscallCache.NO_CACHE, DigestHashFunction.SHA256); @@ -126,6 +123,8 @@ public final void setUp() throws Exception { context = RemoteActionExecutionContext.create(metadata); retryService = MoreExecutors.listeningDecorator(Executors.newScheduledThreadPool(1)); + + BlazeClock.setClock(clock); } @After @@ -167,7 +166,7 @@ public int maxConcurrency() { return 100; } }); - GrpcRemoteDownloader downloader = new GrpcRemoteDownloader( + return new GrpcRemoteDownloader( "none", "none", channel.retain(), @@ -178,8 +177,6 @@ public int maxConcurrency() { remoteOptions, /* verboseFailures= */ false, fallbackDownloader); - downloader.setClock(clock); - return downloader; } private static byte[] downloadBlob( @@ -213,7 +210,6 @@ private static byte[] downloadBlob( public void testDownload() throws Exception { final byte[] content = "example content".getBytes(UTF_8); final Digest contentDigest = DIGEST_UTIL.compute(content); - final Timestamp timestamp = Timestamps.parse(oneHourFromNow); serviceRegistry.addService( new FetchImplBase() { @@ -224,7 +220,7 @@ public void fetchBlob( .isEqualTo( FetchBlobRequest.newBuilder() .setDigestFunction(DIGEST_UTIL.getDigestFunction()) - .setOldestContentAccepted(timestamp) + .setOldestContentAccepted(Timestamps.fromMillis(clock.advance(Duration.ofHours(1)))) .addUris("http://example.com/content.txt") .build()); responseObserver.onNext( @@ -379,8 +375,7 @@ public void testFetchBlobRequest() throws Exception { ImmutableMap.of( "Authorization", ImmutableList.of("Basic Zm9vOmJhcg=="), "X-Custom-Token", ImmutableList.of("foo", "bar")), - StaticCredentials.EMPTY, - clock); + StaticCredentials.EMPTY); assertThat(request) .isEqualTo( @@ -429,8 +424,7 @@ public void testFetchBlobRequest_withCredentialsPropagation() throws Exception { "canonical ID", DIGEST_UTIL.getDigestFunction(), ImmutableMap.of(), - credentials, - clock); + credentials); assertThat(request) .isEqualTo( @@ -473,8 +467,7 @@ public void testFetchBlobRequest_withoutCredentialsPropagation() throws Exceptio "canonical ID", DIGEST_UTIL.getDigestFunction(), ImmutableMap.of(), - credentials, - clock); + credentials); assertThat(request) .isEqualTo( @@ -503,15 +496,14 @@ public void testFetchBlobRequest_withoutChecksum() throws Exception { "canonical ID", DIGEST_UTIL.getDigestFunction(), ImmutableMap.of(), - StaticCredentials.EMPTY, - clock); + StaticCredentials.EMPTY); assertThat(request) .isEqualTo( FetchBlobRequest.newBuilder() .setInstanceName("instance name") .setDigestFunction(DIGEST_UTIL.getDigestFunction()) - .setOldestContentAccepted(Timestamps.parse(oneHourFromNow)) + .setOldestContentAccepted(Timestamps.fromMillis(clock.advance(Duration.ofHours(1)))) .addUris("http://example.com/") .addQualifiers( Qualifier.newBuilder().setName("bazel.canonical_id").setValue("canonical ID")) From 9c58f2e797658b2a7341b8430f0c99d20a9571cb Mon Sep 17 00:00:00 2001 From: Mislav Mandaric Date: Mon, 14 Oct 2024 17:01:22 +0200 Subject: [PATCH 3/3] Replace millisecond arithmetic with Instant class Co-authored-by: Fabian Meumertzheim --- .../build/lib/remote/downloader/GrpcRemoteDownloader.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java index c76d85946a4344..9720347268c81c 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java @@ -241,8 +241,9 @@ static FetchBlobRequest newFetchBlobRequest( } else { // If no checksum is provided, never accept cached content. // Timestamp is offset by an hour to account for clock skew. - long milis = BlazeClock.instance().currentTimeMillis() + Duration.ofHours(1).toMillis(); - requestBuilder.setOldestContentAccepted(Timestamps.fromMillis(milis)); + requestBuilder.setOldestContentAccepted( + Timestamps.fromMillis( + BlazeClock.instance().now().plus(Duration.ofHours(1)).toEpochMilli())); } if (!Strings.isNullOrEmpty(canonicalId)) {