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

RDS IAM support #2173

Open
wants to merge 10 commits into
base: master
from

Conversation

2 participants
@pjdufour-truss
Copy link
Contributor

commented May 22, 2019

Description

Enabling IAM Authentication for connecting to RDS. Since we got migrations using the same database connection code, our migrations, scheduled tasks, and services should all be able to use this new logic.

Will need to test thoroughly.

Tests:

  • Scheduled Tasks on Experimental Pass!
  • Migrations on Experimental Pass!
  • Service on Experimental Works!
  • Acceptance tests don't break (we can't actually run acceptance tests for this, since they are run outside the VPC).

Reviewer Notes

Local Migrations

make db_dev_destroy && make db_dev_start && DB_TIMEOUT=30 make db_dev_create && make db_dev_migrate

Experimental Migrations

make db_deployed_migrations_destroy && make db_deployed_migrations_start && DB_TIMEOUT=30 make db_deployed_migrations_create && AWS_ASSUME_ROLE_DURATION=45m make run_experimental_migrations

Questions

  • Do we want to use a manifest? Or continue to run all migrations in folder?
  • Is there a better way to manage the migration "path" (MIGRATION_PATH env var) that points to folders containing migrations?
  • How long do migrations take on your local machine now? (local dev and deployed migrations).

Setup

None

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).
  • Any new client dependencies (Google Analytics, hosted libraries, CDNs, etc) have been:
  • 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

Screenshots

N.A.

@pjdufour-truss pjdufour-truss changed the title [WIP] RDS IAM support RDS IAM support May 22, 2019

@codecov

This comment has been minimized.

Copy link

commented May 22, 2019

Codecov Report

Merging #2173 into master will decrease coverage by 2.13%.
The diff coverage is 19.34%.

@@            Coverage Diff            @@
##           master   #2173      +/-   ##
=========================================
- Coverage   59.33%   57.2%   -2.13%     
=========================================
  Files         257     276      +19     
  Lines       14540   14931     +391     
=========================================
- Hits         8626    8540      -86     
- Misses       4901    5378     +477     
  Partials     1013    1013

@pjdufour-truss pjdufour-truss force-pushed the iam_auth branch 3 times, most recently from d932c39 to 8526e63 May 24, 2019

@pjdufour-truss pjdufour-truss force-pushed the iam_auth branch 5 times, most recently from 8633296 to 68dd680 May 31, 2019

@pjdufour-truss pjdufour-truss force-pushed the iam_auth branch 5 times, most recently from 6a44048 to 35df346 Jun 11, 2019

@@ -16,16 +9,24 @@
"exec",
"app-{{ .environment }}",
"--",
"/bin/milmove",
"serve"
"/bin/milmove"

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 19, 2019

Contributor

I was talking to Nick about the security of the client containers and the difference between entryPoint and command. The entryPoint cannot be changed by someone who has access to the system running the container but the command does. So for security we probably ought to lock down the entryPoint to use only the command we want that container to run. That's a long winded way for me to say we probably ought to leave "serve" in the entryPoint section instead of moving it. We also ought to correct the other container definitions.

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss Jun 20, 2019

Author Contributor

Hm... I like that idea. Add a little more friction. Maybe we'd want to move all the CMD part for the deployed container definitions into entrypoint actually, since we should never override... I also found this blog post useful.

https://aws.amazon.com/blogs/opensource/demystifying-entrypoint-cmd-docker/

COPY build /build

ENTRYPOINT ["/bin/milmove"]

CMD ["serve", "--debug-logging"]

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 20, 2019

Contributor

Should this be:

ENTRYPOINT ["/bin/milmove", "serve"]
CMD ["--debug-logging"]

I'm wondering how I'd run this docker file locally.

ENTRYPOINT ["/bin/milmove"]
CMD ["migrate", "-p", "/migrate/migrations"]

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 20, 2019

Contributor

Same question here, if this is

ENTRYPOINT ["/bin/milmove", "migrate"]
CMD ["-p", "/migrate/migrations"]

Then I can run this locally, otherwise I feel like I have to rely on the invocation in a Makefile or some test code or the ECS task definition?

@@ -1,9 +1,9 @@
NAME = ppp
DB_NAME_DEV = dev_db
DB_NAME_PROD_MIGRATIONS = prod_migrations
DB_NAME_DEPLOYED_MIGRATIONS = deployed_migrations

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 20, 2019

Contributor

I think this PR would be easier to understand if we pulled out this renaming into its own PR. I can approve that PR today too because this is a good change.

docker rm -f $(DB_DOCKER_CONTAINER_DEV) || \
echo "No database container"
docker rm -f $(DB_DOCKER_CONTAINER_DEV) || echo "No database container"
rm -fr mnt/db_dev # delete mount directory if exists

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 20, 2019

Contributor

I think it would make sense to have this in the clean: target as well. Maybe just add rm -fr mnt/ inside clean:.

-p $(DB_PORT_DEV):$(DB_PORT_DOCKER)\
$(DB_DOCKER_CONTAINER_IMAGE)

.PHONY: db_dev_create
db_dev_create: ## Create Dev DB
@echo "Create the ${DB_NAME_DEV} database..."
DB_NAME=postgres scripts/wait-for-db && \
createdb -p $(DB_PORT_DEV) -h localhost -U postgres $(DB_NAME_DEV) || true
DB_NAME=postgres scripts/wait-for-db && DB_NAME=postgres psql-wrapper "CREATE DATABASE $(DB_NAME_DEV);" || true

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 20, 2019

Contributor

is this supposed to be DB_USER=postgres instead of DB_NAME?

.PHONY: prune
prune: ## Prune docker containers and images
docker container prune
docker image prune -a

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 20, 2019

Contributor

I'm pretty happy to have this target. I'm wondering if we should go further and do docker system prune --volumes which should just do everything?

Btw, several people would find this handy today in case you wanted to make it a separate PR.

echo
echo -e "\e[33mUsing ${SECURE_MIGRATION_BUCKET_NAME} to gather secure migrations\e[39m"
echo
proceed "Running production migrations against the ${DB_NAME} database. This will delete everything in that db."

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 20, 2019

Contributor

I'm pretty happy to be rid of this script. But I'd like to retain this little piece of it which asks if a user really wants to continue. I could probably be persuaded not to keep it but I think some folks rely on it checking with them. Maybe ask in #dp3-engineering to get a feel for it?

@chrisgilmerproj
Copy link
Contributor

left a comment

You'll need to update this line in make-test: https://github.com/transcom/mymove/blob/master/scripts/make-test#L20

diff --git a/scripts/make-test b/scripts/make-test
index b094e40ce..564b225ca 100755
--- a/scripts/make-test
+++ b/scripts/make-test
@@ -17,7 +17,7 @@ make clean build_tools

 make db_dev_reset db_dev_migrate db_dev_e2e_populate
 make db_test_reset db_test_migrate db_test_e2e_populate
-make db_prod_migrations_reset db_prod_migrations_migrate
+make db_deployed_migrations_reset db_deployed_migrations_run

 # Test e2e Init
var cleanMigrationPattern = regexp.MustCompile("^(\\s*)(COPY)(\\s+)([a-zA-Z._]+)(\\s*)\\((.+)\\)(\\s+)(FROM)(\\s+)(stdin)(;)(\\s*)$")

// CleanMigraton cleans migrations so they can be used in a go connection directly.
func CleanMigraton(r io.Reader, w chan string) {

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor
Suggested change
func CleanMigraton(r io.Reader, w chan string) {
func CleanMigration(r io.Reader, w chan string) {

out := make(chan string, 1000)

go migrate.CleanMigraton(inputReader, out)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor
Suggested change
go migrate.CleanMigraton(inputReader, out)
go migrate.CleanMigration(inputReader, out)
func main() {

root := cobra.Command{
Use: "clean-migration INPUT_FILE|- [OUTPUT_FILE]",

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

I tried to run clean-migration migrations/20190410152949_add_new_tdls.up.sql ./out.txt but I only got stdout output whereas I expected from this that it would go to a file. Did I do it wrong?

go migrate.CleanMigraton(inputReader, out)

for line := range out {
_, err := fmt.Fprintln(os.Stdout, line)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

Here is where I see it printing to stdout but not to a file? Maybe os.Stdout is a default if no outfile name is given?

}

if v.GetBool(cli.DbDebugFlag) {
pop.Debug = true
}

migrationPath := expandPaths(strings.Split(v.GetString(cli.MigrationPathFlag), ";"))

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor
Suggested change
migrationPath := expandPaths(strings.Split(v.GetString(cli.MigrationPathFlag), ";"))
migrationPaths := expandPaths(strings.Split(v.GetString(cli.MigrationPathFlag), ";"))

var session *awssession.Session
if v.GetBool(cli.DbIamFlag) || s3Migrations {
c, errorConfig := cli.GetAWSConfig(v, v.GetBool(cli.VerboseFlag))

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

Can we use full names instead of single character names?

Suggested change
c, errorConfig := cli.GetAWSConfig(v, v.GetBool(cli.VerboseFlag))
config, errorConfig := cli.GetAWSConfig(v, v.GetBool(cli.VerboseFlag))
if errorConfig != nil {
return errors.Wrap(errorConfig, "error creating aws config")
}
s, errorSession := awssession.NewSession(c)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

Same here, I'd rather use a name.

)

// ListFiles lists the files in a given directory.
func ListFiles(p string, s3Client *s3.S3) ([]string, error) {

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor
Suggested change
func ListFiles(p string, s3Client *s3.S3) ([]string, error) {
func ListFiles(migrationPath string, s3Client *s3.S3) ([]string, error) {
return f.Readdirnames(-1)
} else if strings.HasPrefix(p, "s3://") {
parts := strings.SplitN(p[len("s3://"):], "/", 2)
if len(parts) == 2 {

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

Should we throw an error if it doesn't equal 2? However, I think that its likely impossible so do we want to just skip this if statement?

s3Client = s3.New(session)
}

migrationFiles := map[string][]string{}

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

This might sound weird but this seems like a great debugging tool: list-migration-files (or something). Because it would be nice to pass in a set of migration paths and see the whole set listed out.

i := 0
for {

//fmt.Println("Blocks:", blocks)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor
Suggested change
//fmt.Println("Blocks:", blocks)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

There are a few of these around. Can you strip out the comments before merging?

if dropComments {
continue
}
} else if strings.Contains(line, "pg_catalog.set_config('search_path'") {

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

can you add a comment about why this is called out explicitly?

"github.com/pkg/errors"
)

func Exec(inputReader io.Reader, tx *pop.Connection, wait time.Duration) error {

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

Add a comment here about what this does.

Suggested change
func Exec(inputReader io.Reader, tx *pop.Connection, wait time.Duration) error {
// Exec executes ...
func Exec(inputReader io.Reader, tx *pop.Connection, wait time.Duration) error {
return errors.Wrap(errGetObject, fmt.Sprintf("error reading migration file at %q", m.Path))
}

switch m.Type {

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

The code in this switch statement feels repeated. Could it be pulled out to a function?

}()

statements := make(chan string, 1000)
go migrate.SplitStatements(lines, statements)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

Is this where the slowdown happens for running prod migrations? I'm curious if I can find an example to use to play around with. What did you use?

func main() {

root := cobra.Command{
Use: "split-migration INPUT_FILE|- [OUTPUT_FILE]",

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

I expected this to go to an outfile but I don't see it.

@@ -1 +0,0 @@
exec("./apply-secure-migration.sh 20180424010930_test.sql")

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

These files aren't needed anymore because we have a manifest file and because we have the ability to list files from buckets, is that right?

@@ -0,0 +1,9 @@
#! /usr/bin/env bash
#
# Show all used packges in our codebase.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 1, 2019

Contributor

Update the description here.

@chrisgilmerproj
Copy link
Contributor

left a comment

I think so far my tests show that this works. I would like to see this rebased on master. I've also got a bunch of minor fixes I'd love to see in this. Finally, can you help me with some more test cases for playing with CleanMigrations and SplitMigrations? or even unit tests? Because these are the places I really want to understand what's going on.

@chrisgilmerproj chrisgilmerproj referenced this pull request Jul 3, 2019

Draft

RDS IAM support (take 2) #2349

0 of 11 tasks complete
listOfColumns := "id, performance_period_start, performance_period_end, traffic_distribution_list_id, quality_band, offer_count, best_value_score, transportation_service_provider_id, created_at, updated_at, rate_cycle_start, rate_cycle_end, linehaul_rate, sit_rate"
columns := parseColumns(listOfColumns)

suite.DB().Transaction(func(tx *pop.Connection) error {

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 12, 2019

Contributor

Return the error to the transaction so it can be tested there. Otherwise we don't test the transaction bit.

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