Disallow tracing configs that trigger tracing for too many requests
Misconfigured tracing configs or tracing configs that are too broad can
trigger tracing for too many requests. This can badly impact the
performance and the availability of the server, if tracing makes
requests more expensive (e.g. if there are expensive logs that are only
written when a request is traced).
Examples for a misconfigured or trace configs that are too broad:
* a tracing config that specifies neither of the match criteria (e.g. it
contains an unsupported parameter, which can happen is the parameter
has typo)
* a tracing config that only specifies match criteria that are very
broad (e.g. requestType=REST which would trigger tracing for all REST
requests)
Fix this by requiring that tracing configs specify at least one of
requestUriPattern, account and projectPattern.
Release-Notes: Disallowed tracing configs that trigger tracing for too many requests
Bug: Google b/355393231
Change-Id: I9c9fac92f45e427087f7fe705169f73296c0b2c8
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 0c218c8..f274a3e 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -5891,9 +5891,10 @@
There can be multiple `tracing.<trace-id>` subsections to configure
automatic tracing of requests. To be traced a request must match all
-conditions of one `tracing.<trace-id>` subsection. The subsection name
-is used as trace ID. Using this trace ID administrators can find
-matching log entries.
+conditions of one `tracing.<trace-id>` subsection. A `tracing.<trace-id>`
+subsection must specify at least one of `requestUriPattern`, `account` and
+`projectPattern`, or otherwise it is ignored. The subsection name is used as
+trace ID. Using this trace ID administrators can find matching log entries.
[[tracing.traceid.requestType]]tracing.<trace-id>.requestType::
+
diff --git a/java/com/google/gerrit/server/RequestConfig.java b/java/com/google/gerrit/server/RequestConfig.java
index f942c5e..f3d3e51 100644
--- a/java/com/google/gerrit/server/RequestConfig.java
+++ b/java/com/google/gerrit/server/RequestConfig.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
@@ -55,6 +57,27 @@
return requestConfigs.build();
}
+ public static ImmutableList<RequestConfig> parseTraceConfigs(Config cfg, String section) {
+ // To prevent that misconfigured tracing configs (e.g. empty configs) cause tracing of too many
+ // requests, require that at least one of the following match criteria have been specified:
+ // request URI pattern, account, project pattern
+ return parseConfigs(cfg, section).stream()
+ .filter(
+ requestConfig -> {
+ if (!requestConfig.requestUriPatterns().isEmpty()
+ || !requestConfig.accountIds().isEmpty()
+ || !requestConfig.projectPatterns().isEmpty()) {
+ return true;
+ }
+ logger.atWarning().log(
+ "Ignoring tracing configuration %s because it is too broad (needs to set at"
+ + " least one of: requestUriPattern, account, projectPattern)",
+ section);
+ return false;
+ })
+ .collect(toImmutableList());
+ }
+
private static ImmutableSet<String> parseRequestTypes(Config cfg, String section, String id) {
return ImmutableSet.copyOf(cfg.getStringList(section, id, "requestType"));
}
@@ -247,9 +270,8 @@
}
}
- // For any match criteria (request type, request URI pattern, request query string pattern,
- // header, account, project pattern) that was specified in the request config, at least one of
- // the configured value matched the request.
+ // All specified match criteria (request type, request URI pattern, request query string
+ // pattern, header, account, project pattern) did match.
return true;
}
diff --git a/java/com/google/gerrit/server/TraceRequestListener.java b/java/com/google/gerrit/server/TraceRequestListener.java
index 6cc0982..f99cc2d 100644
--- a/java/com/google/gerrit/server/TraceRequestListener.java
+++ b/java/com/google/gerrit/server/TraceRequestListener.java
@@ -37,7 +37,7 @@
@Inject
TraceRequestListener(@GerritServerConfig Config cfg) {
- this.traceConfigs = RequestConfig.parseConfigs(cfg, SECTION_TRACING);
+ this.traceConfigs = RequestConfig.parseTraceConfigs(cfg, SECTION_TRACING);
}
@Override
diff --git a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
index c675634..f8d3c6c 100644
--- a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
@@ -564,7 +564,7 @@
@Test
@GerritConfig(name = "tracing.issue123.requestType", value = "REST")
- public void traceRequestType() throws Exception {
+ public void traceConfigWithRequestTypeOnlyDoesntTriggerTracing() throws Exception {
TraceValidatingProjectCreationValidationListener projectCreationListener =
new TraceValidatingProjectCreationValidationListener();
try (Registration registration =
@@ -572,15 +572,18 @@
RestResponse response = adminRestSession.put("/projects/new19");
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
- assertThat(projectCreationListener.traceId).isEqualTo("issue123");
- assertThat(projectCreationListener.isLoggingForced).isTrue();
+ assertThat(projectCreationListener.traceId).isNull();
+ assertThat(projectCreationListener.isLoggingForced).isFalse();
+
+ // The logging tag with the project name is also set if tracing is off.
assertThat(projectCreationListener.tags.get("project")).containsExactly("new19");
}
}
@Test
- @GerritConfig(name = "tracing.issue123.requestType", value = "SSH")
- public void traceRequestTypeNoMatch() throws Exception {
+ @GerritConfig(name = "tracing.issue123.projectPattern", value = "new20")
+ @GerritConfig(name = "tracing.issue123.requestType", value = "REST")
+ public void traceRequestType() throws Exception {
TraceValidatingProjectCreationValidationListener projectCreationListener =
new TraceValidatingProjectCreationValidationListener();
try (Registration registration =
@@ -588,11 +591,28 @@
RestResponse response = adminRestSession.put("/projects/new20");
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
+ assertThat(projectCreationListener.traceId).isEqualTo("issue123");
+ assertThat(projectCreationListener.isLoggingForced).isTrue();
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("new20");
+ }
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.projectPattern", value = "new21")
+ @GerritConfig(name = "tracing.issue123.requestType", value = "SSH")
+ public void traceRequestTypeNoMatch() throws Exception {
+ TraceValidatingProjectCreationValidationListener projectCreationListener =
+ new TraceValidatingProjectCreationValidationListener();
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(projectCreationListener)) {
+ RestResponse response = adminRestSession.put("/projects/new21");
+ assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
assertThat(projectCreationListener.traceId).isNull();
assertThat(projectCreationListener.isLoggingForced).isFalse();
// The logging tag with the project name is also set if tracing is off.
- assertThat(projectCreationListener.tags.get("project")).containsExactly("new20");
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("new21");
}
}
@@ -637,14 +657,14 @@
new TraceValidatingProjectCreationValidationListener();
try (Registration registration =
extensionRegistry.newRegistration().add(projectCreationListener)) {
- RestResponse response = adminRestSession.put("/projects/new21");
+ RestResponse response = adminRestSession.put("/projects/new22");
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
assertThat(projectCreationListener.traceId).isNull();
assertThat(projectCreationListener.isLoggingForced).isFalse();
// The logging tag with the project name is also set if tracing is off.
- assertThat(projectCreationListener.tags.get("project")).containsExactly("new21");
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("new22");
}
}
@@ -656,12 +676,12 @@
new TraceValidatingProjectCreationValidationListener();
try (Registration registration =
extensionRegistry.newRegistration().add(projectCreationListener)) {
- RestResponse response = adminRestSession.put("/projects/new22");
+ RestResponse response = adminRestSession.put("/projects/new23");
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
assertThat(projectCreationListener.traceId).isEqualTo("issue123");
assertThat(projectCreationListener.isLoggingForced).isTrue();
- assertThat(projectCreationListener.tags.get("project")).containsExactly("new22");
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("new23");
}
}
@@ -673,14 +693,14 @@
new TraceValidatingProjectCreationValidationListener();
try (Registration registration =
extensionRegistry.newRegistration().add(projectCreationListener)) {
- RestResponse response = adminRestSession.put("/projects/new23");
+ RestResponse response = adminRestSession.put("/projects/new24");
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
assertThat(projectCreationListener.traceId).isNull();
assertThat(projectCreationListener.isLoggingForced).isFalse();
// The logging tag with the project name is also set if tracing is off.
- assertThat(projectCreationListener.tags.get("project")).containsExactly("new23");
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("new24");
}
}
@@ -692,14 +712,14 @@
new TraceValidatingProjectCreationValidationListener();
try (Registration registration =
extensionRegistry.newRegistration().add(projectCreationListener)) {
- RestResponse response = adminRestSession.put("/projects/new24");
+ RestResponse response = adminRestSession.put("/projects/new25");
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
assertThat(projectCreationListener.traceId).isNull();
assertThat(projectCreationListener.isLoggingForced).isFalse();
// The logging tag with the project name is also set if tracing is off.
- assertThat(projectCreationListener.tags.get("project")).containsExactly("new24");
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("new25");
}
}
@@ -710,12 +730,12 @@
new TraceValidatingProjectCreationValidationListener();
try (Registration registration =
extensionRegistry.newRegistration().add(projectCreationListener)) {
- RestResponse response = adminRestSession.put("/projects/new23");
+ RestResponse response = adminRestSession.put("/projects/new26");
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
assertThat(projectCreationListener.traceId).isEqualTo("issue123");
assertThat(projectCreationListener.isLoggingForced).isTrue();
- assertThat(projectCreationListener.tags.get("project")).containsExactly("new23");
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("new26");
}
}
@@ -726,14 +746,14 @@
new TraceValidatingProjectCreationValidationListener();
try (Registration registration =
extensionRegistry.newRegistration().add(projectCreationListener)) {
- RestResponse response = adminRestSession.put("/projects/new23");
+ RestResponse response = adminRestSession.put("/projects/new27");
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
assertThat(projectCreationListener.traceId).isNull();
assertThat(projectCreationListener.isLoggingForced).isFalse();
// The logging tag with the project name is also set if tracing is off.
- assertThat(projectCreationListener.tags.get("project")).containsExactly("new23");
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("new27");
}
}
@@ -744,14 +764,14 @@
new TraceValidatingProjectCreationValidationListener();
try (Registration registration =
extensionRegistry.newRegistration().add(projectCreationListener)) {
- RestResponse response = adminRestSession.put("/projects/new24");
+ RestResponse response = adminRestSession.put("/projects/new28");
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
assertThat(projectCreationListener.traceId).isNull();
assertThat(projectCreationListener.isLoggingForced).isFalse();
// The logging tag with the project name is also set if tracing is off.
- assertThat(projectCreationListener.tags.get("project")).containsExactly("new24");
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("new28");
}
}
@@ -775,24 +795,7 @@
@Test
@GerritConfig(name = "tracing.issue123.excludedRequestUriPattern", value = "/projects/no-match")
- public void traceExcludedRequestUriPatternNoMatch() throws Exception {
- TraceValidatingProjectCreationValidationListener projectCreationListener =
- new TraceValidatingProjectCreationValidationListener();
- try (Registration registration =
- extensionRegistry.newRegistration().add(projectCreationListener)) {
- RestResponse response = adminRestSession.put("/projects/xyz3");
- assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
- assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
- assertThat(projectCreationListener.traceId).isEqualTo("issue123");
- assertThat(projectCreationListener.isLoggingForced).isTrue();
- assertThat(projectCreationListener.tags.get("project")).containsExactly("xyz3");
- }
- }
-
- @Test
- @GerritConfig(name = "tracing.issue123.requestUriPattern", value = "/projects/.*")
- @GerritConfig(name = "tracing.issue123.excludedRequestUriPattern", value = "/projects/xyz2")
- public void traceRequestUriPatternAndExcludedRequestUriPattern() throws Exception {
+ public void traceConfigWithExcludedRequestUriPatternOnlyDoesntTriggerTracing() throws Exception {
TraceValidatingProjectCreationValidationListener projectCreationListener =
new TraceValidatingProjectCreationValidationListener();
try (Registration registration =
@@ -809,9 +812,9 @@
}
@Test
- @GerritConfig(name = "tracing.issue123.requestUriPattern", value = "/projects/.*")
+ @GerritConfig(name = "tracing.issue123.projectPattern", value = "xyz3")
@GerritConfig(name = "tracing.issue123.excludedRequestUriPattern", value = "/projects/no-match")
- public void traceRequestUriPatternAndExcludedRequestUriPatternNoMatch() throws Exception {
+ public void traceExcludedRequestUriPatternNoMatch() throws Exception {
TraceValidatingProjectCreationValidationListener projectCreationListener =
new TraceValidatingProjectCreationValidationListener();
try (Registration registration =
@@ -826,8 +829,9 @@
}
@Test
- @GerritConfig(name = "tracing.issue123.excludedRequestUriPattern", value = "][")
- public void traceExcludedRequestUriInvalidRegEx() throws Exception {
+ @GerritConfig(name = "tracing.issue123.requestUriPattern", value = "/projects/.*")
+ @GerritConfig(name = "tracing.issue123.excludedRequestUriPattern", value = "/projects/xyz4")
+ public void traceRequestUriPatternAndExcludedRequestUriPattern() throws Exception {
TraceValidatingProjectCreationValidationListener projectCreationListener =
new TraceValidatingProjectCreationValidationListener();
try (Registration registration =
@@ -844,7 +848,59 @@
}
@Test
+ @GerritConfig(name = "tracing.issue123.requestUriPattern", value = "/projects/.*")
+ @GerritConfig(name = "tracing.issue123.excludedRequestUriPattern", value = "/projects/no-match")
+ public void traceRequestUriPatternAndExcludedRequestUriPatternNoMatch() throws Exception {
+ TraceValidatingProjectCreationValidationListener projectCreationListener =
+ new TraceValidatingProjectCreationValidationListener();
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(projectCreationListener)) {
+ RestResponse response = adminRestSession.put("/projects/xyz5");
+ assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
+ assertThat(projectCreationListener.traceId).isEqualTo("issue123");
+ assertThat(projectCreationListener.isLoggingForced).isTrue();
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("xyz5");
+ }
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.excludedRequestUriPattern", value = "][")
+ public void traceExcludedRequestUriInvalidRegEx() throws Exception {
+ TraceValidatingProjectCreationValidationListener projectCreationListener =
+ new TraceValidatingProjectCreationValidationListener();
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(projectCreationListener)) {
+ RestResponse response = adminRestSession.put("/projects/xyz6");
+ assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
+ assertThat(projectCreationListener.traceId).isNull();
+ assertThat(projectCreationListener.isLoggingForced).isFalse();
+
+ // The logging tag with the project name is also set if tracing is off.
+ assertThat(projectCreationListener.tags.get("project")).containsExactly("xyz6");
+ }
+ }
+
+ @Test
@GerritConfig(name = "tracing.issue123.requestQueryStringPattern", value = ".*limit=.*")
+ public void traceConfigWithRequestQueryStringOnlyDoesntTriggerTracing() throws Exception {
+ String changeId = createChange().getChangeId();
+ TraceReviewerSuggestion reviewerSuggestion = new TraceReviewerSuggestion();
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(reviewerSuggestion, /* exportName= */ "foo")) {
+ RestResponse response =
+ adminRestSession.get(String.format("/changes/%s/suggest_reviewers?limit=10", changeId));
+ assertThat(response.getStatusCode()).isEqualTo(SC_OK);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
+ assertThat(reviewerSuggestion.traceId).isNull();
+ assertThat(reviewerSuggestion.isLoggingForced).isFalse();
+ }
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.requestQueryStringPattern", value = ".*limit=.*")
+ @GerritConfig(name = "tracing.issue123.requestUriPattern", value = "/changes/.*")
public void traceRequestQueryString() throws Exception {
String changeId = createChange().getChangeId();
TraceReviewerSuggestion reviewerSuggestion = new TraceReviewerSuggestion();
@@ -861,6 +917,7 @@
@Test
@GerritConfig(name = "tracing.issue123.requestQueryStringPattern", value = ".*query=.*")
+ @GerritConfig(name = "tracing.issue123.requestUriPattern", value = "/changes/.*")
public void traceRequestQueryStringNoMatch() throws Exception {
String changeId = createChange().getChangeId();
TraceReviewerSuggestion reviewerSuggestion = new TraceReviewerSuggestion();
@@ -877,6 +934,7 @@
@Test
@GerritConfig(name = "tracing.issue123.requestQueryStringPattern", value = "][")
+ @GerritConfig(name = "tracing.issue123.requestUriPattern", value = "/changes/.*")
public void traceRequestQueryStringInvalidRegEx() throws Exception {
String changeId = createChange().getChangeId();
TraceReviewerSuggestion reviewerSuggestion = new TraceReviewerSuggestion();
@@ -893,6 +951,27 @@
@Test
@GerritConfig(name = "tracing.issue123.headerPattern", value = "User-Agent=foo.*")
+ public void traceConfigWithHeaderOnlyDoesntTriggerTracing() throws Exception {
+ String changeId = createChange().getChangeId();
+ TraceReviewerSuggestion reviewerSuggestion = new TraceReviewerSuggestion();
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(reviewerSuggestion, /* exportName= */ "foo")) {
+
+ RestResponse response =
+ adminRestSession.getWithHeaders(
+ String.format("/changes/%s/suggest_reviewers?limit=10", changeId),
+ new BasicHeader("User-Agent", "foo-bar"),
+ new BasicHeader("Other-Header", "baz"));
+ assertThat(response.getStatusCode()).isEqualTo(SC_OK);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
+ assertThat(reviewerSuggestion.traceId).isNull();
+ assertThat(reviewerSuggestion.isLoggingForced).isFalse();
+ }
+ }
+
+ @Test
+ @GerritConfig(name = "tracing.issue123.headerPattern", value = "User-Agent=foo.*")
+ @GerritConfig(name = "tracing.issue123.requestUriPattern", value = "/changes/.*")
public void traceHeader() throws Exception {
String changeId = createChange().getChangeId();
TraceReviewerSuggestion reviewerSuggestion = new TraceReviewerSuggestion();
