Manually construct pop URL when using iampg auth #9538
Conversation
* Manually construct pop URL when using iampg auth * Use pop store to ping the db instead of making a new connection
ahobson
force-pushed
the
adh-fix-pop-url-iam
branch
from
9f0e0e4 to
d9b2149
Compare
| dbConnectionDetails.URL = fmt.Sprintf(s, | ||
| dbConnectionDetails.User, dbConnectionDetails.Password, | ||
| dbConnectionDetails.Host, dbConnectionDetails.Port, | ||
| dbConnectionDetails.Database, dbConnectionDetails.OptionsString("")) |
There was a problem hiding this comment.
If we changed the passHolder to something that does not need query escaping, we wouldn't have to do this assignment to URL, but that seems like leaving a footgun around, even if we comment on it.
There was a problem hiding this comment.
@ahobson I think we might be moving quickly on this. I'm concerned this may have a downstream effect on to an issue that I'm currently working with others in the Platform team on an issue with iampostgres.go not being able to exit. Could we please discuss the desired outcome and how this will be testable and ensure no downstream effect?
|
Also, @jesset-truss I believe you're right here with the issue of how this will affect our DMS security solution with ECS. We should be leveraging the AppContext package and the DB Connection library from within our custom ECS solution. We should add this to our system architecture diagrams around our DMS ECS task solution. |
Sure, but this isn't changing any behavior that would affect iampostgres.go, it's just about escaping the placeholder password. The reason iampostgres doesn't quit is because the code is using go channels and the code in dbconn.go isn't providing a way to quit even though the iampostgres code provides that capability. I feel these are unrelated issues, but can work through how to fix the shutdown issue with you if you want |
Bumps [github.com/gobuffalo/pop/v6](https://github.com/gobuffalo/pop) from 6.0.6 to 6.0.8. - [Release notes](https://github.com/gobuffalo/pop/releases) - [Changelog](https://github.com/gobuffalo/pop/blob/main/.goreleaser.yml) - [Commits](gobuffalo/pop@v6.0.6...v6.0.8) --- updated-dependencies: - dependency-name: github.com/gobuffalo/pop/v6 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
|
Talked to @ahobson and decided to cherry-pick my previous Pop 6.0.8 changes from #9390 (which we reverted in #9534) into this PR since we'll need his IAM fixes from this PR, the Pop 6.0.8 upgrade changes, and the fixes I previously made in #9390 for Pop 6.0.8. We'll test all this together in experimental before we consider merging anything. |
* Before the migration code expected in that certain errors would return both a connection and an error, which is very much not idomatic go and was confusing. Now we always retry, regardless of error.
There was a problem hiding this comment.
LGTM! Walked through things with @ahobson and the latest code gets past all checks on experimental. Note that this PR has some of my code from #9390, but that had already been vetted in that PR.
Summary
After gobuffalo/pop#774 iam password auth no longer works because pop is query escaping the password. Manually construct the pg url when iam auth is enabled
Verification Steps for Author
These are to be checked by the author.