Bug Fixes (Initialization, AWS Perms, Config), CloudWatch Event Patterns, and More #205
Conversation
| @@ -352,6 +352,32 @@ def generate_cloudtrail(cluster_name, cluster_dict, config): | |||
| """ | |||
| modules = config['clusters'][cluster_name]['modules'] | |||
| cloudtrail_enabled = bool(modules['cloudtrail']['enabled']) | |||
| existing_trail_default = False | |||
| existing_trail = modules['cloudtrail'].get('existing_trail', existing_trail_default) | |||
| is_global_trail_default = True | |||
There was a problem hiding this comment.
Maybe I'm just not seeing it elsewhere, but it looks like existing_trail_default and is_global_trail_default are just booleans and only used to hold default values for the .get() calls. Can we just place the boolean value directly in the get for brevity?
There was a problem hiding this comment.
Correct, they're just booleans. I defined them as variables to be really explicit about what the defaults are for these options.
| } | ||
| if not set(event_pattern.keys()).issubset(valid_event_pattern_keys): | ||
| LOGGER_CLI.error('Invalid CloudWatch Event Pattern!') | ||
| sys.exit(1) |
There was a problem hiding this comment.
We should probably start returning a value in places like this (maybe just a simple bool == False, or a more comprehensive solution would be returning a non-zero error number) that we could check up the call stack and exit on error. It would avoid having sys.exit(1) calls scattered about the CLI and could allow for us to start using useful exit codes for certain errors (that are not 1). Not a necessary change now, but something to keep in mind.
There was a problem hiding this comment.
Agreed, I'll make some changes
| config['global']['account']['prefix']), | ||
| 'existing_trail': existing_trail, | ||
| 'is_global_trail': is_global_trail, | ||
| 'event_pattern': json.dumps(event_pattern) |
There was a problem hiding this comment.
Why do we want event_pattern to be a json string within a dictionary as opposed to just another python dict? I assume this has something to do with the config stuff below?
There was a problem hiding this comment.
|
PTAL @ryandeivert, tested all CLI commands by bringing up a test cluster, deploying, and destroying |
| @@ -303,6 +316,8 @@ def generate_kinesis(cluster_name, cluster_dict, config): | |||
| 's3_logging_bucket': logging_bucket | |||
| } | |||
|
|
|||
| return True | |||
There was a problem hiding this comment.
@jacknagz in a handful of these functions it looks like we return True and there's really no possibility of the function returning a falsey value. Am I overlooking something? If that's the case, there's not much benefit to having a check for the return value for these function calls.
That said, I do see a few functions below that can return False, so it definitely applies some places.
There was a problem hiding this comment.
I'm doing it for consistency, this could change in the future as we add more checks for validity
stream_alert_cli/runner.py
Outdated
| @@ -87,7 +89,7 @@ def terraform_check(): | |||
| prereqs_message = ('Terraform not found! Please install and add to ' | |||
| 'your $PATH:\n' | |||
| '\t$ export PATH=$PATH:/usr/local/terraform/bin') | |||
| run_command(['terraform', 'version'], | |||
| return run_command(['terraform', 'version'], | |||
There was a problem hiding this comment.
I think the formatting/indent here got messed up when the return was added. (meaning in the following 2 lines, not this one).
to: @ryandeivert (code) @mime-frame (docs)
cc: @airbnb/streamalert-maintainers @zbuc
size: medium
A combination of various bug fixes and feature additions.
Related Issues
Conf Changes
"$LATEST"as the AWS Lambdacurrent_version. This currently throws an error during the first deploy since the versions do not exist yet.force_destroyfor prerequisite buckets. This is makes destroying clusters easier, since this data is not of high value.CLI Changes
stream_alert_cli.py terraform cleancommand to simply remove any Terraform statefiles and other related files. This is useful to have while troubleshooting issues with initialization.inittarget list: This list of Terraform resources is used duringinitto create the prerequisite infrastructure. There was a resource naming issue here causing errors.flow_logsmodule for the log group name. Adds some unit tests too.Terraform Changes
tf_stream_alert_cloudtrailmodule options. This gives control over how users can configure CloudTrail to send to their StreamAlert cluster. For more information, see the module documentation.Documentation Changes
flow_logsandcloudtrailmodules