Skip to content

Commit

Permalink
Downloader rewriter config has all_blocked_message
Browse files Browse the repository at this point in the history
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
  • Loading branch information
illicitonion authored and copybara-github committed Feb 26, 2021
1 parent 2f00d4f commit 63bc1c7
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.net.URL;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;

/**
* Bazel file downloader.
Expand Down Expand Up @@ -85,7 +86,7 @@ public void setDisableDownload(boolean disableDownload) {
* @throws InterruptedException if this thread is being cast into oblivion
*/
public Path download(
List<URL> urls,
List<URL> originalUrls,
Map<URI, Map<String, String>> authHeaders,
Optional<Checksum> checksum,
String canonicalId,
Expand All @@ -99,20 +100,21 @@ public Path download(
throw new InterruptedException();
}

List<URL> 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<String> candidateFileNames = getCandidateFileNames(mainUrl, destination);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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());
}
Expand All @@ -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<URL> 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<String> type, Path output) {
if (!type.isPresent()) {
return output;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -196,4 +197,9 @@ private ImmutableList<URL> applyRewriteRules(URL url) {
})
.collect(toImmutableList());
}

@Nullable
public String getAllBlockedMessage() {
return config.getAllBlockedMessage();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -36,11 +37,15 @@
* <li>{@code block hostName} Will block access to the given host and subdomains
* <li>{@code rewrite pattern pattern} Rewrite a URL using the given pattern. Back references are
* numbered from `$1`
* <li>{@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.
* </ul>
*
* The directives are applied in the order `rewrite, allow, block'. An example config may look like:
*
* <pre>
* 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
Expand All @@ -62,13 +67,16 @@ 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<String> allowList;
// A set of domain names that should be blocked.
private final Set<String> blockList;
// A set of patterns matching "everything in the url after the scheme" to rewrite rules.
private final ImmutableMultimap<Pattern, String> 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.
Expand All @@ -81,6 +89,7 @@ public UrlRewriterConfig(String filePathForErrorReporting, Reader config)
ImmutableSet.Builder<String> allowList = ImmutableSet.builder();
ImmutableSet.Builder<String> blockList = ImmutableSet.builder();
ImmutableMultimap.Builder<Pattern, String> rewrites = ImmutableMultimap.builder();
String allBlockedMessage = null;

try (BufferedReader reader = new BufferedReader(config)) {
int lineNumber = 1;
Expand Down Expand Up @@ -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);
}
Expand All @@ -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. */
Expand All @@ -155,4 +177,9 @@ public Set<String> getBlockList() {
public Map<Pattern, Collection<String>> getRewrites() {
return rewrites.asMap();
}

@Nullable
public String getAllBlockedMessage() {
return allBlockedMessage;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
}

0 comments on commit 63bc1c7

Please sign in to comment.