[core] Moved the secret_store from S3 to SSM #1123
Conversation
[testing] Updated unit_tests to use SSM instead of S3 [terraform] Updated alert_processor permissions so it can pull from param store Signed-off-by: jack1902 <39212456+jack1902@users.noreply.github.com>
[testing] Added SSMDriver Tests Signed-off-by: jack1902 <39212456+jack1902@users.noreply.github.com>
|
The code looks solid. How would the migration process look? Is there going to be a script? You might need to keep some of the code around to be able to pull from S3 and save into SSM |
|
@Ryxias You raise an interesting point. How should migration scripts be handled. I personally prefer creating a one time script which contains EVERYTHING. This removes redundant code from the code base and allows for migration scripts to be dropped in future PRs. What are your thoughts on this? The code removed to pull from S3 can easily be put into a one time script and create a local .JSON file for use with the seperate PR to |
|
we could have a subcommand of the |
This script should be used prior to upgrading to the release which uses SSM Parameter store as the secret_store. Once you have upgraded, run the command printed out to the console Signed-off-by: jack1902 <39212456+jack1902@users.noreply.github.com>
Consider a script in |
|
Also, @ryandeivert as i can see you have a HUGE PR open currently, that makes a TONNE of changes to the documentation. i would prefer to create a seperate issue to "update" the documentation to reflect these changes after |
| Raises: | ||
| ClientError | ||
| """ | ||
| client = boto3.client('ssm', config=default_config(region=region)) |
There was a problem hiding this comment.
Some boto3 clients maintain some sort of "internal state". For example, the athena one (I think?) performs API calls when you first instantiate it. Does SSM use this?
Alternative pattern with this class could be something like:
class AwsSsmClient:
def __init__(config):
self._client = boto3.client('ssm', config)
def get_parameter(parameter_name, with_decryption=True):
# ...
There was a problem hiding this comment.
Since this is a one time throw away script I'd argue this is good enough as is. Since after the migration is complete this can be thrown away
There was a problem hiding this comment.
The previous one had this pattern since everywhere it was called was via the staticmethod. i could instantiate this Class within the callers init so it is cached etc between invocations but the parameter/secret that is pulled back is already cached so this seems like complexity for complexities sake
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| import os |
There was a problem hiding this comment.
Is there any particular reason the imports are here instead of at the top of the file?
There was a problem hiding this comment.
Just cleanliness and the fact these are only relevant if name... Since I have to append streamalert to path in order to import it due to the location of the script
| region = config["global"]["account"]["region"] | ||
| prefix = config["global"]["account"]["prefix"] | ||
|
|
||
| bucket_name = f"{prefix}.streamalert.secrets" |
There was a problem hiding this comment.
Subtle thing: @ryandeivert is working on changing the default bucket to include dashes instead of periods. If that PR goes into 3.1.0 you may have to change this to pull from config instead of hardcoding here
There was a problem hiding this comment.
Nice and easy to change if this is the case
| # An error occured so exit with code 1 | ||
| sys.exit(1) | ||
|
|
||
| print( |
|
I'll pull down release-3-1-0 tomorrow and rebase this work and resolve conflicts |
|
@Ryxias The migration script wouldn't be required if we were to have this in |
|
Closed in favour of #1142 |
to: @ryandeivert @Ryxias
cc: @airbnb/streamalert-maintainers
related to: #240
resolves: #240
Background
Using S3 as a
secret_storeis no longer desirable. Using SSM Parameter store makes visibility into what secrets are set much easier and requires less setup by StreamAlertChanges
copy&pastafrom the S3Driver)ssm:GetParameterpermissions to thealert_processorTesting
./tests/scripts/pylint.sh- currently there are warnings (i aim to clean this code up but want to ensure this initial commit is good)./tests/scripts/unit_tests.sh- All created tests now pass, i need to add some for SSMDriver