From 63bc1c7d0853dc187e4b96a490d733fb29f79664 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Fri, 26 Feb 2021 04:22:40 -0800 Subject: [PATCH] Downloader rewriter config has all_blocked_message This allows for the config author to specify a useful error message to adorn the user-facing error, so that if the user is unaware or forgets about the config, they get a more clear error message than: ``` ERROR: java.io.IOException: Error downloading [] to /some/path ``` or ``` Error in download: java.io.IOException: Cache miss and no url specified ``` Currently there is a log line like: ``` INFO: Rewritten [https://doesnotexist.com/beep] as [] ``` printed, but this is often quite far from the error - this puts all the information in one place. If you don't configure an all_blocked_message, this now looks like: ``` ERROR: java.io.IOException: Cache miss and no url specified - Configured URL rewriter blocked all URLs: [https://doesnotexist.com/beep] ``` And if you do, it will look like something like: ``` ERROR: java.io.IOException: Cache miss and no url specified - Configured URL rewriter blocked all URLs: [https://doesnotexist.com/beep] - See https://mycorp.com/bazel-rewriter-issue for more details. ``` Closes #12997. PiperOrigin-RevId: 359730842 --- .../downloader/DownloadManager.java | 48 +++++++++++++++---- .../repository/downloader/UrlRewriter.java | 6 +++ .../downloader/UrlRewriterConfig.java | 27 +++++++++++ .../downloader/UrlRewriterTest.java | 27 +++++++++++ 4 files changed, 100 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java index a117b7d2b268c3..a8d0b1fdaacccb 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/DownloadManager.java @@ -33,6 +33,7 @@ import java.net.URL; import java.util.List; import java.util.Map; +import javax.annotation.Nullable; /** * Bazel file downloader. @@ -85,7 +86,7 @@ public void setDisableDownload(boolean disableDownload) { * @throws InterruptedException if this thread is being cast into oblivion */ public Path download( - List urls, + List originalUrls, Map> authHeaders, Optional checksum, String canonicalId, @@ -99,20 +100,21 @@ public Path download( throw new InterruptedException(); } + List rewrittenUrls = originalUrls; if (rewriter != null) { - urls = rewriter.amend(urls); + rewrittenUrls = rewriter.amend(originalUrls); } URL mainUrl; // The "main" URL for this request // Used for reporting only and determining the file name only. - if (urls.isEmpty()) { + if (rewrittenUrls.isEmpty()) { if (type.isPresent() && !Strings.isNullOrEmpty(type.get())) { mainUrl = new URL("http://nonexistent.example.org/cacheprobe." + type.get()); } else { mainUrl = new URL("http://nonexistent.example.org/cacheprobe"); } } else { - mainUrl = urls.get(0); + mainUrl = rewrittenUrls.get(0); } Path destination = getDownloadDestination(mainUrl, type, output); ImmutableSet candidateFileNames = getCandidateFileNames(mainUrl, destination); @@ -153,8 +155,13 @@ public Path download( } } - if (urls.isEmpty()) { - throw new IOException("Cache miss and no url specified"); + if (rewrittenUrls.isEmpty()) { + StringBuilder message = new StringBuilder("Cache miss and no url specified"); + if (!originalUrls.isEmpty()) { + message.append(" - "); + message.append(getRewriterBlockedAllUrlsMessage(originalUrls)); + } + throw new IOException(message.toString()); } for (Path dir : distdir) { @@ -204,9 +211,20 @@ public Path download( String.format("Failed to download repo %s: download is disabled.", repo)); } + if (rewrittenUrls.isEmpty() && !originalUrls.isEmpty()) { + throw new IOException(getRewriterBlockedAllUrlsMessage(originalUrls)); + } + try { downloader.download( - urls, authHeaders, checksum, canonicalId, destination, eventHandler, clientEnv, type); + rewrittenUrls, + authHeaders, + checksum, + canonicalId, + destination, + eventHandler, + clientEnv, + type); } catch (InterruptedIOException e) { throw new InterruptedException(e.getMessage()); } @@ -216,12 +234,26 @@ public Path download( checksum.get().toString(), destination, checksum.get().getKeyType(), canonicalId); } else if (repositoryCache.isEnabled()) { String newSha256 = repositoryCache.put(destination, KeyType.SHA256, canonicalId); - eventHandler.handle(Event.info("SHA256 (" + urls.get(0) + ") = " + newSha256)); + eventHandler.handle(Event.info("SHA256 (" + rewrittenUrls.get(0) + ") = " + newSha256)); } return destination; } + @Nullable + private String getRewriterBlockedAllUrlsMessage(List originalUrls) { + if (rewriter == null) { + return null; + } + StringBuilder message = new StringBuilder("Configured URL rewriter blocked all URLs: "); + message.append(originalUrls); + String rewriterMessage = rewriter.getAllBlockedMessage(); + if (rewriterMessage != null) { + message.append(" - ").append(rewriterMessage); + } + return message.toString(); + } + private Path getDownloadDestination(URL url, Optional type, Path output) { if (!type.isPresent()) { return output; diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriter.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriter.java index dfeee8866b3202..a081f433b5c96c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriter.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriter.java @@ -40,6 +40,7 @@ import java.util.function.Function; import java.util.regex.Matcher; import java.util.regex.Pattern; +import javax.annotation.Nullable; /** * Helper class for taking URLs and converting them according to an optional config specified by @@ -196,4 +197,9 @@ private ImmutableList applyRewriteRules(URL url) { }) .collect(toImmutableList()); } + + @Nullable + public String getAllBlockedMessage() { + return config.getAllBlockedMessage(); + } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriterConfig.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriterConfig.java index 322cf11df46bd3..ad3b1705af82b2 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriterConfig.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriterConfig.java @@ -25,6 +25,7 @@ import java.util.Map; import java.util.Set; import java.util.regex.Pattern; +import javax.annotation.Nullable; import net.starlark.java.syntax.Location; /** @@ -36,11 +37,15 @@ *
  • {@code block hostName} Will block access to the given host and subdomains *
  • {@code rewrite pattern pattern} Rewrite a URL using the given pattern. Back references are * numbered from `$1` + *
  • {@code all_blocked_message message which may contain spaces} If the rewriter causes all + * URLs for a particular resource to be blocked, this informational message will be rendered + * to the user. This directive may only be present at most once. * * * The directives are applied in the order `rewrite, allow, block'. An example config may look like: * * 
    + *     all_blocked_message See mycorp.com/blocked-bazel-fetches for more information.
  *     block mvnrepository.com
  *     block maven-central.storage.googleapis.com
  *     block gitblit.github.io
@@ -62,6 +67,7 @@ class UrlRewriterConfig {
 
   private static final Splitter SPLITTER =
       Splitter.onPattern("\\s+").omitEmptyStrings().trimResults();
+  private static final String ALL_BLOCKED_MESSAGE_DIRECTIVE = "all_blocked_message";
 
   // A set of domain names that should be accessible.
   private final Set allowList;
@@ -69,6 +75,8 @@ class UrlRewriterConfig {
   private final Set blockList;
   // A set of patterns matching "everything in the url after the scheme" to rewrite rules.
   private final ImmutableMultimap rewrites;
+  // Message to display if the rewriter caused all URLs to be blocked.
+  @Nullable private final String allBlockedMessage;
 
   /**
    * Constructor to use. The {@code config} will be read to completion.
@@ -81,6 +89,7 @@ public UrlRewriterConfig(String filePathForErrorReporting, Reader config)
     ImmutableSet.Builder allowList = ImmutableSet.builder();
     ImmutableSet.Builder blockList = ImmutableSet.builder();
     ImmutableMultimap.Builder rewrites = ImmutableMultimap.builder();
+    String allBlockedMessage = null;
 
     try (BufferedReader reader = new BufferedReader(config)) {
       int lineNumber = 1;
@@ -125,6 +134,18 @@ public UrlRewriterConfig(String filePathForErrorReporting, Reader config)
             rewrites.put(Pattern.compile(parts.get(1)), parts.get(2));
             break;
 
+          case ALL_BLOCKED_MESSAGE_DIRECTIVE:
+            if (parts.size() == 1) {
+              throw new UrlRewriterParseException(
+                  "all_blocked_message must be followed by a message", location);
+            }
+            if (allBlockedMessage != null) {
+              throw new UrlRewriterParseException(
+                  "At most one all_blocked_message directive is allowed", location);
+            }
+            allBlockedMessage = line.substring(ALL_BLOCKED_MESSAGE_DIRECTIVE.length() + 1);
+            break;
+
           default:
             throw new UrlRewriterParseException("Unable to parse: " + line, location);
         }
@@ -136,6 +157,7 @@ public UrlRewriterConfig(String filePathForErrorReporting, Reader config)
     this.allowList = allowList.build();
     this.blockList = blockList.build();
     this.rewrites = rewrites.build();
+    this.allBlockedMessage = allBlockedMessage;
   }
 
   /** Returns all {@code allow} directives. */
@@ -155,4 +177,9 @@ public Set getBlockList() {
   public Map> getRewrites() {
     return rewrites.asMap();
   }
+
+  @Nullable
+  public String getAllBlockedMessage() {
+    return allBlockedMessage;
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriterTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriterTest.java
index 10ec84edbac2f6..805cf016d2438f 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriterTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriterTest.java
@@ -185,4 +185,31 @@ public void parseError() throws Exception {
       assertThat(e.getLocation()).isEqualTo(Location.fromFileLineColumn("/some/file", 2, 0));
     }
   }
+
+  @Test
+  public void noAllBlockedMessage() throws Exception {
+    String config = "";
+    UrlRewriterConfig munger = new UrlRewriterConfig("/some/file", new StringReader(config));
+    assertThat(munger.getAllBlockedMessage()).isNull();
+  }
+
+  @Test
+  public void singleAllBlockedMessage() throws Exception {
+    String config =
+        "all_blocked_message I'm sorry Dave, I'm afraid I can't do that.\n" + "allow *\n";
+    UrlRewriterConfig munger = new UrlRewriterConfig("/some/file", new StringReader(config));
+    assertThat(munger.getAllBlockedMessage())
+        .isEqualTo("I'm sorry Dave, I'm afraid I can't do that.");
+  }
+
+  @Test
+  public void multipleAllBlockedMessage() throws Exception {
+    String config = "all_blocked_message one\n" + "block *\n" + "all_blocked_message two\n";
+    try {
+      new UrlRewriterConfig("/some/file", new StringReader(config));
+      fail();
+    } catch (UrlRewriterParseException e) {
+      assertThat(e.getLocation()).isEqualTo(Location.fromFileLineColumn("/some/file", 3, 0));
+    }
+  }
 }