Elasticsearch: Simplify configuration of servers

Instead of configuring each server in its own section with separate
values for protocol, host and port, i.e.:

  [elasticsearch "elastic1"]
    hostname = elastic1
    protocol = https
    port = 1234

  [elasticsearch "elastic2"]
    hostname = elastic2
    protocol = https
    port = 1234

Configure them all under the main "elasticsearch" section as multiple
values, i.e.

  [elasticsearch]
    server = https://elastic1:1234
    server = https://elastic2:1234

Allow the port to be omitted, and default it to 9200.

Require at least one server to be explicitly configured and throw a
provision exception if there are none.

During init prompt for the server, defaulting to http://localhost:9200.

Since the init framework doesn't support reading or setting string lists,
if there are multiple server values it will prompt the last one. Mention
in the documentation that configuration of multiple servers must be done
manually.

Since it is not expected that Elasticsearch is being used in production
anywhere, no backwards compatibility with the previous configuration is
provided. Users must adjust their configuration to the new format.

Add a new test, ElasticConfigurationTest, which in this initial version
only tests the configuration of the elasticsearch.server. More tests may
be added in follow-up commits.

Bug: Issue 9372
Bug: Issue 9383
Change-Id: Iad2c547ae82ba89b7ede777072e0869bc77d2025

5 files changed, 122 insertions, 57 deletions

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index e14e041716..a0f211b34b 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -2812,6 +2812,18 @@ change index named `gerrit1_changes_0001`.
 +
 Not set by default.
 
+[[elasticsearch.server]]elasticsearch.server::
++
+Elasticsearch server URI in the form `http[s]://hostname:port`. The `port` is
+optional and defaults to `9200` if not specified.
++
+At least one server must be specified. May be specified multiple times to
+configure multiple Elasticsearch servers.
++
+Note that the site initialization program only allows to configure a single
+server. To configure multiple servers the `gerrit.config` file must be edited
+manually.
+
 [[elasticsearch.maxRetryTimeout]]elasticsearch.maxRetryTimeout::
 +
 Sets the maximum timeout to honor in case of multiple retries of the same request.
@@ -2844,28 +2856,6 @@ Password used to connect to Elasticsearch.
 +
 Not set by default.
 
-==== Elasticsearch server(s) configuration
-
-Each section corresponds to one Elasticsearch server.
-
-[[elasticsearch.name.protocol]]elasticsearch.name.protocol::
-+
-Elasticsearch server protocol. Can be `http` or `https`.
-+
-Defaults to `http`.
-
-[[elasticsearch.name.hostname]]elasticsearch.name.hostname::
-+
-Elasticsearch server hostname.
-+
-Defaults to `localhost`.
-
-[[elasticsearch.name.port]]elasticsearch.name.port::
-+
-Elasticsearch server port.
-+
-Defaults to `9200`.
-
 [[ldap]]
 === Section ldap
 
diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticConfiguration.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticConfiguration.java
index 6e4e872da5..694e7ab3f7 100644
--- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticConfiguration.java
+++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticConfiguration.java
@@ -16,15 +16,15 @@ package com.google.gerrit.elasticsearch;
 
 import static com.google.common.base.MoreObjects.firstNonNull;
 
-import com.google.common.base.MoreObjects;
 import com.google.common.base.Strings;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.inject.Inject;
+import com.google.inject.ProvisionException;
 import com.google.inject.Singleton;
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
-import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import org.apache.http.HttpHost;
 import org.eclipse.jgit.lib.Config;
@@ -35,9 +35,7 @@ import org.slf4j.LoggerFactory;
 class ElasticConfiguration {
   private static final Logger log = LoggerFactory.getLogger(ElasticConfiguration.class);
 
-  private static final String DEFAULT_HOST = "localhost";
   private static final String DEFAULT_PORT = "9200";
-  private static final String DEFAULT_PROTOCOL = "http";
   private static final String DEFAULT_USERNAME = "elastic";
 
   private final Config cfg;
@@ -60,25 +58,25 @@ class ElasticConfiguration {
         (int)
             cfg.getTimeUnit("elasticsearch", null, "maxRetryTimeout", 30000, TimeUnit.MILLISECONDS);
     this.prefix = Strings.nullToEmpty(cfg.getString("elasticsearch", null, "prefix"));
-
-    Set<String> subsections = cfg.getSubsections("elasticsearch");
-    if (subsections.isEmpty()) {
-      HttpHost httpHost =
-          new HttpHost(DEFAULT_HOST, Integer.valueOf(DEFAULT_PORT), DEFAULT_PROTOCOL);
-      this.hosts = Collections.singletonList(httpHost);
-    } else {
-      this.hosts = new ArrayList<>(subsections.size());
-      for (String subsection : subsections) {
-        String port = getString(cfg, subsection, "port", DEFAULT_PORT);
-        String host = getString(cfg, subsection, "hostname", DEFAULT_HOST);
-        String protocol = getString(cfg, subsection, "protocol", DEFAULT_PROTOCOL);
-
-        HttpHost httpHost = new HttpHost(host, Integer.valueOf(port), protocol);
+    this.hosts = new ArrayList<>();
+    for (String server : cfg.getStringList("elasticsearch", null, "server")) {
+      try {
+        URI uri = new URI(server);
+        int port = uri.getPort();
+        HttpHost httpHost =
+            new HttpHost(
+                uri.getHost(), port == -1 ? Integer.valueOf(DEFAULT_PORT) : port, uri.getScheme());
         this.hosts.add(httpHost);
+      } catch (URISyntaxException | IllegalArgumentException e) {
+        log.error("Invalid server URI {}: {}", server, e.getMessage());
       }
     }
 
-    log.info("Elasticsearch hosts: {}", hosts);
+    if (hosts.isEmpty()) {
+      throw new ProvisionException("No valid Elasticsearch servers configured");
+    }
+
+    log.info("Elasticsearch servers: {}", hosts);
   }
 
   Config getConfig() {
@@ -92,8 +90,4 @@ class ElasticConfiguration {
   String getIndexName(String name, int schemaVersion) {
     return String.format("%s%s_%04d", prefix, name, schemaVersion);
   }
-
-  private String getString(Config cfg, String subsection, String name, String defaultValue) {
-    return MoreObjects.firstNonNull(cfg.getString("elasticsearch", subsection, name), defaultValue);
-  }
 }
diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/testing/ElasticTestUtils.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/testing/ElasticTestUtils.java
index e8bfce1a28..4e83317361 100644
--- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/testing/ElasticTestUtils.java
+++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/testing/ElasticTestUtils.java
@@ -34,9 +34,7 @@ public final class ElasticTestUtils {
 
   public static void configure(Config config, int port, String prefix, String password) {
     config.setEnum("index", null, "type", IndexType.ELASTICSEARCH);
-    config.setString("elasticsearch", "test", "protocol", "http");
-    config.setString("elasticsearch", "test", "hostname", "localhost");
-    config.setInt("elasticsearch", "test", "port", port);
+    config.setString("elasticsearch", null, "server", "http://localhost:" + port);
     config.setString("elasticsearch", null, "prefix", prefix);
     config.setString("index", null, "maxLimit", "10000");
     if (password != null) {
diff --git a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticConfigurationTest.java b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticConfigurationTest.java
new file mode 100644
index 0000000000..f2a09cbc48
--- /dev/null
+++ b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticConfigurationTest.java
@@ -0,0 +1,90 @@
+// Copyright (C) 2018 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.elasticsearch;
+
+import static com.google.common.truth.Truth.assertThat;
+import static java.util.stream.Collectors.toList;
+
+import com.google.common.collect.ImmutableList;
+import com.google.inject.ProvisionException;
+import java.util.Arrays;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+public class ElasticConfigurationTest {
+  @Rule public ExpectedException exception = ExpectedException.none();
+
+  @Test
+  public void singleServer() throws Exception {
+    Config cfg = new Config();
+    cfg.setString("elasticsearch", null, "server", "http://elastic:1234");
+    ElasticConfiguration esCfg = new ElasticConfiguration(cfg);
+    assertHosts(esCfg, "http://elastic:1234");
+  }
+
+  @Test
+  public void serverWithoutPortSpecified() throws Exception {
+    Config cfg = new Config();
+    cfg.setString("elasticsearch", null, "server", "http://elastic");
+    ElasticConfiguration esCfg = new ElasticConfiguration(cfg);
+    assertHosts(esCfg, "http://elastic:9200");
+  }
+
+  @Test
+  public void multipleServers() throws Exception {
+    Config cfg = new Config();
+    cfg.setStringList(
+        "elasticsearch",
+        null,
+        "server",
+        ImmutableList.of("http://elastic1:1234", "http://elastic2:1234"));
+    ElasticConfiguration esCfg = new ElasticConfiguration(cfg);
+    assertHosts(esCfg, "http://elastic1:1234", "http://elastic2:1234");
+  }
+
+  @Test
+  public void noServers() throws Exception {
+    assertProvisionException(new Config());
+  }
+
+  @Test
+  public void singleServerInvalid() throws Exception {
+    Config cfg = new Config();
+    cfg.setString("elasticsearch", null, "server", "foo");
+    assertProvisionException(cfg);
+  }
+
+  @Test
+  public void multipleServersIncludingInvalid() throws Exception {
+    Config cfg = new Config();
+    cfg.setStringList(
+        "elasticsearch", null, "server", ImmutableList.of("http://elastic1:1234", "foo"));
+    ElasticConfiguration esCfg = new ElasticConfiguration(cfg);
+    assertHosts(esCfg, "http://elastic1:1234");
+  }
+
+  private void assertHosts(ElasticConfiguration cfg, Object... hostURIs) throws Exception {
+    assertThat(Arrays.asList(cfg.getHosts()).stream().map(h -> h.toURI()).collect(toList()))
+        .containsExactly(hostURIs);
+  }
+
+  private void assertProvisionException(Config cfg) throws Exception {
+    exception.expect(ProvisionException.class);
+    exception.expectMessage("No valid Elasticsearch servers configured");
+    new ElasticConfiguration(cfg);
+  }
+}
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitIndex.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitIndex.java
index e051b38640..2e67bfbc11 100644
--- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitIndex.java
+++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitIndex.java
@@ -15,7 +15,6 @@
 package com.google.gerrit.pgm.init;
 
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Sets;
 import com.google.gerrit.pgm.init.api.ConsoleUI;
 import com.google.gerrit.pgm.init.api.InitFlags;
 import com.google.gerrit.pgm.init.api.InitStep;
@@ -63,13 +62,7 @@ class InitIndex implements InitStep {
     if (type == IndexType.ELASTICSEARCH) {
       Section elasticsearch = sections.get("elasticsearch", null);
       elasticsearch.string("Index Prefix", "prefix", "gerrit_");
-      String name = ui.readString("default", "Server Name");
-
-      Section defaultServer = sections.get("elasticsearch", name);
-      defaultServer.select(
-          "Transport protocol", "protocol", "http", Sets.newHashSet("http", "https"));
-      defaultServer.string("Hostname", "hostname", "localhost");
-      defaultServer.string("Port", "port", "9200");
+      elasticsearch.string("Server", "server", "http://localhost:9200");
       index.string("Result window size", "maxLimit", "10000");
     }