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<>();
