Merge branch 'stable-2.16' into stable-3.0 * stable-2.16: Revert "HC returns 500 only with TIMEOUT or FAILED" HC returns 500 only with TIMEOUT or FAILED AuthHealthCheck: evict account cache by username Fix disabling of healthchecks Add ability to disable healthchecks Fix typo when getting auth username from configuration file Change-Id: I7e0b68d5a610283399a0919eac2558d2233342b7
diff --git a/README.md b/README.md index f5af98a..d5cb3db 100644 --- a/README.md +++ b/README.md
@@ -49,6 +49,7 @@ - result: result of the health check - passed: the check passed successfully + - disabled: the check was disabled - failed: the check failed with an error - timeout: the check took too long and timed out
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java index 5e1c7ea..4e4fab1 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfig.java
@@ -14,6 +14,8 @@ package com.googlesource.gerrit.plugins.healthcheck; +import static com.google.common.base.Preconditions.checkNotNull; + import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; import com.google.common.base.Strings; @@ -40,6 +42,7 @@ private static final int LIMIT_DEFAULT = 10; private static final String USERNAME_DEFAULT = "healthcheck"; private static final String PASSWORD_DEFAULT = ""; + private static final boolean HEALTH_CHECK_ENABLED_DEFAULT = true; private final AllProjectsName allProjectsName; private final AllUsersName allUsersName; @@ -57,7 +60,7 @@ } @VisibleForTesting - public HealthCheckConfig(String configText) { + HealthCheckConfig(String configText) { config = new Config(); if (!Strings.isNullOrEmpty(configText)) { try { @@ -70,6 +73,11 @@ allUsersName = new AllUsersName("All-Users"); } + @VisibleForTesting + void fromText(String configText) throws ConfigInvalidException { + config.fromText(configText); + } + public long getTimeout() { return getTimeout(null); } @@ -100,13 +108,18 @@ } public String getUsername(String healthCheckName) { - return getStringWithFallback("userame", healthCheckName, USERNAME_DEFAULT); + return getStringWithFallback("username", healthCheckName, USERNAME_DEFAULT); } public String getPassword(String healthCheckName) { return getStringWithFallback("password", healthCheckName, PASSWORD_DEFAULT); } + public boolean healthCheckEnabled(String healthCheckName) { + return config.getBoolean( + HEALTHCHECK, checkNotNull(healthCheckName), "enabled", HEALTH_CHECK_ENABLED_DEFAULT); + } + private String getStringWithFallback( String parameter, String healthCheckName, String defaultValue) { String fallbackDefault =
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java index 26a2042..c508222 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/api/HealthCheckStatusEndpoint.java
@@ -51,7 +51,7 @@ .filter( res -> res instanceof HealthCheck.StatusSummary - && ((HealthCheck.StatusSummary) res).result != HealthCheck.Result.PASSED) + && ((HealthCheck.StatusSummary) res).isFailure()) .findFirst() .isPresent()) { return HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java index 26ef1ee..a6eae79 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/AbstractHealthCheck.java
@@ -29,12 +29,14 @@ private final String name; private final ListeningExecutorService executor; protected StatusSummary latestStatus; + protected HealthCheckConfig config; protected AbstractHealthCheck( ListeningExecutorService executor, HealthCheckConfig config, String name) { this.executor = executor; this.name = name; this.timeout = config.getTimeout(name); + this.config = config; this.latestStatus = StatusSummary.INITIAL_STATUS; } @@ -45,13 +47,14 @@ @Override public StatusSummary run() { + boolean enabled = config.healthCheckEnabled(name); final long ts = System.currentTimeMillis(); ListenableFuture<StatusSummary> resultFuture = executor.submit( () -> { Result healthy; try { - healthy = doCheck(); + healthy = enabled ? doCheck() : Result.DISABLED; } catch (Exception e) { log.warn("Check {} failed", name, e); healthy = Result.FAILED;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java index 4f6a532..de571d1 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java +++ b/src/main/java/com/googlesource/gerrit/plugins/healthcheck/check/HealthCheck.java
@@ -15,6 +15,9 @@ package com.googlesource.gerrit.plugins.healthcheck.check; import com.google.gson.annotations.SerializedName; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; public interface HealthCheck { @@ -24,7 +27,9 @@ @SerializedName("failed") FAILED, @SerializedName("timeout") - TIMEOUT; + TIMEOUT, + @SerializedName("disabled") + DISABLED; } public class StatusSummary { @@ -33,6 +38,8 @@ public final Result result; public final long ts; public final long elapsed; + public static final Set<Result> failingResults = + new HashSet<>(Arrays.asList(Result.FAILED, Result.TIMEOUT)); public StatusSummary(Result result, long ts, long elapsed) { this.result = result; @@ -41,7 +48,7 @@ } public Boolean isFailure() { - return this.result != Result.PASSED; + return failingResults.contains(this.result); } }
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index 838db79..91d1c0a 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -49,7 +49,10 @@ - `projectslist` : check the ability to list projects with their descriptions - `auth`: check the ability to authenticate with username and password -The follwing parameters are available: +Each check name can be disabled by setting the `enabled` parameter to **false**, +by default this parameter is set to **true** + +The following parameters are available: - `healthcheck.<checkName>.timeout` : Specific timeout (msec) for the healthcheck to complete. Zero means that there is no timeout. @@ -71,11 +74,11 @@ to the default ones that are always included. Default: All-Projects, All-Users - + - `healthcheck.auth.username` : Username to use for authentication - + Default: healthcheck - + - `healthcheck.auth.password` : Password to use for authentication - Default: no password + Default: no password \ No newline at end of file
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfigTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfigTest.java index fc215ff..264db94 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfigTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckConfigTest.java
@@ -37,6 +37,22 @@ } @Test + public void shouldHaveAuthUsername() { + HealthCheckConfig config = + new HealthCheckConfig("[healthcheck \"auth\"]\n" + "username=test_user"); + + assertThat(config.getUsername("auth")).isEqualTo("test_user"); + } + + @Test + public void shouldHaveAuthPassword() { + HealthCheckConfig config = + new HealthCheckConfig("[healthcheck \"auth\"]\n" + "password=secret"); + + assertThat(config.getPassword("auth")).isEqualTo("secret"); + } + + @Test public void shouldHaveCheckOverriddenTimeout() { HealthCheckConfig config = new HealthCheckConfig( @@ -45,4 +61,28 @@ assertThat(config.getTimeout("fooCheck")).isEqualTo(1000); assertThat(config.getTimeout("barCheck")).isEqualTo(2000); } + + @Test + public void shouldHaveAnEnabledValue() { + HealthCheckConfig config = + new HealthCheckConfig("[healthcheck \"fooCheck\"]\n" + "enabled=false"); + + assertThat(config.healthCheckEnabled("fooCheck")).isEqualTo(false); + } + + @Test + public void shouldHaveEnabledAndDisabledValue() { + HealthCheckConfig config = + new HealthCheckConfig( + "[healthcheck \"fooCheck\"]\n" + + "enabled=false\n" + + "[healthcheck \"barCheck\"]\n" + + "timeout=1000" + + "[healthcheck \"bazCheck\"]\n" + + "enabled=true\n"); + + assertThat(config.healthCheckEnabled("fooCheck")).isEqualTo(false); + assertThat(config.healthCheckEnabled("barCheck")).isEqualTo(true); + assertThat(config.healthCheckEnabled("bazCheck")).isEqualTo(true); + } }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java index 24bfc81..148f8f5 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/HealthCheckIT.java
@@ -16,6 +16,7 @@ import static com.google.common.net.HttpHeaders.CONTENT_TYPE; import static com.google.common.truth.Truth.assertThat; +import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.AUTH; import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.JGIT; import static com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames.QUERYCHANGES; @@ -23,12 +24,11 @@ import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.acceptance.Sandboxed; import com.google.gerrit.acceptance.TestPlugin; -import com.google.gerrit.extensions.annotations.PluginName; import com.google.gson.Gson; import com.google.gson.JsonObject; -import com.google.inject.AbstractModule; import com.googlesource.gerrit.plugins.healthcheck.check.HealthCheckNames; import java.io.IOException; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.junit.Before; import org.junit.Test; @@ -43,17 +43,7 @@ public void setUpTestPlugin() throws Exception { super.setUpTestPlugin(); - config = - server - .getTestInjector() - .createChildInjector( - new AbstractModule() { - @Override - protected void configure() { - bind(String.class).annotatedWith(PluginName.class).toInstance("healthcheck"); - } - }) - .getInstance(HealthCheckConfig.class); + config = plugin.getSysInjector().getInstance(HealthCheckConfig.class); int numChanges = config.getLimit(HealthCheckNames.QUERYCHANGES); for (int i = 0; i < numChanges; i++) { createChange("refs/for/master"); @@ -74,11 +64,35 @@ @Test public void shouldReturnJGitCheck() throws Exception { RestResponse resp = getHealthCheckStatus(); + resp.assertOK(); + assertCheckResult(getResponseJson(resp), JGIT, "passed"); + } - JsonObject respPayload = gson.fromJson(resp.getReader(), JsonObject.class); + @Test + public void shouldReturnJGitCheckAsDisabled() throws Exception { + disableCheck(JGIT); + RestResponse resp = getHealthCheckStatus(); - assertCheckResult(respPayload, JGIT, "passed"); + resp.assertOK(); + assertCheckResult(getResponseJson(resp), JGIT, "disabled"); + } + + @Test + public void shouldReturnAuthCheck() throws Exception { + RestResponse resp = getHealthCheckStatus(); + + resp.assertOK(); + assertCheckResult(getResponseJson(resp), AUTH, "passed"); + } + + @Test + public void shouldReturnAuthCheckAsDisabled() throws Exception { + disableCheck(AUTH); + RestResponse resp = getHealthCheckStatus(); + + resp.assertOK(); + assertCheckResult(getResponseJson(resp), AUTH, "disabled"); } @Test @@ -87,9 +101,16 @@ RestResponse resp = getHealthCheckStatus(); resp.assertOK(); - JsonObject respPayload = gson.fromJson(resp.getReader(), JsonObject.class); + assertCheckResult(getResponseJson(resp), QUERYCHANGES, "passed"); + } - assertCheckResult(respPayload, QUERYCHANGES, "passed"); + @Test + public void shouldReturnQueryChangesCheckAsDisabled() throws Exception { + disableCheck(QUERYCHANGES); + RestResponse resp = getHealthCheckStatus(); + + resp.assertOK(); + assertCheckResult(getResponseJson(resp), QUERYCHANGES, "disabled"); } @Test @@ -99,9 +120,7 @@ RestResponse resp = getHealthCheckStatus(); resp.assertOK(); - JsonObject respPayload = gson.fromJson(resp.getReader(), JsonObject.class); - - assertCheckResult(respPayload, QUERYCHANGES, "passed"); + assertCheckResult(getResponseJson(resp), QUERYCHANGES, "passed"); } private RestResponse getHealthCheckStatus() throws IOException { @@ -114,4 +133,13 @@ assertThat(reviewDbStatus.has("result")).isTrue(); assertThat(reviewDbStatus.get("result").getAsString()).isEqualTo(result); } + + private void disableCheck(String check) throws ConfigInvalidException { + config.fromText(String.format("[healthcheck \"%s\"]\n" + "enabled = false", check)); + } + + private JsonObject getResponseJson(RestResponse resp) throws IOException { + JsonObject respPayload = gson.fromJson(resp.getReader(), JsonObject.class); + return respPayload; + } }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ProjectsListHealthCheckTest.java b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ProjectsListHealthCheckTest.java index 6845686..2ab0d83 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ProjectsListHealthCheckTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/healthcheck/ProjectsListHealthCheckTest.java
@@ -65,6 +65,7 @@ private ListProjects getFailingProjectList() { return new ListProjects(null, null, null, null, null, null, null, null, null, gerritConfig) { + @Override public SortedMap<String, ProjectInfo> apply() throws BadRequestException { throw new IllegalArgumentException("Unable to return project list"); @@ -74,6 +75,7 @@ private ListProjects getWorkingProjectList(long execTime) { return new ListProjects(null, null, null, null, null, null, null, null, null, gerritConfig) { + @Override public SortedMap<String, ProjectInfo> apply() throws BadRequestException { SortedMap<String, ProjectInfo> projects = new TreeMap<>();