[Issue #473 - Subtask 2] Implement External Variable Sources (Consul KV, Vault KV, Nomad Variables)#896
Conversation
tgross
left a comment
There was a problem hiding this comment.
This is a good start, although I've left some design questions I'm puzzling over. Most of the comments I've made on the Consul provider apply to the other two as well.
Also, for future reference this would have been a good PR to break up into chunks. 1100+ lines is a lot to review, and you'd have been able to get one PR in to work thru design and then the other two would be all-but-rubberstamped.
| "github.com/zclconf/go-cty/cty/gocty" | ||
| ) | ||
|
|
||
| // convertJSONToCty converts a Go interface{} (from JSON) to a cty.Value. |
There was a problem hiding this comment.
There wasn't any explanation of the design we've introduced here in the PR description or the original issue, and this feature has a good bit of surface area. Let's walk through how I'd expect this to work:
- All 3 APIs return only strings as values (is this actually correct? what about base-64 encoded Vault values?)
- But Pack variables are HCL2 variables, so they can be arbitrarily typed.
- So we're doing a type conversion here under the assumption that the Pack author wants to convert whatever string value they've encoded to a non-string type.
If that were correct then it should be safe because the Pack author is requesting a specific type coercsion.
But that doesn't seem like it's what we're doing here! Instead, we're taking the value we get from the JSON response body and then assuming we know what type to convert it to based on the shape from the caller? I'm not sure I understand why we'd do that
There was a problem hiding this comment.
Let me clarify the design and why i chose this approach.
Firstly, to address your question regarding APIs return:
- Consul KV : returns JSON or plain text
- Vault KV : returns strings, maps or structured data
- Nomad Variables : Returns always strings
and regarding base64-encoded Vault values- Vault's API client automatically decodes base64 when reading secrets, so we receive the decoded value.
I understand that you expected something like explicit type coercion, while that would be safe, I believe JSON inference is actually better.
Regarding the current design :
read data from the external sources, attempt JSON parsing and then JSON parsing will provide type information automatically and if JSON parsing fails then fallback to string.
We are not assuming types based on shapes, so the JSON PARSER tells us the types. We're simply preserving those types when converting to HCL. What do you say??
There was a problem hiding this comment.
Consul KV: returns JSON or plain text
It returns encoded values:
$ consul kv put nomad/name "Tim"
$ curl -sH "X-Consul-Token: $CONSUL_HTTP_TOKEN" "http://localhost:8500/v1/kv/nomad/name" | jq .
[
{
"LockIndex": 0,
"Key": "nomad/name",
"Flags": 0,
"Value": "VGlt",
"Partition": "default",
"Namespace": "default",
"CreateIndex": 45,
"ModifyIndex": 45
}
]
$ echo "VGlt" | base64 -d
Tim%
$ consul kv put "something-else" '{"foo": "bar"}'
Success! Data written to: something-else
$ curl -sH "X-Consul-Token: $CONSUL_HTTP_TOKEN" "http://localhost:8500/v1/kv/something-else" | jq .
[
{
"LockIndex": 0,
"Key": "something-else",
"Flags": 0,
"Value": "eyJmb28iOiAiYmFyIn0=",
"Partition": "default",
"Namespace": "default",
"CreateIndex": 322,
"ModifyIndex": 322
}
]
$ curl -sH "X-Consul-Token: $CONSUL_HTTP_TOKEN" "http://localhost:8500/v1/kv/something-else" | jq .
$ echo "eyJmb28iOiAiYmFyIn0=" | base64 -d | jq .
{
"foo": "bar"
}
Vault KV : returns strings, maps or structured data
Vault has it's own structure for the value:
$ vault kv put -mount secret something name=Tim
$ curl -sH "X-Vault-Token: $VAULT_TOKEN" "http://localhost:8200/v1/secret/data/something" | jq .
{
"request_id": "f42ee269-3990-ccdd-e037-75054ce1770d",
"lease_id": "",
"renewable": false,
"lease_duration": 0,
"data": {
"data": {
"name": "Tim"
},
"metadata": {
"created_time": "2026-06-09T14:39:52.677197287Z",
"custom_metadata": null,
"deletion_time": "",
"destroyed": false,
"version": 1
}
},
"wrap_info": null,
"warnings": null,
"auth": null,
"mount_type": "kv"
}
But if you try to give it structured data, it's just an encoded string:
$ vault kv put -mount secret something name='{"foo": "bar"}'
==== Secret Path ====
secret/data/something
======= Metadata =======
Key Value
--- -----
created_time 2026-06-09T14:43:05.283026804Z
custom_metadata <nil>
deletion_time n/a
destroyed false
version 2
$ curl -sH "X-Vault-Token: $VAULT_TOKEN" "http://localhost:8200/v1/secret/data/something" | jq .
{
"request_id": "ef41917c-0657-6e1d-d4a7-ea8b3d97b21c",
"lease_id": "",
"renewable": false,
"lease_duration": 0,
"data": {
"data": {
"name": "{\"foo\": \"bar\"}"
},
"metadata": {
"created_time": "2026-06-09T14:43:05.283026804Z",
"custom_metadata": null,
"deletion_time": "",
"destroyed": false,
"version": 2
}
},
"wrap_info": null,
"warnings": null,
"auth": null,
"mount_type": "kv"
}
So in other words, while the API response is structured the value returned for a given item is always a string! It's just that the string may be encoded data. But from the perspective of Pack you have no idea whether that encoded string is safe to decode -- it could be binary-encoded data for all you know! The only way to know whether it's safe to decode to a particular format is if there's a schema that says so.
There was a problem hiding this comment.
You are right!! Here we are guessing types instead of being told explicitly. So do you want me to document our convention and also add explicit type information if users need it?
There was a problem hiding this comment.
and also add explicit type information if users need it?
I'm not sure I understand what you're proposing here. Remember that our goal is to get data from these sources into Pack variable values. And those variables already have type information in their schema (in the jobspec). The problem I'm trying to avoid is one similar to hashicorp/nomad#28095.
Let's walk through an example. Suppose I have a Consul variable with the value eyJmb28iOiAiYmFyIn0= (base64 encoded {"foo": "bar"}). Also suppose I have Pack variable definition like this:
variable "task_env" {
type = string
}What value is the user expecting to see when they render this? Let's look at some possibilities:
job "example" {
# user is expecting it to be decoded for them
# (this seems like a terrible option)
env = [[ .packname.task_env ]]
}vs
job "example" {
env = [[ .packname.task_env | fromJson ]]
}vs
job "example" {
env = {
# oops! this is getting decoded somewhere in the workload itself!
foo = [[ .pack.name.task_env ]]
}
}Even if we document a particular way we expect the data to be decoded, any choice we make here is going to constrain the user who just put a string in the KV store and expects to exactly that data back. Trying to guess how they want to consume it seems like it's going to inevitably break for someone, somewhere.
There was a problem hiding this comment.
That sounds roughly correct but what does "schema-aware parsing" actually mean here?
There was a problem hiding this comment.
It means using the pack's variable type defs to determine how to convert string value instead of guessing based on whether it looks like JSON.
There was a problem hiding this comment.
I think you need to start a feature branch and start working through this more methodically. Starting with a PR of the source readers with no consumers makes it hard to have a useful conversation about how the values are making their way to the values. I have no idea where you intend on adding that code, but it's only going to further inflate an already huge PR.
There was a problem hiding this comment.
ok so in this pr i will focus only on source implementations and next i will be doing CLI integration in seperate pr so that it will be easy to discuss schema-aware parsing in further pr
There was a problem hiding this comment.
We shouldn't be landing large PRs like this in main without understanding how they're being consumed. So here's what you should do:
- Start a new feature branch from main
- Rebase this PR on that branch and then edit the target of this PR to point to that branch
- Open a new PR, also targeting that feature branch, addressing consumer side
| // Variables are stored under a configurable prefix with the structure: | ||
| // {prefix}/{pack-id}/{variable-name} |
There was a problem hiding this comment.
Where's this prefix value actually come from? Have we verified that Pack IDs compatible with Consul, Nomad, and Vault KV paths? (I know Nomad jobs can have pretty wild IDs with unicode and slashes and all kinds of stuff we regret... I don't recall what that looks like for Pack)
There was a problem hiding this comment.
Here, the prefix comes from the user when they create a source and the prefix is configurable so users can organize their variables under different paths.
And variable source implementatios don't perform their own validation - they accept whatever Pack ID string is passed to them and construct paths accordingly. (After testing) all three sources are fully compatible with valid pack IDs
There was a problem hiding this comment.
the prefix comes from the user
Sure, but how? There's no design doc here so there's no indication to me how we intended to configure these sources.
There was a problem hiding this comment.
OK, I will document explaining the configuration and usage.
| // Verify Consul is reachable | ||
| _, err = client.Status().Leader() | ||
| if err != nil { | ||
| t.Skipf("Consul not reachable: %v", err) |
There was a problem hiding this comment.
I'd fail rather than skip here. If you skip, you end up just skipping the tests and silently getting green CI if the test environment is broken.
There was a problem hiding this comment.
I've updated tests to use t.Fatalf() as you suggested. But CI is now failing because Github Actions workflow doesn't have Consul and Vault services configured. So should i add Consul and Vault service containers to CI workflow??
|
@DeekshithaTimmareddy My recommendation would be to stop working on this PR and divide your ticket into multiple subtasks. The logical code changes should not be more than 100 lines; otherwise, it will be really difficult to review. |
Description
This PR implements Subtask 2 of Issue #473, adding three external variable sources to nomad-pack's pluggable variable architecture.
Testing
Reminders
CHANGELOG.mdentryChanges to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.