-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add lambda to bootstrap tower database #16
Conversation
289f22e
to
4af90ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another suggestion that's not part of this change.. Would it make sense to replace your ClusterSettings map with AWS::RDS::DBClusterParameterGroup
[1]
@@ -125,6 +152,137 @@ Resources: | |||
VpcSecurityGroupIds: | |||
- !Ref ClusterSecurityGroup | |||
|
|||
# this custom resource exists to trigger the bootstrap lambda on creation | |||
BootstrapLambdaTrigger: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion but not required.. This resource will create a lambda instance for every new DB instance. Would it make sense to decouple the lambda. Install one lambda and reuse it to setup each instance of a DB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this were going to be used more than once, I would say that's the thing to do. I expect this to be used only once per account. Creating a lambda to run a couple of SQL statements is already overkill, and I'd prefer not to over-optimize when there is almost no chance that we would create more than one.
templates/nextflow-aurora-mysql.yaml
Outdated
@@ -65,6 +65,18 @@ Resources: | |||
ExcludeCharacters: '"@/\$`&' | |||
PasswordLength: 16 | |||
|
|||
AuroraTowerUserSecret: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to change Tower
to a more generic name in case nexflow change's it's DB user? something like AuroraNextflowUserSecret
would make it agnostic to a specific Tower
username. That way you can just pass in any username you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can name it something like AuroraNextflowTowerDatabaseUserSecret
templates/nextflow-aurora-mysql.yaml
Outdated
try: | ||
secret_response = boto3.client('secretsmanager').get_secret_value(SecretId=environ['Tower_User_Secret_ARN']) | ||
secret_string = secret_response['SecretString'] | ||
alter_db_response = submit_sql("ALTER DATABASE tower CHARACTER SET utf8 COLLATE utf8_bin;") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to parameterize db name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, might as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah actually I already did that; just missed replacing the string here
templates/nextflow-aurora-mysql.yaml
Outdated
secret_string = secret_response['SecretString'] | ||
alter_db_response = submit_sql("ALTER DATABASE tower CHARACTER SET utf8 COLLATE utf8_bin;") | ||
log.debug(f"Alter DB response: {alter_db_response}") | ||
create_user_response = submit_sql("CREATE USER 'tower' IDENTIFIED BY '" + secret_string + "';") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per above suggestion, instead of the static tower db username maybe you can parameterize it and allow passing in any username?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
@zaro0508 regarding the question #16 (review) |
templates/nextflow-aurora-mysql.yaml
Outdated
Name: !Sub '${AWS::StackName}-TowerUserSecret' | ||
Description: !Sub 'Aurora MySQL Tower User Secret for CloudFormation Stack ${AWS::StackName}' | ||
Name: !Sub '${AWS::StackName}-NextflowTowerDatabaseUserSecret' | ||
Description: !Sub 'Aurora MySQL Nextflow Tower User Secret for CloudFormation Stack ${AWS::StackName}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be
!Sub 'Aurora MySQL ${DBUserName} User Secret for CloudFormation Stack ${AWS::StackName}'
templates/nextflow-aurora-mysql.yaml
Outdated
log.debug(f"Grant privileges response: {grant_response}") | ||
return { | ||
'status': cfnresponse.SUCCESS, | ||
'message': 'Succeeded bootstrapping tower database', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tower/{db_name}
templates/nextflow-aurora-mysql.yaml
Outdated
log.error(f'An error was encountered when bootstrapping: {e}') | ||
return { | ||
'status': cfnresponse.FAILED, | ||
'message': 'Failed bootstrapping tower database', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tower/{db_name}
9b1afee
to
7d12e34
Compare
This adds a Lambda to perform some bootstrapping statements on the tower database in the Aurora DB cluster.
https://sagebionetworks.jira.com/browse/WORKFLOWS-12