Move to k8s service names, exceptions

Issue-ID: DCAEGEN2-1537
Change-Id: Ibb7bd0233b5de9eb853f3516ba0907e148eae21d
Signed-off-by: Tommy Carpenter <tommy@research.att.com>

12 files changed, 248 insertions, 135 deletions

diff --git a/onap-dcae-cbs-docker-client/Changelog.md b/onap-dcae-cbs-docker-client/Changelog.md
index 354bc2a..d108e85 100644
--- a/onap-dcae-cbs-docker-client/Changelog.md
+++ b/onap-dcae-cbs-docker-client/Changelog.md
@@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
+## [2.0.0] - 6/19/2019
+* The env variable CONFIG_BINDING_SERVICE now has a different meaning per DCAEGEN2-1537. Specifically this variable now holds a resolvable hostname for the CBS, rather than a consul lookup key
+* Since the API was broken anyway, the decision not to throw an exception was revisted and overturned. This was causing problems for some users, who were getting `{}` back in their configuration, but without knowing why; either the config wasn't set up, the config was set but as `{}`, or the CBS being unreachable altogether. This client library now throws native python exceptions, rather than logging and returning `{}`. The application client code can handle the exceptions, and retry if they choose.
+* Add more tests, move fixtures to conftest
+* Move exceptions to own module
+
 ## [1.0.4]
 * Allow the CBS to be registered in Consul under a different name than "config_binding_service"; instead read from the already-set ENV variable CONFIG_BINDING_SERVICE
 
diff --git a/onap-dcae-cbs-docker-client/MANIFEST.in b/onap-dcae-cbs-docker-client/MANIFEST.in
deleted file mode 100644
index f9bd145..0000000
--- a/onap-dcae-cbs-docker-client/MANIFEST.in
+++ /dev/null
@@ -1 +0,0 @@
-include requirements.txt
diff --git a/onap-dcae-cbs-docker-client/README.md b/onap-dcae-cbs-docker-client/README.md
index 8466e50..5c5c986 100644
--- a/onap-dcae-cbs-docker-client/README.md
+++ b/onap-dcae-cbs-docker-client/README.md
@@ -1,21 +1,30 @@
 # Python CBS Docker Client
 
-Used for DCAE Dockerized microservices written in Python. Pulls your configuration from the config_binding_service. Expects that CONSUL_HOST, HOSTNAME, CONFIG_BINDING_SERVICE  are set as env variables, which is true in DCAE.
+Used for DCAE Dockerized microservices written in Python. Pulls your configuration from the Config Binding Service
 
 # Client Usage
 
-## Development outside of Docker
-To test your raw code without Docker, you will need to set the env variables CONSUL_HOST and HOSTNAME (name of your key to pull from) that are set in DCAEs Docker environment.
-1. `CONSUL_HOST` is the hostname only of the Consul instance you are talking to
-2. `HOSTNAME` is the name of your component in Consul
-3. `CONFIG_BINDING_SERVICE` is the name under which the CBS is registered in Consul
+The environment that this client runs in, whether it be in Docker or "natievely", needs to have the following env variables:
+1. `HOSTNAME` is the name of your component in Consul
+2. `CONFIG_BINDING_SERVICE` a resolvable hostname to the CBS
 
 ## Usage in your code
-```
->>> from onap_dcae_cbs_docker_client import client
->>> client.get_config()
->>> client.get_all()
-```
+
+    >>> from onap_dcae_cbs_docker_client import client
+    >>> client.get_config()
+    >>> client.get_all()
+
+
+If the CBS is reachable, but your configuration key is not there, you will get a CantGetConfig exception:
+
+    onap_dcae_cbs_docker_client.exceptions.CantGetConfig
+
+You can access the original HTTP status code and text via the `code` and `text` attributes.
+
+If the CBS is unreachable, you will get an exception:
+
+    onap_dcae_cbs_docker_client.exceptions.CBSUnreachable
+
 
 # Installation
 
@@ -28,3 +37,10 @@ pip install onap-dcae-cbs-docker-client
 ```
 tox
 ```
+
+# Version Changes
+When changes are made, the versions to be bumped are in:
+
+1. setup.py
+2. Changelog.md
+3. pom.xml
diff --git a/onap-dcae-cbs-docker-client/onap_dcae_cbs_docker_client/__init__.py b/onap-dcae-cbs-docker-client/onap_dcae_cbs_docker_client/__init__.py
index e9d0246..ce9debe 100644
--- a/onap-dcae-cbs-docker-client/onap_dcae_cbs_docker_client/__init__.py
+++ b/onap-dcae-cbs-docker-client/onap_dcae_cbs_docker_client/__init__.py
@@ -1,5 +1,5 @@
 # ================================================================================
-# Copyright (c) 2017-2018 AT&T Intellectual Property. All rights reserved.
+# Copyright (c) 2017-2019 AT&T Intellectual Property. All rights reserved.
 # ================================================================================
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -15,3 +15,17 @@
 # ============LICENSE_END=========================================================
 #
 # ECOMP is a trademark and service mark of AT&T Intellectual Property.
+import logging
+
+
+def get_module_logger(mod_name):
+    """
+    To use this, do logger = get_module_logger(__name__)
+    """
+    logger = logging.getLogger(mod_name)
+    handler = logging.StreamHandler()
+    formatter = logging.Formatter("%(asctime)s [%(name)-12s] %(levelname)-8s %(message)s")
+    handler.setFormatter(formatter)
+    logger.addHandler(handler)
+    logger.setLevel(logging.DEBUG)
+    return logger
diff --git a/onap-dcae-cbs-docker-client/onap_dcae_cbs_docker_client/client.py b/onap-dcae-cbs-docker-client/onap_dcae_cbs_docker_client/client.py
index 80c9f75..ef0dfbc 100644
--- a/onap-dcae-cbs-docker-client/onap_dcae_cbs_docker_client/client.py
+++ b/onap-dcae-cbs-docker-client/onap_dcae_cbs_docker_client/client.py
@@ -1,5 +1,5 @@
 # ================================================================================
-# Copyright (c) 2017-2018 AT&T Intellectual Property. All rights reserved.
+# Copyright (c) 2017-2019 AT&T Intellectual Property. All rights reserved.
 # ================================================================================
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -18,82 +18,53 @@
 
 import json
 import os
-import logging
 import requests
+from onap_dcae_cbs_docker_client import get_module_logger
+from onap_dcae_cbs_docker_client.exceptions import ENVsMissing, CantGetConfig, CBSUnreachable
 
-LOGGER = logging.getLogger().getChild(__name__)
 
-
-class ENVsMissing(Exception):
-    """
-    Exception to represent critical ENVs are missing
-    """
-    pass
+logger = get_module_logger(__name__)
 
 
 #########
 # HELPERS
-
-
-def _get_uri_from_consul(consul_url, name):
-    """
-    Call consul's catalog
-    TODO: currently assumes there is only one service with this hostname
-    """
-    url = "{0}/v1/catalog/service/{1}".format(consul_url, name)
-    LOGGER.debug("Trying to lookup service: {0}".format(url))
-    res = requests.get(url)
-    res.raise_for_status()
-    services = res.json()
-    return "http://{0}:{1}".format(services[0]["ServiceAddress"], services[0]["ServicePort"])
-
-
-def _get_envs():
+def _get_path(path):
     """
-    Returns hostname, consul_host.
-    If the necessary ENVs are not found, this is fatal, and raises an exception.
+    Try to get the config, and return appropriate exceptions otherwise
     """
     try:
         hostname = os.environ["HOSTNAME"]  # this is the name of the component itself
-        consul_host = os.environ["CONSUL_HOST"]  # this is the host of consul itself
-        cbs_name = os.environ["CONFIG_BINDING_SERVICE"]  # this is the name under which the CBS is registered in Consul.
+        # in most cases, this is the K8s service name which is a resolvable DNS name
+        # if running outside k8s, this name needs to be resolvable by DNS via other means.
+        cbs_resolvable_hostname = os.environ["CONFIG_BINDING_SERVICE"]
     except KeyError as e:
         raise ENVsMissing("Required ENV Variable {0} missing".format(e))
-    return hostname, consul_host, cbs_name
 
+    # TODO: https
+    cbs_url = "http://{0}:10000".format(cbs_resolvable_hostname)
 
-def _get_path(path):
-    """
-    This call does not raise an exception if Consul or the CBS cannot complete the request.
-    It logs an error and returns {} if the config is not bindable.
-    It could be a temporary network outage. Call me again later.
-
-    It will raise an exception if the necessary env parameters were not set because that is irrecoverable.
-    This function is called in my /heatlhcheck, so this will be caught early.
-    """
-
-    config = {}
-
-    hostname, consul_host, cbs_name = _get_envs()
-
-    # not sure how I as the component developer is supposed to know consul port
-    consul_url = "http://{0}:8500".format(consul_host)
-
+    # get my config
     try:
-        # get my config
-        cbs_url = _get_uri_from_consul(consul_url, cbs_name)
         my_config_endpoint = "{0}/{1}/{2}".format(cbs_url, path, hostname)
         res = requests.get(my_config_endpoint)
-
         res.raise_for_status()
         config = res.json()
-        LOGGER.info("get_config returned the following configuration: {0}".format(
-            json.dumps(config)))
-    except requests.exceptions.HTTPError:
-        LOGGER.error("in get_config, the config binding service endpoint %s was not reachable. Error code: %d, Error text: %s", my_config_endpoint, res.status_code, res.text)
-    except Exception as exc:
-        LOGGER.exception(exc)
-    return config
+        logger.debug(
+            "get_config returned the following configuration: %s using the config url %s",
+            json.dumps(config),
+            my_config_endpoint,
+        )
+        return config
+    except requests.exceptions.HTTPError:  # this is thrown by raise_for_status
+        logger.error(
+            "The config binding service endpoint %s returned a bad status. code: %d, text: %s",
+            my_config_endpoint,
+            res.status_code,
+            res.text,
+        )
+        raise CantGetConfig(res.status_code, res.text)
+    except requests.exceptions.ConnectionError:  # this is thrown if requests.get cant even connect to the endpoint
+        raise CBSUnreachable()
 
 
 #########
@@ -108,5 +79,8 @@ def get_all():
 def get_config():
     """
     Hit the CBS service_component endpoint
+
+    TODO: should we take in a "retry" boolean, and retry on behalf of the caller?
+    Currently, we return an exception and let the application decide how it wants to proceed (Crash, try again, etc).
     """
     return _get_path("service_component")
diff --git a/onap-dcae-cbs-docker-client/onap_dcae_cbs_docker_client/exceptions.py b/onap-dcae-cbs-docker-client/onap_dcae_cbs_docker_client/exceptions.py
new file mode 100644
index 0000000..6b73b91
--- /dev/null
+++ b/onap-dcae-cbs-docker-client/onap_dcae_cbs_docker_client/exceptions.py
@@ -0,0 +1,45 @@
+# ================================================================================
+# Copyright (c) 2019 AT&T Intellectual Property. All rights reserved.
+# ================================================================================
+# 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.
+# ============LICENSE_END=========================================================
+#
+# ECOMP is a trademark and service mark of AT&T Intellectual Property.
+
+
+class ENVsMissing(Exception):
+    """
+    Exception to represent critical ENVs are missing
+    """
+
+    pass
+
+
+class CantGetConfig(Exception):
+    """
+    Configuration could not be fetched likely due to the config not being in Consul
+    """
+
+    def __init__(self, code, text):
+        self.code = code
+        self.text = text
+
+    pass
+
+
+class CBSUnreachable(Exception):
+    """
+    CBS was not reachable at all
+    """
+
+    pass
diff --git a/onap-dcae-cbs-docker-client/pom.xml b/onap-dcae-cbs-docker-client/pom.xml
index 0a2c763..f76b872 100644
--- a/onap-dcae-cbs-docker-client/pom.xml
+++ b/onap-dcae-cbs-docker-client/pom.xml
@@ -28,7 +28,7 @@ ECOMP is a trademark and service mark of AT&T Intellectual Property.
   <groupId>org.onap.dcaegen2.utils</groupId>
   <artifactId>onap-dcae-cbs-docker-client</artifactId>
   <name>dcaegen2-utils-python-cbs-docker-client</name>
-  <version>1.0.4-SNAPSHOT</version>
+  <version>2.0.0-SNAPSHOT</version>
   <url>http://maven.apache.org</url>
 
   <properties>
diff --git a/onap-dcae-cbs-docker-client/setup.py b/onap-dcae-cbs-docker-client/setup.py
index 3d4467c..2f04827 100644
--- a/onap-dcae-cbs-docker-client/setup.py
+++ b/onap-dcae-cbs-docker-client/setup.py
@@ -19,14 +19,12 @@ from setuptools import setup, find_packages
 setup(
     name="onap_dcae_cbs_docker_client",
     description="very lightweight client for a DCAE dockerized component to get it's config from the CBS",
-    version="1.0.4",
+    version="2.0.0",
     packages=find_packages(),
     author="Tommy Carpenter",
     author_email="tommy@research.att.com",
-    license='Apache 2',
+    license="Apache 2",
     keywords="",
     url="",
-    install_requires=[
-        "requests>= 2.0.0, < 3.0.0"
-    ]
+    install_requires=["requests>= 2.0.0, < 3.0.0"],
 )
diff --git a/onap-dcae-cbs-docker-client/tests/conftest.py b/onap-dcae-cbs-docker-client/tests/conftest.py
new file mode 100644
index 0000000..05d2eb5
--- /dev/null
+++ b/onap-dcae-cbs-docker-client/tests/conftest.py
@@ -0,0 +1,83 @@
+# ================================================================================
+# Copyright (c) 2019 AT&T Intellectual Property. All rights reserved.
+# ================================================================================
+# 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.
+# ============LICENSE_END=========================================================
+#
+# ECOMP is a trademark and service mark of AT&T Intellectual Property.
+import pytest
+from requests.exceptions import HTTPError, ConnectionError
+
+
+class FakeResponse:
+    def __init__(self, status_code, thejson):
+        self.status_code = status_code
+        self.thejson = thejson
+        self.text = ""
+
+    def raise_for_status(self):
+        if self.status_code > 299:
+            raise HTTPError
+
+    def json(self):
+        return self.thejson
+
+
+@pytest.fixture
+def monkeyed_requests_get():
+    """
+    mock for the CBS get
+    """
+
+    def _monkeyed_requests_get(url):
+        if url == "http://config_binding_service:10000/service_component_all/mybestfrienddotcom":
+            return FakeResponse(
+                status_code=200,
+                thejson={
+                    "config": {"key_to_your_heart": 666},
+                    "dti": {"some amazing": "dti stuff"},
+                    "policies": {"event": {"foo": "bar"}, "items": [{"foo2": "bar2"}]},
+                    "otherkey": {"foo3": "bar3"},
+                },
+            )
+
+        elif url == "http://config_binding_service:10000/service_component/mybestfrienddotcom":
+            return FakeResponse(status_code=200, thejson={"key_to_your_heart": 666})
+        else:
+            raise Exception("Unexpected URL {0}!".format(url))
+
+    return _monkeyed_requests_get
+
+
+@pytest.fixture
+def monkeyed_requests_get_404():
+    def _monkeyed_requests_get_404(url):
+        """
+        get that pretends that key doesnt exist
+        """
+        if url in [
+            "http://config_binding_service:10000/service_component_all/mybestfrienddotcom",
+            "http://config_binding_service:10000/service_component/mybestfrienddotcom",
+        ]:
+            return FakeResponse(status_code=404, thejson={})
+        raise Exception("Unexpected URL {0}!".format(url))
+
+    return _monkeyed_requests_get_404
+
+
+@pytest.fixture
+def monkeyed_requests_get_unreachable():
+    def _monkeyed_requests_get_unreachable(_url):
+        raise ConnectionError()
+
+    return _monkeyed_requests_get_unreachable
diff --git a/onap-dcae-cbs-docker-client/tests/test_client.py b/onap-dcae-cbs-docker-client/tests/test_client.py
index c87f573..b1589c2 100644
--- a/onap-dcae-cbs-docker-client/tests/test_client.py
+++ b/onap-dcae-cbs-docker-client/tests/test_client.py
@@ -13,50 +13,51 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 # ============LICENSE_END=========================================================
-
+import pytest
 from onap_dcae_cbs_docker_client.client import get_config, get_all
+from onap_dcae_cbs_docker_client.exceptions import CantGetConfig, CBSUnreachable, ENVsMissing
 
 
-class FakeResponse:
-    def __init__(self, status_code, thejson):
-        self.status_code = status_code
-        self.thejson = thejson
+def test_config(monkeypatch, monkeyed_requests_get):
+    monkeypatch.setattr("requests.get", monkeyed_requests_get)
+    assert get_config() == {"key_to_your_heart": 666}
 
-    def raise_for_status(self):
-        pass
 
-    def json(self):
-        return self.thejson
+def test_all(monkeypatch, monkeyed_requests_get):
+    monkeypatch.setattr("requests.get", monkeyed_requests_get)
+    assert get_all() == {
+        "config": {"key_to_your_heart": 666},
+        "dti": {"some amazing": "dti stuff"},
+        "policies": {"event": {"foo": "bar"}, "items": [{"foo2": "bar2"}]},
+        "otherkey": {"foo3": "bar3"},
+    }
 
 
-def monkeyed_requests_get(url):
-    # mock all the get calls for existent and non-existent
-    if url == "http://consuldotcom:8500/v1/catalog/service/config_binding_service":
-        return FakeResponse(status_code=200,
-                            thejson=[{"ServiceAddress": "666.666.666.666",
-                                      "ServicePort": 8888}])
-    elif url == "http://666.666.666.666:8888/service_component_all/mybestfrienddotcom":
-        return FakeResponse(status_code=200,
-                            thejson={"config": {"key_to_your_heart": 666},
-                                     "dti": {"some amazing": "dti stuff"},
-                                     "policies": {"event": {"foo": "bar"},
-                                                  "items": [{"foo2": "bar2"}]},
-                                     "otherkey": {"foo3": "bar3"}})
+def test_bad_hostname(monkeypatch, monkeyed_requests_get_404):
+    monkeypatch.setattr("requests.get", monkeyed_requests_get_404)
+    with pytest.raises(CantGetConfig):
+        get_config()
+    with pytest.raises(CantGetConfig):
+        get_all()
 
-    elif url == "http://666.666.666.666:8888/service_component/mybestfrienddotcom":
-        return FakeResponse(status_code=200,
-                            thejson={"key_to_your_heart": 666})
+    try:
+        get_config()
+    except CantGetConfig as e:
+        assert e.code == 404
+        assert e.text == ""
 
 
-def test_config(monkeypatch):
-    monkeypatch.setattr('requests.get', monkeyed_requests_get)
-    assert(get_config() == {"key_to_your_heart": 666})
+def test_unreachable(monkeypatch, monkeyed_requests_get_unreachable):
+    monkeypatch.setattr("requests.get", monkeyed_requests_get_unreachable)
+    with pytest.raises(CBSUnreachable):
+        get_config()
+    with pytest.raises(CBSUnreachable):
+        get_all()
 
 
-def test_all(monkeypatch):
-    monkeypatch.setattr('requests.get', monkeyed_requests_get)
-    assert(get_all() == {"config": {"key_to_your_heart": 666},
-                         "dti": {"some amazing": "dti stuff"},
-                         "policies": {"event": {"foo": "bar"},
-                                      "items": [{"foo2": "bar2"}]},
-                         "otherkey": {"foo3": "bar3"}})
+def test_badenv(monkeypatch):
+    monkeypatch.delenv("CONFIG_BINDING_SERVICE")
+    with pytest.raises(ENVsMissing):
+        get_config()
+    with pytest.raises(ENVsMissing):
+        get_all()
diff --git a/onap-dcae-cbs-docker-client/tox-local.ini b/onap-dcae-cbs-docker-client/tox-local.ini
deleted file mode 100644
index 7faf386..0000000
--- a/onap-dcae-cbs-docker-client/tox-local.ini
+++ /dev/null
@@ -1,22 +0,0 @@
-[tox]
-envlist = py27,py36,flake8
-
-[testenv]
-deps=
-    pytest
-    coverage
-    pytest-cov
-setenv =
-    CONSUL_HOST = consuldotcom
-    HOSTNAME = mybestfrienddotcom
-    CONFIG_BINDING_SERVICE = config_binding_service
-commands=pytest --cov {envsitepackagesdir}/onap_dcae_cbs_docker_client  --cov-report html
-
-[testenv:flake8]
-basepython = python3.6
-skip_install = true
-deps = flake8
-commands = flake8 setup.py onap_dcae_cbs_docker_client tests
-
-[flake8]
-ignore = E501
diff --git a/onap-dcae-cbs-docker-client/tox.ini b/onap-dcae-cbs-docker-client/tox.ini
index a8c9272..e5eeb43 100644
--- a/onap-dcae-cbs-docker-client/tox.ini
+++ b/onap-dcae-cbs-docker-client/tox.ini
@@ -1,6 +1,6 @@
 # content of: tox.ini , put in same dir as setup.py
 [tox]
-envlist = py27,py36,flake8
+envlist = py36,flake8
 
 [testenv]
 deps=
@@ -8,7 +8,6 @@ deps=
     coverage
     pytest-cov
 setenv =
-    CONSUL_HOST = consuldotcom
     HOSTNAME = mybestfrienddotcom
     CONFIG_BINDING_SERVICE = config_binding_service
     PYTHONPATH={toxinidir}