Stop listening on 127.0.0.1:4001 with http (#212)

* Stop listening on 127.0.0.1:4001 with http * Ensure the restore action uses the secure endpoint as well * linting * Update config.yaml Co-authored-by: Kevin W Monroe <[email protected]> --------- Co-authored-by: Kevin W Monroe <[email protected]>
charmed-kubernetes · Sep 22, 2023 · 12d8186 · 12d8186
1 parent 7d5da4a
commit 12d8186
Show file tree

Hide file tree

Showing 9 changed files with 70 additions and 72 deletions.
diff --git a/actions/restore.py b/actions/restore.py
@@ -3,7 +3,7 @@
 from charms import layer
 from charmhelpers.core.templating import render
 from charmhelpers.core import hookenv
-from charmhelpers.core.hookenv import function_fail
+from charmhelpers.core.hookenv import action_fail
 from charmhelpers.core.hookenv import action_get
 from charmhelpers.core.hookenv import action_set
 from charmhelpers.core.hookenv import config
@@ -56,10 +56,10 @@
 def preflight_check():
     """Check preconditions for data restoration"""
     if not is_leader():
-        function_fail("This action can only be run on the leader unit")
+        action_fail("This action can only be run on the leader unit")
         sys.exit(0)
     if not SNAPSHOT_ARCHIVE:
-        function_fail({"result.failed": "Missing snapshot. See: README.md"})
+        action_fail("Missing snapshot. See: README.md")
         sys.exit(0)
 
 
@@ -112,7 +112,7 @@ def restore_v3_backup():
     # Use the insecure 4001 port we have open in our deployment
     environ = dict(os.environ, ETCDCTL_API="3")
     cmd = (
-        "/snap/bin/etcdctl --endpoints=http://localhost:4001 snapshot "
+        "/snap/bin/etcdctl --endpoints=https://127.0.0.1:2379 snapshot "
         "restore /root/tmp/restore-v3/db --skip-hash-check "
         "--data-dir='/root/tmp/restore-v3/etcd' "
         "--initial-cluster='{}' --initial-cluster-token='{}' "

diff --git a/config.yaml b/config.yaml
@@ -30,6 +30,12 @@ options:
     description: |
       The service binds to all network interfaces if true. The service binds
       only to the first found bind address of each relation if false
+  bind_with_insecure_http:
+    type: boolean
+    default: false
+    description: |
+      The service binds to localhost:4001 with http if true. This exposes an insecure
+      endpoint for the service and is not recommended in production environments.
   tls_cipher_suites:
     type: string
     default: ""

diff --git a/lib/etcd_databag.py b/lib/etcd_databag.py
@@ -38,12 +38,16 @@ def __init__(self):
         self.db = unitdata.kv()
         self.build_uri = build_uri
         self.cluster_bind_address = self.get_bind_address("cluster")
-        self.db_bind_address = self.get_bind_address("db")
         self.port = config("port")
+        self.listen_client_urls = [
+            build_uri("https", self.get_bind_address("db"), self.port)
+        ]
+        if config("bind_with_insecure_http"):
+            self.listen_client_urls.insert(0, build_uri("http", "127.0.0.1", 4001))
+        self.advertise_urls = [build_uri("https", get_ingress_address("db"), self.port)]
         self.management_port = config("management_port")
         # Live polled properties
         self.cluster_address = get_ingress_address("cluster")
-        self.db_address = get_ingress_address("db")
         self.unit_name = os.getenv("JUJU_UNIT_NAME").replace("/", "")
 
         # Pull the TLS certificate paths from layer data

diff --git a/lib/etcdctl.py b/lib/etcdctl.py
@@ -3,6 +3,7 @@
 from subprocess import CalledProcessError
 from subprocess import check_output
 from subprocess import STDOUT
+from typing import Optional
 import os
 from etcd_lib import build_uri
 
@@ -66,7 +67,7 @@ def unregister(self, unit_id, leader_address=None):
         """
         return self.run(["member", "remove", unit_id], endpoints=leader_address, api=2)
 
-    def member_list(self, leader_address=False):
+    def member_list(self, leader_address=None):
         """Returns the output from `etcdctl member list` as a python dict
         organized by unit_name, containing all the data-points in the resulting
         response."""
@@ -122,7 +123,7 @@ def cluster_health(self, output_only=False):
         organized by topical information with detailed unit output"""
         health = {}
         try:
-            out = self.run("cluster-health", endpoints=False, api=2)
+            out = self.run("cluster-health", endpoints=None, api=2)
             if output_only:
                 return out
             health_output = out.strip("\n").split("\n")
@@ -134,7 +135,7 @@ def cluster_health(self, output_only=False):
             health["units"] = []
         return health
 
-    def run(self, arguments, endpoints=None, api=3):
+    def run(self, arguments, endpoints: Optional[str] = None, api=3):
         """Wrapper to subprocess calling output. This is a convenience
         method to clean up the calls to subprocess and append TLS data"""
         env = {}
@@ -149,20 +150,18 @@ def run(self, arguments, endpoints=None, api=3):
             env["ETCDCTL_CACERT"] = ca_path
             env["ETCDCTL_CERT"] = crt_path
             env["ETCDCTL_KEY"] = key_path
-            if endpoints is None:
-                endpoints = "http://127.0.0.1:4001"
 
         elif api == 2:
             env["ETCDCTL_API"] = "2"
             env["ETCDCTL_CA_FILE"] = ca_path
             env["ETCDCTL_CERT_FILE"] = crt_path
             env["ETCDCTL_KEY_FILE"] = key_path
-            if endpoints is None:
-                endpoints = ":4001"
-
         else:
             raise NotImplementedError("etcd api version {} not supported".format(api))
 
+        if not endpoints:
+            endpoints = "https://127.0.0.1:2379"
+
         if isinstance(arguments, str):
             command.extend(arguments.split())
         elif isinstance(arguments, list) or isinstance(arguments, tuple):
@@ -172,12 +171,11 @@ def run(self, arguments, endpoints=None, api=3):
                 "arguments not correct type; must be string, list or tuple"
             )
 
-        if endpoints is not False:
-            if api == 3:
-                command.extend(["--endpoints", endpoints])
-            elif api == 2:
-                command.insert(1, "--endpoint")
-                command.insert(2, endpoints)
+        if api == 3:
+            command.extend(["--endpoints", endpoints])
+        elif api == 2:
+            command.insert(1, "--endpoint")
+            command.insert(2, endpoints)
 
         try:
             return check_output(command, env=env, stderr=STDOUT).decode("utf-8")

diff --git a/templates/check_etcd-alarms.cron b/templates/check_etcd-alarms.cron
@@ -1,2 +1,2 @@
 # check_etcd_alarms
-* * * * * root [ -x /snap/bin/etcdctl ] && ETCDCTL_API=3 /snap/bin/etcdctl --endpoints=127.0.0.1:4001 alarm list 2>&1| tee /var/lib/nagios/etcd-alarm-list.txt > /dev/null
+* * * * * root [ -x /snap/bin/etcdctl ] && ETCDCTL_API=3 ETCDCTL_KEY=/var/snap/etcd/common/client.key ETCDCTL_CERT=/var/snap/etcd/common/client.crt ETCDCTL_CACERT=/var/snap/etcd/common/ca.crt /snap/bin/etcdctl --endpoints=https://127.0.0.1:2379 alarm list 2>&1| tee /var/lib/nagios/etcd-alarm-list.txt > /dev/null
diff --git a/templates/etcd2.conf b/templates/etcd2.conf
@@ -1,8 +1,8 @@
 # This file is rendered by Juju, manual edits will not be persisted
 ETCD_DATA_DIR={{ etcd_data_dir }}/{{ unit_name }}.etcd
 ETCD_NAME={{ unit_name }}
-ETCD_ADVERTISE_CLIENT_URLS="{{ build_uri('https', db_address, port) }}"
-ETCD_LISTEN_CLIENT_URLS="http://127.0.0.1:4001,{{ build_uri('https', db_bind_address, port) }}"
+ETCD_ADVERTISE_CLIENT_URLS="{{ advertise_urls | join(',') }}"
+ETCD_LISTEN_CLIENT_URLS="{{ listen_client_urls | join(',') }}"
 ETCD_LISTEN_PEER_URLS="{{ build_uri('https', cluster_bind_address, management_port) }}"
 ETCD_INITIAL_ADVERTISE_PEER_URLS="{{ build_uri('https', cluster_address, management_port) }}"
 {% if cluster %}

diff --git a/templates/etcd3.conf b/templates/etcd3.conf
@@ -31,7 +31,7 @@ quota-backend-bytes: 0
 
 listen-peer-urls: {{ build_uri('https', cluster_bind_address, management_port) }}
 # List of comma separated URLs to listen on for client traffic.
-listen-client-urls: http://127.0.0.1:4001,{{ build_uri('https', db_bind_address, port) }}
+listen-client-urls: {{ listen_client_urls | join(",") }}
 
 # Maximum number of snapshot files to retain (0 is unlimited).
 max-snapshots: 5
@@ -49,7 +49,7 @@ initial-advertise-peer-urls: {{ build_uri('https', cluster_address, management_p
 
 # List of this member's client URLs to advertise to the public.
 # The URLs needed to be a comma-separated list.
-advertise-client-urls: {{ build_uri('https', db_address, port) }}
+advertise-client-urls: {{ advertise_urls | join(",") }}
 
 # Discovery URL used to bootstrap the cluster.
 discovery: 

diff --git a/tests/integration/test_etcd.py b/tests/integration/test_etcd.py
@@ -1,21 +1,29 @@
 from typing import List
-import pytest
 from pytest_operator.plugin import OpsTest
 from juju.unit import Unit
 import logging
 import os
+from pathlib import Path
+import pytest
 
 log = logging.getLogger(__name__)
 
-certs = [
+v2_env = [
+    "ETCDCTL_API=2",
     "ETCDCTL_KEY_FILE=/var/snap/etcd/common/client.key",
     "ETCDCTL_CERT_FILE=/var/snap/etcd/common/client.crt",
     "ETCDCTL_CA_FILE=/var/snap/etcd/common/ca.crt",
+]
+v3_env = [
+    "ETCDCTL_API=3",
     "ETCDCTL_KEY=/var/snap/etcd/common/client.key",
     "ETCDCTL_CERT=/var/snap/etcd/common/client.crt",
     "ETCDCTL_CACERT=/var/snap/etcd/common/ca.crt",
 ]
 
+etcdctl_2 = f"{' '.join(v2_env)} /snap/bin/etcdctl --endpoint=https://127.0.0.1:2379"
+etcdctl_3 = f"{' '.join(v3_env)} /snap/bin/etcdctl --endpoints=https://127.0.0.1:2379"
+
 
 async def _unit_run(unit: Unit, jcmd: str, check: bool = True):
     action = await unit.run(jcmd)
@@ -34,14 +42,10 @@ async def test_build_and_deploy(series: str, ops_test: OpsTest):
     await ops_test.model.add_relation("easyrsa:client", "etcd:certificates")
     await ops_test.model.wait_for_idle(wait_for_active=True, timeout=60 * 60)
 
-    jcmd = "{} ETCDCTL_API=2 /snap/bin/etcd.etcdctl set juju rocks".format(
-        " ".join(certs)
-    )
+    jcmd = f"{etcdctl_2} set juju rocks"
     await _unit_run(ops_test.model.applications["etcd"].units[0], jcmd)
 
-    nscmd = "{} ETCDCTL_API=2 /snap/bin/etcd.etcdctl set nested/data works".format(
-        " ".join(certs)
-    )
+    nscmd = f"{etcdctl_2} set nested/data works"
     await _unit_run(ops_test.model.applications["etcd"].units[0], nscmd)
 
 
@@ -82,17 +86,15 @@ async def test_node_scale_up(ops_test: OpsTest):
 
 async def test_cluster_health(ops_test: OpsTest):
     for unit in ops_test.model.applications["etcd"].units:
-        cmd = "{} ETCDCTL_API=2 /snap/bin/etcdctl cluster-health".format(
-            " ".join(certs)
-        )
+        cmd = f"{etcdctl_2} cluster-health"
         action = await _unit_run(unit, cmd)
         assert "unhealthy" not in action.results["stdout"]
         assert "unavailable" not in action.results["stdout"]
 
 
 async def test_leader_knows_all_members(ops_test: OpsTest):
     leader = await _get_leader(ops_test.model.applications["etcd"].units)
-    cmd = "{} ETCDCTL_API=2 /snap/bin/etcdctl member list".format(" ".join(certs))
+    cmd = f"{etcdctl_2} member list"
     action = await _unit_run(leader, cmd)
     members = action.results["stdout"].strip().split("\n")
     assert "etcd cluster is unavailable" not in members
@@ -130,13 +132,11 @@ async def test_snap_upgrade_to_three_oh(ops_test: OpsTest):
 
 
 async def validate_etcd_fixture_data(ops_test: OpsTest):
-    jcmd = "{} ETCDCTL_API=2 /snap/bin/etcd.etcdctl get juju".format(" ".join(certs))
+    jcmd = f"{etcdctl_2} get juju"
     action = await _unit_run(ops_test.model.applications["etcd"].units[0], jcmd)
     assert "rocks" in action.results["stdout"]
 
-    nscmd = "{} ETCDCTL_API=2 /snap/bin/etcd.etcdctl get nested/data".format(
-        " ".join(certs)
-    )
+    nscmd = f"{etcdctl_2} get nested/data"
     action = await _unit_run(ops_test.model.applications["etcd"].units[0], nscmd)
     assert "works" in action.results["stdout"]
 
@@ -147,7 +147,7 @@ async def validate_running_snap_daemon(ops_test: OpsTest):
     assert "active" in action.results["stdout"]
 
 
-async def test_snapshot_restore(ops_test: OpsTest):
+async def test_snapshot_restore(ops_test: OpsTest, tmp_path: Path):
     # Make sure there is only 1 unit of etcd running
     for unit in ops_test.model.applications["etcd"].units:
         if len(ops_test.model.applications["etcd"].units) > 1:
@@ -167,23 +167,22 @@ async def test_snapshot_restore(ops_test: OpsTest):
         log.info(action.status)
         log.info(action.results)
         assert action.status == "completed"
-        await leader.scp_from(action.results["snapshot"]["path"], ".")
-        filenames[dataset] = os.path.basename(action.results["snapshot"]["path"])
+        await leader.scp_from(action.results["snapshot"]["path"], tmp_path)
+        filenames[dataset] = tmp_path / os.path.basename(
+            action.results["snapshot"]["path"]
+        )
 
     await delete_data(ops_test)
     assert not await is_data_present(ops_test, "v2")
     assert not await is_data_present(ops_test, "v3")
 
-    # Below code is better but waiting for python-libjuju #654 fix, can't attach binary files yet due to the bug
-    # with open(filenames["v2"], mode='rb') as file:
-    #     ops_test.model.applications["etcd"].attach_resource("snapshot", filenames["v2"], file)
+    with filenames["v2"].open(mode="rb") as file:
+        ops_test.model.applications["etcd"].attach_resource(
+            "snapshot", filenames["v2"], file
+        )
 
     await ops_test.model.wait_for_idle(wait_for_active=True, timeout=60 * 60)
 
-    await ops_test.juju(
-        "attach-resource", "etcd", "snapshot={}".format(filenames["v2"])
-    )
-
     leader = await _get_leader(ops_test.model.applications["etcd"].units)
     action = await leader.run_action("restore")
     await action.wait()
@@ -195,16 +194,13 @@ async def test_snapshot_restore(ops_test: OpsTest):
     assert await is_data_present(ops_test, "v2")
     assert not await is_data_present(ops_test, "v3")
 
-    # Below code is better but waiting for python-libjuju #654 fix, can't attach binary files yet due to the bug
-    # with open(filenames["v3"], mode='rb') as file:
-    #     ops_test.model.applications["etcd"].attach_resource("snapshot", filenames["v3"], file)
+    with filenames["v3"].open(mode="rb") as file:
+        ops_test.model.applications["etcd"].attach_resource(
+            "snapshot", filenames["v3"], file
+        )
 
     await ops_test.model.wait_for_idle(wait_for_active=True, timeout=60 * 60)
 
-    await ops_test.juju(
-        "attach-resource", "etcd", "snapshot={}".format(filenames["v3"])
-    )
-
     leader = await _get_leader(ops_test.model.applications["etcd"].units)
     action = await leader.run_action("restore")
     await action.wait()
@@ -219,20 +215,16 @@ async def test_snapshot_restore(ops_test: OpsTest):
 
 async def load_data(ops_test: OpsTest):
     leader = await _get_leader(ops_test.model.applications["etcd"].units)
-    cmd = "{} ETCDCTL_API=2 /snap/bin/etcdctl set /etcd2key etcd2value".format(
-        " ".join(certs)
-    )
+    cmd = f"{etcdctl_2} set /etcd2key etcd2value"
     await _unit_run(leader, cmd)
-    cmd = "{} ETCDCTL_API=3 /snap/bin/etcdctl --endpoints=http://localhost:4001 put etcd3key etcd3value".format(
-        " ".join(certs[3:])
-    )
+    cmd = f"{etcdctl_3} put etcd3key etcd3value"
     await _unit_run(leader, cmd)
 
 
 async def is_data_present(ops_test: OpsTest, version: str):
     leader = await _get_leader(ops_test.model.applications["etcd"].units)
     if version == "v2":
-        cmd = "{} ETCDCTL_API=2 /snap/bin/etcdctl ls".format(" ".join(certs))
+        cmd = f"{etcdctl_2} ls"
         action = await _unit_run(leader, cmd)
         log.info(action.status)
         log.info(action.results)
@@ -242,9 +234,7 @@ async def is_data_present(ops_test: OpsTest, version: str):
             else False
         )
     elif version == "v3":
-        cmd = '{} ETCDCTL_API=3 /snap/bin/etcdctl --endpoints=http://localhost:4001 get "" --prefix --keys-only'.format(
-            " ".join(certs[3:])
-        )
+        cmd = f'{etcdctl_3} get "" --prefix --keys-only'
         action = await _unit_run(leader, cmd)
         log.info(action.status)
         log.info(action.results)
@@ -258,10 +248,8 @@ async def is_data_present(ops_test: OpsTest, version: str):
 
 async def delete_data(ops_test: OpsTest):
     leader = await _get_leader(ops_test.model.applications["etcd"].units)
-    cmd = "{} ETCDCTL_API=2 /snap/bin/etcdctl rm /etcd2key".format(" ".join(certs))
+    cmd = f"{etcdctl_2} rm /etcd2key"
     await _unit_run(leader, cmd)
 
-    cmd = "{} ETCDCTL_API=3 /snap/bin/etcdctl --endpoints=http://localhost:4001 del etcd3key".format(
-        " ".join(certs[3:])
-    )
+    cmd = f"{etcdctl_3} del etcd3key"
     await _unit_run(leader, cmd)
diff --git a/tests/unit/lib/test_etcd_databag.py b/tests/unit/lib/test_etcd_databag.py
@@ -42,6 +42,7 @@ def test_render_etcd2(
 ):
     config.set("management_port", 1234)
     config.set("port", 5678)
+    config.set("bind_with_insecure_http", True)
     bag = etcd_databag.EtcdDatabag()
     template_env = Environment(loader=FileSystemLoader("templates"))
     config = template_env.get_template("etcd2.conf").render(bag.__dict__)
@@ -62,6 +63,7 @@ def test_render_etcd3(
 ):
     config.set("management_port", 1234)
     config.set("port", 5678)
+    config.set("bind_with_insecure_http", True)
     bag = etcd_databag.EtcdDatabag()
     template_env = Environment(loader=FileSystemLoader("templates"))
     config = template_env.get_template("etcd3.conf").render(bag.__dict__)