Non-production environments invoke CBC Proxy during broadcast event creation by tlwr · Pull Request #3003 · alphagov/notifications-api

tlwr · 2020-10-20T11:03:16Z

What

When a broadcast is approved, and a new Broadcast Event is despatched with statusALERT we want to invoke the CBC Proxy which will generate a CAP XML message and send it to the CBC

Checklist

Plumbing up client to app
Celery task invokes CBC Proxy client when necessary
CBC Proxy client invokes AWS lambda function

Pair

@klssmith @CrystalPea

We are going to invoke a lambda to send a message to the CBC We need a CBC Proxy Client to do this The Client will be able to send/update/cancel broadcasts in the CBC Unless we have configured the app with AWS credentials for the CBCProxyClient, we just want to use a client that does nothing: the noop client The AWS access keys are separate for the CBC Proxy vs other Notify AWS things because the CBC Proxy lives in another AWS account Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk> Co-authored-by: Pea <pea.tyczynska@digital.cabinet-office.gov.uk> Co-authored-by: Katie <katie.smith@digital.cabinet-office.gov.uk>

When we create a broadcast message, we should invoke the cbc proxy to send a cap message Either a function will be invoked within AWS, or a noop function call is made, depending on the environment We have only implemented CB message creation in the CBC Proxy, without polygons, therefore we: * only invoke the CBC Proxy during message creation * only send description, identifier, and hard-coded headline Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk> Co-authored-by: Pea <pea.tyczynska@digital.cabinet-office.gov.uk> Co-authored-by: Katie <katie.smith@digital.cabinet-office.gov.uk>

it is global rather than local but python cannot infer this and we get UnboundLocalError Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>

Using correct: * key id * secret key * region Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk> Co-authored-by: Pea <pea.tyczynska@digital.cabinet-office.gov.uk> Co-authored-by: Katie <katie.smith@digital.cabinet-office.gov.uk>

_ld is better than _lambda because it causes primitive python syntax highlighting to not get confused _lambda is better than _ld because it is less jargon Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk> Co-authored-by: Pea <pea.tyczynska@digital.cabinet-office.gov.uk> Co-authored-by: Katie <katie.smith@digital.cabinet-office.gov.uk>

right now we are doing an end-to-end journey with a CBC from Notify (the CBE) and we would like to approve a broadcast in notify and have it appear on our test handset in order to do this, we: * hook up the lambda that we made in the correct VPC to cbc_proxy client * test that it is called correctly Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk> Co-authored-by: Pea <pea.tyczynska@digital.cabinet-office.gov.uk> Co-authored-by: Katie <katie.smith@digital.cabinet-office.gov.uk>

Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>

Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk> Co-authored-by: Katie <katie.smith@digital.cabinet-office.gov.uk>

idavidmcdonald

Nice. Generally looks pretty good and clear.

I think I've spotted two small things that do need changing please, the rest are more optional comments about structure and variable naming that you can implement as you see fit.

idavidmcdonald · 2020-10-22T10:45:17Z

+
+
+# Noop = no operation
+class CBCProxyNoopClient:


Optional but definitely worth considering: I don't know how complex these two classes are going to get but I think I'd suggest creating an abstract class to use as a base for both the CBCProxyClient and the CBCProxyNoopClient that will ensure they have the same methods always

Given we have no type checker, I am not sure what an abstract class would achieve. Additionally I cannot find any references in the codebase to abstract classes, with the exception of models and celery where __abstract__ and abstract respectively have specific meanings

idavidmcdonald · 2020-10-22T10:46:44Z

+        # identifier=broadcast_message.identifier,
+        # headline="GOV.UK Notify Broadcast",
+        # description=broadcast_message.description,
+        pass


Optional: I wonder if we should put info level logging on the noop methods so that if you ever call say create_and_send_broadcast we get a log like 'CBC no op client mthod create_and_send_broadcast was called with x, y z' for useful debugging

idavidmcdonald · 2020-10-22T10:47:54Z

+        # identifier=broadcast_message.identifier,
+        # headline="GOV.UK Notify Broadcast",
+        # description=broadcast_message.description,


Optional: Do we want to keep these? Personally, I'm not really that much clearer as someone who hasn't spent much time with the spec what an identifier, headline or description is to be honest by your examples.

In fact, is 'identifier' semantically different to an 'id'? I don't see an identifier property on the BroadcastMessageEvent class either? Should this argument be an broadcastMessageEvent ID? If so, I might suggest using a param name event_id rather than identifier.

The comments, as they were, are not useful. I have, in adc2ce8, moved them to the top of the file and attempted to explain why the variable names are why they are: ie they represent specific named fields within CAP

Cheers, that makes sense. I would wonder if we should abstract the CAP fields to inside the CBCProxy class such that someone calling the methods doesn't need to know what these things are. You don't need to necessarily understand what a CAP identifier is if you only need to know that you always need to provide a broadcast event ID every time you wish to send a broadcast event to a locality. I'll leave for you though, this is much better with the comments you've added, thank you.

idavidmcdonald · 2020-10-22T10:52:12Z

+    def init_app(self, app):
+        self._ld_client = boto3.client(
+            'lambda',
+            region_name='eu-west-2',


Optional: eu-west-2 could live in config rather than being hard coded

idavidmcdonald · 2020-10-22T11:04:08Z

+        pass
+
+    # We have not implementated updating a broadcast
+    def update_and_send_broadcast(


Optional:

I'm not too sure on the method names chosen: create_and_send_broadcast, update_and_send_broadcast and cancel_broadcast.

Each one of them ends up doing the same thing which is sending a broadcast event to the CBC proxy I believe? If so it would be more consistent to have create_and_send_broadcast_event, update_and_send_broadcast_event, cancel_and_send_broadcast_event but in that case I'd be tempted to keep it simpler as create_broadcast, update_broadcast, cancel_broadcast.

I'm not too sure on my suggestions though as my mental is still quite immature on the whole broadcast process so have a read and feel free to make a decision based on what you see fit :)

We started off with create_broadcast, update_broadcast, and cancel_broadcast

However I did not like create_broadcast and update_broadcast because it seems a bit like an object constructor rather than a thing-doer which is why the _and_send_ infix is present

Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>

for code clarity Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>

idavidmcdonald

Nice work, cheers for the edits too

Toby Lorne and others added 7 commits October 20, 2020 11:23

clients: explicitly declare cbc_proxy_client

efa6fb0

it is global rather than local but python cannot infer this and we get UnboundLocalError Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>

clients: cbc_proxy handles lambda invoke response

75de4ab

Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>

tlwr force-pushed the staging-preview-cbc branch from b79d704 to 62951fa Compare October 20, 2020 14:26

clients: cbc_proxy tests for handling lambda response

62951fa

Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>

tlwr force-pushed the staging-preview-cbc branch from 23b4642 to a3293d3 Compare October 20, 2020 15:59

manifest: add cbc proxy env vars

a3293d3

Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk> Co-authored-by: Katie <katie.smith@digital.cabinet-office.gov.uk>

tlwr changed the title ~~[work in progress] Non-production environments invoke CBC Proxy during broadcast event creation~~ Non-production environments invoke CBC Proxy during broadcast event creation Oct 20, 2020

idavidmcdonald suggested changes Oct 22, 2020

View reviewed changes

Toby Lorne added 4 commits October 22, 2020 12:11

config: correctly get cbc_proxy_client env vars

1badf93

Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>

clients: cbc_proxy has clarifying comments

adc2ce8

Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>

clients: cbc_proxy client tests remove unused stmt

c9eb9c8

Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>

clients: cbc_proxy lambda client is unabbreviated

ff1ffc7

for code clarity Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>

idavidmcdonald approved these changes Oct 22, 2020

View reviewed changes

idavidmcdonald merged commit 890a66d into master Oct 22, 2020

idavidmcdonald deleted the staging-preview-cbc branch October 22, 2020 15:21

Uh oh!

Conversation

tlwr commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Checklist

Pair

Uh oh!

idavidmcdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

idavidmcdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tlwr commented Oct 20, 2020 •

edited

Loading