Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global Alert Firehose #468

Merged
merged 4 commits into from
Nov 13, 2017
Merged

Global Alert Firehose #468

merged 4 commits into from
Nov 13, 2017

Conversation

jacknagz
Copy link
Contributor

to: @chunyong-lin
cc: @airbnb/streamalert-maintainers
size: medium
follow up: #457

Background

The Firehose to deliver alerts to S3 should not be cluster based, it should be global. See the tagged PR above for more context.

Changes

  • Introduce a new tf_stream_alert_globals module to house global configuration sets. The idea with this module is to be a placeholder for any other global resources we'll need in the future. We can eventually move resources into it as well.
  • Update the CLI to allow for adding new aws-firehose outputs.

Testing

Deployed and verified end-to-end in testing account

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.636% when pulling 1ec9881 on jacknaglieri-global-alert-firehose into 2839cab on master.

Copy link
Contributor

@chunyong-lin chunyong-lin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I only have one comment regarding TF code.
  • For changes in manage.py, I don't think it is necessary. I think you are trying to make manage.py shorter (number of lines). But I would prefer original format, breaking one line to multiple lines to have better readability. What you think?
    But generally, LGTM.

@@ -234,7 +234,7 @@ data "aws_iam_policy_document" "alert_processor_firehose" {
]

resources = [
"${aws_kinesis_firehose_delivery_stream.stream_alerts.arn}",
"arn:aws:firehose:${var.region}:${var.account_id}:deliverystream/${var.prefix}_streamalert_alert_delivery",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to use full arn of firehose delivery stream, instead of ${aws_kinesis_firehose_delivery_stream.stream_alerts.arn}?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That resource isn't in this module, so that interpolation isn't possible. I'd have to pass it in as a variable to this module instead, which I can do!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Thanks for the explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually going to leave it, since we do the same thing for Athena data streams

@jacknagz
Copy link
Contributor Author

@chunyong-lin we are moving towards using yapf to format all files in the repo, those changes were not made by hand.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.636% when pulling df1d9c7 on jacknaglieri-global-alert-firehose into 2839cab on master.

@jacknagz jacknagz merged commit dd1f870 into master Nov 13, 2017
@jacknagz jacknagz deleted the jacknaglieri-global-alert-firehose branch November 13, 2017 20:54
@jacknagz jacknagz added this to the 1.6.0 milestone Jan 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants