Merge ~xavpaice/charm-openstack-service-checks/+git/charm-openstack-service-checks:ssl-nagios-checks into ~canonical-bootstack/charm-openstack-service-checks:master

[Xav Paice]

Add NRPE checks for all API endpoints in the Keystone catalog

This change implements the first stage of spec https://code.launchpad.net/~xavpaice/bootstack-charm-specs/+git/bootstack-charm-specs/+ref/ssl-nagios-checks

The intent is to add an NRPE check to the openstack-services-checks unit, checking each endpoint found in the Keystone catalog using a healthcheck URL if known, otherwise the root URL found in the catalog. If the service is using TLS, then the expiry date of the certificate is also checked.

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

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

[Alvaro Uria (aluria)]

Please find comments inline. Other than that, I tested it in the charmlab and it looks good to me.

review: Needs Fixing

[Xav Paice (xavpaice)]

Thanks for the review - couple of responses inline. Will update the actual change shortly.

7bac062... by Xav Paice on 2019-01-07

Address review points for additional service checks

90ee556... by Xav Paice on 2019-01-08

Add PEP8 test and fix failures

[Joel Sing (jsing)]

Please add an actual commit message to this merge proposal - what is this change doing and why?

Readability/standards review comments inline (mostly consistency related).

[Andrea Ieri (aieri)]

A bunch of inline comments :)

review: Needs Fixing

1d372dd... by Xav Paice on 2019-01-11

Address review comments

[Xav Paice (xavpaice)]

Thanks for the detailed review(s). Fresh commit coming.

Couple of inline responses.

[Joel Sing (jsing)]

LGTM for a readability/standards review - couple of minor comments inline.

Please ensure you get a peer approval before merging.

review: Approve (+1)

f6a6d60... by Xav Paice on 2019-02-17

address review nits

diff --git a/.gitignore b/.gitignore
new file mode 100644
index 0000000..5835925
--- /dev/null
+++ b/.gitignore
@@ -0,0 +1,9 @@
+*~
+/bin
+debian/files
+/pkg
+*.pyc
+__pycache__
+*.swp
+.tox
+.venv
diff --git a/Makefile b/Makefile
index a1ad3a5..27eeff6 100644
--- a/Makefile
+++ b/Makefile
@@ -1,6 +1,6 @@
 #!/usr/bin/make
 
-all: lint unit_test
+all: lint test
 
 
 .PHONY: clean
@@ -10,15 +10,17 @@ clean:
 .PHONY: apt_prereqs
 apt_prereqs:
 	@# Need tox, but don't install the apt version unless we have to (don't want to conflict with pip)
-	@which tox >/dev/null || (sudo apt-get install -y python-pip && sudo pip install tox)
+	@which tox >/dev/null || (sudo apt-get install -y python3-pip && sudo pip3 install tox)
 
 .PHONY: lint
 lint: apt_prereqs
-	@tox --notest
-	@PATH=.tox/py34/bin:.tox/py35/bin flake8 $(wildcard hooks reactive lib unit_tests tests)
+	@tox -e pep8
 	@charm proof
 
-.PHONY: unit_test
+.PHONY: test
 unit_test: apt_prereqs
 	@echo Starting tests...
 	tox
+
+build:
+	charm build
diff --git a/README.md b/README.md
index 24a4b54..b3159af 100644
--- a/README.md
+++ b/README.md
@@ -2,7 +2,7 @@
 
 This charm provides OpenStack services checks for Nagios
 
- # Build
+# Build
 The fully built charm needs the following source branch
 * https://git.launchpad.net/~canonical-bootstack/bootstack-ops/+git/charm-openstack-services-checks
 
@@ -29,16 +29,20 @@ Build the charm, and symlink for juju-1 compatibility
 
     juju deploy local:xenial/openstack-services-checks
 
-This charm supports relating to keystone, but keystone-credentials interface
-seems to be flaky, and hard to remove-relation, so the charm works around this
-by adding 'os-credentials' setting (see setting description hints)
+This charm supports relating to keystone via the keystone-credentials
+interface.  If you do not wish to use this, you can supply your own credential
+set for Openstack by  adding 'os-credentials' setting (see setting description
+hints)
 
     juju set openstack-services-checks os-credentials=" ... "
-
     juju add-relation openstack-services-checks nagios
 
 With Keystone
 
     juju add-relation openstack-services-checks:identity-credentials keystone:identity-credentials
 
+If your OpenStack API endpoints have a common URL for the Admin, Public and
+Internal addresses, you should consider disabling some endpoints which would be
+duplicated otherwise, e.g.
 
+    juju config openstack-service-checks check_internal_urls=False check_admin_urls=False
diff --git a/config.yaml b/config.yaml
index e4e8783..b71208c 100644
--- a/config.yaml
+++ b/config.yaml
@@ -48,3 +48,28 @@ options:
     default: false
     description: |
       An option to specify whether you want Warning alerts in nagios for disabled nova-compute hosts.
+  tls_warn_days:
+    type: int
+    default: 30
+    description: |
+      Number of days left for the TLS certificate to expire before warning.
+  tls_crit_days:
+    type: int
+    default: 14
+    description: |
+      Number of days left for the TLS certificate to expire before alerting Critical.
+  check_public_urls:
+    type: boolean
+    default: True
+    description: |
+      If true, create NRPE checks matching all 'public' URLs in the Keystone catalog.
+  check_internal_urls:
+    type: boolean
+    default: True
+    description: |
+      If true, create NRPE checks matching all 'internal' URLs in the Keystone catalog.
+  check_admin_urls:
+    type: boolean
+    default: True
+    description: |
+      If true, create NRPE checks matching all 'admin' URLs in the Keystone catalog.
diff --git a/files/plugins/check_nova_services.py b/files/plugins/check_nova_services.py
index 1279c6a..6498542 100755
--- a/files/plugins/check_nova_services.py
+++ b/files/plugins/check_nova_services.py
@@ -43,11 +43,11 @@ def check_hosts_up(args, aggregate, hosts, services_compute):
         local_msg.append("Host Aggregate {} has {} hosts alive".format(
             aggregate, counts['ok']))
     nova_status = {
-            'agg_name': aggregate,
-            'msg_text': ", ".join(local_msg),
-            'critical': status_crit,
-            'warning': status_warn,
-            }
+        'agg_name': aggregate,
+        'msg_text': ", ".join(local_msg),
+        'critical': status_crit,
+        'warning': status_warn,
+    }
     return nova_status
 
 
@@ -103,8 +103,8 @@ if __name__ == '__main__':
     command = ['/bin/bash', '-c', "source {} && env".format(args.env)]
     proc = subprocess.Popen(command, stdout=subprocess.PIPE)
     for line in proc.stdout:
-        (key, _, value) = line.partition("=")
-        os.environ[key] = value.rstrip()
+        (key, _, value) = line.partition(b'=')
+        os.environ[key.decode('utf-8')] = value.rstrip().decode('utf-8')
     proc.communicate()
     nova = os_client_config.session_client('compute', cloud='envvars')
     nagios_plugin.try_check(check_nova_services, args, nova)
diff --git a/layer.yaml b/layer.yaml
index f364d90..72f0af5 100644
--- a/layer.yaml
+++ b/layer.yaml
@@ -1,6 +1,16 @@
 includes:
+  - 'layer:apt'
   - 'layer:basic'
-  - 'interface:nrpe-external-master'
   - 'interface:keystone-credentials'
+  - 'interface:nrpe-external-master'
 ignore: ['.*.swp' ]
 repo: 'lp:~canonical-bootstack/+git/charm-openstack-service-checks'
+options:
+  basic:
+    use_venv: true
+  apt:
+    packages:
+      - nagios-nrpe-server
+      - python3-keystoneclient
+      - python3-openstackclient
+      - python-openstackclient
diff --git a/reactive/service_checks.py b/reactive/service_checks.py
index 8341324..982b1a5 100644
--- a/reactive/service_checks.py
+++ b/reactive/service_checks.py
@@ -17,112 +17,109 @@ from charmhelpers.core import (
     unitdata,
 )
 
-from charmhelpers.fetch import (
-    apt_install,
-    apt_update,
-)
-
 from charmhelpers.contrib.charmsupport.nrpe import NRPE
+from urllib.parse import urlparse
 
 config = hookenv.config()
-install_packages = ['nagios-nrpe-server', 'python-openstackclient']
+NOVARC = '/var/lib/nagios/nagios.novarc'
+PLUGINS_DIR = '/usr/local/lib/nagios/plugins/'
 
 
 @when_not('os-service-checks.installed')
 def install_service_checks():
     hookenv.status_set('maintenance', 'Installing software')
-    apt_update()
-    apt_install(install_packages)
+    # moved to apt layer
     set_state('os-service-checks.installed')
     remove_state('os-service-checks.configured')
     hookenv.status_set('active', 'Ready')
-# setup openstack user
 
 
 @when('identity-credentials.connected')
-def configure_keystone_username(keystone):
+def configure_ident_username(keystone):
     username = 'nagios'
     keystone.request_credentials(username)
 
 
 @when('identity-credentials.available')
 def save_creds(keystone):
-    creds = get_creds(keystone)
-    unitdata.kv().set('keystone-relation-creds', creds)
-    remove_state('os-service-checks.configured')
-
-
-def get_creds(keystone):
-
+    """
+    Get credentials from the Keystone relation,
+    reformat them into something the Keystone client
+    can use, and save them into the unitdata.
+    """
+    creds = {
+        'username': keystone.credentials_username(),
+        'password': keystone.credentials_password(),
+        'region': keystone.region(),
+    }
     if keystone.api_version() == '3':
         api_url = "v3"
         try:
             domain = keystone.domain()
         except AttributeError:
             domain = 'service_domain'
-        creds = {
-            'credentials_username': keystone.credentials_username(),
-            'credentials_password': keystone.credentials_password(),
-            'credentials_project': keystone.credentials_project(),
+        # keystone relation sends info back with funny names, fix here
+        creds.update({
+            'project_name': keystone.credentials_project(),
             'auth_version': '3',
-            'region': keystone.region(),
-            'credentials_user_domain': domain,
-            'credentials_project_domain': domain
-            }
+            'user_domain_name': domain,
+            'project_domain_name': domain
+        })
     else:
         api_url = "v2.0"
-        creds = {
-            'credentials_username': keystone.credentials_username(),
-            'credentials_password': keystone.credentials_password(),
-            'credentials_project': keystone.credentials_project(),
-            'region': keystone.region(),
-            }
+        creds['tenant_name'] = keystone.credentials_project()
 
     auth_url = "%s://%s:%s/%s" % (keystone.auth_protocol(),
                                   keystone.auth_host(), keystone.auth_port(),
                                   api_url)
     creds['auth_url'] = auth_url
-    return creds
+    unitdata.kv().set('keystonecreds', creds)
+    remove_state('os-service-checks.configured')
 
 
-# allow user to override credentials (and the need to be related to keystone)
+# allow user to override credentials (and the need to be related to ident)
 # with 'os-credentials'
 def get_credentials():
-    keystone_creds = config_flags_parser(config.get('os-credentials'))
-    if keystone_creds:
-        if '/v3' in keystone_creds['auth_url']:
-            creds = {
-                'credentials_username': keystone_creds['username'],
-                'credentials_password': keystone_creds['password'],
-                'credentials_project': keystone_creds['credentials_project'],
-                'region': keystone_creds['region_name'],
-                'auth_url': keystone_creds['auth_url'],
+    ident_creds = config_flags_parser(config.get('os-credentials'))
+    if ident_creds:
+        creds = {
+            'username': ident_creds['username'],
+            'password': ident_creds['password'],
+            'region': ident_creds['region_name'],
+            'auth_url': ident_creds['auth_url'],
+        }
+        if '/v3' in ident_creds['auth_url']:
+            creds.update({
+                'project_name': ident_creds['credentials_project'],
                 'auth_version': '3',
-                'credentials_user_domain': keystone_creds['domain'],
-                'credentials_project_domain': keystone_creds['domain'],
-            }
+                'user_domain_name': ident_creds['domain'],
+                'project_domain_name': ident_creds['domain'],
+            })
         else:
-            creds = {
-                'credentials_username': keystone_creds['username'],
-                'credentials_password': keystone_creds['password'],
-                'credentials_project': keystone_creds['credentials_project'],
-                'region': keystone_creds['region_name'],
-                'auth_url': keystone_creds['auth_url'],
-            }
+            creds['tenant_name'] = ident_creds['credentials_project']
     else:
         kv = unitdata.kv()
-        creds = kv.get('keystone-relation-creds')
+        creds = kv.get('keystonecreds')
+        old_creds = kv.get('keystone-relation-creds')
+        if old_creds and not creds:
+            # This set of creds needs an update to a newer format
+            creds['username'] = old_creds.pop('credentials_username')
+            creds['password'] = old_creds.pop('credentials_password')
+            creds['project_name'] = old_creds.pop('credentials_project')
+            creds['tenant_name'] = old_creds['project_name']
+            creds['user_domain_name'] = old_creds.pop('credentials_user_domain', None)
+            creds['project_domain_name'] = old_creds.pop('credentials_project_domain', None)
+            kv.set('keystonecreds', creds)
+            kv.update(creds, 'keystonecreds')
     return creds
 
 
 def render_checks():
     nrpe = NRPE()
-    plugins_dir = '/usr/local/lib/nagios/plugins/'
-    if not os.path.exists(plugins_dir):
-        os.makedirs(plugins_dir)
+    if not os.path.exists(PLUGINS_DIR):
+        os.makedirs(PLUGINS_DIR)
     charm_file_dir = os.path.join(hookenv.charm_dir(), 'files')
     charm_plugin_dir = os.path.join(charm_file_dir, 'plugins')
-
     host.rsync(
         charm_plugin_dir,
         '/usr/local/lib/nagios/',
@@ -133,9 +130,8 @@ def render_checks():
     crit = config.get("nova_crit")
     skip_disabled = config.get("skip-disabled")
     check_dns = config.get("check-dns")
-
-    check_command = plugins_dir + 'check_nova_services.py --warn ' \
-                                + str(warn) + ' --crit ' + str(crit)
+    nova_check_command = os.path.join(PLUGINS_DIR, 'check_nova_services.py')
+    check_command = '{} --warn {} --crit {}'.format(nova_check_command, warn, crit)
 
     if skip_disabled:
         check_command = check_command + ' --skip-disabled'
@@ -146,15 +142,22 @@ def render_checks():
 
     nrpe.add_check(shortname='neutron_agents',
                    description='Check that enabled Neutron agents are up',
-                   check_cmd=plugins_dir + 'check_neutron_agents.sh')
+                   check_cmd=os.path.join(PLUGINS_DIR, 'check_neutron_agents.sh'))
 
     if len(check_dns):
+        dns_check_cmd = "{} {}".format(
+            os.path.join(PLUGINS_DIR, 'check_dns_multi.sh'),
+            ' '.join(check_dns.split())
+        )
         nrpe.add_check(shortname='dns_multi',
                        description='Check DNS names are resolvable',
-                       check_cmd=plugins_dir + 'check_dns_multi.sh ' + ' '.join(check_dns.split()))
+                       check_cmd=dns_check_cmd)
     else:
         nrpe.remove_check(shortname='dns_multi')
 
+    endpoint_checks = create_endpoint_checks()
+    for check in endpoint_checks:
+        nrpe.add_check(**check)
     nrpe.write()
 
 
@@ -171,8 +174,8 @@ def render_config():
         hookenv.log('render_config: No credentials yet, skipping')
         return
     hookenv.log('render_config: Got credentials for username={}'.format(
-        creds['credentials_username']))
-    render('nagios.novarc', '/var/lib/nagios/nagios.novarc', creds,
+        creds['username']))
+    render('nagios.novarc', NOVARC, creds,
            owner='nagios', group='nagios')
     render_checks()
     if config.get('trusted_ssl_ca', None):
@@ -198,3 +201,94 @@ def fix_ssl():
     with open(cert_file, 'w') as f:
         print(cert_content, file=f)
     subprocess.call(["/usr/sbin/update-ca-certificates"])
+
+
+def create_endpoint_checks():
+    """
+    Read the Keystone catalog, and create a check for each endpoint listed.
+    If there is a healthcheck endpoint for the API, use that URL, otherwise check
+    the url '/'.
+    If SSL, add a check for the cert.
+    """
+    # provide URLs that can be used for healthcheck for some services
+    # This also provides a nasty hack-ish way to add switches if we need
+    # for some services.
+    health_urls = {
+        'keystone': '/healthcheck',
+        's3': '/healthcheck',
+        'swift': '/healthcheck',
+        'aodh': '/ -e Unauthorized -d x-openstack-request-id',
+        'cinderv3': '/v3 -e Unauthorized -d x-openstack-request-id',
+        'cinderv2': '/v2 -e Unauthorized -d x-openstack-request-id',
+        'cinderv1': '/v1 -e Unauthorized -d x-openstack-request-id',
+        'glance': '/healthcheck',
+        'nova': '/healthcheck',
+    }
+
+    creds = get_credentials()
+    keystone_client = get_keystone_client(creds)
+    endpoints = keystone_client.endpoints.list()
+    services = [x for x in keystone_client.services.list() if x.enabled]
+    nrpe_checks = []
+    for endpoint in endpoints:
+        endpoint.service_names = [x.name for x in services if x.id == endpoint.service_id]
+        service_name = endpoint.service_names[0]
+        endpoint.healthcheck_url = health_urls.get(service_name, '/')
+        if config.get('check_{}_urls'.format(endpoint.interface)):
+            cmd_params = ['/usr/lib/nagios/plugins/check_http ']
+            check_url = urlparse(endpoint.url)
+            host_port = check_url.netloc.split(':')
+            cmd_params.append('-H {} -p {}'.format(host_port[0], host_port[1]))
+            cmd_params.append('-u {}'.format(endpoint.healthcheck_url))
+            # if this is https, we want to add a check for cert expiry
+            # also need to tell check_http use use TLS
+            if check_url.scheme == 'https':
+                cmd_params.append('-S')
+                # Add an extra check for TLS cert expiry
+                cert_check = ['-C {},{}'.format(config.get('tls_warn_days'),
+                                                config.get('tls_crit_days'))]
+                nrpe_checks.append({
+                    'shortname': "{}_{}_cert".format(service_name,
+                                                     endpoint.interface),
+                    'description': 'Certificate expiry check for {} {}'.format(
+                        service_name,
+                        endpoint.interface),
+                    'check_cmd': ' '.join(cmd_params + cert_check)})
+            # Add the actual health check for the URL
+            nrpe_checks.append({
+                'shortname': "{}_{}".format(service_name,
+                                            endpoint.interface),
+                'description': 'Endpoint url check for {} {}'.format(
+                    service_name,
+                    endpoint.interface),
+                'check_cmd': (' '.join(cmd_params))})
+    return nrpe_checks
+
+
+def get_keystone_client(creds):
+    '''
+    Import the appropriate Keystone client depending on API version.
+    Return a client object.
+    '''
+    from keystoneclient import session
+    if int(creds['auth_version']) >= 3:
+        from keystoneclient.v3 import client
+        from keystoneclient.auth.identity import v3
+        auth_fields = ['auth_url', 'password', 'username', 'user_domain_name',
+                       'project_domain_name', 'project_name']
+        auth_creds = {}
+        for key in auth_fields:
+            auth_creds[key] = creds[key]
+        auth = v3.Password(**auth_creds)
+
+    else:
+        from keystoneclient.v2_0 import client
+        from keystoneclient.auth.identity import v2
+        auth_fields = ['auth_url', 'password', 'username', 'tenant_name']
+        auth_creds = {}
+        for key in auth_fields:
+            auth_creds[key] = creds[key]
+        auth = v2.Password(**auth_creds)
+
+    sess = session.Session(auth=auth)
+    return client.Client(session=sess)
diff --git a/templates/nagios.novarc b/templates/nagios.novarc
index e4c7e68..27f1c2a 100644
--- a/templates/nagios.novarc
+++ b/templates/nagios.novarc
@@ -1,7 +1,7 @@
-export OS_USERNAME={{ credentials_username }}
-export OS_TENANT_NAME={{ credentials_project }}
-export OS_PROJECT_NAME={{ credentials_project }}
-export OS_PASSWORD={{ credentials_password }}
+export OS_USERNAME={{ username }}
+export OS_TENANT_NAME={{ tenant_name }}
+export OS_PROJECT_NAME={{ project_name }}
+export OS_PASSWORD={{ password }}
 export OS_REGION_NAME={{ region }}
 export OS_AUTH_URL={{ auth_url }}
 {%- if cacert %}
@@ -13,6 +13,6 @@ export HOME=${SNAP_COMMON}
 {%- if auth_version %}
 export OS_IDENTITY_API_VERSION={{ auth_version }}
 export OS_AUTH_VERSION={{ auth_version }}
-export OS_USER_DOMAIN_NAME={{ credentials_user_domain }}
-export OS_PROJECT_DOMAIN_NAME={{ credentials_project_domain }}
+export OS_USER_DOMAIN_NAME={{ user_domain_name }}
+export OS_PROJECT_DOMAIN_NAME={{ project_domain_name }}
 {%- endif %}
diff --git a/test-requirements.txt b/test-requirements.txt
new file mode 100644
index 0000000..77ec3f5
--- /dev/null
+++ b/test-requirements.txt
@@ -0,0 +1,9 @@
+# Lint and unit test requirements
+flake8>=2.2.4,<=2.4.1
+os-testr>=0.4.1
+requests>=2.18.4
+charms.reactive
+mock>=1.2
+nose>=1.3.7
+coverage>=3.6
+netifaces
diff --git a/tox.ini b/tox.ini
index 0b8b27a..ba58ab5 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,12 +1,50 @@
 [tox]
 skipsdist=True
-envlist = py34, py35
+envlist = pep8, py34, py35, py36
 skip_missing_interpreters = True
 
 [testenv]
-commands = py.test -v
+setenv = VIRTUAL_ENV={envdir}
+         PYTHONHASHSEED=0
+         TERM=linux
+         LAYER_PATH={toxinidir}/layers
+         INTERFACE_PATH={toxinidir}/interfaces
+         JUJU_REPOSITORY={toxinidir}/build
+passenv = http_proxy https_proxy
+install_command =
+  pip install {opts} {packages}
 deps =
     -r{toxinidir}/requirements.txt
 
+[testenv:build]
+basepython = python3
+commands =
+    charm-build --log-level DEBUG -o {toxinidir}/build src {posargs}
+
+[testenv:py34]
+basepython = python3.4
+deps = -r{toxinidir}/test-requirements.txt
+commands = ostestr {posargs}
+
+[testenv:py35]
+basepython = python3.5
+deps = -r{toxinidir}/test-requirements.txt
+commands = ostestr {posargs}
+
+[testenv:py36]
+basepython = python3.6
+deps = -r{toxinidir}/test-requirements.txt
+commands = ostestr {posargs}
+
+[testenv:pep8]
+basepython = python3.6
+deps = -r{toxinidir}/test-requirements.txt
+commands = flake8 {posargs} --max-complexity=10 --max-line-length=120 reactive files unit_tests
+
+[testenv:venv]
+basepython = python3
+commands = {posargs}
+
 [flake8]
-exclude=docs
+# E402 ignore necessary for path append before sys module import in actions
+ignore = E402