feat: Add Consul KV integration for template variables (GH-473)#850
feat: Add Consul KV integration for template variables (GH-473)#850DeekshithaTimmareddy wants to merge 9 commits into
Conversation
DeekshithaTimmareddy
force-pushed
the
feat/consul-kv-integration-gh-473
branch
from
0f45210 to
6e6da00
Compare
- Add consulKey() and consulKeys() template functions to access Consul KV store - Add --consul-kv-address, --consul-kv-token, --consul-kv-namespace flags to run, plan, and render commands - Update PackManager and Renderer to support Consul client - Add comprehensive documentation with examples - Follows same pattern as existing Nomad Variables integration Implements #473
DeekshithaTimmareddy
force-pushed
the
feat/consul-kv-integration-gh-473
branch
from
6e6da00 to
6cd6fa5
Compare
jrasell
left a comment
jrasell
left a comment
There was a problem hiding this comment.
Thanks for the changes @TIMMAREDDYDEEKSHITHA-ship-it! There is still a bit of code duplication I think we can tidy up. It would also be good to think about what tests we can add as part of this PR, as currently, we are adding ~600 LOC without one test.
jrasell
left a comment
jrasell
left a comment
There was a problem hiding this comment.
The Consul client setup is slightly duplicated in the run, plan, and render commands. It'll be good to use a single helper function that is called to generate a client if needed, that includes all the config parsing and precedence handling.
In the documentation it's probably a good idea to call out that the presence of the Consul address indicates Nomad Pack will attempt to generate an API client.
The manual testing steps are great, but this doesn't cover automated CI and adds additional manual overhead on all code review. It would also be great to have some minimal tests to cover the Consul API functionality. I think there are a few possibilities here:
- have a basic test HTTP server that can be used to mock the Consul API and return data
- run Consul in dev mode via helper scripts, populate with data in the test
- check if Consul has a similar HTTP test agent that can be used in tests
jrasell
left a comment
jrasell
left a comment
There was a problem hiding this comment.
Thanks @TIMMAREDDYDEEKSHITHA-ship-it, I think this is getting close. Along with some inline comments, it would be really good to have some additional testing of the new code, including the template functions (via a mocked http server).
| } | ||
|
|
||
| // Package-level variable to track if Consul flags have been registered | ||
| var consulFlagsRegistered = false |
There was a problem hiding this comment.
This is a global variable that persists across all command instances within the same process. Because run, plan, and render all call AddFlagsToSet, only the first command to call it will actually register Consul flags. All subsequent calls silently return early. This also breaks any test that creates more than one command instance, since the flag state is never reset. We should remove the global and use per-flag.Sets or similar if needed.
|
|
||
| // AddFlags adds all Consul KV configuration flags to the provided flag set. | ||
| // This method is called by run, plan, and render commands to register flags. | ||
| func (c *ConsulKVConfig) AddFlags(flags *flag.FlagSet) { |
There was a problem hiding this comment.
This function seems unused, so we can remove it and the import it creates.
| // Returns nil client if no Consul address is configured (not an error). | ||
| // The presence of a Consul address (via CLI flag or CONSUL_HTTP_ADDR env var) | ||
| // indicates that Nomad Pack should attempt to create a Consul API client. | ||
| func getConsulClient(consulKV *ConsulKVConfig, errorContext *errors.UIErrorContext, ui terminal.UI) (*consulapi.Client, error) { |
There was a problem hiding this comment.
errorContextandui` are accepted but never referenced in the function body. Should we be using them, or if not, lets remove them from the function signature.
| wantAddr string | ||
| wantToken string |
There was a problem hiding this comment.
It doesn't look like these parameters are ever tested via an assertion. Is this something we want to add, or should we remove them?
| os.Setenv(k, v) | ||
| defer os.Unsetenv(k) |
There was a problem hiding this comment.
In tests we can use t.Setenv(k, v): https://pkg.go.dev/testing#T.Setenv
2 participants
Description
Adds support for retrieving variables from Consul's Key-Value store in pack templates, addressing feature request #473.
This implementation adds two new template functions:
consulKey(key)- Retrieves a single key-value pairconsulKeys(prefix)- Retrieves multiple key-value pairs with a given prefixCurrently, variables must be provided via CLI or files, which can be limiting for dynamic configurations. This feature enables packs to fetch configuration directly from Consul KV, similar to the existing Nomad Variables integration.
Changes
consulKey()andconsulKeys()template functionsConsulClientfield to Renderer structrun,plan,render) to support Consul KV flags:--consul-kv-address- Consul server address--consul-kv-token- Consul ACL token--consul-kv-namespace- Consul namespace (Enterprise)docs/functions.mdgo.modto include Consul API as direct dependencyTesting Steps
Building the nomad-pack and verifying the flag exists
After creating variables.hcl and metadata.hcl files
Successful render
Test - Error Handling
wrong port(9999) gave clear error which shows proper error handling when Consul is unavailable
Reminders
CHANGELOG.mdentryChanges to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.