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

Cached FIM - Part 0 - Terraform and RDS Bastion Setup #599

Merged
merged 8 commits into from
Dec 22, 2023

Conversation

TylerSchrag-NOAA
Copy link
Contributor

@TylerSchrag-NOAA TylerSchrag-NOAA commented Dec 12, 2023

This PR is the first step to implementing the new Cached FIM Workflow I've been developing (I'll have 1-3 follow-up PRs - I'm trying to split this all up into appropriately sized chunks).

This PR:

  • Adds a new IAM role for the Redshift cluster (which notably has permission to read the viz RDS user secret for external schema setup).
  • Adds a new security group for the Redshift cluster
  • Adds a Redshift cluster (including a cluster subnet group)
  • Adds two steps to the RDS Bastion:
    • Redshift cluster setup: Adds a user for use in the pipelines & sets up a external schema pointed to the ingest schema on viz RDS database (I didn't abstract this yet, like what was done for the foreign data wrapper connections on RDS, since it's only one schema, but we could do that in the future)
    • DB_Link Foreign Data Wrapper From RDS to Redshift: I went ahead and put this in a separate util script, as the syntax is a little different than the other foreign data wrapper connections between RDS instances (it's possible that the same syntax could work for all, but I'm not prioritizing figuring that our right now).

Testing / Deployment Considerations:
I've tested the IAM Role and Redshift setup in terraform, and the RDS Bastion steps manually on both Redshift and RDS... but I haven't tested it all together through the central terraform module yet - I'm hoping this should be pretty smooth, and I can help troubleshoot any deployment issues in real-time, Nick. I've also included a couple comments in the code below for your consideration.

@@ -58,6 +58,7 @@ module "iam-roles" {
prod_account_id = local.env.prod_account_id
region = local.env.region
nws_shared_account_s3_bucket = local.env.nws_shared_account_s3_bucket
viz_proc_admin_rw_secret_arn = module.secrets-manager.secret_arns["viz-proc-admin-rw-user"]
Copy link
Contributor Author

@TylerSchrag-NOAA TylerSchrag-NOAA Dec 12, 2023

Choose a reason for hiding this comment

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

@nickchadwick-noaa - I'd love your thoughts on how I did this / am wondering if it will work, given that I'm referencing a module that exists further down in the file.

Basically, I wanted to use the best practice approach of letting Redshift communicate directly with secrets manager to get the password it needs for the external data schema connection to the Viz RDS database, instead of running SQL with the text password included (we should probably switch to this IAM-based route on the RDS foreign data wrapper setups as well - I think it is supported).

Anyways, I figured it was good to specify that Redshift only has the ability to access this one secret, hence passing it in as variable here. Seems like it could get messy, but I figure we can just pass that entire secret_arns list if we want, and then reference specific secrets within the IAM policy json, as required... if we need to add more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this approach. It will technically work even if that secrets module is further down the file.

The IAM Roles section of the repo is the one piece I have not completely revamped due to it's complexity and how messy the IAM policies are. I think it would be a really good idea to eventually replace all RDS authentication with IAM Roles instead of username/passwords https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.IAMDBAuth.html

I think this is good for now, and then eventually I'll try to just redo the IAM module entirely.

Copy link
Contributor Author

@TylerSchrag-NOAA TylerSchrag-NOAA Dec 13, 2023

Choose a reason for hiding this comment

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

Cool, thanks. That sounds good. I've wondered about doing IAM DB authentication across the board as well, and the only thing I'm concerned about with that is an implication on the number of connections (which are many, from our lambda functions), which they vaguely say may be affected. But it would be really nice to only have to worry about the IAM roles on the Lambdas. But at the least, we could go this route on the RDS Bastion, to keep the passwords out of the raw SQL sent to the dbs for the foreign data wrapper setups.

Of course, the other option on the lambdas is to just store the secret name in an env variable, and have the aws api lookup the password during run-time. That would be a little more secure than how we currently have it, but would also have some [I think negligible] performance cost. Not sure how that performance cost compares to the IAM role route, maybe it's a wash,

egis_db_user_secret_string = module.secrets-manager.secret_strings["egis-pg-rds-secret"]
egis_portal_password = local.env.viz_ec2_hydrovis_egis_pass
viz_redshift_host = module.redshift-viz.dns_name
viz_redshift_db_name = local.env.viz_redshift_db_name
Copy link
Contributor Author

@TylerSchrag-NOAA TylerSchrag-NOAA Dec 12, 2023

Choose a reason for hiding this comment

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

I really only added the viz_redshift variables here. (had to change indentation, so it highlighted everything)

viz_db_address = module.rds-viz.instance.address
viz_db_port = module.rds-viz.instance.port
viz_db_name = local.env.viz_db_name
viz_redshift_master_secret_string = module.secrets-manager.secret_strings["viz_redshift_master"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really only added the viz_redshift variables here. (had to change indentation, so it highlighted everything)

@nickchadwick-noaa nickchadwick-noaa merged commit f248eeb into ti Dec 22, 2023
1 check passed
@nickchadwick-noaa nickchadwick-noaa deleted the cached_fim_part0 branch December 22, 2023 16:25
@nickchadwick-noaa nickchadwick-noaa modified the milestones: V2.1.5, V2.1.6 Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants