-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
cli: Add 'agent generate-config' sub-command #20530
Conversation
command/agent_generate_config.go
Outdated
var configPath string | ||
if len(args) == 1 { | ||
configPath = args[0] | ||
} else { | ||
configPath = "agent.hcl" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the default be stdout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe 🤷 We kind of want to give a warning about the token auto-auth method so I'm slightly leaning towards writing to a file. We could also require a file to be specified.
) | ||
|
||
// TestConstructTemplates tests the construcTemplates helper function | ||
func TestConstructTemplates(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after we add the config, should add tests that the generated config should be readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, we definitely should
Looks great! Added some comments so far but broadly it looks fantastic. One thing I'm also interested in: we should add docs for this subcommand. I don't really mind if that's as part of a different PR or if it's part of this one, but we should make sure docs are added. I'm happy to review them all the same. |
) | ||
|
||
// TestConstructTemplates tests the construcTemplates helper function | ||
func TestConstructTemplates(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, we definitely should
Thanks! Yes, docs is definitely next 👍 I will probably hold off on merging this till the docs PR is ready. |
Co-authored-by: Daniel Huckins <[email protected]>
Just to make sure we're not waiting on each other: I'm hoping to see the note about changing the auto_auth type and the additional tests for the other parts of generated config before I approve. Once these are added, feel free to re-request me as a viewer to ping me! |
Thanks! I added both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Nice work.
@@ -0,0 +1,3 @@ | |||
```release-note:feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@averche could you please update this changelog file using the format for announcing a new feature? https://hashicorp.atlassian.net/wiki/spaces/VAULT/pages/1311244491/Changelog+Process#New-and-Major-Features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhuckins Are these two PRs the same or different "features"? If you scroll through historical changelog sections for major releases, you can see what's been called out in the "features" section. For the features section, it's expected that features will have multiple PRs and it doesn't always make sense to have a changelog entry for every PR that composes a major new feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha, same feature (at least they are related). so just one should be fine. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like in the RC changelog this line is still showing up in the Features section - https://github.com/hashicorp/vault/pull/21077/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR43. Could someone update the file on the 1.14 release branch OR delete it on that branch please?
Background
This pull request implements a new sub-command
agent generate-config
. It is part of the larger effort to add environment variable support within Vault Agent (VLT-253).The goal of this sub-command is to help new users with an easy on-boarding to Vault, specifically for integrating Vault into their applications. It will recurse through the Vault path tree for the given path(s) and produce an
agent.hcl
configuration file withtoken_file
authentication method and default environment-variable-to-secret mappings. The file can then be used by Vault Agent in a process supervisor mode (will be implemented in future PRs).For the first release of this template configuration helper, we will only support KV-V1 and KV-V2 secret types.
Example
agent.hcl:
Related pull requests