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

Vault 28310 hvs integrations poc #125

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

pmmukh
Copy link

@pmmukh pmmukh commented Jun 25, 2024

Changes proposed in this PR:

This PR shows one possible way that integrations support could be added to the CLI. The goal of this approach is to achieve this without having bespoke CLI subcommands for each integration type, instead having a generic CLI pattern into which integration specific details can be added in. The approach to support creating rotating secrets also follows a similar mentality and pattern.

For integration creation, a config file input is supported to a create subcommand, like hcp vs integrations create --config-file=config.yaml. The config file will contain a type, based on which the CLI code will pick the endpoint to call and request params to parse out, a name for the integration name, and a details struct containing details needed to create the integration.

For rotating secret creation, a config file input is supported to a create-rotating subcommand, like hcp vs secrets create-rotating --config-file=mongo-config.yaml. The config file here also contains a type and name, alongside integration specific details, and a details field for any additional details needed to create the secret.

The POC code and the 2 example config files, deal only with creating a mongodb rotationg integration, and a mongodb rotating secret. Below are screenshots showing how they can be used.

Screenshot 2024-06-28 at 2 16 55 PM Screenshot 2024-06-28 at 2 17 16 PM

How I've tested this PR:

How I expect reviewers to test this PR:

Checklist:

  • Tests added if applicable
  • CHANGELOG entry added or label 'pr/no-changelog' added to PR

    Run CHANGELOG_PR=<PR number> make changelog/new-entry for guidance
    in authoring a changelog entry, and commit the resulting file, which should
    have a name matching your PR number. Entries should use imperative present
    tense (e.g. Add support for...)

@pmmukh
Copy link
Author

pmmukh commented Jun 28, 2024

The upside of this approach, is that we have no integration specific subcommands, and so our command structure is relatively uncluttered.

The downsides of this approach, are that (a) we'll need to do some integration specific handling for now anyways in the CLI, and (b) without some additional backend changes to furnish a help blurb detailing the expected fields to fit into the details struct for each integration and secret, it would be really hard to use. On the point of docs, in our external CLI docs, it would be a little hard to show what the expected things in details would need to be based on type

The alternative to this approach, at least for integrations, would be one where integrations have their own bespoke subcommands like integrations create-twilio, with the information needed captured via CLI flags or well defined config files. This would be with the goal that in the future where a generic creation flow can be better supported, bespoke commands like these would get deprecated.

@pmmukh
Copy link
Author

pmmukh commented Jun 28, 2024

The config files in the screenshots are present in this PR, with the sensitive data parts being stubbed out. Another thing to keep in mind, is that with a config file approach we probably want to have some sort of templating support, so that data in env vars can be used easily in the files.

@mercedesbh
Copy link
Contributor

mercedesbh commented Jul 1, 2024

The upside of this approach, is that we have no integration specific subcommands, and so our command structure is relatively uncluttered.

The downsides of this approach, are that (a) we'll need to do some integration specific handling for now anyways in the CLI, and (b) without some additional backend changes to furnish a help blurb detailing the expected fields to fit into the details struct for each integration and secret, it would be really hard to use. On the point of docs, in our external CLI docs, it would be a little hard to show what the expected things in details would need to be based on type

The alternative to this approach, at least for integrations, would be one where integrations have their own bespoke subcommands like integrations create-twilio, with the information needed captured via CLI flags or well defined config files. This would be with the goal that in the future where a generic creation flow can be better supported, bespoke commands like these would get deprecated.

Hmmm, this is interesting. Definitely seems like the approach where each integration type has its own subcommands would (a) be easier to document on the hcp cli docs page and (b) match to API endpoints better since there's one for each type. This would obviously be quite different from the standard CRUD operations across other command groups, but seems like it might be the most helpful for users. Would be interested in @dadgar opinion here.

@dadgar
Copy link
Collaborator

dadgar commented Jul 2, 2024

So two main thoughts:

  1. Nit but I would use HCL and not yaml, as is fairly standard with HashiCorp tooling.
  2. The config file based approach looks right to me overall. However, I would expect the config file to be unmarshalled into a standard request message that gets sent to the server and the server side looks at the type + Details payload and does the validation / creation and sends a standard payload back down. Trying to have the CLI know about every possible integration type / validation will be challenging to maintain, especially if integrations become more of a marketplace which I think is the long term direction.

@briankassouf
Copy link

without some additional backend changes to furnish a help blurb detailing the expected fields to fit into the details struct for each integration and secret, it would be really hard to use

This is a valid point, but i think we can assume we will have this API at some point. We can then add some helper commands to the CLI like:

hcp vs secrets creation-help --type=twilio

or 

hcp vs secrets create-config-template --type=twilio --output=/path/to/my/config.hcl

@pmmukh
Copy link
Author

pmmukh commented Jul 3, 2024

Thanks everyone for the feedback!!
@dadgar for the file format, so yaml was just used as an example, I think our current plan is to support multiple input formats i.e json/yaml/hcl, so whichever makes most sense for users. I figure with the same Go struct and multiple struct tags that should be possible.
And regarding the part about the standard request message with type specific parsing happening in the backend, that's definitely something we're going to be exploring as we plan this out further. Cause having provider specific code in the CLI will for sure hamper the long term vision of pluggable integration types.

@briankassouf ahh perfect, yeah if we have an API to support such help and sample config commands, that'll be awesome!

For further followups around this work, I'll leave y'all in the very capable hands of @mercedesbh who'll take over here while i move on to PnP work.

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.

4 participants