Skip to content

Docs: create your own parameters provider - Vault #2250

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

Closed
1 task done
dreamorosi opened this issue May 12, 2023 · 6 comments · Fixed by #3090
Closed
1 task done

Docs: create your own parameters provider - Vault #2250

dreamorosi opened this issue May 12, 2023 · 6 comments · Fixed by #3090
Assignees
Labels
documentation Improvements or additions to documentation revisit-in-3-months Requires more customers feedback before making or revisiting a decision

Comments

@dreamorosi
Copy link
Contributor

What were you searching in the docs?

I am looking at creating my own Parameters provider and followed the example that shows how to create a provider for HashiCorp Vault. I'm doing this work because I'm writing a similar piece for the TS version.

The VaultProvider.get method has a return type of str, however after testing the code with Vault, I think it's not correct as at the best of my knowledge you can't store plain text strings into Vault and the SDK returns at least dictionary (key-value).

Steps to reproduce

  1. Run Vault in dev mode using Docker docker run -e VAULT_DEV_ROOT_TOKEN_ID=abcde --cap-add=IPC_LOCK -p 8200:8200 --name=dev-vault vault
  2. Open http://0.0.0.0:8200 in your browser and create a secret under the secret/ path (i.e. my-secret)
  3. The UI will allow you to create only a key-value secret and not a string (see image below) - so create one (i.e. { "foo": "bar" }

image

  1. Copy the two files in the docs: custom_provider_vault.py & working_with_own_provider_vault.py
  2. Update the following fields:
  • L13 -> vault_provider = VaultProvider(vault_url="http://0.0.0.0:8200/", vault_token="abcde")
  • L20 -> endpoint_comments: Any = vault_provider.get("my-secret", transform="json")
  1. Run the code

Comments

I find the implementation slightly confusing for two reasons:

  1. The self.vault_client.secrets.kv.v2.read_secret method already returns a dict because the secret is stored as a JSON in the first place
  2. Before returning the secret we stringify it (json.dumps) so that the method returns a string and then in the usage we pass transform="json" to transform it back to a dict, which seems counterintuitive.

Is this related to an existing documentation section?

https://awslabs.github.io/aws-lambda-powertools-python/2.15.0/utilities/parameters/#create-your-own-provider

How can we improve?

I think we should review the return type, and potentially the implementation of the VaultProvider.get method.

Got a suggestion in mind?

No response

Acknowledgment

  • I understand the final update might be different from my proposed suggestion, or refused.
@dreamorosi dreamorosi added documentation Improvements or additions to documentation triage Pending triage from maintainers labels May 12, 2023
@leandrodamascena
Copy link
Contributor

Hi @dreamorosi! Thank you for pointing that out!
Looking at it now.

@leandrodamascena
Copy link
Contributor

Hi @dreamorosi, you are 100% right! The return type for this method is wrong, it should be Dict[str, str], the same as _get_multiple. I'll send a PR with this fix and one more change.

Related to the implementation of the Vault provider as an example, I think Hashicorp Vault is a very specific case when you always have a key:value dictionary and we don't have much to do but return that dictionary. In the S3 provider example, we can return a string because we can read the file in S3 and return the content.

Are you thinking of implementing this same example in Typescript? Please let us know if you think we can improve this with a more accurate example, it will be awesome!!

Thanks so much for taking the time to help us improve our documentation, that's priceless!

@dreamorosi
Copy link
Contributor Author

Hey, yes, I was thinking of implementing the same in TS but it's not as simple.

VaultProvider would need to extend the BaseProvider and implement a _get method, which currently is defined in the BaseProvider like this:

protected abstract _get(
    name: string,
    options?: unknown
  ): Promise<string | Uint8Array | undefined>;

This means that VaultProvider can't return an object/dict since the BaseProvider at the moment allows only these other types.

I'm unsure about how to move forward if I'm being honest, but these are the things I'm considering:

On a conceptual level, the get method of any Parameters provider is supposed to return a single value not an object (at least regarding how I think about it). Returning an object would also be problematic for the current implementation of the transform mechanism.

With the above in mind, we could implement this method similar to this (pseudo code):

getFromVault(name, keyToGet, options) {
  const responseObject = client.getKeyValue(name);
  const secretValue = responseObject[keyToGet];
  return secretValue;
}

Note that in the code above we are asking the user to also tell us what's the key in the Vault secret that they want to get the value of.

This would solve the problems, however I'm unsure about two points:

  1. I wonder if this would make the example too complex/involved
  2. I wonder if we are (at least in TS) bending the implementation only to please the TypeScript types and we should instead relax the types

Sorry for the long message, I know it's a bit all over the place, but I'd really appreciate having this discussion at your own pace/time.

@leandrodamascena
Copy link
Contributor

Andrea, let me step back and correct some information that I got wrong. I was the one who created this documentation and looked at the notes I made at the time and the reason I put the return str is because in Python any new parameter provider will have to use the BaseProvider class as a base and implement the abstract methods _get and _get_multiple. In the BaseProvider class, the _get method implements the return as str or bytes and not as Dict[str,str], so, in this case, I won't be able to change the example. From what I'm seeing, it's pretty similar to what Typescript is doing too, right?
As I said in the previous comment, perhaps the case of VaultProvider is a very specific case, but it might make sense to consider, as many customers use Vault as a secrets provider (in addition to other things) and we can think of bringing this as one more provider in the Powertools package.

From the solution you proposed to relax the types, do you mention relaxing the type of the example or the base code? If it's in the example, I think that for now, we can keep it that way, but if it's in the base code, I think that in Python we can change the signature of the function _get to consider the return of str or bytes or Dict [str,str], I just need to check that we won't have any kind of breaking changes with this change. Does it make sense to do this in Typescript as well if we decide to do this?

In general, this is a good discussion for all utilities that we allow the customer to extend and build their own provider/code. What are the use cases that customers might have? Are the implementations we have of the base classes too restrictive? We will never cover 100% of cases and we may need to rethink as requests from the community arrive, but at least we will keep this in mind when implementing.

Please don't apologize for the message, this is a valid and necessary discussion for the Powertools (all runtimes) to improve the developer experience.

Thank you

@github-actions
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

@github-actions github-actions bot added the pending-release Fix or implementation already in dev waiting to be released label Sep 14, 2023
@github-actions
Copy link
Contributor

This is now released under 2.25.0 version!

@github-actions github-actions bot removed the pending-release Fix or implementation already in dev waiting to be released label Sep 15, 2023
@heitorlessa heitorlessa moved this from Coming soon to Shipped in Powertools for AWS Lambda (Python) Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation revisit-in-3-months Requires more customers feedback before making or revisiting a decision
Projects
Status: Shipped
2 participants