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

Add a range for refresh on the postgres RDS IAM credential call to avoid API throttling limits #2781

Open
wants to merge 6 commits into
base: master
from

Conversation

@chrisgilmerproj
Copy link
Contributor

commented Oct 7, 2019

Description

We might hit throttling limits if we scale up to a lot of containers when hitting RDS IAM auth endpoint. This adds just a little bit of entropy but its the same per container and doesn't change on each refresh.

Code Review Verification Steps

  • Have the Pivotal acceptance criteria been met for this change?

References

…oid API throttling limits
@chrisgilmerproj chrisgilmerproj requested review from mr337 and rdhariwal Oct 7, 2019
@chrisgilmerproj chrisgilmerproj self-assigned this Oct 7, 2019
Copy link
Contributor

left a comment

This is almost there but the sleep needs to wait in another place. So the issue is having Fargate dispatch containers within one second in themselves.

A better spot to wait (0-5s) would be https://github.com/transcom/mymove/pull/2781/files#diff-cf59a2817d48bad8d9ea4a4fb28552ebL75.

The wait is perfect just in the wrong location. Moving the wait will change when the goroutine is first fired altering the refresh cycle between containers.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

@mr337 - apparently the code you linked doesn't have the line numbers. Can you send me a new link? And then I'll update this.

@mr337

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

@mr337 - apparently the code you linked doesn't have the line numbers. Can you send me a new link? And then I'll update this.

What the heck! Try this link https://github.com/transcom/mymove/pull/2781/files#diff-cf59a2817d48bad8d9ea4a4fb28552ebR82

@codecov

This comment has been minimized.

Copy link

commented Oct 8, 2019

Codecov Report

Merging #2781 into master will decrease coverage by 0.3%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #2781     +/-   ##
========================================
- Coverage    58.2%   57.9%   -0.2%     
========================================
  Files         291     278     -13     
  Lines       12755   12626    -129     
========================================
- Hits         7412    7305    -107     
+ Misses       4593    4573     -20     
+ Partials      750     748      -2
Impacted Files Coverage Δ
pkg/iampostgres/iampostgres.go 76.1% <100%> (+1.7%) ⬆️
pkg/models/validators.go 56.8% <0%> (-6.3%) ⬇️
pkg/cli/devlocal.go 13% <0%> (-2.4%) ⬇️
pkg/cli/services.go 83.4% <0%> (-1.8%) ⬇️
pkg/services/query/query_builder.go 84.8% <0%> (-1.3%) ⬇️
pkg/auth/authentication/login_gov.go 17.7% <0%> (-1.1%) ⬇️
pkg/cli/swagger.go 88.9% <0%> (-1.1%) ⬇️
pkg/services/office_user/office_user_updater.go 87.5% <0%> (-0.7%) ⬇️
pkg/auth/authentication/auth.go 40.1% <0%> (-0.2%) ⬇️
pkg/handlers/adminapi/office_users.go 91% <0%> (-0.1%) ⬇️
... and 21 more
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

@mr337 - that link worked! Take a look and see if this is better.

Also, should it be 1-5s? Or 0-5s? Or 0-1s? Not sure how this might block on future requests to get the password.

@mr337

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

Gah I don't think that link worked properly :S

And I think that would clear up your question above.

@chrisgilmerproj chrisgilmerproj requested a review from mr337 Oct 8, 2019
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

That makes a lot more sense to me. Alright, its updated. Definitely review the time range I set.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

Interestingly I keep getting a failure. Feels like there's some kind of race condition? I get IAM Credentials are missing.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

Here's the failure: ```{"level":"error","ts":1570568296.272855,"caller":"iampostgres/iampostgres.go:89","msg":"IAM Credentials are missing","stacktrace":"github.com/transcom/mymove/pkg/iampostgres.EnableIAM.func1\n\t/Users/cgilmer/Projects/transcom/mymove/pkg/iampostgres/iampostgres.go:89"}
{"level":"info","ts":1570568300.1639419,"caller":"iampostgres/iampostgres.go:92","msg":"Using IAM Authentication"}
{"level":"warn","ts":1570568300.1639302,"caller":"iampostgres/iampostgres.go:51","msg":"Waiting 250000000ms for IAM password to populate"}
{"level":"info","ts":1570568300.163979,"caller":"iampostgres/iampostgres.go:102","msg":"Successfully generated new IAM token"}
{"level":"info","ts":1570568300.276337,"caller":"iampostgres/iampostgres.go:92","msg":"Using IAM Authentication"}
{"level":"info","ts":1570568300.276375,"caller":"iampostgres/iampostgres.go:102","msg":"Successfully generated new IAM token"}
{"level":"info","ts":1570568302.27647,"caller":"iampostgres/iampostgres.go:92","msg":"Using IAM Authentication"}
{"level":"info","ts":1570568302.2765188,"caller":"iampostgres/iampostgres.go:102","msg":"Successfully generated new IAM token"}
{"level":"info","ts":1570568302.4615018,"caller":"iampostgres/iampostgres.go:92","msg":"Using IAM Authentication"}
{"level":"info","ts":1570568302.461545,"caller":"iampostgres/iampostgres.go:102","msg":"Successfully generated new IAM token"}
{"level":"info","ts":1570568302.461559,"caller":"iampostgres/iampostgres.go:92","msg":"Using IAM Authentication"}
{"level":"info","ts":1570568302.461581,"caller":"iampostgres/iampostgres.go:102","msg":"Successfully generated new IAM token"}
{"level":"info","ts":1570568304.277387,"caller":"iampostgres/iampostgres.go:92","msg":"Using IAM Authentication"}
{"level":"info","ts":1570568304.2774222,"caller":"iampostgres/iampostgres.go:102","msg":"Successfully generated new IAM token"}
{"level":"info","ts":1570568304.418045,"caller":"iampostgres/iampostgres.go:92","msg":"Using IAM Authentication"}
{"level":"info","ts":1570568304.4181252,"caller":"iampostgres/iampostgres.go:102","msg":"Successfully generated new IAM token"}
{"level":"info","ts":1570568306.2769852,"caller":"iampostgres/iampostgres.go:92","msg":"Using IAM Authentication"}
{"level":"info","ts":1570568306.2770472,"caller":"iampostgres/iampostgres.go:102","msg":"Successfully generated new IAM token"}
{"level":"info","ts":1570568306.414509,"caller":"iampostgres/iampostgres.go:92","msg":"Using IAM Authentication"}
{"level":"info","ts":1570568306.4145439,"caller":"iampostgres/iampostgres.go:102","msg":"Successfully generated new IAM token"}
{"level":"info","ts":1570568308.276735,"caller":"iampostgres/iampostgres.go:92","msg":"Using IAM Authentication"}
{"level":"info","ts":1570568308.276785,"caller":"iampostgres/iampostgres.go:102","msg":"Successfully generated new IAM token"}
{"level":"info","ts":1570568308.4145,"caller":"iampostgres/iampostgres.go:92","msg":"Using IAM Authentication"}
{"level":"info","ts":1570568308.414603,"caller":"iampostgres/iampostgres.go:102","msg":"Successfully generated new IAM token"}
--- FAIL: TestEnableIAMNormal (9.07s)
iampostgres_test.go:83:
Error Trace: iampostgres_test.go:83
Error: Not equal:
expected: "123"
actual : "abc"

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1 +1 @@
                            -123
                            +abc
            Test:           TestEnableIAMNormal
iampostgres_test.go:84: Current password: 123
iampostgres_test.go:89: Current password: xyz
iampostgres_test.go:90:
            Error Trace:    iampostgres_test.go:90
            Error:          Not equal:
                            expected: "xyz"
                            actual  : "123"

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1 +1 @@
                            -xyz
                            +123
            Test:           TestEnableIAMNormal
iampostgres_test.go:95: Current password: 999
iampostgres_test.go:96:
            Error Trace:    iampostgres_test.go:96
            Error:          Not equal:
                            expected: "999"
                            actual  : "xyz"

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1 +1 @@
                            -999
                            +xyz
            Test:           TestEnableIAMNormal
iampostgres_test.go:101: Current password: 999

FAIL
FAIL github.com/transcom/mymove/pkg/iampostgres 16.315s```

@mr337

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

I don't think is related but may want to double check

{"level":"warn","ts":1570568300.1639302,"caller":"iampostgres/iampostgres.go:51","msg":"Waiting 250000000ms for IAM password to populate"}

250000000ms 😆

The test is failing due to this roughly put test I'm not super proud of

time.Sleep(time.Second)
. I was pretty hesitant to do it like so but didn't know another way. It makes an assumption that the go rountine with a predictable refresh and the test itself matching the gofunc refresh cycle will be in sync.

Since the added entropy broke that loose assumption. I think with another goroutine I can poll that password every 250ms to work around the entropy this PR makes. Thoughts?

@mr337

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

Interestingly I keep getting a failure. Feels like there's some kind of race condition? I get IAM Credentials are missing.

Also is this still an issue?

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

@mr337 - I feel like there is something up with the test with this wait in place. Maybe we can pair tomorrow?

@mr337

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

@mr337 - I feel like there is something up with the test with this wait in place. Maybe we can pair tomorrow?

Agreed and maybe some magic golang that I'm unaware can solve this. Lets pair when you got some free time.

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.