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

Consolidate StreamAlerts Buckets #224

Merged
merged 2 commits into from
Jul 13, 2017
Merged

Conversation

jacknagz
Copy link
Contributor

to @ryandeivert
cc @airbnb/streamalert-maintainers

Background

A current issue with per-cluster S3 Alert buckets is that a rule acting across multiple environments will throw a permissions error. The reason is that a given Lambda function will try to send to another cluster's bucket, and no permission exists to allow that.

Change

Move the creation of StreamAlerts buckets out of each cluster and into a single bucket per StreamAlert setup.

Also update unit tests accordingly.

Tested

Verified in a side account with two clusters. Alerts were sending to the same bucket from multiple Lambda functions.

Important!

Before pulling this update and migrating to a unified bucket scheme, perform the following:

  1. Copy all existing StreamAlerts to a temporary bucket.
  2. Empty bucket/clear out old alerts.
  3. Run python stream_alert_cli.py terraform build after pulling updates from this repo.
  4. S3 sync temp bucket to new unified bucket <prefix>.streamalerts.

Jack Naglieri added 2 commits July 13, 2017 11:08
@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.383% when pulling cd63e12 on jacknaglieri-single-alert-bucket into 424f6dd on master.

Copy link
Contributor

@ryandeivert ryandeivert left a comment

Choose a reason for hiding this comment

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

One comment but lgtm

input_mapping = {
'input_sns_topics': 'aws-sns'
}
for tf_key, input_key in input_mapping.iteritems():
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the for loop when there is only one item? Is there chance that this will be expanded in the future with more keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there chance that this will be expanded in the future with more keys?

Yep, that's the idea.

@jacknagz jacknagz merged commit c50856c into master Jul 13, 2017
@jacknagz jacknagz deleted the jacknaglieri-single-alert-bucket branch July 13, 2017 23:23
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