Allow to select packaged plugins in gerrit-master chart

The gerrit-master chart installed some selected plugins packaged in the
gerrit.war on startup, but did not allow to select which core plugins
should be installed.

This change adds an option to the values.yaml chart, which allows to
hand over a list of packaged plugins to install for Gerrit. The default
value is the fixed list of plugins, which was previously installed:

    - commit-message-length-validator
    - download-commands
    - replication
    - reviewnotes

Change-Id: I88900e62824f27e51da72387dc7630b360cb5da8
diff --git a/container-images/gerrit-init/README.md b/container-images/gerrit-init/README.md
index 669ae39..5266dbb 100644
--- a/container-images/gerrit-init/README.md
+++ b/container-images/gerrit-init/README.md
@@ -2,7 +2,7 @@
 
 Kubernetes init container for initializing gerrit. The python script running in
 the container initializes Gerrit including the installation of configured
-core plugins.
+plugins.
 
 ## Content
 
diff --git a/helm-charts/gerrit-master/README.md b/helm-charts/gerrit-master/README.md
index 3e6d271..3dfe685 100644
--- a/helm-charts/gerrit-master/README.md
+++ b/helm-charts/gerrit-master/README.md
@@ -144,30 +144,31 @@
 is mandatory, if access to Gerrit is required!
 ***
 
-| Parameter                                    | Description                                                                                     | Default                           |
-|----------------------------------------------|-------------------------------------------------------------------------------------------------|-----------------------------------|
-| `gerritMaster.images.gerritInit`             | Image name of the Gerrit init container image                                                   | `k8s-gerrit/gerrit-init`          |
-| `gerritMaster.images.gerritMaster`           | Image name of the Gerrit master container image                                                 | `k8s-gerrit/gerrit-master`        |
-| `gerritMaster.replicas`                      | Number of replica pods to deploy                                                                | `1`                               |
-| `gerritMaster.maxSurge`                      | Max. percentage or number of pods allowed to be scheduled above the desired number              | `25%`                             |
-| `gerritMaster.maxUnavailable`                | Max. percentage or number of pods allowed to be unavailable at a time                           | `100%`                            |
-| `gerritMaster.resources`                     | Configure the amount of resources the pod requests/is allowed                                   | `requests.cpu: 1`                 |
-|                                              |                                                                                                 | `requests.memory: 5Gi`            |
-|                                              |                                                                                                 | `limits.cpu: 1`                   |
-|                                              |                                                                                                 | `limits.memory: 6Gi`              |
-| `gerritMaster.persistence.enabled`           | Whether to persist the Gerrit site                                                              | `true`                            |
-| `gerritMaster.persistence.size`              | Storage size for persisted Gerrit site                                                          | `10Gi`                            |
-| `gerritMaster.service.type`                  | Which kind of Service to deploy                                                                 | `NodePort`                        |
-| `gerritMaster.service.http.port`             | Port over which to expose HTTP                                                                  | `80`                              |
-| `gerritMaster.ingress.host`                  | REQUIRED: Host name to use for the Ingress (required for Ingress)                               | `nil`                             |
-| `gerritMaster.ingress.additionalAnnotations` | Additional annotations for the Ingress                                                          | `nil`                             |
-| `gerritMaster.ingress.tls.enabled`           | Whether to enable TLS termination in the Ingress                                                | `false`                           |
-| `gerritMaster.ingress.tls.cert`              | Public SSL server certificate                                                                   | `-----BEGIN CERTIFICATE-----`     |
-| `gerritMaster.ingress.tls.key`               | Private SSL server certificate                                                                  | `-----BEGIN RSA PRIVATE KEY-----` |
-| `gerritMaster.keystore`                      | base64-encoded Java keystore (`cat keystore.jks | base64`) to be used by Gerrit, when using SSL | `nil`                             |
-| `gerritMaster.config.gerrit`                 | The contents of the gerrit.config                                                               | [see here](#Gerrit-config-files)  |
-| `gerritMaster.config.secure`                 | The contents of the secure.config                                                               | [see here](#Gerrit-config-files)  |
-| `gerritMaster.config.replication`            | The contents of the replication.config                                                          | [see here](#Gerrit-config-files)  |
+| Parameter                                    | Description                                                                                     | Default                                                                                  |
+|----------------------------------------------|-------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------|
+| `gerritMaster.images.gerritInit`             | Image name of the Gerrit init container image                                                   | `k8s-gerrit/gerrit-init`                                                                 |
+| `gerritMaster.images.gerritMaster`           | Image name of the Gerrit master container image                                                 | `k8s-gerrit/gerrit-master`                                                               |
+| `gerritMaster.replicas`                      | Number of replica pods to deploy                                                                | `1`                                                                                      |
+| `gerritMaster.maxSurge`                      | Max. percentage or number of pods allowed to be scheduled above the desired number              | `25%`                                                                                    |
+| `gerritMaster.maxUnavailable`                | Max. percentage or number of pods allowed to be unavailable at a time                           | `100%`                                                                                   |
+| `gerritMaster.resources`                     | Configure the amount of resources the pod requests/is allowed                                   | `requests.cpu: 1`                                                                        |
+|                                              |                                                                                                 | `requests.memory: 5Gi`                                                                   |
+|                                              |                                                                                                 | `limits.cpu: 1`                                                                          |
+|                                              |                                                                                                 | `limits.memory: 6Gi`                                                                     |
+| `gerritMaster.persistence.enabled`           | Whether to persist the Gerrit site                                                              | `true`                                                                                   |
+| `gerritMaster.persistence.size`              | Storage size for persisted Gerrit site                                                          | `10Gi`                                                                                   |
+| `gerritMaster.service.type`                  | Which kind of Service to deploy                                                                 | `NodePort`                                                                               |
+| `gerritMaster.service.http.port`             | Port over which to expose HTTP                                                                  | `80`                                                                                     |
+| `gerritMaster.ingress.host`                  | REQUIRED: Host name to use for the Ingress (required for Ingress)                               | `nil`                                                                                    |
+| `gerritMaster.ingress.additionalAnnotations` | Additional annotations for the Ingress                                                          | `nil`                                                                                    |
+| `gerritMaster.ingress.tls.enabled`           | Whether to enable TLS termination in the Ingress                                                | `false`                                                                                  |
+| `gerritMaster.ingress.tls.cert`              | Public SSL server certificate                                                                   | `-----BEGIN CERTIFICATE-----`                                                            |
+| `gerritMaster.ingress.tls.key`               | Private SSL server certificate                                                                  | `-----BEGIN RSA PRIVATE KEY-----`                                                        |
+| `gerritMaster.keystore`                      | base64-encoded Java keystore (`cat keystore.jks | base64`) to be used by Gerrit, when using SSL | `nil`                                                                                    |
+| `gerritMaster.plugins.packaged`              | List of Gerrit plugins that are packaged into the Gerrit-war-file to install                    | `["commit-message-length-validator", "download-commands", "replication", "reviewnotes"]` |
+| `gerritMaster.config.gerrit`                 | The contents of the gerrit.config                                                               | [see here](#Gerrit-config-files)                                                         |
+| `gerritMaster.config.secure`                 | The contents of the secure.config                                                               | [see here](#Gerrit-config-files)                                                         |
+| `gerritMaster.config.replication`            | The contents of the replication.config                                                          | [see here](#Gerrit-config-files)                                                         |
 
 ### Gerrit config files
 
diff --git a/helm-charts/gerrit-master/templates/gerrit-master.stateful-set.yaml b/helm-charts/gerrit-master/templates/gerrit-master.stateful-set.yaml
index fdca7ed..2c05a00 100644
--- a/helm-charts/gerrit-master/templates/gerrit-master.stateful-set.yaml
+++ b/helm-charts/gerrit-master/templates/gerrit-master.stateful-set.yaml
@@ -73,11 +73,12 @@
           fi
 
           /var/tools/gerrit_init.py \
-            -s /var/gerrit \
-            -p replication \
-            -p commit-message-length-validator \
-            -p download-commands \
-            -p reviewnotes
+            {{- range .Values.gerritMaster.plugins.packaged }}
+            {{- if . }}
+            -p {{ . }} \
+            {{- end }}
+            {{- end }}
+            -s /var/gerrit
 
           symlink_config_to_site
         volumeMounts:
diff --git a/helm-charts/gerrit-master/values.yaml b/helm-charts/gerrit-master/values.yaml
index 44bf9ad..1637d42 100644
--- a/helm-charts/gerrit-master/values.yaml
+++ b/helm-charts/gerrit-master/values.yaml
@@ -129,6 +129,13 @@
   # automatic encoding using helm does not work here.
   keystore:
 
+  plugins:
+    packaged:
+    - commit-message-length-validator
+    - download-commands
+    - replication
+    - reviewnotes
+
   config:
     # Some values are expected to have a specific value for the deployment installed
     # by this chart to work. These are marked with `# FIXED`.
diff --git a/tests/helm-charts/gerrit-master/conftest.py b/tests/helm-charts/gerrit-master/conftest.py
index 510a4b2..1331fc3 100644
--- a/tests/helm-charts/gerrit-master/conftest.py
+++ b/tests/helm-charts/gerrit-master/conftest.py
@@ -15,7 +15,6 @@
 # limitations under the License.
 
 import os.path
-import time
 
 import pytest
 
@@ -26,8 +25,8 @@
 GERRIT_MASTER_STARTUP_TIMEOUT = 240
 
 
-@pytest.fixture(scope="module")
-def gerrit_master_deployment(
+@pytest.fixture(scope="session")
+def gerrit_master_deployment_factory(
     request,
     repository_root,
     test_cluster,
@@ -36,51 +35,70 @@
     gitgc_image,
     gerrit_init_image,
 ):
-    chart_path = os.path.join(repository_root, "helm-charts", "gerrit-master")
-    chart_name = "gerrit-master-" + utils.create_random_string()
+    def install_chart(chart_opts):
+        chart_path = os.path.join(repository_root, "helm-charts", "gerrit-master")
+        chart_name = "gerrit-master-" + utils.create_random_string()
+        namespace_name = utils.create_random_string()
+        test_cluster.create_namespace(namespace_name)
+
+        core_v1 = client.CoreV1Api()
+        core_v1.create_namespaced_persistent_volume_claim(
+            namespace_name,
+            body=client.V1PersistentVolumeClaim(
+                kind="PersistentVolumeClaim",
+                api_version="v1",
+                metadata=client.V1ObjectMeta(name="repo-storage"),
+                spec=client.V1PersistentVolumeClaimSpec(
+                    access_modes=["ReadWriteMany"],
+                    storage_class_name="shared-storage",
+                    resources=client.V1ResourceRequirements(
+                        requests={"storage": "1Gi"}
+                    ),
+                ),
+            ),
+        )
+
+        chart_opts["gitRepositoryStorage.externalPVC.use"] = "false"
+        chart_opts["gitRepositoryStorage.externalPVC.name"] = "repo-storage"
+
+        test_cluster.helm.install(
+            chart_path,
+            chart_name,
+            set_values=chart_opts,
+            fail_on_err=True,
+            namespace=namespace_name,
+        )
+
+        return {"chart": chart_path, "name": chart_name, "namespace": namespace_name}
+
+    return install_chart
+
+
+@pytest.fixture(scope="module")
+def gerrit_master_deployment(
+    request, docker_tag, test_cluster, gerrit_master_deployment_factory
+):
     chart_opts = {
         "images.registry.name": request.config.getoption("--registry"),
         "images.version": docker_tag,
         "gerritMaster.ingress.host": "master.%s"
         % request.config.getoption("--ingress-url"),
     }
-    namespace_name = utils.create_random_string()
-    test_cluster.create_namespace(namespace_name)
-    test_cluster.helm.install(
-        chart_path,
-        chart_name,
-        set_values=chart_opts,
-        fail_on_err=True,
-        namespace=namespace_name,
-    )
+    chart = gerrit_master_deployment_factory(chart_opts)
 
-    yield {"name": chart_name, "namespace": namespace_name}
+    yield chart
 
-    test_cluster.helm.delete(chart_name)
-    test_cluster.delete_namespace(namespace_name)
+    test_cluster.helm.delete(chart["name"])
+    test_cluster.delete_namespace(chart["namespace"])
 
 
 @pytest.fixture(scope="module")
 def gerrit_master_ready_deployment(gerrit_master_deployment):
-    def wait_for_readiness():
-        pod_labels = "app=gerrit-master,release=%s" % gerrit_master_deployment["name"]
-        core_v1 = client.CoreV1Api()
-        pod_list = core_v1.list_pod_for_all_namespaces(
-            watch=False, label_selector=pod_labels
-        )
-        for condition in pod_list.items[0].status.conditions:
-            if condition.type == "Ready" and condition.status == "True":
-                return True
-        return False
 
-    timeout = time.time() + GERRIT_MASTER_STARTUP_TIMEOUT
-
-    finished_in_time = False
-
-    while time.time() <= timeout:
-        finished_in_time = wait_for_readiness()
-        if finished_in_time:
-            break
+    pod_labels = "app=gerrit-master,release=%s" % gerrit_master_deployment["name"]
+    finished_in_time = utils.wait_for_pod_readiness(
+        pod_labels, timeout=GERRIT_MASTER_STARTUP_TIMEOUT
+    )
 
     if not finished_in_time:
         raise utils.TimeOutException(
diff --git a/tests/helm-charts/gerrit-master/test_chart_gerrit_master_plugins.py b/tests/helm-charts/gerrit-master/test_chart_gerrit_master_plugins.py
new file mode 100644
index 0000000..63edd10
--- /dev/null
+++ b/tests/helm-charts/gerrit-master/test_chart_gerrit_master_plugins.py
@@ -0,0 +1,142 @@
+# pylint: disable=W0613
+
+# Copyright (C) 2019 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.
+
+import json
+import time
+
+import pytest
+import requests
+
+import utils
+
+
+@pytest.fixture(scope="class")
+def gerrit_master_deployment_with_plugins_factory(
+    request, gerrit_master_deployment_factory
+):
+    def install_chart(chart_opts):
+        chart = gerrit_master_deployment_factory(chart_opts)
+        pod_labels = "app=gerrit-master,release=%s" % chart["name"]
+        finished_in_time = utils.wait_for_pod_readiness(pod_labels, timeout=300)
+        if not finished_in_time:
+            raise utils.TimeOutException("Gerrit master pod was not ready in time.")
+
+        return chart
+
+    return install_chart
+
+
+@pytest.fixture(
+    scope="class",
+    params=[["replication"], ["replication", "download-commands"]],
+    ids=["single-packaged-plugin", "multiple-packaged-plugins"],
+)
+def gerrit_master_deployment_with_packaged_plugins(
+    request, docker_tag, test_cluster, gerrit_master_deployment_with_plugins_factory
+):
+
+    plugins_opts_string = "{%s}" % (",".join(request.param))
+    chart_opts = {
+        "images.registry.name": request.config.getoption("--registry"),
+        "images.version": docker_tag,
+        "images.ImagePullPolicy": "IfNotPresent",
+        "gerritMaster.ingress.host": "master.%s"
+        % request.config.getoption("--ingress-url"),
+        "gerritMaster.plugins.packaged": plugins_opts_string,
+    }
+    chart = gerrit_master_deployment_with_plugins_factory(chart_opts)
+    chart["installed_plugins"] = request.param
+
+    yield chart
+
+    test_cluster.helm.delete(chart["name"])
+    test_cluster.delete_namespace(chart["namespace"])
+
+
+def update_chart(helm, chart, opts):
+    helm.upgrade(
+        chart=chart["chart"],
+        name=chart["name"],
+        set_values=opts,
+        reuse_values=True,
+        recreate_pods=True,
+        fail_on_err=True,
+    )
+
+
+def get_gerrit_plugin_list(gerrit_url, user="admin", password="secret"):
+    list_plugins_url = "%s/a/plugins/?all" % gerrit_url
+    response = requests.get(list_plugins_url, auth=(user, password))
+    if not response.status_code == 200:
+        return None
+    body = response.text
+    return json.loads(body[body.index("\n") + 1 :])
+
+
+@pytest.mark.slow
+@pytest.mark.incremental
+@pytest.mark.integration
+@pytest.mark.kubernetes
+class TestGerritMasterChartPackagedPluginInstall:
+    def _assert_installed_plugins(self, expected_plugins, installed_plugins):
+        for plugin in expected_plugins:
+            assert plugin in installed_plugins
+            assert installed_plugins[plugin]["filename"] == "%s.jar" % plugin
+
+    @pytest.mark.timeout(300)
+    def test_install_packaged_plugins(
+        self, request, test_cluster, gerrit_master_deployment_with_packaged_plugins
+    ):
+        response = None
+        while not response:
+            try:
+                response = get_gerrit_plugin_list(
+                    "http://master.%s" % (request.config.getoption("--ingress-url"))
+                )
+            except requests.exceptions.ConnectionError:
+                time.sleep(1)
+
+        self._assert_installed_plugins(
+            gerrit_master_deployment_with_packaged_plugins["installed_plugins"],
+            response,
+        )
+
+    @pytest.mark.timeout(300)
+    def test_install_packaged_plugins_are_removed_with_update(
+        self, request, test_cluster, gerrit_master_deployment_with_packaged_plugins
+    ):
+        chart = gerrit_master_deployment_with_packaged_plugins
+        chart["removed_plugin"] = chart["installed_plugins"].pop()
+        plugins_opts_string = "{%s}" % (",".join(chart["installed_plugins"]))
+        update_chart(
+            test_cluster.helm,
+            chart,
+            {"gerritMaster.plugins.packaged": plugins_opts_string},
+        )
+
+        response = None
+        while True:
+            try:
+                response = get_gerrit_plugin_list(
+                    "http://master.%s" % (request.config.getoption("--ingress-url"))
+                )
+                if response is not None and chart["removed_plugin"] not in response:
+                    break
+            except requests.exceptions.ConnectionError:
+                time.sleep(1)
+
+        assert chart["removed_plugin"] not in response
+        self._assert_installed_plugins(chart["installed_plugins"], response)
diff --git a/tests/helpers/helm.py b/tests/helpers/helm.py
index 28ed2f3..1c5eb3f 100644
--- a/tests/helpers/helm.py
+++ b/tests/helpers/helm.py
@@ -120,34 +120,38 @@
         values_file=None,
         set_values=None,
         reuse_values=True,
+        recreate_pods=False,
         fail_on_err=True,
     ):
         """Updates a chart on the cluster
 
-    Arguments:
-      chart {str} -- Release name or path of a helm chart
-      name {str} -- Name with which the chart will be installed on the cluster
+      Arguments:
+        chart {str} -- Release name or path of a helm chart
+        name {str} -- Name with which the chart will be installed on the cluster
 
-    Keyword Arguments:
-      values_file {str} -- Path to a custom values.yaml file (default: {None})
-      set_values {dict} -- Dictionary containing key-value-pairs that are used
-                           to overwrite values in the values.yaml-file.
-                           (default: {None})
-      reuse_values {bool} -- Whether to reuse existing not overwritten values
-                            (default: {True})
-      fail_on_err {bool} -- Whether to fail with an exception if the installation
-                            fails (default: {True})
+      Keyword Arguments:
+        values_file {str} -- Path to a custom values.yaml file (default: {None})
+        set_values {dict} -- Dictionary containing key-value-pairs that are used
+                            to overwrite values in the values.yaml-file.
+                            (default: {None})
+        reuse_values {bool} -- Whether to reuse existing not overwritten values
+                              (default: {True})
+        recreate_pods {bool} -- Whether to restart changed pods (default: {False})
+        fail_on_err {bool} -- Whether to fail with an exception if the installation
+                              fails (default: {True})
 
-    Returns:
-      CompletedProcess -- CompletedProcess-object returned by subprocess
-                          containing details about the result and output of the
-                          executed command.
-    """
+      Returns:
+        CompletedProcess -- CompletedProcess-object returned by subprocess
+                            containing details about the result and output of the
+                            executed command.
+      """
         helm_cmd = ["upgrade", name, chart, "--wait"]
         if values_file:
             helm_cmd.extend(("-f", values_file))
         if reuse_values:
             helm_cmd.append("--reuse-values")
+        if recreate_pods:
+            helm_cmd.append("--recreate-pods")
         if set_values:
             opt_list = ["%s=%s" % (k, v) for k, v in set_values.items()]
             helm_cmd.extend(("--set", ",".join(opt_list)))
@@ -156,18 +160,18 @@
     def delete(self, name, purge=True):
         """Deletes a chart from the cluster
 
-    Arguments:
-      name {str} -- Name of the chart to delete
+      Arguments:
+        name {str} -- Name of the chart to delete
 
-    Keyword Arguments:
-      purge {bool} -- Whether to also remove the release metadata as well
-                      (default: {True})
+      Keyword Arguments:
+        purge {bool} -- Whether to also remove the release metadata as well
+                        (default: {True})
 
-    Returns:
-      CompletedProcess -- CompletedProcess-object returned by subprocess
-                          containing details about the result and output of the
-                          executed command.
-    """
+      Returns:
+        CompletedProcess -- CompletedProcess-object returned by subprocess
+                            containing details about the result and output of the
+                            executed command.
+      """
 
         helm_cmd = ["delete", name]
         if purge:
diff --git a/tests/helpers/utils.py b/tests/helpers/utils.py
index 7b9bf72..9db934e 100644
--- a/tests/helpers/utils.py
+++ b/tests/helpers/utils.py
@@ -14,12 +14,71 @@
 
 import random
 import string
+import time
+
+from kubernetes import client
 
 
 class TimeOutException(Exception):
     """ Exception to be raised, if some action does not finish in time. """
 
 
+def exec_fn_with_timeout(func, limit=60):
+    """Helper function that executes a given function until it returns True or a
+     given time limit is reached.
+
+  Arguments:
+    func {function} -- Function to execute. The function can return some output
+                    (or None) and as a second return value a boolean indicating,
+                    whether the event the function was waiting for has happened.
+
+  Keyword Arguments:
+    limit {int} -- Maximum time in seconds to wait for a positive response of
+                   the function (default: {60})
+
+  Returns:
+    boolean -- False, if the timeout was reached
+    any -- Last output of fn
+  """
+
+    timeout = time.time() + limit
+    while time.time() < timeout:
+        is_finished = func()
+        if is_finished:
+            return True
+    return False
+
+
+def wait_for_pod_readiness(pod_labels, timeout=180):
+    """Helper function that can be used to wait for all pods with a given set of
+     labels to be ready.
+
+  Arguments:
+    pod_labels {str} -- Label selector string to be used to select pods.
+      (https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors)
+
+  Keyword Arguments:
+    timeout {int} -- Time in seconds to wait for the pod status to become ready.
+      (default: {180})
+
+  Returns:
+    boolean -- Whether pods were ready in time.
+  """
+
+    def check_pod_readiness():
+        core_v1 = client.CoreV1Api()
+        pod_list = core_v1.list_pod_for_all_namespaces(
+            watch=False, label_selector=pod_labels
+        )
+        for pod in pod_list.items:
+            for condition in pod.status.conditions:
+                if condition.type != "Ready" and condition.status != "True":
+                    return False
+        return True
+
+    return exec_fn_with_timeout(check_pod_readiness, limit=timeout)
+
+
 def check_if_ancestor_image_is_inherited(image, ancestor):
     """Helper function that looks for a given ancestor image in the layers of a
     provided image. It can be used to check, whether an image uses the expected