-
Notifications
You must be signed in to change notification settings - Fork 106
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
CUMULUS-3536 Update Cumulus Core from Aurora Serverless V1 to V2 #3632
base: master
Are you sure you want to change the base?
Conversation
* update CL * update terraform templates to serverless v2 * add terraform variable validation * remove upgrade variables * add prevent_destroy = true * add prevent_destroy = true --------- Co-authored-by: Tim Clark <tim.clark@nasa.gov>
# Conflicts: # CHANGELOG.md
…o v2 (#3643) * remove prevent_destroy to allow automated CI migrations --------- Co-authored-by: Tim Clark <tim.clark@nasa.gov>
Co-authored-by: Tim Clark <tim.clark@nasa.gov>
* initial commit * serverless v2 doc updates * Update serverless V2 docs * Fix lint issue
# Conflicts: # CHANGELOG.md
@@ -83,6 +83,7 @@ export const handler = async (event: HandlerEvent): Promise<void> => { | |||
database: `${dbUser}_db`, | |||
host: (rootKnexConfig.connection as Knex.PgConnectionConfig).host, | |||
port: (rootKnexConfig.connection as Knex.PgConnectionConfig).port, | |||
disableSSL: 'true', |
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.
I had to add disableSSL: 'true' to this file in order for Bamboo CI in order for the Deploy Dev Integration Stack to complete successfully. Perhaps we need to add documentation around this addition?
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.
@Jkovarik This is the additional change I needed to make in order for Bamboo CI in order for the Deploy Dev Integration Stack to complete successfully
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.
We should probably consider making this a user option as part of the incoming event.
That said, the behavior is documented (not as part of this module, but in Core) as part of the ticket CL:
- **CUMULUS-3323**
- Added `disableSSL` as a valid database secret key - setting this in your database credentials will
disable SSL for all Core database connection attempts.
- Added `rejectUnauthorized` as a valid database secret key - setting
this to `false` in your database credentials will allow self-signed certs/certs with an unrecognized authority.
- Updated the default parameter group for `cumulus-rds-tf` to set `force_ssl`
to 1. This setting for the Aurora Serverless v1 database disallows non-SSL
connections to the database, and is intended to help enforce security
compliance rules. This update can be opted-out by supplying a non-default
`db_parameters` set in the terraform configuration.
and
That said - why do we need to disable SSL for v2? Is this the cert concern, and if so can we leave SSL enabled but allow unverified certs (e.g. rejectUnauthorized
)?
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.
I have removed the disableSsl entry and added rejectUnauthorized = false. Bamboo CI ran successfully.
# Conflicts: # CHANGELOG.md
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.
Good work - just one question re: the current SSL default settings.
# Conflicts: # CHANGELOG.md
# Conflicts: # tf-modules/cumulus-rds-tf/main.tf # tf-modules/cumulus-rds-tf/variables.tf
@Jkovarik I have removed the disableSsl entry and added rejectUnauthorized = false. Bamboo CI ran successfully. |
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
@Jkovarik Hi Jonathan, it looks like Github is showing "Merging is blocked" due to a change request you have added to this PR. Are there any further changes you are looking for? If not, can you clear the change request so that this feature branch is able to merge, once the time is right? Thanks |
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.
@tclark-innovim the updates look good so I'm gonna go ahead and approve. I do want to be sure we (I) understand the rejectUnauthorized
though. It's reasonable to default that the way we have it?
I set rejectUnauthorized = false as default because our Cumulus Lambda does not yet have the AWS RDS SSL root authority certificate installed. We have a future ticket for that. Once that is in place, we can remove the rejectUnauthorized = false. |
# Conflicts: # CHANGELOG.md
Summary: Summary of changes
Addresses CUMULUS-3536: Update Cumulus Core from Aurora Serverless V1 to V2
Changes
PR Checklist