Merge ~hloeung/content-cache-charm:metrics into content-cache-charm:master

[Haw Loeung]

Enable Nginx basic server status metrics - LP:1916078

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

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

[Stuart Bishop (stub)]

Looks good. Confirmed the usual use case is the metrics will be exposed via a telegraf subordinate.

review: Approve

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

Change successfully merged at revision 7cdad621afbaf34f094a19243c12b807536e841b

diff --git a/config.yaml b/config.yaml
index a7cd0e9..84f8834 100644
--- a/config.yaml
+++ b/config.yaml
@@ -45,7 +45,7 @@ options:
     type: boolean
     description: >
       Export metrics for the number of requests and the number of cache hits.
-      Prometheus metrics are exposed at /metrics on port 9145.
+      Prometheus metrics are exposed at /metrics on localhost:9145.
   haproxy_hard_stop_after:
     default: "5m"
     type: string
@@ -85,6 +85,11 @@ options:
     description: >
       Configure maximum number of connections per site on frontend
       HAProxy. Defaults to auto-calculate (0).
+  metrics_listen_address:
+    type: string
+    default: "localhost"
+    description: >
+      Restrict exposed metrics to listen on certain addresses.
   nagios_context:
     type: string
     default: "juju"
diff --git a/lib/nginx.py b/lib/nginx.py
index d880d72..2b733a3 100644
--- a/lib/nginx.py
+++ b/lib/nginx.py
@@ -9,6 +9,7 @@ from lib import utils
 
 
 INDENT = ' ' * 4
+METRICS_LISTEN = 'localhost'
 METRICS_PORT = 9145
 METRICS_SITE = 'nginx_metrics'
 NGINX_BASE_PATH = '/etc/nginx'
@@ -189,7 +190,7 @@ class NginxConf:
 
         return changed
 
-    def toggle_metrics_site(self, enable_prometheus_metrics):
+    def toggle_metrics_site(self, enable_prometheus_metrics, listen_address=None):
         """Create/delete the metrics site configuration and links.
 
         :param bool enable_prometheus_metrics: True if metrics are exposed to prometheus
@@ -203,8 +204,12 @@ class NginxConf:
         # If no cache metrics, remove the site
         if not enable_prometheus_metrics:
             return self._remove_metrics_site(available, enabled)
+
+        if listen_address is None:
+            listen_address = METRICS_LISTEN
+
         template = self.jinja_env.get_template('templates/nginx_metrics_cfg.tmpl')
-        content = template.render({'nginx_conf_path': self.conf_path, 'port': METRICS_PORT})
+        content = template.render({'nginx_conf_path': self.conf_path, 'address': listen_address, 'port': METRICS_PORT})
         # Check if contents changed
         try:
             with open(available, 'r', encoding='utf-8') as f:
diff --git a/reactive/content_cache.py b/reactive/content_cache.py
index a215f62..d396779 100644
--- a/reactive/content_cache.py
+++ b/reactive/content_cache.py
@@ -102,7 +102,7 @@ def service_start_or_reload():
             reactive.clear_flag('content_cache.{}.reload-required'.format(name))
 
 
-def configure_nginx_metrics(ngx_conf, enable_prometheus_metrics):
+def configure_nginx_metrics(ngx_conf, enable_prometheus_metrics, listen_address):
     """Configure nginx to expose metrics.
 
     Create the dedicated server exposing the metrics and add the logging of the cache hits for the other sites.
@@ -114,7 +114,7 @@ def configure_nginx_metrics(ngx_conf, enable_prometheus_metrics):
     changed = False
     if copy_file('files/prometheus.lua', os.path.join(ngx_conf.conf_path, 'prometheus.lua')):
         changed = True
-    if ngx_conf.toggle_metrics_site(enable_prometheus_metrics):
+    if ngx_conf.toggle_metrics_site(enable_prometheus_metrics, listen_address):
         changed = True
     old_ports = [int(port.split('/')[0]) for port in hookenv.opened_ports()]
     hookenv.log("Current opened ports: {}".format(old_ports))
@@ -187,7 +187,8 @@ def configure_nginx(conf_path=None):
             hookenv.log('Wrote out new configs for site: {}:{}'.format(site, conf['listen_port']))
             changed = True
 
-    if configure_nginx_metrics(ngx_conf, enable_prometheus_metrics):
+    metrics_listen = config.get('metrics_listen_address', None)
+    if configure_nginx_metrics(ngx_conf, enable_prometheus_metrics, listen_address=metrics_listen):
         hookenv.log('nginx metrics exposed to prometheus')
         changed = True
 
diff --git a/templates/nginx_metrics_cfg.tmpl b/templates/nginx_metrics_cfg.tmpl
index b3c8b9e..bad2db4 100644
--- a/templates/nginx_metrics_cfg.tmpl
+++ b/templates/nginx_metrics_cfg.tmpl
@@ -4,14 +4,14 @@ init_by_lua_block {
   prometheus = require("prometheus").init("prometheus_metrics")
   http_request_total = prometheus:counter(
     "nginx_http_request_total", "Number of HTTP requests per site", {"host", "status"})
-  cache_request_total = prometheus:counter(                                    
+  cache_request_total = prometheus:counter(
     "nginx_cache_request_total", "Number of cache requests per site", {"host"})
   cache_request_hit_total = prometheus:counter(
     "nginx_cache_request_hit_total", "Number of cache hits per site", {"host"})
 }
 
 server {
-  listen {{ port }};
+  listen {% if address %}{{address}}:{% endif %}{{port}};
   allow all;
   location /metrics {
     content_by_lua_block {
@@ -19,6 +19,10 @@ server {
     }
   }
 
+  location /server_status {
+    stub_status;
+  }
+
   access_log /var/log/nginx/nginx_metrics-access.log;
   error_log /var/log/nginx/nginx_metrics-error.log;
 }
diff --git a/tests/unit/test_content_cache.py b/tests/unit/test_content_cache.py
index d8300a9..4d2a8e8 100644
--- a/tests/unit/test_content_cache.py
+++ b/tests/unit/test_content_cache.py
@@ -1299,6 +1299,7 @@ site1.local:
             'enable_cache_background_update': False,
             'enable_cache_lock': False,
             'enable_prometheus_metrics': True,
+            'metrics_listen_address': '127.0.0.2',
             'sites': ngx_config,
             'worker_connections': 768,
             'worker_processes': 0,
@@ -1344,7 +1345,11 @@ site1.local:
             jinja_env = jinja2.Environment(loader=jinja2.FileSystemLoader(script_dir))
             template = jinja_env.get_template('templates/nginx_metrics_cfg.tmpl')
             content = template.render(
-                {'nginx_conf_path': os.path.join(self.tmpdir, 'conf.d'), 'port': nginx.METRICS_PORT}
+                {
+                    'address': '127.0.0.2',
+                    'nginx_conf_path': os.path.join(self.tmpdir, 'conf.d'),
+                    'port': nginx.METRICS_PORT,
+                }
             )
             want = content
             test_file = os.path.join(self.tmpdir, 'sites-available/{0}.conf'.format(nginx.METRICS_SITE))
diff --git a/tests/unit/test_nginx.py b/tests/unit/test_nginx.py
index b76c674..b0abde9 100644
--- a/tests/unit/test_nginx.py
+++ b/tests/unit/test_nginx.py
@@ -170,15 +170,21 @@ class TestLibNginx(unittest.TestCase):
         script_dir = os.path.join(os.path.dirname(os.path.realpath(__file__)), '..', '..')
         env = jinja2.Environment(loader=jinja2.FileSystemLoader(script_dir))
         template = env.get_template('templates/nginx_metrics_cfg.tmpl')
-        content = template.render({'nginx_conf_path': os.path.join(self.tmpdir, 'conf.d'), 'port': nginx.METRICS_PORT})
+        content = template.render(
+            {
+                'address': '',
+                'nginx_conf_path': os.path.join(self.tmpdir, 'conf.d'),
+                'port': nginx.METRICS_PORT,
+            }
+        )
         nginx_metrics_conf = content
 
         metrics_site_available = os.path.join(self.tmpdir, 'sites-available', metrics_site_conf)
         metrics_site_enabled = os.path.join(self.tmpdir, 'sites-enabled', metrics_site_conf)
-        self.assertTrue(ngx_conf.toggle_metrics_site(enable_prometheus_metrics=True))
+        self.assertTrue(ngx_conf.toggle_metrics_site(enable_prometheus_metrics=True, listen_address=''))
         # Write again with same contents, this time it should return 'False'
         # as there's no change, thus no need to restart/reload Nginx.
-        self.assertFalse(ngx_conf.toggle_metrics_site(enable_prometheus_metrics=True))
+        self.assertFalse(ngx_conf.toggle_metrics_site(enable_prometheus_metrics=True, listen_address=''))
         self.assertTrue(os.path.exists(metrics_site_available))
 
         # Compare what's been written out matches what's in tests/unit/files.