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

feat(aws iam): support aws iam auth for postgresql components #3324

Merged
merged 70 commits into from May 16, 2024

Conversation

sicoyle
Copy link
Contributor

@sicoyle sicoyle commented Jan 29, 2024

Description

Allow for AWS IAM auth with postgres v2. These changes rotate the authentication token automatically before the 15 minute expiration on the authentication token.

Adding support for v1 and v2 postgres statestore and config and bindings postgres types.

Please note that I removed the master connection string setup I had initially. There is no reason to use aws iam enabled on postgres and a master connection string, as the dapr user could just use the regular postgres connection string auth for that. I now have it setup to where you just use an iam enabled username and the pwd is auto generated based on the aws config file or the secretkey/accesskey for the iam enabled user.

Issue reference

#3254

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

…eds automatically

Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
@sicoyle sicoyle requested review from a team as code owners January 29, 2024 01:16
sicoyle and others added 2 commits January 28, 2024 19:17
Copy link
Contributor

@ItalyPaleAle ItalyPaleAle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR.

Please move all the code for authenticating with AWS IAM to a shared package. If it's specific for Postgres, it should be in one of the common packages we have for Postgres.

This way, you can also enable it for all PG components (not just the state store v1, but also the binding and configuration store)

common/authentication/postgresql/metadata.go Outdated Show resolved Hide resolved
common/authentication/postgresql/metadata.go Outdated Show resolved Hide resolved
configuration/postgres/metadata.go Outdated Show resolved Hide resolved
state/postgresql/v2/metadata.go Outdated Show resolved Hide resolved
bindings/postgres/metadata.go Outdated Show resolved Hide resolved
common/authentication/postgresql/metadata.go Outdated Show resolved Hide resolved
common/component/postgresql/v1/postgresql.go Show resolved Hide resolved
state/postgresql/v2/postgresql.go Outdated Show resolved Hide resolved
@ItalyPaleAle ItalyPaleAle added this to the v1.14 milestone Jan 30, 2024
@sicoyle sicoyle changed the title feat(aws iam): support aws iam auth for postgresql v2 feat(aws iam): support aws iam auth for postgresql v1 & v2 Feb 4, 2024
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
@sicoyle
Copy link
Contributor Author

sicoyle commented May 3, 2024

Can I get help/advice on what y'all want for the dependencies pls? From what I see is that the certification tests go.mod dependencies are behind what the main module dependencies are for kit and dapr. However, the main module is using kit:

github.com/dapr/kit](http://github.com/dapr/kit) v0.13.1-0.20240306152601-e33fbab74548

And contrib in main branch also has dependencies on dapr/dapr v1.13.2 bc of fields brought into

http://github.com/forks/dapr/components-contrib/tests/certification/embedded/embedded.go

For:

DaprHTTPMaxRequestSize:    maxRequestBodySize,
DaprHTTPReadBufferSize:    runtime.DefaultReadBufferSize,

How should I proceed with fixing? bc my cert tests still fail but the main module dependencies if I bump the cert tests to are incompatible...

@sicoyle
Copy link
Contributor Author

sicoyle commented May 15, 2024

this is ready pls

@@ -50,7 +50,7 @@ const (
maxConcurrency = -1
enableMTLS = false
sentryAddress = ""
maxRequestBodySize = 4
maxRequestBodySize = 4 << 20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this magic number do?

Copy link
Contributor Author

@sicoyle sicoyle May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @JoshVanL pls lmk if I'm missing anything on the below 👇 . This is based on chatting with him on it previously.

Copy link
Contributor Author

@sicoyle sicoyle May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That change Josh recommended we use to help standardize the unit of measurement of config options, so that number now represents bits or bytes copied from dapr/dapr#7546

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see here in that pr where they are using similarly
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@berndverst lmk if you need further clarification

@berndverst
Copy link
Member

@sicoyle I mentioned a few times to remove the patch file from your PR please. The 3346.patch file should not be in the PR. I will not approve and merge until this file is gone.

@berndverst berndverst merged commit eb82293 into dapr:main May 16, 2024
85 of 91 checks passed
ItalyPaleAle added a commit that referenced this pull request May 21, 2024
Fixes some bugs and other issues, including unnecessary metadata options and uneven info in `metadata.yaml` files, introduced in #3324
@ItalyPaleAle ItalyPaleAle mentioned this pull request May 21, 2024
ItalyPaleAle added a commit that referenced this pull request May 21, 2024
Fixes some bugs and other issues, including unnecessary metadata options and uneven info in `metadata.yaml` files, introduced in #3324

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
ItalyPaleAle added a commit that referenced this pull request May 21, 2024
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants