Add cloud secret managers

[tomasz-sadura]

Added impl. for AWS, GCP and Azure secrets managers. The code is in common-go to reuse for linter.

[nicolaferraro]

Very nice.
In cloud, we need to use the same lookup logic in the API linter, is it possible (also as followup) to expose here some public service that we can reuse?

I'm not sure if the cloud project can be depend on connect tbh...

[nicolaferraro]

I'm thinking we might move the lookup code here: https://github.com/redpanda-data/common-go

[tomasz-sadura]

Yea. So the code in RPCN currently reads secret values, and the RPCN requires perms to do so. If we want to check secrets from the API we need some perms here as well. On one hand we do not need to read the secret, just check if it exists. On the other hand, we also extract JSON fields form the secret value, so we actually need to read it.
So finally - yes, we can have linter to check the secrets and add permissions for the API to do so.

[bojand]

We can probably pass context and use the client's GetSecretValueWithContext() method.
https://docs.aws.amazon.com/sdk-for-go/api/service/secretsmanager/#SecretsManager.GetSecretValueWithContext

[bojand]

can we maybe comment some of this stuff so it's clearer why we have the prefix and what's being done here

[bojand]

maybe it's just me but it seems weird and excessive to wrap a single line of code into a new function that returns the same return value as the called function.

strings.TrimPrefix(path, "/")

seems clear enough not to have to warrant a name like trimLeftSlash

[tomasz-sadura]

yea, at first this was meant to be reused, but then moved to a common file. Indeed no longer sense for it, inlined.

[rockwotj]

Does the v2 one have the APIs you need?

[rockwotj]

We're not going to use redis in cloud - can we keep this implementation in this repo? In case others want to add stuff - we should decouple cloud from whatever the community wants to add.

[tomasz-sadura]

Is that what you mean?: 667da58

[tomasz-sadura]

and: redpanda-data/common-go#35

[rockwotj]

Kind of - but now the cloud secret managers are removed? Also the lookup thing is a little clunky because it's not used here (but I guess will be in linting).

Maybe the right thing here is to do something like this:

Make LookupFn a single method interface instead. All the implementations in connect only need to implement that single lookup function. The switch statement of which URNs are supported is in this repo.

Secondarily in the common-go repo, have the implementations of the cloud secret managers that are used here in the switch statement. The ones in common-go will satisfy an additional interface (ontop of the LookupFn method) that provides existing. The switch statement for URNs and linting can be duplicated in cloudv2 since the logic to determine which secret manager to use will be based on the cloud account, then we won't be required to stuff the parameters into a URN (which is really only an artifact of it being a commandline arg, but we don't need to do that when linting in the connect api).

[tomasz-sadura]

Yes. It makes perfect sense. Wanted to reuse the secret provider creation mechanism (from URL), but this way looks a lot simpler, thanks!

[rockwotj]

Seems OK to me - I would prefer the logic to setup cloud config from a URL be in this repo.

I will let @Jeffail make the final call here.

[Jeffail]

Ideally I'd want the secret manager clients to be within this repo, or at least that the library we're referencing (common-go) to be tagged at a stable version. The problem with this arrangement is that breaking changes in common-go could break custom builds of RPCN if they end up with an incompatible dependency graph. It's not likely but I've seen it happen in the wild already.

In the interest of getting a release out this evening I think this is fine to merge and tag, but it'd be good to do the above in time for the next release if possible.

[tomasz-sadura]

Just tagged it but was 2 minutes late. Feel free to update the dep to v0.1.0.