Merge ~sajoupa/charm-telegraf:expose_ip_ranges into charm-telegraf:master

[Laurent Sesquès]

Add support for prometheus_ip_range, allowing to restrict access to prometheus-client.

Also fixing:
- allow setting prometheus_output_port (and prometheus_ip_range) even when related to prometheus
- fix restart condition when the prometheus-client plugin is changed
- make the default port 9103 instead of 9216 when related to prometheus (https://github.com/prometheus/prometheus/wiki/Default-port-allocations says 9216 is "New Relic exporter", according to IANA it's bacula-sd)

[Haw Loeung (hloeung)]

Quick first pass, see comment.

[Laurent Sesquès (sajoupa)]

Thanks, I addressed them.

[Laurent Sesquès (sajoupa)]

(saving comments, hopefully for real this time)

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

[Paul Goins (vultaire)]

Thanks for the MR.

This seems like a fairly large MR if the intent is purely to address bug #1890102. I wonder if the code can be reduced here? I haven't read the whole MR yet, but I've read 1/3rd to 1/2, and it seems like maybe it's addressing something else as well? Could you explain?

Additionally, as axino noted on the above bug, this can be addressed, albeit more verbose, via the extra_options config value. Is this insufficient? And if it is, could we focus the change on adding the config and updating the template to consume that config value?

To be clear, I'm not trying to brush off these changes - but I am not understanding why all these changes are needed based upon the bug this is connected to. If there are other bugs to be fixed, it'd be better to fix those via additional MRs, in my opinion.

Best Regards,
Paul Goins

review: Needs Information

[Laurent Sesquès (sajoupa)]

Thanks for the review.
We can do about anything with exta_options, but that's not ideal. Also, if we set an IP range manually with extra_options, then to allow related prometheuses, we must manually add their IP addresses to extra_options (and remove them on relation-departed, etc.).

This change also allows setting prometheus_output_port (and prometheus_ip_range) even when related to prometheus. Previously in a relation with prometheus, the charm could only set the port to 9126. https://github.com/prometheus/prometheus/wiki/Default-port-allocations says 9216 is "New Relic exporter", according to IANA it's bacula-sd so I made it default to 9103 (admittedly, changing the default could be in a separate MP).

[Laurent Sesquès (sajoupa)]

> https://github.com/prometheus/prometheus/wiki/Default-port-allocations says
> 9216 is "New Relic exporter", according to IANA it's bacula-sd so I made it
> default to 9103 (admittedly, changing the default could be in a separate MP).
9103 is bacula-sd actually according to IANA. But Prometheus considers it as the collectd port, so still a better default (also the telegraf charm default).

[Laurent Sesquès (sajoupa)]

I've split this MP into 4 smaller ones:
https://code.launchpad.net/~sajoupa/charm-telegraf/+git/telegraf-charm/+merge/397010
on which depends:
https://code.launchpad.net/~sajoupa/charm-telegraf/+git/telegraf-charm/+merge/397005
on which depends:
https://code.launchpad.net/~sajoupa/charm-telegraf/+git/telegraf-charm/+merge/397007
on which depends:
https://code.launchpad.net/~sajoupa/charm-telegraf/+git/telegraf-charm/+merge/397009

Will mark this one 'rejected'.

Unmerged commits

a9d2bea... by Laurent Sesquès on 2021-01-22

prometheus_ip_range support: a few code improvements

fa4e7c9... by Laurent Sesquès on 2021-01-22

add unit tests for prometheus_ip_range

68a5407... by Laurent Sesquès on 2021-01-21

remove TODO comment about get_prometheus_port(), actually not relevant

689fb2b... by Laurent Sesquès on 2021-01-21

rework flags and wording for prometheus-client

8623c5f... by Laurent Sesquès on 2021-01-20

prometheus_client output plugin: fix reactive logic, update unit tests

9359340... by Laurent Sesquès on 2021-01-19

revamp prometheus-client output configuration to add ip_range, fix bugs with prometheus relation

diff --git a/src/config.yaml b/src/config.yaml
index 66f6105..b94071b 100644
--- a/src/config.yaml
+++ b/src/config.yaml
@@ -206,3 +206,9 @@ options:
         Note: The complete datasource inserted into dashboard will be
         "$prometheus_datasource - Juju generated source"
         This matches how the Prometheus2/Grafana charms format the datasource.
+  prometheus_ip_range:
+    default: ""
+    type: string
+    description: >
+        If set, the list of IP Ranges which are allowed to access metrics.
+        ex: prometheus_ip_range = "[192.168.0.0/24, 192.168.1.0/30]"
diff --git a/src/reactive/telegraf.py b/src/reactive/telegraf.py
index 136c271..fe71d43 100644
--- a/src/reactive/telegraf.py
+++ b/src/reactive/telegraf.py
@@ -159,7 +159,14 @@ def list_config_files():
     current_states = get_states()
 
     for plugin in list_supported_plugins():
-        if "plugins.{}.configured".format(plugin) in current_states.keys():
+        # The prometheus_client plugin can be configured either from a relation or
+        # from the juju config
+        if ("plugins.{}.configured".format(plugin) in current_states.keys()) or (
+            "{}.configured".format(plugin) in current_states.keys()
+        ):
+            # The "prometheus-client" relation sets the "prometheus_client" plugin
+            if plugin == "prometheus-client":
+                plugin = "prometheus_client"
             config_path = "{}/{}.conf".format(get_configs_dir(), plugin)
             config_files.append(config_path)
 
@@ -327,8 +334,14 @@ def get_prometheus_port():
         return False
     if config.get("prometheus_output_port") == "default":
         return 9103
-    else:
-        return int(config.get("prometheus_output_port"))
+    return int(config.get("prometheus_output_port"))
+
+
+def get_prometheus_ip_range():
+    config = hookenv.config()
+    if config.get("prometheus_ip_range") == "":
+        return []
+    return config.get("prometheus_ip_range").split(",")
 
 
 def get_socket_listener_port():
@@ -415,6 +428,7 @@ def update_sysstat_config_with_sdac_xall(path="/etc/sysstat/sysstat"):
 
 
 def configure_telegraf():  # noqa: C901
+    hookenv.log("Generating telegraf.conf", level=hookenv.DEBUG)
     update_sysstat_config_with_sdac_xall()
     config = hookenv.config()
     context = config.copy()
@@ -503,9 +517,6 @@ def configure_telegraf():  # noqa: C901
         )
         context["logfile"] = ""
 
-    if get_prometheus_port():
-        context["prometheus_output_port"] = get_prometheus_port()
-        check_prometheus_port("prometheus_output", get_prometheus_port())
     context["extra_options"] = get_extra_options()
 
     if get_socket_listener_port():
@@ -566,10 +577,10 @@ def configure_telegraf():  # noqa: C901
         context=context,
     )
 
+    # Make sure that only the right service is enabled, then defer to start_or_restart()
     for service in [DEB_SERVICE, SNAP_SERVICE]:
         if service == get_service():
             host.service_resume(service)
-            host.service_reload(service)
         else:
             try:
                 host.service_pause(service)
@@ -621,6 +632,7 @@ def install_telegraf():
 
 @when("telegraf.installed")
 @when("apt.installed.telegraf")
+@when("plugins.prometheus-client.configured")
 @when_not("telegraf.configured")
 @when_not("telegraf.apt.configured")
 def configure_telegraf_deb():
@@ -629,6 +641,7 @@ def configure_telegraf_deb():
 
 @when("telegraf.installed")
 @when("snap.installed.telegraf")
+@when("plugins.prometheus-client.configured")
 @when_not("telegraf.configured")
 @when_not("telegraf.snap.configured")
 def configure_telegraf_snap():
@@ -653,6 +666,7 @@ def handle_config_changes():
     if config.changed("extra_options"):
         for plugin in list_supported_plugins():
             clear_flag("plugins.{}.configured".format(plugin))
+        clear_flag("prometheus-client.relation.configured")
     # if something else changed, let's reconfigure telegraf itself just in case
 
     if config.changed("extra_plugins"):
@@ -665,6 +679,14 @@ def handle_config_changes():
     ):
         clear_flag("telegraf.installed")
         clear_flag("extra_plugins.configured")
+        clear_flag("plugins.prometheus-client.configured")
+        clear_flag("prometheus-client.relation.configured")
+
+    if config.changed("prometheus_output_port") or config.changed(
+        "prometheus_ip_range"
+    ):
+        clear_flag("plugins.prometheus-client.configured")
+        clear_flag("prometheus-client.relation.configured")
     clear_flag("telegraf.configured")
     clear_flag("telegraf.apt.configured")
     clear_flag("telegraf.snap.configured")
@@ -983,7 +1005,7 @@ def redis_input(redis):
 [[inputs.redis]]
   servers = ["tcp://{{ host }}:{{ port }}"]
   # Until https://github.com/influxdata/telegraf/issues/5036 is fixed
-  fielddrop = ["aof_last_bgrewrite_status","aof_last_write_status","maxmemory_policy","rdb_last_bgsave_status","used_memory_dataset_perc","used_memory_peak_perc"]
+  fielddrop = ["aof_last_bgrewrite_status","aof_last_write_status","maxmemory_policy","rdb_last_bgsave_status","used_memory_dataset_perc","used_memory_peak_perc"] # noqa E501
 """  # noqa E501 (inline template)
     config_path = "{}/{}.conf".format(get_configs_dir(), "redis")
 
@@ -1185,59 +1207,124 @@ def influxdb_api_output(influxdb):
     set_flag("telegraf.needs_reload")
 
 
+def generate_prometheus_output_config(prometheus_output_port, prometheus_ip_range):
+    # If extra_options are set for prometheus_client, let's integrate them
+    extra_options = get_extra_options()
+    options = extra_options["outputs"].get("prometheus_client", {})
+    listen = options.pop("listen", None)
+    if not listen:
+        listen = ":{}".format(prometheus_output_port)
+    elif int(listen.split(":", 1)[1]) != prometheus_output_port:
+        hookenv.log(
+            """prometheus_output_port is {}, but extra_options would set it
+                       to {}. Choosing {} from prometheus_output_port.""".format(
+                prometheus_output_port,
+                int(listen.split(":", 1)[1]),
+                prometheus_output_port,
+            ),
+            level=hookenv.WARNING,
+        )
+        listen = "{}:{}".format(listen.split(":", 1)[0], prometheus_output_port)
+
+    options_ip_range = options.pop("ip_range", [])
+    ip_range = options_ip_range + prometheus_ip_range
+
+    return {
+        "listen": listen,
+        "ip_range": ip_range,
+        "extra_options": render_extra_options(
+            "outputs", "prometheus_client", extra_options=extra_options
+        ),
+    }
+
+
+def render_prometheus_client_config(port, ip_range):
+    config_path = "{}/{}.conf".format(get_configs_dir(), "prometheus_client")
+    hookenv.log(
+        "Updating {} plugin config file. Port is {} and ip_range is {}".format(
+            "prometheus_client", port, ip_range
+        ),
+        level=hookenv.INFO,
+    )
+    context = generate_prometheus_output_config(port, ip_range)
+    render(
+        source="prometheus_client.tmpl",
+        templates_dir=get_templates_dir(),
+        target=config_path,
+        context=context,
+    )
+
+
 @when("prometheus-client.available")
-@when("telegraf.installed")
-def prometheus_client(prometheus):
-    template = """
-[[outputs.prometheus_client]]
-  listen = "{{ listen }}"
-"""
+@when_not("prometheus-client.relation.configured")
+def configure_prometheus_client_with_relation(prometheus):
+    hookenv.log(
+        "Configuring prometheus_client output plugin, with prometheus-client relation",
+        level=hookenv.DEBUG,
+    )
+    if get_prometheus_port():
+        port = get_prometheus_port()
+    else:
+        # We have a relation with prometheus, but removed the port number config.
+        # Default to 9103.
+        port = "9103"
+    # We'll iterate through the prometheus-client relation counterparts,
+    # inform them of our address so that they scrape it, and get their egress subnets
+    # so that we can allow them
+    remote_egress_subnets = []
     for relation_id in hookenv.relation_ids("prometheus-client"):
         # if juju 2.x+ then we'll attempt to get the network space address
         try:
-            hookenv.log("Network Info")
+            hookenv.log("Getting local betwork info", level=hookenv.DEBUG)
             network_info = hookenv.network_get(
                 "prometheus-client", relation_id=relation_id
             )
-            hookenv.log(network_info)
+            hookenv.log(network_info, level=hookenv.DEBUG)
             if "ingress-addresses" in network_info:
                 ip_addr = network_info.get("ingress-addresses")[0]
             else:
                 ip_addr = hookenv.network_get_primary_address("prometheus-client")
+            for unit in hookenv.related_units(relation_id):
+                hookenv.log(
+                    "Getting remote egress subnet for relation {} - {}".format(
+                        unit, relation_id
+                    ),
+                    level=hookenv.DEBUG,
+                )
+                remote_egress_subnets.append(
+                    hookenv.relation_get("egress-subnets", unit, relation_id)
+                )
         except NotImplementedError:
             # if that fails, just let prometheus.configure(...) do it's default
             ip_addr = None
-        if get_prometheus_port():
-            hookenv.log("Prometheus configured globally, skipping plugin setup")
-            prometheus.configure(
-                get_prometheus_port(), hostname=ip_addr, private_address=ip_addr
-            )
-            set_flag("prometheus-client.configured")
-            # bail out, nothing more need to be configured here
-            return
-        port = 9126
-        extra_options = get_extra_options()
-        options = extra_options["outputs"].get("prometheus-client", {})
-        listen = options.pop("listen", None)
-        if listen is not None:
-            hookenv.log(
-                "Configuring prometheus_client plugin to listen on: '{}'".format(listen)
-            )
-            port = int(listen.split(":", 1)[1])
-        else:
-            listen = ":{}".format(port)
-        check_prometheus_port("prometheus_output", port)
         prometheus.configure(port, hostname=ip_addr, private_address=ip_addr)
-        config_path = "{}/{}.conf".format(get_configs_dir(), "prometheus-client")
-        hookenv.log("Updating {} plugin config file".format("prometheus-client"))
-        context = {"listen": listen}
-        content = render_template(template, context) + render_extra_options(
-            "outputs", "prometheus_client", extra_options=extra_options
-        )
-        host.write_file(config_path, content.encode("utf-8"))
+    check_prometheus_port("prometheus_output", port)
+    # If prometheus_ip_range is empty, all remote IPs are allowed
+    ip_range = get_prometheus_ip_range()
+    if ip_range != []:
+        ip_range = ip_range + remote_egress_subnets
+    render_prometheus_client_config(port, ip_range)
+    set_flag("plugins.prometheus-client.configured")
+    set_flag("prometheus-client.relation.configured")
+    set_flag("telegraf.needs_reload")
+
+
+@when_not("prometheus-client.available")
+@when_not("plugins.prometheus-client.configured")
+def configure_prometheus_client():
+    hookenv.log("Configuring prometheus_client output plugin", level=hookenv.DEBUG)
+    if get_prometheus_port():
+        port = get_prometheus_port()
+    else:
+        # No relation tp prometheus, no port configured: do not configure the plugin
         set_flag("plugins.prometheus-client.configured")
-        set_flag("telegraf.needs_reload")
-        set_flag("prometheus-client.configured")
+        return
+    check_prometheus_port("prometheus_output", port)
+    ip_range = get_prometheus_ip_range()
+    render_prometheus_client_config(port, ip_range)
+    set_flag("plugins.prometheus-client.configured")
+    set_flag("telegraf.needs_reload")
+    clear_flag("prometheus-client.relation.configured")
 
 
 def convert_days(time_string):
@@ -1295,21 +1382,21 @@ def render_prometheus_rules(prometheus_rules):
 
 
 @when_not("prometheus-client.available")
-@when("plugins.prometheus-client.configured")
+@when("prometheus-client.relation.configured")
 def prometheus_client_departed():
     hookenv.log("prometheus-client relation not available")
-    config_path = "{}/{}.conf".format(get_configs_dir(), "prometheus-client")
+    config_path = "{}/{}.conf".format(get_configs_dir(), "prometheus_client")
     rels = hookenv.relations_of_type("prometheus-client")
     if not rels and os.path.exists(config_path):
         hookenv.log("Deleting {} plugin config file".format("prometheus-client"))
         os.unlink(config_path)
         clear_flag("plugins.prometheus-client.configured")
-        set_flag("telegraf.needs_reload")
-    clear_flag("prometheus-client.configured")
 
 
 @when(
-    "prometheus-client.configured", "endpoint.dashboards.joined", "leadership.is_leader"
+    "plugins.prometheus-client.configured",
+    "endpoint.dashboards.joined",
+    "leadership.is_leader",
 )
 @when_not("grafana.configured")
 def register_grafana_dashboard():
@@ -1384,6 +1471,8 @@ def grafana_dashboard_import_failed():
 
 
 @when("telegraf.needs_reload")
+@when("telegraf.installed")
+@when("telegraf.configured")
 def start_or_restart():
     states = sorted(
         [
diff --git a/src/templates/prometheus_client.tmpl b/src/templates/prometheus_client.tmpl
new file mode 100644
index 0000000..a6e3c06
--- /dev/null
+++ b/src/templates/prometheus_client.tmpl
@@ -0,0 +1,4 @@
+[[outputs.prometheus_client]]
+  listen = "{{ listen }}"
+  ip_range = {{ ip_range }}
+{%- if extra_options %}{{ extra_options }}{%- endif %}
diff --git a/src/templates/telegraf.conf.tmpl b/src/templates/telegraf.conf.tmpl
index af59ed5..966b4a9 100644
--- a/src/templates/telegraf.conf.tmpl
+++ b/src/templates/telegraf.conf.tmpl
@@ -80,12 +80,6 @@
 
 {{ outputs }}
 
-{% if prometheus_output_port %}
-[[outputs.prometheus_client]]
-  listen = ":{{ prometheus_output_port }}"
-  {%- if extra_options['outputs']['prometheus_client'] %}{{ render_options('prometheus_client', 'outputs', extra_options['outputs']) }}{%- endif %}
-{%- endif %}
-
 ###############################################################################
 #                                  INPUTS                                     #
 ###############################################################################
diff --git a/src/tests/functional/tests/bundles/bionic-monitoring.yaml b/src/tests/functional/tests/bundles/bionic-monitoring.yaml
index 120eab8..c7a487b 100644
--- a/src/tests/functional/tests/bundles/bionic-monitoring.yaml
+++ b/src/tests/functional/tests/bundles/bionic-monitoring.yaml
@@ -8,6 +8,7 @@ applications:
     num_units: 0
     options:
       prometheus_datasource: prometheus2
+      prometheus_ip_range: '0.0.0.0/0'
   grafana:
     charm: cs:grafana
     num_units: 1
diff --git a/src/tests/unit/test_telegraf.py b/src/tests/unit/test_telegraf.py
index afc3625..9b7343b 100644
--- a/src/tests/unit/test_telegraf.py
+++ b/src/tests/unit/test_telegraf.py
@@ -753,12 +753,11 @@ def test_prometheus_global(monkeypatch, config):
     monkeypatch.setattr(telegraf.hookenv, "open_port", lambda p: open_ports.add(p))
     monkeypatch.setattr(telegraf.hookenv, "close_port", lambda p: open_ports.remove(p))
     config["prometheus_output_port"] = "default"
-    telegraf.configure_telegraf()
-    expected = """
-[[outputs.prometheus_client]]
+    telegraf.configure_prometheus_client()
+    expected = """[[outputs.prometheus_client]]
   listen = ":9103"
-"""
-    config_file = base_dir().join("telegraf.conf")
+  ip_range = []"""
+    config_file = base_dir().join("telegraf.d", "prometheus_client.conf")
     assert expected in config_file.read()
 
 
@@ -777,15 +776,14 @@ outputs:
       cpu: ["cpu0"]
 
 """
-    telegraf.configure_telegraf()
-    expected = """
-[[outputs.prometheus_client]]
+    telegraf.configure_prometheus_client()
+    expected = """[[outputs.prometheus_client]]
   listen = ":9103"
+  ip_range = []
   namedrop = ["aerospike*"]
   [outputs.prometheus_client.tagpass]
-    cpu = ["cpu0"]
-"""
-    config_file = base_dir().join("telegraf.conf")
+    cpu = ["cpu0"]"""
+    config_file = base_dir().join("telegraf.d", "prometheus_client.conf")
     assert expected in config_file.read()
 
 
@@ -1072,7 +1070,9 @@ def test_prometheus_client_output(mocker, monkeypatch, config):
     monkeypatch.setattr(telegraf.hookenv, "open_port", lambda p: None)
 
     relation_ids = ["prometheus-client:0"]
+    related_units = ["prometheus/0"]
     monkeypatch.setattr(telegraf.hookenv, "relation_ids", lambda r: relation_ids)
+    monkeypatch.setattr(telegraf.hookenv, "related_units", lambda r: related_units)
 
     network_get_primary_address = mocker.patch.object(
         telegraf.hookenv, "network_get_primary_address", return_value="foo"
@@ -1082,28 +1082,48 @@ def test_prometheus_client_output(mocker, monkeypatch, config):
         telegraf.hookenv, "network_get", return_value={"ingress_addresses": ["1.2.3.4"]}
     )
 
+    mocker.patch.object(telegraf.hookenv, "relation_get", return_value="5.6.7.8/32")
+
     interface = mocker.Mock(spec=RelationBase)
     interface.configure = mocker.Mock()
-    telegraf.prometheus_client(interface)
+
+    # Test that telegraf will allow scraping from related prometheus, and from ranges in
+    # prometheus_ip_range
+    config["prometheus_ip_range"] = "127.0.0.1/32,10.0.0.0/8"
+    telegraf.configure_prometheus_client_with_relation(interface)
     expected = """
     [[outputs.prometheus_client]]
-  listen = ":9126"
+  listen = ":9103"
+  ip_range = ['127.0.0.1/32', '10.0.0.0/8', '5.6.7.8/32']
 """
     assert (
-        configs_dir().join("prometheus-client.conf").read().strip() == expected.strip()
+        configs_dir().join("prometheus_client.conf").read().strip() == expected.strip()
     )
+    # prometheus_ip_range is empty, test that all IPs are allowed, not just the related
+    # prometheus
+    # prometheus_output_port is also empty, we should get 9103 by default
     network_get_primary_address.assert_called_once_with("prometheus-client")
     interface.configure.assert_called_once_with(
-        9126, hostname="foo", private_address="foo"
+        "9103", hostname="foo", private_address="foo"
+    )
+    config["prometheus_ip_range"] = ""
+    telegraf.configure_prometheus_client_with_relation(interface)
+    expected = """
+    [[outputs.prometheus_client]]
+  listen = ":9103"
+  ip_range = []
+"""
+    assert (
+        configs_dir().join("prometheus_client.conf").read().strip() == expected.strip()
     )
 
 
 def test_prometheus_client_output_departed(mocker, monkeypatch, config):
-    configs_dir().join("prometheus-client.conf").write("empty")
+    configs_dir().join("prometheus_client.conf").write("empty")
     relations = [1]
     monkeypatch.setattr(telegraf.hookenv, "relations_of_type", lambda n: relations)
     telegraf.prometheus_client_departed()
-    assert configs_dir().join("prometheus-client.conf").exists()
+    assert configs_dir().join("prometheus_client.conf").exists()
     relations.pop()
     telegraf.prometheus_client_departed()
     assert not configs_dir().join("prometheus-client.conf").exists()
@@ -1198,17 +1218,18 @@ def test_restart_on_output_plugin_relation_departed(mocker, monkeypatch, config)
     mocker.patch.object(
         telegraf.hookenv, "network_get", return_value={"ingress_addresses": ["1.2.3.4"]}
     )
-    config["prometheus_output_port"] = ""
+    config["prometheus_output_port"] = "9103"
     bus.discover()
+    set_flag("telegraf.installed")
     set_flag("snap.telegraf.installed")
     set_flag("telegraf.configured")
     interface = mocker.Mock(spec=RelationBase)
     interface.configure = mocker.Mock()
-    telegraf.prometheus_client(interface)
-    assert configs_dir().join("prometheus-client.conf").exists()
-    # dispatch, file should be gone and telegraf restarted.
+    telegraf.configure_prometheus_client_with_relation(interface)
+    assert configs_dir().join("prometheus_client.conf").exists()
+    # dispatch, file should still be present and telegraf restarted.
     bus.dispatch()
-    assert not configs_dir().join("prometheus-client.conf").exists()
+    assert configs_dir().join("prometheus_client.conf").exists()
     service_restart.assert_called_with(telegraf.DEB_SERVICE)