Feature RT-66: SDS Client #22

Philip-Woods · 2019-06-28T12:37:30Z

This PR contains SDS Lookup implementation for RT-66. Primarily includes the following:

SDS Client: Handles LDAP lookup for accredited supplier details and MHS details of a supplier.
MHSAttributeLookup: Acts mainly as a placeholder wrapper for the SDS Client and what will be the caching of the SDS responses
Testing for the above implementations, also added an async test wrapper to the common folder allowing us to execute asynchronous tests easily

ghost

I appreciate there are quite a lot of comments here, but I've been fairly detailed with it being the first review, so we can make sure we're all on the same page in terms of approach. Generally, this looks good though!

One point I wanted to raise was regarding logging. I appreciate we've still got an outstanding task to define standards around levels, etc. and the required output format, but we should be trying to ensure that we include appropriate logging as we go, rather than having to include it afterwards. If you use the standard logging module with sensible log levels, it should mean it's relatively easy to convert to whatever format we settle on, and we won't have to actually add the log messages later. (CC @ismail-s) One good example here would be that I think we should log at warning level if we find more than one result for an SDS query - the spec says we shouldn't, and we can safely continue using the first result, but we should flag the fact that it's happened.

.gitignore

common/utilities/test_utilities.py

mhs-reference-implementation/mhs/SDSLookup/tests/test_sdsclient.py

mhs-reference-implementation/mhs/SDSLookup/tests/ldap_mocks.py

ghost · 2019-07-01T13:19:15Z

mhs-reference-implementation/mhs/SDSLookup/tests/test_sdsclient.py

+        result = await client._mhs_details_lookup(PARTY_KEY, SERVICE_ID)
+
+        self.assertIsNotNone(result)
+        self.assertEqual(result[0]['attributes']['nhsMHSEndPoint'], ['https://vpn-client-1411.opentest.hscic.gov.uk/'])


I think this should probably test that the full list of attributes is returned, since it's one of the acceptance criteria on the story. Also, we need to verify that attributes not on the list are filtered.

Updated the test to check for all attributes, however because of the mhs attribute call returning 2 sets of attributes if I check the values for an exact match it will fail half the time (due to responses including a unique id). I've included all the attributes bar the two unique ones and still checked that they are present, just not their values. Does this seem reasonable?

Also added length checking to the attribute list to ensure it contains exactly the number we need

I think this does the job, but isn't the MHS attribute call returning 2 sets of attributes because that's what we have in the dummy LDAP server data? Couldn't we edit it so that it doesn't return two sets for this query?

This is a good point, I've removed the duplicate data and added all attribute checks in so the test should be complete now.

mhs-reference-implementation/mhs/SDSLookup/tests/test_sdsclient.py

mhs-reference-implementation/mhs/SDSLookup/tests/test_sds_handler.py

This commit includes the code for the SDS lookup client. Including the following: - SDSClient: This handles LDAP calls to the directory - SDSHandler which handles the interaction with the SDSClient, in future this will consult a cache for SDS data - Test code which emulates the opentest directory to provide responses for unit tests - .gitignore for credentials

ismail-s

Bear in mind that I didn't look at Gareth's comments before doing my review.

ismail-s · 2019-06-28T13:55:35Z

mhs-reference-implementation/mhs/SDSLookup/tests/test_sdsclient.py

+    async def test_tls(self):
+        pass
+        # client = sds.SDSClient(DEV_ADDRESS)
+        # result = await client.get_mhs_details(ODS_CODE, SERVICE_ID)


Commented-out code

Removed in a previous commit

ismail-s · 2019-07-01T15:39:29Z

mhs-reference-implementation/Pipfile

@@ -9,6 +9,7 @@ verify_ssl = true
 integration-adaptors-common = {editable = true,path = "./../common"}
 requests = "*"
 tornado = "*"
+ldap3 = "*"


General question not specific to this PR, but should we be specifying versions (ie the major version)?

ismail-s · 2019-07-01T16:23:24Z

mhs-reference-implementation/mhs/routing/sds.py

+# These need replacing as the appropriate TLS certs are received for PTL
+PRIVATE_KEY = pathlib.Path(ROOT_DIR) / 'data' / 'certs' / 'client.key'
+LOCAL_CERT_FILE = pathlib.Path(ROOT_DIR) / 'data' / 'certs' / 'client.cert'
+CA_CERTS_FILE = pathlib.Path(ROOT_DIR) / 'data' / 'certs' / 'client.cert'


If this is the same as the previous constant and may change, then simpler to do:

Suggested change

CA_CERTS_FILE = pathlib.Path(ROOT_DIR) / 'data' / 'certs' / 'client.cert'

CA_CERTS_FILE = LOCAL_CERT_FILE

for the timebeing

Ultimately these will be different files, I'll update the paths to demonstrate this and I've added a constant holding the certs dir path

Removed these completely actually

ismail-s · 2019-07-02T08:25:57Z

common/utilities/test_utilities.py

+        coro = asyncio.coroutine(f)
+        future = coro(*args, **kwargs)
+        loop = asyncio.get_event_loop()
+        loop.run_until_complete(future)


It looks like asyncio.new_event_loop() would be better for test isolation.

And then it looks like asyncio.run(future) would be better than that.

I've replaced the loop = ... and loop.run_until_complete(future) with just asyncio.run(future). Is this the correct interpretation that we don't need asyncio.new_event_loop()?

ismail-s · 2019-07-02T08:31:48Z

mhs-reference-implementation/mhs/routing/sds.py

+                            f"ods: {ods_code} - interaction: {interaction_id}")
+        return details[0]['attributes']
+
+    async def _accredited_system_lookup(self, ods_code: str, interaction_id: str):


As a general comment (not sure that we have/want a rule around whether we want type hints everywhere or not), if you want to type hint the return type, then you can do async def blah() -> someType:

ismail-s · 2019-07-02T09:46:13Z

mhs-reference-implementation/mhs/routing/tests/test_sds_handler.py

+    'nhsMhsFQDN': 'vpn-client-1411.opentest.hscic.gov.uk',
+    'nhsMhsSvcIA': 'urn:nhs:names:services:psis:MCCI_IN010000UK13',
+    'nhsProductKey': '7374',
+    # 'uniqueIdentifier': ['S918999410559']


Commented-out code

ismail-s · 2019-07-02T10:02:23Z

mhs-reference-implementation/mhs/routing/tests/test_sds.py

+    'nhsMHSSyncReplyMode': 'MSHSignalsOnly',
+    'nhsMHsIN': 'MCCI_IN010000UK13',
+    'nhsMHsSN': 'urn:nhs:names:services:psis',
+    # 'nhsMhsCPAId': 'S918999410559', # Exclude this value since there seems to be two responses causing errors on this


What do you mean by this?

The sds call should return exactly 1 value for this lookup, however my opentest instance seems to return 2, this had been copied to the test data and caused an issue due to this value being an unique identifier. I've deleted the second instance in the test data so I've readded these attributes

ismail-s · 2019-07-02T10:02:33Z

mhs-reference-implementation/mhs/routing/tests/test_sds.py

+    'nhsMhsFQDN': 'vpn-client-1411.opentest.hscic.gov.uk',
+    'nhsMhsSvcIA': 'urn:nhs:names:services:psis:MCCI_IN010000UK13',
+    'nhsProductKey': '7374',
+    # 'uniqueIdentifier': ['S918999410559']


Commented-out code

ismail-s · 2019-07-02T10:09:07Z

mhs-reference-implementation/mhs/routing/tests/test_sds.py

+import mhs.routing.sds as sds
+import mhs.routing.tests.ldap_mocks as mocks
+from utilities.test_utilities import async_test
+import mhs.routing.routing_exception as re


Minor: re is the name of the regex stdlib module, so it's a little confusing to use that name.

Updated to just use the module name

ismail-s · 2019-07-02T10:10:24Z

mhs-reference-implementation/mhs/routing/tests/test_sds_handler.py

+
+
+class TestMHSAttributeLookupHandler(TestCase):
+    opentest_expected_reliability = {'nhsMHSAckRequested': 'always',


Is this variable being used?

Good point, it's no longer being used since we're using the full attribute set defined out of the class

mhs-reference-implementation/mhs/routing/sds.py

ghost · 2019-07-02T10:30:56Z

mhs-reference-implementation/mhs/routing/sds.py

+            raise re.RoutingException('No response from accredited system lookup')
+
+        if len(accredited_system_lookup) > 1:
+            logging.warning(f"More than one accredited system details returned on inputs: "


No issues with the log statements added, but we also need to think about how we would trace successful flows through the code, etc. Given log aggregation tools are pretty standard nowadays, our logging approach should be "more is better". As long as we make use of the correct log levels to distinguish between what we're logging, an admin using Splunk, etc. should be able to filter our log messages easily enough.

I've added the module reference Ismail suggested and added a couple extra logs around message IDs. Noted that we should try to consist logs throughout

mhs-reference-implementation/mhs/routing/tests/ldap_mocks.py

mhs-reference-implementation/mhs/routing/tests/test_sds.py

mhs-reference-implementation/mhs/SDSLookup/tests/test_sdsclient.py

ismail-s · 2019-07-02T13:55:26Z

mhs-reference-implementation/mhs/routing/sds.py

+            raise re.RoutingException('No response from accredited system lookup')
+
+        if len(accredited_system_lookup) > 1:
+            logging.warning(f"More than one accredited system details returned on inputs: "


I think we should start including logger = logging.getLogger(__name__) in files that we log in, so as to include the location of the file the log message came from. This is a standard line from the Python website.

Yes, agreed.

briandiggle · 2019-07-02T14:34:57Z

I'm noticing in the code that there is a mixture of terminology used for the interation ID. It is called "Service ID" in places, and "interaction ID" in others. Can we standardise on "interaction ID" as this is commonly accepted term.

ghost

Generally looks good, just a couple of comments on logging that I think we should address before this goes in.

mhs-reference-implementation/mhs/routing/tests/test_sds.py

mhs-reference-implementation/mhs/routing/sds.py

ismail-s · 2019-07-03T09:41:09Z

mhs-reference-implementation/mhs/routing/sds.py

+        self.connection = sds_connection
+        self.timeout = timeout
+
+    async def get_mhs_details(self, ods_code: str, interaction_id: str) -> Dict:


Another point: Dict can be made more specific like eg Dict[keyType, valueType]. As can other container types eg List[str]

Yep, I'd noticed this when looking at type hints, I refrained from using it here since the value type can be more than 1 type but I think in the future we should probably include the key/value types

In that case, you could use Any, or Union[someType, someOtherType], though that could get long-winded.

ismail-s · 2019-07-03T13:19:11Z

mhs-reference-implementation/mhs/routing/sds.py

        return response

-    async def _mhs_details_lookup(self, party_key: str, interaction_id: str):
+    async def _mhs_details_lookup(self, party_key: str, interaction_id: str) -> List:
        """
        Given a party key and a service, this will return an object containing the attributes of that party key,


I'm assuming interaction id and service mean the same thing (not familiar with all this terminology yet).

This is correct, I will change this as Brian has specified previously that we should try to stick to interaction id, must have missed this one!

ismail-s · 2019-07-03T13:21:39Z

mhs-reference-implementation/mhs/routing/sds_connection_factory.py

+
+def build_sds_connection(ldap_address: str) -> ldap3.Connection:
+    """
+    Given an ip address this will return a ldap3 connection object


Pedantic/minor: do you have to pass an ip address or any address?

Looking at the ldap lib it seems like any valid ldap address with be accepted, I'll update to reflect this

Philip-Woods changed the title ~~Feature/rt66 sds client~~ Feature RT-66: SDS Client Jun 28, 2019

Philip-Woods force-pushed the feature/RT66-SDS-Client branch 2 times, most recently from 2aa98c6 to 8bcb5e4 Compare July 1, 2019 09:41

ghost suggested changes Jul 1, 2019

View reviewed changes

Philip-Woods force-pushed the feature/RT66-SDS-Client branch from 8bcb5e4 to 0817497 Compare July 1, 2019 16:21

Philip-Woods force-pushed the feature/RT66-SDS-Client branch from 8070641 to 564ff81 Compare July 1, 2019 16:28

ismail-s suggested changes Jul 2, 2019

View reviewed changes

ghost reviewed Jul 2, 2019

View reviewed changes

ismail-s reviewed Jul 2, 2019

View reviewed changes

Philip Woods added 3 commits July 2, 2019 16:52

Review changes

7a856aa

Changed how sds response was returned

ee04c16

Updated documentation

55bc12c

ghost reviewed Jul 3, 2019

View reviewed changes

mhs-reference-implementation/mhs/routing/tests/test_sds.py Outdated Show resolved Hide resolved

mhs-reference-implementation/mhs/routing/sds.py Show resolved Hide resolved

mhs-reference-implementation/mhs/routing/sds.py Outdated Show resolved Hide resolved

Added additional logging statements

ca03773

ghost previously approved these changes Jul 3, 2019

View reviewed changes

ismail-s previously approved these changes Jul 3, 2019

View reviewed changes

Docs changes

66bd93b

Philip-Woods dismissed stale reviews from ismail-s and ghost via 66bd93b July 3, 2019 13:34

ismail-s approved these changes Jul 3, 2019

View reviewed changes

Philip-Woods merged commit 7876d4b into develop Jul 3, 2019

Philip-Woods deleted the feature/RT66-SDS-Client branch July 16, 2019 15:23

	CA_CERTS_FILE = pathlib.Path(ROOT_DIR) / 'data' / 'certs' / 'client.cert'
	CA_CERTS_FILE = LOCAL_CERT_FILE



		class TestMHSAttributeLookupHandler(TestCase):
		opentest_expected_reliability = {'nhsMHSAckRequested': 'always',

Feature RT-66: SDS Client #22

Feature RT-66: SDS Client #22

Conversation

Philip-Woods commented Jun 28, 2019 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismail-s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briandiggle commented Jul 2, 2019

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philip-Woods commented Jun 28, 2019 •

edited

Loading