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

Refresh RDS IAM token #2633

Open
wants to merge 6 commits into
base: master
from

Conversation

@mr337
Copy link
Contributor

commented Aug 27, 2019

Description

This PR adds a goroutine to update the IAM token used for RDS authentication. The token is immediately generate to not delay server start and refreshed every 10 minutes. The token updates the database connection details globally used so as connections are created in the pool the latest token is used.

This change is backwards compatible with plain text password authentication.

Semi related (not required PR): #2620

Reviewer Notes

Few things to check:

  • Use of goroutine applicable
  • Is the error handling sufficient?
  • Is there proper logging to debug this if it fails?
  • If the IAM token refresh fails the app fill block indefinitely. Is this proper behavior or should we have a max timeout that the server exits after X minutes?

Setup

If using MilMove AWS environment refer to IAM Doc on how to connect.

Testing IAM auth

 ./bin/milmove serve --db-iam --db-iam-role DB_ARN  \
 --db-region us-east-2 \
--db-host rdstest.chsgqg6ccrq7.us-east-2.rds.amazonaws.com  \
--db-ssl-mode verify-full --db-ssl-root-cert  PATH/TO/CERT --db-user db_user

Browse for longer than 15 minutes as the original IAM token will have expired.

Testing Plain Text: simply run local dev environment which uses plain text password for database auth

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #dp3-engineering
    • Secure migrations have been tested using scripts/run-prod-migrations
  • There are no aXe warnings for UI.
  • This works in Supported Browsers and their phone views (Chrome, Firefox, IE, Edge).
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • User facing changes have been reviewed by design.
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@mr337 mr337 force-pushed the lh-refresh-rds-iam branch from 4954ba7 to a7be954 Aug 27, 2019

@codecov

This comment has been minimized.

Copy link

commented Aug 27, 2019

Codecov Report

Merging #2633 into master will increase coverage by 0.8%.
The diff coverage is 0%.

@@           Coverage Diff            @@
##           master   #2633     +/-   ##
========================================
+ Coverage    55.4%   56.2%   +0.8%     
========================================
  Files         271     242     -29     
  Lines       12453   11873    -580     
========================================
- Hits         6894    6667    -227     
+ Misses       4858    4513    -345     
+ Partials      701     693      -8
Impacted Files Coverage Δ
pkg/cli/dbconn.go 14.2% <0%> (-4%) ⬇️
pkg/models/document.go 26.7% <0%> (-56.6%) ⬇️
pkg/models/upload.go 46.7% <0%> (-37.7%) ⬇️
pkg/models/shipment_summary_worksheet.go 72.1% <0%> (-17.3%) ⬇️
pkg/models/personally_procured_move.go 37.9% <0%> (-11.1%) ⬇️
pkg/models/move_document.go 41.8% <0%> (-1.4%) ⬇️
pkg/models/service_member.go 20% <0%> (-1.1%) ⬇️
pkg/models/move_documents_extractor.go 92% <0%> (-0.5%) ⬇️
pkg/paperwork/shipment_summary.go 89.1% <0%> (-0.1%) ⬇️
pkg/rateengine/rateengine.go 77.6% <0%> (ø) ⬆️
... and 35 more
@mr337

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

Not for sure why codecov is going crazy on the coverage. I'll need to add some tests to bring coverage percentage back up.

@mr337 mr337 force-pushed the lh-refresh-rds-iam branch from d8fd3c9 to 55b8432 Sep 3, 2019


var useIAM = false
var passHolder = ""
var currentIamPass = ""

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Sep 4, 2019

Contributor

Let's make this a Struct at some point.

@mr337

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

Decided to revert a commit that used the existing pq driver for plain text auth and the custom driver for RDS. This was a work around for the real problem that affects both plain text and IAM auth for the custom driver. CI was telling me something and I ignored it, so now it will always test both ways.

More info here: jmoiron/sqlx#559

@mr337 mr337 force-pushed the lh-refresh-rds-iam branch from 8d39259 to 63e7f62 Sep 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.