aws secrets provider #153
aws secrets provider #153
Conversation
JaCoCo agent module code coverage report - spark:2 - scala 2.12.18
|
JaCoCo server module code coverage report - scala 2.13.11
|
| val layer: ZLayer[Any, Config.Error, AwsSecretsProviderImpl] = ZLayer { | ||
| for { | ||
| awsConfig <- ZIO.config[AwsConfig](AwsConfig.config) | ||
| } yield new AwsSecretsProviderImpl(SecretsManagerClient.builder().region(Region.of(awsConfig.region)).build()) |
There was a problem hiding this comment.
I like the fact that you used the region from the config file, very awesome!!!
TebaleloS
requested review from
benedeki,
lsulak,
Zejnilovic and
dk1844
as code owners
# Conflicts: # project/Dependencies.scala # server/src/main/resources/reference.conf # server/src/main/scala/za/co/absa/atum/server/Main.scala
JaCoCo agent module code coverage report - scala 2.12.18
|
JaCoCo model module code coverage report - scala 2.12.18
|
| @@ -10,6 +10,10 @@ | |||
| # maximum number of connections that HikariCP will keep in the pool, including both idle and in-use connections | |||
| maxPoolSize=10 | |||
| } | |||
| aws { | |||
| region = "af-south-1" | |||
| serviceUserSecretKey = "serviceUserSecretKey" | |||
There was a problem hiding this comment.
I would definitely mention DB here. This is for secret for accessing database
| serviceUserSecretKey = "serviceUserSecretKey" | |
| secretNameForDBServiceUserPwd = "changeme" |
or something like that..or
| serviceUserSecretKey = "serviceUserSecretKey" | |
| postgresPwdSecretName = "changeme" |
There was a problem hiding this comment.
I was actually not entirely sure I like this to be here, as I think that it's closer to the DB. But I understand that we might have more AWS stuff and it can belong close to each other for having better visibility of what's used from AWS...but hmm on the other hand, if working with the DB, such config-using code would be using other unrelated things, such as config for SSM or so...
I guess I'm fine with this currently
There was a problem hiding this comment.
You are right, it wasn't obvious it's related to a database. And as you mentioned, it's difficult to foresee how things will evolve, what sort of AWS services we will be using. And even then we will face the dilemma of whether to group it by aws service or by technology. Anyway I have changed the parameter to dbPasswordSecretName as I think it makes it clear and still isn't crazy long. Hope that's acceptable.
There was a problem hiding this comment.
it is, thanks it looks better :)
There was a problem hiding this comment.
Apart from the config name for the DB's PWD in AWS Secrets Manager, it looks really good and fully functional.
- code reviewed
- pulled
- built
- ran tests
- ran on localhost with real configuration, created partitioning against local server and checked DEV DB - it all worked very nicely end to end
Reads credentials from AWS Secrets Manager.
Closes #107