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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

RDS IAM support (take 2) #2349

Draft
wants to merge 28 commits into
base: master
from

Conversation

3 participants
@chrisgilmerproj
Copy link
Contributor

commented Jul 3, 2019

Description

Replaces #2173

Requires transcom/ppp-infra#527

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

Setup

Add any steps or code to run in this section to help others prepare to run your code:

echo "Code goes here"

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

Screenshots

If this PR makes visible UI changes, an image of the finished UI can help reviewers and casual
observers understand the context of the changes. A before image is optional and
can be included at the submitter's discretion.

Consider using an animated image to show an entire workflow instead of using multiple images. You may want to use GIPHY CAPTURE for this! 馃摳

Please frame screenshots to show enough useful context but also highlight the affected regions.

type Buffer struct {
*sync.RWMutex
buffer strings.Builder
closed bool

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 3, 2019

Author Contributor

I don't understand what closed is for exactly. I think its supposed to mean that the Buffer cannot be written to anymore. But the various Write*() functions don't refer to it. Can you help me figure that out?

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss Jul 4, 2019

Contributor

Closed is used in the Exec function to confirm that there is no more data left to be downloaded and you have reached the end of the file.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Author Contributor

I think then that I'd prefer it also disabled writing to the buffer. That way we don't get someone hacking this struct to do something fancy for a single migration or something. Let's have "closed" mean "closed for good".

// Migrate to use the Runner
migrator := pop.NewMigrator(suite.DB())
migrator.Migrations[migration.Direction] = append(migrator.Migrations[migration.Direction], *migration)
err = migrator.Up()

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 3, 2019

Author Contributor

I thought this would instantiate the m.Runner() function in the builder but It didn't seem to in the test coverage. Maybe I don't understand how to run that code?

// 9 : whitespace
// 10 : stdin
// 11 : ;
// 12 : whitespace

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 3, 2019

Author Contributor

This turns out to be wrong. The regex has 13 values in it (an extra space captured between stdin and ;). Let's remove the comments like this one and rely on the Test to document what's going on in here.

This comment has been minimized.

Copy link
@pjdufour-truss
func (suite *MigrateSuite) TestExecWithLoopSQL() {

// Load the fixture with the sql example
f, err := os.Open("./fixtures/loop.sql")

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 3, 2019

Author Contributor

This sql file in particular has a lot going on. I'd love to use it as a test case for parsing the sql as well.

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

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 3, 2019

Author Contributor

can we document what happens here in the search path?

"github.com/pkg/errors"
)

func untilSpace(in *Buffer, i int, wait time.Duration) (int, string, error) {

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 3, 2019

Author Contributor

untilSpace seems like a superset of untilNewLine. Can we have just one underlying function like untilChar and then make untilSpace and untilNewLine pass in the character set they want to use?

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss Jul 4, 2019

Contributor

Code would have be written a little differently, because you couldn't use a multi-value case, but can be done.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Author Contributor

Is it possible to use a spread operator here to pass in a list to the case statement? I'm curious.

})

// Create the builder and point to the fixture path
uri := "file://./fixtures"

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Author Contributor

This should be a file name

// Create the builder and point to the fixture path
uri := "file://./fixtures"
m := pop.Match{
Version: "",

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 5, 2019

Author Contributor

Add version.


dbType := ""
direction := ""
if len(m[3]) == 0 {

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal Jul 5, 2019

Contributor

What is the if condition checking here?

@codecov

This comment has been minimized.

Copy link

commented Jul 5, 2019

Codecov Report

Merging #2349 into master will decrease coverage by 0.63%.
The diff coverage is 44.08%.

@@            Coverage Diff             @@
##           master    #2349      +/-   ##
==========================================
- Coverage   59.58%   58.95%   -0.63%     
==========================================
  Files         266      283      +17     
  Lines       14892    15363     +471     
==========================================
+ Hits         8872     9056     +184     
- Misses       4987     5260     +273     
- Partials     1033     1047      +14
@@ -31,7 +31,8 @@ repos:
rev: v1.17.1
hooks:
- id: golangci-lint
entry: golangci-lint run
entry: golangci-lint run --verbose
verbose: true

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 11, 2019

Author Contributor

I think @rdhariwal removed this - did we want to re-add it?

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss Jul 11, 2019

Contributor

No. It was failing earlier and needed that to debug.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Experimental

time make db_deployed_migrations_reset run_experimental_migrations

real    6m10.438s
user    1m18.356s
sys     0m17.714s

Staging

time make db_deployed_migrations_reset run_staging_migrations

real    6m39.969s
user    1m28.520s
sys     0m20.041s

Prod

time make db_deployed_migrations_reset run_prod_migrations

[POP] 2019/07/11 23:15:38 info - > add_office_users
[POP] 2019/07/11 23:15:38 info - > add-show-column-to-moves
[POP] 2019/07/11 23:15:38 info - 4.9622 minutes
panic: error running migrations: problem inserting migration version 20190403150700: pq: current transaction is aborted, commands ignored until end of transaction block

goroutine 1 [running]:
main.main()
        /Users/cgilmer/Projects/transcom/mymove/cmd/milmove/main.go:90 +0x7db
make: *** [run_prod_migrations] Error 2

real    5m5.052s
user    1m9.364s
sys     0m16.431s

I think the issue is that the Prod version of 20190403150700_update-moves-show-flag.sql has an extra newline in it. Maybe there's another issue as well but that pops up as an issue.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

The migration that is failing can be tested with:

download-secure-migrations 20190403150700_update-moves-show-flag.sql
cp tmp/secure_migrations/prod/20190403150700_update-moves-show-flag.sql local_migrations/20190403150700_update-moves-show-flag.sql
make db_dev_reset db_dev_migrate

I get this error:

panic: error running migrations: problem inserting migration version 20190403150700: pq: current transaction is aborted, commands ignored until end of transaction block

goroutine 1 [running]:
main.main()
        /Users/cgilmer/Projects/transcom/mymove/cmd/milmove/main.go:90 +0x7db
make: *** [db_dev_migrate_standalone] Error 2
}
}

if bytes.Contains(line, []byte("pg_catalog.set_config('search_path'")) {

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 12, 2019

Author Contributor

What is this line? Also, is this supposed to be changed to:

Suggested change
if bytes.Contains(line, []byte("pg_catalog.set_config('search_path'")) {
if bytes.Contains(line, []byte("pg_catalog.set_config('search_path')")) {

This comment has been minimized.

Copy link
@pjdufour-truss

pjdufour-truss Jul 12, 2019

Contributor

Closing parentheses aren't strictly required and wouldn't hit on this line if there was a space after search_path, but maybe more strict matching is fine given the inputs.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 12, 2019

Author Contributor

Let's add a comment about what this is doing so someone doesn't come along and try to "fix" it like I just did.

@@ -748,7 +748,7 @@ jobs:

# `deploy_prod_migrations` deploys migrations to the staging environment
deploy_prod_migrations:
executor: mymove_small
executor: mymove_medium

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 16, 2019

Author Contributor

This won't actually change anything. The instance size you need to change is the AWS ECS container instance size, not the CircleCI docker container instance size. I know that's a bit confusing :-P

@@ -57,7 +57,7 @@ readonly container_definition_json

# create new task definition with the given image
echo "Create a new task definition for task family ${task_family}"
task_definition_arn=$(aws ecs register-task-definition --network-mode awsvpc --task-role-arn "ecs-task-role-app-$environment" --family "$task_family" --container-definitions "$container_definition_json" --requires-compatibilities FARGATE --execution-role-arn "ecs-task-execution-role-app-$environment" --cpu 256 --memory 512 --query 'taskDefinition.taskDefinitionArn' --output text)
task_definition_arn=$(aws ecs register-task-definition --network-mode awsvpc --task-role-arn "ecs-task-role-app-$environment" --family "$task_family" --container-definitions "$container_definition_json" --requires-compatibilities FARGATE --execution-role-arn "ecs-task-execution-role-app-$environment" --cpu 512 --memory 1024 --query 'taskDefinition.taskDefinitionArn' --output text)

This comment has been minimized.

Copy link
@rdhariwal

rdhariwal Jul 16, 2019

Contributor

@chrisgilmerproj does this look right to you? I doubled memory and cpu

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 16, 2019

Author Contributor

yeah, that looks right.

@chrisgilmerproj chrisgilmerproj force-pushed the iam_auth_2 branch from 1897bf7 to e4f28ff Jul 16, 2019

@chrisgilmerproj chrisgilmerproj force-pushed the iam_auth_2 branch from e4f28ff to 2e87c9f Jul 16, 2019

chrisgilmerproj added some commits Jul 16, 2019

suite.Nil(err)
return nil
})
suite.NoError(errTransaction)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 17, 2019

Author Contributor

Currently this fails and we want it to pass. The test is correct but the Exec() function is broken when we use ON CONFLICT DO NOTHING

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