[core] drop sns publish for lambda invoke #225

ryandeivert · 2017-07-14T02:50:08Z

to @austinbyers
cc @jacknagz , @airbnb/dashed-maintainers
size: large
resolves: #189

Why

We have been noticing an increased number of SNS message failures in cloudwatch, which means potentially meaningful alerts are being lost in transit between the Rule Processor and the Alert Processor. We currently have no insight into why these failure occur.
Switching to lambda, with an asynchronous invocation, provides the benefit of automatic retry logic (see: here). The invocation will now retry twice if there was a failure.
This greatly simplifies how the Rule Processor and Alert Processor interact, removing a good amount of previously necessary logic.

Changes in collab branch with @jacknagz

Migrating to using direct lambda invocation of the alert processor function from the rule processor function.
This is an async invocation ('Event' type) and sends the payload directly to the alert processor lambda (no 'sns' formatting required).
The alert processor now receives the payload directly in the event parameter of the lambda handler.
Updating the alert processor to perform some input validation of the alert before processing occurs. This should avoid errors due to a malformed payload.
- Specific errors indicating what is wrong with the input payload will be logged as errors, and the alert payload itself will be reported after all other errors..
Updating all unit tests and add new ones.
Updating integration tests to conform to other updates.
Thanks to @jacknagz for the necessary terraform, terraform generation, and doc updates. (please fill in details here as you see fit..).

Fixes

Resolving bug where importing pip as a module broke the CLI logging. The fix is in this commit and is described here.

To consider:

Setting up a DLQ (Dead Letter Queue) for our lambda functions that would give us insight into any lambda invocation failures in the future.
Checking the payload size being sent in the Alert Processor lambda invocation, and segmenting messages if it exceeds the limit (see: here).

…or of direct lambda invocation to the alert processor

…changes

…sts included

…tions

* Optionally create an SNS topic for receiving metric alarms * Optionall accept an existing SNS topic to receive metric alarms * Update documentation on usage * Unit tests

…lways go to $LATEST lambda

coveralls · 2017-07-14T02:51:52Z

Coverage increased (+3.5%) to 82.922% when pulling 3485350 on ryandeivert-drop-sns-publish-for-lambda-invoke into c50856c on master.

jacknagz

A couple inline comments

jacknagz · 2017-07-14T03:08:45Z

stream_alert/rule_processor/sink.py

+                                 data)
+                return
+
+            if response['ResponseMetadata']['HTTPStatusCode'] != 202:


Is this block slightly repetitive to the preceding one?

Can you be more specific? What's repetitive about it (and are you referring to the try/except above?)?

If so, I think an HTTP response of non-202 could possibly occur even if there is no ClientError occurred during invocation. I could also be off on that.. We could also add FunctionError to this error logging.

jacknagz · 2017-07-14T03:09:57Z

stream_alert/alert_processor/main.py

-            continue
-
-        status_values.extend(run(loaded_sns_message, region, function_name, config))
+    # Yield back the current status to the caller


Can this comment be a little more descriptive?

jacknagz · 2017-07-14T03:11:06Z

stream_alert/alert_processor/main.py

                }
            }
-        }

        region [string]: the AWS region being used


let's update this to: The AWS region of the currently executing Lambda function

austinbyers

This will be awesome!

austinbyers · 2017-07-14T16:44:15Z

stream_alert/alert_processor/helpers.py

+    if not set(alert['metadata'].keys()) == metadata_keys:
+        LOGGER.error('The value of the \'metadata\' key must be a map (dict) '
+                     'that contains the following keys: %s',
+                     ', '.join('\'{}\''.format(key) for key in metadata_keys))


All of these comparisons in this file test for set equality (the keys must match the hard-coded set), but the errors messages say it must contain such and such keys.

Just to mitigate potential confusion, can we either update the error messages to indicate it must contain exactly the required keys, or else update the tests to check for a set inclusion instead of equality?

@austinbyers making the logger messages indication that only those keys should be included. would rather have a strict validation of input instead of a loose validation.

austinbyers · 2017-07-14T16:50:49Z

stream_alert/alert_processor/helpers.py

+                    LOGGER.error('The value of the \'%s\' key within \'%s\' must be '
+                                 'a string (str).', entry, key)
+                    valid = False
+                    continue


This is redundant (at the end of the inner for loop)

thanks! removed.

austinbyers · 2017-07-14T16:50:59Z

stream_alert/alert_processor/helpers.py

+                    LOGGER.error('The value of each entry in the \'outputs\' list '
+                                 'must be a string (str).')
+                    valid = False
+                    continue


thanks! removed.

austinbyers · 2017-07-14T16:51:09Z

stream_alert/alert_processor/helpers.py

+            LOGGER.error('The value of the \'%s\' key must be a string (str), not %s',
+                         key, type(alert['metadata'][key]))
+            valid = False
+            continue


Technically redundant, but if you want to keep this one to be consistent with all the other checks, that's fine

thanks! removed for brevity.

austinbyers · 2017-07-14T16:53:20Z

stream_alert/alert_processor/main.py

-            LOGGER.error('An error occurred while decoding message to JSON: %s', err)
-            continue
+    # Return the current list of statuses back to the caller
+    return [status for status in run(event, region, function_name, config)]


Would return list(run(event, region, function_name, config)) do the trick?

same effect yes. I'll update with explicit list casting.

austinbyers · 2017-07-14T16:58:19Z

terraform/modules/tf_stream_alert/sns.tf

 resource "aws_sns_topic_subscription" "input_topic_subscriptions" {
  count     = "${length(var.input_sns_topics)}"
  topic_arn = "${element(var.input_sns_topics, count.index)}"
  endpoint  = "${aws_lambda_function.streamalert_rule_processor.arn}:production"
  protocol  = "lambda"
 }
+
+// Lambda Permission: Allow SNS to invoke the Rule Processor


I'm confused - how did we not have this before? SNS is currently alerting the rule processor just fine...

We had it, it just moved into this file from here

austinbyers · 2017-07-14T17:00:03Z

test/unit/stream_alert_alert_processor/test_helpers.py

+    invalid_metadata_non_string['metadata']['type'] = 4.5
+
+    # Test with invalid metadata non-string value
+    assert_false(validate_alert(invalid_metadata_non_string))


Can we break each of these up into smaller unit tests? Each assert condition with a comment should be its own self-contained test.

austinbyers · 2017-07-14T17:20:07Z

stream_alert/rule_processor/sink.py

 import json
 import logging
 import sys

 import boto3
 from botocore.exceptions import ClientError

-_SNS_MAX_SIZE = (256*1024)


There is still a maximum Lambda invocation size (6MB for asynchronous requests), but I don't think we'll be anywhere close to that limit. Just something to keep in mind

Adding a comment to the PR body for a future consideration.

ryandeivert · 2017-07-17T20:15:43Z

@jacknagz @austinbyers PTAL at new commit and comments

coveralls · 2017-07-17T20:18:18Z

Coverage increased (+3.5%) to 82.873% when pulling 21cb539 on ryandeivert-drop-sns-publish-for-lambda-invoke into c50856c on master.

austinbyers

Thanks for doing this!

austinbyers · 2017-07-17T20:52:11Z

stream_alert/alert_processor/main.py

-            LOGGER.error('An error occurred while decoding message to JSON: %s', err)
-            continue
+    # Return the current list of statuses back to the caller
+    return list(status for status in run(event, region, function_name, config))


You can simplify this further:

return list(run(event, region, function_name, config))

list will automatically transform the generator into a list by iterating through it; you don't have to manually iterate

ah sorry overlooked that bit.

austinbyers · 2017-07-17T20:53:58Z

terraform/modules/tf_stream_alert/sns.tf

 resource "aws_sns_topic_subscription" "input_topic_subscriptions" {
  count     = "${length(var.input_sns_topics)}"
  topic_arn = "${element(var.input_sns_topics, count.index)}"
  endpoint  = "${aws_lambda_function.streamalert_rule_processor.arn}:production"
  protocol  = "lambda"
 }
+
+// Lambda Permission: Allow SNS to invoke the Rule Processor


austinbyers · 2017-07-17T20:57:57Z

test/unit/stream_alert_rule_processor/test_sink.py

+            '"metadata": {"source": {"service": "payload_service_01", ' \
+            '"entity": "payload_entity_01"}, "rule_name": "test_rule_01", ' \
+            '"type": "payload_type_01", "log": "payload_data_01", ' \
+            '"outputs": "rule.outputs_01"}}')


You don't need the line continuation markers (\) (this is discouraged by Google Python style guide); Python has implicit string joining.

not in py 2 :) you need to wrap in parenths for that to be effective, but I can add that

Oh yes, you always need the parens (in both py2 and py3). Good catch

@austinbyers fixed in latest commit.. as a param in a function call it looks like it doesn't need to be encapsulated

coveralls · 2017-07-17T23:44:17Z

Coverage increased (+3.5%) to 82.873% when pulling bf8433e on ryandeivert-drop-sns-publish-for-lambda-invoke into c50856c on master.

ryandeivert and others added 11 commits July 13, 2017 18:48

[lambda][rule] removing sns dependency from the rule processor in fav…

16c624b

…or of direct lambda invocation to the alert processor

[lambda][alert] updating alert processor side for rule processor sns …

bc59d64

…changes

[tests] updating all tests to go with previous changes

3d352e6

[lambda][alert] rebasing and addition of helper to validate alert. te…

33694f6

…sts included

[tests] adding unit tests to cover new code

bfd7111

[tf] stream_alert module changes for direct invocation of lambda func…

90f0739

…tions

[modules] lower the threshold on iterator age alarm

252b5ea

[tf] SNS topic management for cloudwatch alarms

dd79093

* Optionally create an SNS topic for receiving metric alarms * Optionall accept an existing SNS topic to receive metric alarms * Update documentation on usage * Unit tests

[tf] fixing a few terraform bugs

d71981a

[lambda][alert] fixing bug in alert processor that caused alerts to a…

b48c3a6

…lways go to $LATEST lambda

[docs] fixing error in json blocks that failed build

3485350

ryandeivert assigned austinbyers and jacknagz Jul 14, 2017

ryandeivert added documentation improvement labels Jul 14, 2017

ryandeivert added this to the 1.5.0 milestone Jul 14, 2017

jacknagz reviewed Jul 14, 2017

View reviewed changes

austinbyers reviewed Jul 14, 2017

View reviewed changes

[pr] feedback from @jacknagz & @austinbyers

21cb539

austinbyers approved these changes Jul 17, 2017

View reviewed changes

[pr] second round of feedback

5c54db5

ryandeivert force-pushed the ryandeivert-drop-sns-publish-for-lambda-invoke branch from d0de948 to 5c54db5 Compare July 17, 2017 21:17

airbnb deleted a comment from coveralls Jul 17, 2017

[package] fixing issue where pip messes up logger

bf8433e

airbnb deleted a comment from coveralls Jul 17, 2017

ryandeivert merged commit 51ea4c1 into master Jul 18, 2017

ryandeivert deleted the ryandeivert-drop-sns-publish-for-lambda-invoke branch July 18, 2017 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] drop sns publish for lambda invoke #225

[core] drop sns publish for lambda invoke #225

ryandeivert commented Jul 14, 2017 •

edited

Loading

coveralls commented Jul 14, 2017

jacknagz left a comment

jacknagz Jul 14, 2017

ryandeivert Jul 14, 2017

jacknagz Jul 14, 2017

jacknagz Jul 14, 2017

austinbyers left a comment

austinbyers Jul 14, 2017

ryandeivert Jul 17, 2017 •

edited

Loading

austinbyers Jul 14, 2017

ryandeivert Jul 17, 2017

austinbyers Jul 14, 2017

ryandeivert Jul 17, 2017

austinbyers Jul 14, 2017

ryandeivert Jul 17, 2017

austinbyers Jul 14, 2017

ryandeivert Jul 17, 2017

austinbyers Jul 14, 2017

jacknagz Jul 14, 2017

austinbyers Jul 17, 2017

austinbyers Jul 14, 2017 •

edited

Loading

austinbyers Jul 14, 2017

ryandeivert Jul 17, 2017

ryandeivert commented Jul 17, 2017

coveralls commented Jul 17, 2017

austinbyers left a comment

austinbyers Jul 17, 2017

ryandeivert Jul 17, 2017

austinbyers Jul 17, 2017

austinbyers Jul 17, 2017

ryandeivert Jul 17, 2017

austinbyers Jul 17, 2017

ryandeivert Jul 17, 2017

coveralls commented Jul 17, 2017

[core] drop sns publish for lambda invoke #225

[core] drop sns publish for lambda invoke #225

Conversation

ryandeivert commented Jul 14, 2017 • edited Loading

Why

Changes in collab branch with @jacknagz

Fixes

To consider:

coveralls commented Jul 14, 2017

jacknagz 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

austinbyers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryandeivert Jul 17, 2017 • edited Loading

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

austinbyers Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryandeivert commented Jul 17, 2017

coveralls commented Jul 17, 2017

austinbyers 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

coveralls commented Jul 17, 2017

ryandeivert commented Jul 14, 2017 •

edited

Loading

ryandeivert Jul 17, 2017 •

edited

Loading

austinbyers Jul 14, 2017 •

edited

Loading