From 7558c1d10c437807a0ac463b9a8749569f377e9b Mon Sep 17 00:00:00 2001 From: ishikhman Date: Thu, 9 May 2019 02:49:30 -0700 Subject: [PATCH] remote: made CombinedCache a composition of Disk and Http Cache Fixes #8052 The problem: CombinedCache is not working. The cause: both `OnDiskBlobStore` and `HttpBlobStore` store action results differently from other actions, but not in the same way. The `OnDiskBlobStore` appends "ac_" prefix to the key, while the `HttpBlobStore` stores it in a different directory. Current implementation added "ac_" prefix for both blob stores, which is not correct for the `HttpBlobStore`. The solution: treat actionResults' `gets` and `puts` in accordance with a particular Blob Store used. The problem is described in #8052 and the root cause is described in [this](https://github.com/bazelbuild/bazel/issues/8052#issuecomment-483556888) and [this](https://github.com/bazelbuild/bazel/issues/8052#issuecomment-487068204) comments. Closes #8141. PiperOrigin-RevId: 247387377 --- .../remote/SimpleBlobStoreActionCache.java | 4 +- .../lib/remote/SimpleBlobStoreFactory.java | 6 +- .../blobstore/CombinedDiskHttpBlobStore.java | 185 +++++++++--------- .../blobstore/ConcurrentMapBlobStore.java | 24 +-- .../lib/remote/blobstore/OnDiskBlobStore.java | 33 ++-- .../lib/remote/blobstore/SimpleBlobStore.java | 12 +- .../remote/blobstore/http/HttpBlobStore.java | 13 +- .../remote/remote_execution_http_test.sh | 8 +- src/tools/remote/BUILD | 5 +- .../remote/worker/HttpCacheServerHandler.java | 19 ++ .../google/devtools/build/remote/worker/BUILD | 22 +++ .../worker/HttpCacheServerHandlerTest.java | 99 ++++++++++ 12 files changed, 289 insertions(+), 141 deletions(-) create mode 100644 src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/BUILD create mode 100644 src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/HttpCacheServerHandlerTest.java diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java index 1429347d2fb560..4ae26046135aee 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java @@ -186,7 +186,7 @@ public Digest uploadStream(Digest digest, InputStream in) } public boolean containsKey(Digest digest) throws IOException, InterruptedException { - return blobStore.containsKey(digest.getHash()); + return blobStore.contains(digest.getHash()); } @Override @@ -206,7 +206,7 @@ private byte[] downloadActionResult(Digest digest) throws IOException, Interrupt } // This unconditionally downloads the whole blob into memory! ByteArrayOutputStream out = new ByteArrayOutputStream(); - boolean success = blobStore.getActionResult(digest.getHash(), out); + boolean success = getFromFuture(blobStore.getActionResult(digest.getHash(), out)); if (!success) { throw new CacheNotFoundException(digest, digestUtil); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java index 8611530433a4a0..f7bed3c0c90ed1 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java +++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreFactory.java @@ -114,12 +114,16 @@ private static SimpleBlobStore createDiskCache(Path workingDirectory, PathFragme private static SimpleBlobStore createCombinedCache( Path workingDirectory, PathFragment diskCachePath, RemoteOptions options, Credentials cred) throws IOException { + Path cacheDir = workingDirectory.getRelative(Preconditions.checkNotNull(diskCachePath, "diskCachePath")); if (!cacheDir.exists()) { cacheDir.createDirectoryAndParents(); } - return new CombinedDiskHttpBlobStore(cacheDir, createHttp(options, cred)); + + OnDiskBlobStore diskCache = new OnDiskBlobStore(cacheDir); + SimpleBlobStore httpCache = createHttp(options, cred); + return new CombinedDiskHttpBlobStore(diskCache, httpCache); } private static boolean isDiskCache(RemoteOptions options) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/blobstore/CombinedDiskHttpBlobStore.java b/src/main/java/com/google/devtools/build/lib/remote/blobstore/CombinedDiskHttpBlobStore.java index 4fc872325f9ca4..f2320ee0da22a0 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/blobstore/CombinedDiskHttpBlobStore.java +++ b/src/main/java/com/google/devtools/build/lib/remote/blobstore/CombinedDiskHttpBlobStore.java @@ -14,14 +14,10 @@ package com.google.devtools.build.lib.remote.blobstore; import com.google.common.base.Preconditions; -import com.google.common.io.ByteStreams; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.MoreExecutors; -import com.google.common.util.concurrent.SettableFuture; import com.google.devtools.build.lib.vfs.Path; -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -31,119 +27,122 @@ /** * A {@link SimpleBlobStore} implementation combining two blob stores. A local disk blob store and a - * remote http blob store. If a blob isn't found in the first store, the second store is used, and - * the blob added to the first. Put puts the blob on both stores. + * remote blob store. If a blob isn't found in the first store, the second store is used, and the + * blob added to the first. Put puts the blob on both stores. */ -public final class CombinedDiskHttpBlobStore extends OnDiskBlobStore { +public final class CombinedDiskHttpBlobStore implements SimpleBlobStore { private static final Logger logger = Logger.getLogger(CombinedDiskHttpBlobStore.class.getName()); - private final SimpleBlobStore bsHttp; - public CombinedDiskHttpBlobStore(Path root, SimpleBlobStore bsHttp) { - super(root); - this.bsHttp = Preconditions.checkNotNull(bsHttp); + private final SimpleBlobStore remoteCache; + private final OnDiskBlobStore diskCache; + + public CombinedDiskHttpBlobStore(OnDiskBlobStore diskCache, SimpleBlobStore remoteCache) { + this.diskCache = Preconditions.checkNotNull(diskCache); + this.remoteCache = Preconditions.checkNotNull(remoteCache); } @Override - public boolean containsKey(String key) { - // HTTP cache does not support containsKey. - // Don't support it here either for predictable semantics. - throw new UnsupportedOperationException("HTTP Caching does not use this method."); + public boolean contains(String key) { + return diskCache.contains(key); } @Override - public ListenableFuture get(String key, OutputStream out) { - boolean foundOnDisk = super.containsKey(key); - - if (foundOnDisk) { - return super.get(key, out); - } else { - // Write a temporary file first, and then rename, to avoid data corruption in case of a crash. - Path temp = toPath(UUID.randomUUID().toString()); - - OutputStream tempOut; - try { - tempOut = temp.getOutputStream(); - } catch (IOException e) { - return Futures.immediateFailedFuture(e); - } - ListenableFuture chained = - Futures.transformAsync( - bsHttp.get(key, tempOut), - (found) -> { - if (!found) { - return Futures.immediateFuture(false); - } else { - Path target = toPath(key); - // The following note and line is taken from OnDiskBlobStore.java - // TODO(ulfjack): Fsync temp here before we rename it to avoid data loss in the - // case of machine - // crashes (the OS may reorder the writes and the rename). - temp.renameTo(target); - - SettableFuture f = SettableFuture.create(); - try (InputStream in = target.getInputStream()) { - ByteStreams.copy(in, out); - f.set(true); - } catch (IOException e) { - f.setException(e); - } - return f; - } - }, - MoreExecutors.directExecutor()); - chained.addListener( - () -> { - try { - tempOut.close(); - } catch (IOException e) { - // not sure what to do here, we either are here because of another exception being - // thrown, - // or we have successfully used the file we are trying (and failing) to close - logger.log(Level.WARNING, "Failed to close temporary file on get", e); - } - }, - MoreExecutors.directExecutor()); - return chained; - } + public boolean containsActionResult(String key) { + return diskCache.containsActionResult(key); } @Override - public boolean getActionResult(String key, OutputStream out) + public void put(String key, long length, InputStream in) throws IOException, InterruptedException { - if (super.getActionResult(key, out)) { - return true; + diskCache.put(key, length, in); + try (InputStream inFile = diskCache.toPath(key, /* actionResult= */ false).getInputStream()) { + remoteCache.put(key, length, inFile); } + } - try (ByteArrayOutputStream tmpOut = new ByteArrayOutputStream()) { - if (bsHttp.getActionResult(key, tmpOut)) { - byte[] tmp = tmpOut.toByteArray(); - super.putActionResult(key, tmp); - ByteStreams.copy(new ByteArrayInputStream(tmp), out); - return true; - } - } + @Override + public void putActionResult(String key, byte[] in) throws IOException, InterruptedException { + diskCache.putActionResult(key, in); + remoteCache.putActionResult(key, in); + } - return false; + @Override + public void close() { + diskCache.close(); + remoteCache.close(); } @Override - public void put(String key, long length, InputStream in) - throws IOException, InterruptedException { - super.put(key, length, in); - try (InputStream inFile = toPath(key).getInputStream()) { - bsHttp.put(key, length, inFile); + public ListenableFuture get(String key, OutputStream out) { + return get(key, out, /* actionResult= */ false); + } + + private ListenableFuture get(String key, OutputStream out, boolean actionResult) { + boolean foundOnDisk = + actionResult ? diskCache.containsActionResult(key) : diskCache.contains(key); + + if (foundOnDisk) { + return getFromCache(diskCache, key, out, actionResult); + } else { + return getFromRemoteAndSaveToDisk(key, out, actionResult); } } @Override - public void putActionResult(String key, byte[] in) throws IOException, InterruptedException { - super.putActionResult(key, in); - bsHttp.putActionResult(key, in); + public ListenableFuture getActionResult(String key, OutputStream out) { + return get(key, out, /* actionResult= */ true); } - @Override - public void close() { - super.close(); - bsHttp.close(); + private ListenableFuture getFromRemoteAndSaveToDisk( + String key, OutputStream out, boolean actionResult) { + // Write a temporary file first, and then rename, to avoid data corruption in case of a crash. + Path temp = diskCache.toPath(UUID.randomUUID().toString(), /* actionResult= */ false); + + OutputStream tempOut; + try { + tempOut = temp.getOutputStream(); + } catch (IOException e) { + return Futures.immediateFailedFuture(e); + } + ListenableFuture chained = + Futures.transformAsync( + getFromCache(remoteCache, key, tempOut, actionResult), + (found) -> { + if (!found) { + return Futures.immediateFuture(false); + } else { + saveToDiskCache(key, temp, actionResult); + return getFromCache(diskCache, key, out, actionResult); + } + }, + MoreExecutors.directExecutor()); + chained.addListener( + () -> { + try { + tempOut.close(); + } catch (IOException e) { + // not sure what to do here, we either are here because of another exception being + // thrown, or we have successfully used the file we are trying (and failing) to close + logger.log(Level.WARNING, "Failed to close temporary file on get", e); + } + }, + MoreExecutors.directExecutor()); + return chained; + } + + private void saveToDiskCache(String key, Path temp, boolean actionResult) throws IOException { + Path target = diskCache.toPath(key, actionResult); + // TODO(ulfjack): Fsync temp here before we rename it to avoid data loss in the + // case of machine crashes (the OS may reorder the writes and the rename). + temp.renameTo(target); + } + + private ListenableFuture getFromCache( + SimpleBlobStore blobStore, String key, OutputStream tempOut, boolean actionResult) { + if (!actionResult) { + return blobStore.get(key, tempOut); + } else { + return blobStore.getActionResult(key, tempOut); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/blobstore/ConcurrentMapBlobStore.java b/src/main/java/com/google/devtools/build/lib/remote/blobstore/ConcurrentMapBlobStore.java index 6340e040982412..8049a07c0ec1d8 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/blobstore/ConcurrentMapBlobStore.java +++ b/src/main/java/com/google/devtools/build/lib/remote/blobstore/ConcurrentMapBlobStore.java @@ -21,7 +21,6 @@ import java.io.InputStream; import java.io.OutputStream; import java.util.concurrent.ConcurrentMap; -import java.util.concurrent.ExecutionException; /** A {@link SimpleBlobStore} implementation using a {@link ConcurrentMap}. */ public final class ConcurrentMapBlobStore implements SimpleBlobStore { @@ -33,10 +32,15 @@ public ConcurrentMapBlobStore(ConcurrentMap map) { } @Override - public boolean containsKey(String key) { + public boolean contains(String key) { return map.containsKey(key); } + @Override + public boolean containsActionResult(String key) { + return map.containsKey(ACTION_KEY_PREFIX + key); + } + @Override public ListenableFuture get(String key, OutputStream out) { byte[] data = map.get(key); @@ -55,9 +59,8 @@ public ListenableFuture get(String key, OutputStream out) { } @Override - public boolean getActionResult(String key, OutputStream out) - throws IOException, InterruptedException { - return getFromFuture(get(ACTION_KEY_PREFIX + key, out)); + public ListenableFuture getActionResult(String key, OutputStream out) { + return get(ACTION_KEY_PREFIX + key, out); } @Override @@ -75,15 +78,4 @@ public void putActionResult(String key, byte[] in) { @Override public void close() {} - private static T getFromFuture(ListenableFuture f) - throws IOException, InterruptedException { - try { - return f.get(); - } catch (ExecutionException e) { - if (e.getCause() instanceof IOException) { - throw (IOException) e.getCause(); - } - throw new IOException(e.getCause()); - } - } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/blobstore/OnDiskBlobStore.java b/src/main/java/com/google/devtools/build/lib/remote/blobstore/OnDiskBlobStore.java index d4fc2e5ebad97e..933037e665f941 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/blobstore/OnDiskBlobStore.java +++ b/src/main/java/com/google/devtools/build/lib/remote/blobstore/OnDiskBlobStore.java @@ -13,7 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.remote.blobstore; -import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; import com.google.common.io.ByteStreams; import com.google.common.util.concurrent.ListenableFuture; @@ -28,21 +27,26 @@ /** A on-disk store for the remote action cache. */ public class OnDiskBlobStore implements SimpleBlobStore { private final Path root; - static final String ACTION_KEY_PREFIX = "ac_"; + private static final String ACTION_KEY_PREFIX = "ac_"; public OnDiskBlobStore(Path root) { this.root = root; } @Override - public boolean containsKey(String key) { - return toPath(key).exists(); + public boolean contains(String key) { + return toPath(key, /* actionResult= */ false).exists(); + } + + @Override + public boolean containsActionResult(String key) { + return toPath(key, /* actionResult= */ true).exists(); } @Override public ListenableFuture get(String key, OutputStream out) { SettableFuture f = SettableFuture.create(); - Path p = toPath(key); + Path p = toPath(key, /* actionResult= */ false); if (!p.exists()) { f.set(false); } else { @@ -57,21 +61,20 @@ public ListenableFuture get(String key, OutputStream out) { } @Override - public boolean getActionResult(String key, OutputStream out) - throws IOException, InterruptedException { - return getFromFuture(get(ACTION_KEY_PREFIX + key, out)); + public ListenableFuture getActionResult(String key, OutputStream out) { + return get(getDiskKey(key, /* actionResult= */ true), out); } @Override public void put(String key, long length, InputStream in) throws IOException, InterruptedException { - Path target = toPath(key); + Path target = toPath(key, /* actionResult= */ false); if (target.exists()) { return; } // Write a temporary file first, and then rename, to avoid data corruption in case of a crash. - Path temp = toPath(UUID.randomUUID().toString()); + Path temp = toPath(UUID.randomUUID().toString(), /* actionResult= */ false); try (OutputStream out = temp.getOutputStream()) { ByteStreams.copy(in, out); } @@ -82,13 +85,17 @@ public void put(String key, long length, InputStream in) @Override public void putActionResult(String key, byte[] in) throws IOException, InterruptedException { - put(ACTION_KEY_PREFIX + key, in.length, new ByteArrayInputStream(in)); + put(getDiskKey(key, /* actionResult= */ true), in.length, new ByteArrayInputStream(in)); } @Override public void close() {} - protected Path toPath(String key) { - return root.getChild(key); + protected Path toPath(String key, boolean actionResult) { + return root.getChild(getDiskKey(key, actionResult)); + } + + private String getDiskKey(String key, boolean actionResult) { + return actionResult ? OnDiskBlobStore.ACTION_KEY_PREFIX + key : key; } } diff --git a/src/main/java/com/google/devtools/build/lib/remote/blobstore/SimpleBlobStore.java b/src/main/java/com/google/devtools/build/lib/remote/blobstore/SimpleBlobStore.java index 3bf67460fa7244..ef798a7e09e3a1 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/blobstore/SimpleBlobStore.java +++ b/src/main/java/com/google/devtools/build/lib/remote/blobstore/SimpleBlobStore.java @@ -25,10 +25,11 @@ *

Implementations must be thread-safe. */ public interface SimpleBlobStore { - /** - * Returns {@code key} if the provided {@code key} is stored in the CAS. - */ - boolean containsKey(String key) throws IOException, InterruptedException; + /** Returns {@code true} if the provided {@code key} is stored in the CAS. */ + boolean contains(String key) throws IOException, InterruptedException; + + /** Returns {@code true} if the provided {@code key} is stored in the Action Cache. */ + boolean containsActionResult(String key) throws IOException, InterruptedException; /** * Fetches the BLOB associated with the {@code key} from the CAS and writes it to {@code out}. @@ -47,8 +48,7 @@ public interface SimpleBlobStore { * * @return {@code true} if the {@code key} was found. {@code false} otherwise. */ - boolean getActionResult(String actionKey, OutputStream out) - throws IOException, InterruptedException; + ListenableFuture getActionResult(String actionKey, OutputStream out); /** * Uploads a BLOB (as {@code in}) with length {@code length} indexed by {@code key} to the CAS. diff --git a/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/HttpBlobStore.java b/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/HttpBlobStore.java index 441e5a759c5b61..09c0979d0ea0ba 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/HttpBlobStore.java +++ b/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/HttpBlobStore.java @@ -13,7 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.remote.blobstore.http; -import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; import com.google.auth.Credentials; import com.google.common.util.concurrent.ListenableFuture; @@ -392,7 +391,12 @@ private boolean isChannelPipelineEmpty(ChannelPipeline pipeline) { } @Override - public boolean containsKey(String key) { + public boolean contains(String key) { + throw new UnsupportedOperationException("HTTP Caching does not use this method."); + } + + @Override + public boolean containsActionResult(String key) { throw new UnsupportedOperationException("HTTP Caching does not use this method."); } @@ -509,9 +513,8 @@ private void getAfterCredentialRefresh(DownloadCommand cmd, SettableFuture getActionResult(String actionKey, OutputStream out) { + return get(actionKey, out, false); } @Override diff --git a/src/test/shell/bazel/remote/remote_execution_http_test.sh b/src/test/shell/bazel/remote/remote_execution_http_test.sh index a9ae38c8869aa0..50193aeec9a1fc 100755 --- a/src/test/shell/bazel/remote/remote_execution_http_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_http_test.sh @@ -389,7 +389,7 @@ EOF bazel clean bazel build $disk_flags //a:test &> $TEST_log \ || fail "Failed to fetch //a:test from disk cache" - expect_log "1 remote cache hit" + expect_log "1 remote cache hit" "Fetch from disk cache failed" diff bazel-genfiles/a/test.txt ${TEST_TMPDIR}/test_expected \ || fail "Disk cache generated different result" @@ -397,7 +397,7 @@ EOF bazel clean bazel build $http_flags //a:test &> $TEST_log \ || fail "Failed to fetch //a:test from http cache" - expect_log "1 remote cache hit" + expect_log "1 remote cache hit" "Fetch from http cache failed" diff bazel-genfiles/a/test.txt ${TEST_TMPDIR}/test_expected \ || fail "HTTP cache generated different result" @@ -408,7 +408,7 @@ EOF bazel clean bazel build $disk_flags $http_flags //a:test &> $TEST_log \ || fail "Failed to copy //a:test from http cache to disk cache" - expect_log "1 remote cache hit" + expect_log "1 remote cache hit" "Copy from http cache to disk cache failed" diff bazel-genfiles/a/test.txt ${TEST_TMPDIR}/test_expected \ || fail "HTTP cache generated different result" @@ -416,7 +416,7 @@ EOF bazel clean bazel build $disk_flags //a:test &> $TEST_log \ || fail "Failed to fetch //a:test from disk cache" - expect_log "1 remote cache hit" + expect_log "1 remote cache hit" "Fetch from disk cache after copy from http cache failed" diff bazel-genfiles/a/test.txt ${TEST_TMPDIR}/test_expected \ || fail "Disk cache generated different result" diff --git a/src/tools/remote/BUILD b/src/tools/remote/BUILD index 02551671d17d34..49551e120ed18b 100644 --- a/src/tools/remote/BUILD +++ b/src/tools/remote/BUILD @@ -1,6 +1,9 @@ filegroup( name = "srcs", - srcs = glob(["**"]) + ["//src/tools/remote/src/main/java/com/google/devtools/build/remote/worker:srcs"], + srcs = glob(["**"]) + [ + "//src/tools/remote/src/main/java/com/google/devtools/build/remote/worker:srcs", + "//src/tools/remote/src/test/java/com/google/devtools/build/remote/worker:srcs", + ], visibility = ["//src:__pkg__"], ) diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/HttpCacheServerHandler.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/HttpCacheServerHandler.java index 23c095b331d148..e75104bdc5c805 100644 --- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/HttpCacheServerHandler.java +++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/HttpCacheServerHandler.java @@ -14,6 +14,7 @@ package com.google.devtools.build.remote.worker; +import com.google.common.annotations.VisibleForTesting; import io.netty.buffer.Unpooled; import io.netty.channel.ChannelFuture; import io.netty.channel.ChannelFutureListener; @@ -31,12 +32,15 @@ import io.netty.util.CharsetUtil; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.regex.Matcher; +import java.util.regex.Pattern; /** A simple HTTP REST in-memory cache used during testing the LRE. */ @Sharable public class HttpCacheServerHandler extends SimpleChannelInboundHandler { final ConcurrentMap cache = new ConcurrentHashMap<>(); + private static final Pattern URI_PATTERN = Pattern.compile("^/?(.*/)?(ac/|cas/)([a-f0-9]{64})$"); @Override protected void channelRead0(ChannelHandlerContext ctx, FullHttpRequest request) { @@ -54,7 +58,18 @@ protected void channelRead0(ChannelHandlerContext ctx, FullHttpRequest request) } } + @VisibleForTesting + static boolean isUriValid(String uri) { + Matcher matcher = URI_PATTERN.matcher(uri); + return matcher.matches(); + } + private void handleGet(ChannelHandlerContext ctx, FullHttpRequest request) { + if (!isUriValid(request.uri())) { + sendError(ctx, request, HttpResponseStatus.BAD_REQUEST); + return; + } + byte[] contents = cache.get(request.uri()); if (contents == null) { @@ -79,6 +94,10 @@ private void handlePut(ChannelHandlerContext ctx, FullHttpRequest request) { sendError(ctx, request, HttpResponseStatus.INTERNAL_SERVER_ERROR); return; } + if (!isUriValid(request.uri())) { + sendError(ctx, request, HttpResponseStatus.BAD_REQUEST); + return; + } byte[] contentBytes = new byte[request.content().readableBytes()]; request.content().readBytes(contentBytes); diff --git a/src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/BUILD b/src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/BUILD new file mode 100644 index 00000000000000..8cbd29b7b6ab1f --- /dev/null +++ b/src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/BUILD @@ -0,0 +1,22 @@ +package( + default_testonly = 1, + default_visibility = ["//src/tools/remote:__pkg__"], +) + +filegroup( + name = "srcs", + testonly = 0, + srcs = glob(["**"]), + visibility = ["//src/tools/remote:__pkg__"], +) + +java_test( + name = "worker", + srcs = glob(["*.java"]), + test_class = "com.google.devtools.build.remote.worker.HttpCacheServerHandlerTest", + deps = [ + "//src/tools/remote/src/main/java/com/google/devtools/build/remote/worker", + "//third_party:junit4", + "//third_party:truth", + ], +) diff --git a/src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/HttpCacheServerHandlerTest.java b/src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/HttpCacheServerHandlerTest.java new file mode 100644 index 00000000000000..5f50b6f1b1935e --- /dev/null +++ b/src/tools/remote/src/test/java/com/google/devtools/build/remote/worker/HttpCacheServerHandlerTest.java @@ -0,0 +1,99 @@ +// Copyright 2019 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.remote.worker; + +import static com.google.common.truth.Truth.assertThat; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Unit tests for {@link HttpCacheServerHandler} */ +@RunWith(JUnit4.class) +public class HttpCacheServerHandlerTest { + + @Test + public void testValidUri() { + assertThat( + HttpCacheServerHandler.isUriValid( + "http://some-path.co.uk:8080/ac/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isTrue(); + assertThat( + HttpCacheServerHandler.isUriValid( + "http://127.12.12.0:8080/ac/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isTrue(); + assertThat( + HttpCacheServerHandler.isUriValid( + "http://localhost:8080/ac/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isTrue(); + assertThat( + HttpCacheServerHandler.isUriValid( + "https://localhost:8080/ac/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isTrue(); + assertThat( + HttpCacheServerHandler.isUriValid( + "localhost:8080/ac/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isTrue(); + assertThat( + HttpCacheServerHandler.isUriValid( + "ac/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isTrue(); + + assertThat( + HttpCacheServerHandler.isUriValid( + "http://some-path.co.uk:8080/cas/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isTrue(); + assertThat( + HttpCacheServerHandler.isUriValid( + "http://127.12.12.0:8080/cas/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isTrue(); + assertThat( + HttpCacheServerHandler.isUriValid( + "http://localhost:8080/cas/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isTrue(); + assertThat( + HttpCacheServerHandler.isUriValid( + "https://localhost:8080/cas/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isTrue(); + assertThat( + HttpCacheServerHandler.isUriValid( + "localhost:8080/cas/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isTrue(); + assertThat( + HttpCacheServerHandler.isUriValid( + "cas/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isTrue(); + } + + @Test + public void testInvalidUri() { + assertThat( + HttpCacheServerHandler.isUriValid( + "http://localhost:8080/ac_e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isFalse(); + assertThat( + HttpCacheServerHandler.isUriValid( + "http://localhost:8080/cas_e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")) + .isFalse(); + assertThat(HttpCacheServerHandler.isUriValid("http://localhost:8080/ac/111111111111111111111")) + .isFalse(); + assertThat(HttpCacheServerHandler.isUriValid("http://localhost:8080/cas/111111111111111111111")) + .isFalse(); + assertThat(HttpCacheServerHandler.isUriValid("http://localhost:8080/cas/823rhf&*%OL%_^")) + .isFalse(); + assertThat(HttpCacheServerHandler.isUriValid("http://localhost:8080/ac/823rhf&*%OL%_^")) + .isFalse(); + } +}