- Notifications
You must be signed in to change notification settings - Fork928
feat: add awsiamrds db auth driver#12566
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
My tests are failing over atcoder/coder#12566, enums are failing to marshal correctly, and this patch is what made it work on my branch when it was `clibase`.
@@ -154,6 +166,7 @@ type DeploymentValues struct { | |||
CacheDir serpent.String `json:"cache_directory,omitempty" typescript:",notnull"` | |||
InMemoryDatabase serpent.Bool `json:"in_memory_database,omitempty" typescript:",notnull"` | |||
PostgresURL serpent.String `json:"pg_connection_url,omitempty" typescript:",notnull"` | |||
PostgresAuth string `json:"pg_auth,omitempty" typescript:",notnull"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a specific string format the connection has when connecting via AWS IAM? I think it'd be preferred to check for that over adding a new server option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just a missing password field, maybe an AWS domain in the URL, but I don't think there's anything that's enough to make an inference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
One thing that might be possible is if we detected the URL scheme and had people setawsiamrds://...
instead ofpostgres://
, or add a query param? It is unforunate we need a new flag just for aws auth, but it's probable we would exapand this in the future. My only worry is that just detecting it in the connection string would be non-obvious, since I haven't really seen it anywhere else, but to be fair I haven't really done much research.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah I wouldn't wanna change the protocol because that kinda implies it's not speakingpostgres
protocol. I think given we may have other auth schemes in the future we need to support, a flag seems fine to me and better than any parsing options I can think of.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#11669
Closeshttps://github.com/coder/customers/issues/413