diff --git a/fixtures/v2/simple_raw_exec_v2/variables.hcl b/fixtures/v2/simple_raw_exec_v2/variables.hcl index 79ef78bd..42eafb6d 100644 --- a/fixtures/v2/simple_raw_exec_v2/variables.hcl +++ b/fixtures/v2/simple_raw_exec_v2/variables.hcl @@ -46,8 +46,8 @@ nomad_variable "app_config" { path = "nomad/jobs/simple_raw_exec/config" items = { database_url = "postgres://localhost:5432/mydb" - api_key = "secret-api-key-123" - environment = "production" + api_key = "secret-api-key-123" + environment = "production" } } @@ -55,6 +55,6 @@ nomad_variable "secrets" { path = "nomad/jobs/simple_raw_exec/secrets" items = { admin_password = "super-secret-password" - jwt_secret = "jwt-signing-key-xyz" + jwt_secret = "jwt-signing-key-xyz" } } diff --git a/go.mod b/go.mod index aee5da8a..10791f9f 100644 --- a/go.mod +++ b/go.mod @@ -21,6 +21,7 @@ require ( github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc github.com/fatih/color v1.19.0 github.com/go-git/go-git/v5 v5.19.1 + github.com/hashicorp/consul/api v1.33.4 github.com/hashicorp/go-getter v1.8.6 github.com/hashicorp/go-hclog v1.6.3 github.com/hashicorp/go-multierror v1.1.1 @@ -192,7 +193,6 @@ require ( github.com/hashicorp/cap v0.12.0 // indirect github.com/hashicorp/cli v1.1.7 // indirect github.com/hashicorp/consul-template v0.41.4 // indirect - github.com/hashicorp/consul/api v1.33.4 // indirect github.com/hashicorp/consul/sdk v0.17.2 // indirect github.com/hashicorp/cronexpr v1.1.3 // indirect github.com/hashicorp/errwrap v1.1.0 // indirect diff --git a/internal/cli/commands.go b/internal/cli/commands.go index 205ce86c..d69005c7 100644 --- a/internal/cli/commands.go +++ b/internal/cli/commands.go @@ -71,6 +71,10 @@ type baseCommand struct { // for defined input variables varFiles []string + // varSources is a list of external variable source URLs + // (e.g., consul:///path) + varSources []string + // allowUnsetVars suppresses errors from variables with nil values, // i.e. those that are not set and have no default allowUnsetVars bool @@ -267,6 +271,31 @@ func (c *baseCommand) flagSet(bit flagSetBit, f func(*flag.Sets)) *flag.Sets { Shorthand: "f", }) + // --var-source performs remote reads (e.g. Consul KV) at render time, so + // it is only offered by commands that compute a fresh deployment + // (run/plan/render). Commands like stop operate on an already-deployed + // job and must not depend on a remote source still being reachable or + // its keys still existing. + if bit&flagSetExternalVarSources != 0 { + f.StringSliceVar(&flag.StringSliceVar{ + Name: "var-source", + Target: &c.varSources, + Default: make([]string, 0), + Usage: `Specifies an external variable source as a URL, and may be + given more than once to read from several sources. Consul KV + is the only source currently supported, using the form + consul://:/; for example, + consul://localhost:8500/nomad-pack. The host is optional: omit + it (consul:///) to use the standard Consul environment + configuration, such as CONSUL_HTTP_ADDR and CONSUL_HTTP_TOKEN. + Each variable is read from /, so include + any per-pack grouping in the path yourself. Variable sources + are applied in order of precedence, highest first: --var, then + --var-source, then --var-file, then environment variables. A + higher-precedence source overrides any lower one.`, + }) + } + f.BoolVar(&flag.BoolVar{ Name: "allow-unset-vars", Target: &c.allowUnsetVars, @@ -435,10 +464,11 @@ func (c *baseCommand) helpUsageMessage() string { type flagSetBit uint const ( - flagSetNone flagSetBit = 1 << iota // nolint:deadcode,varcheck,unused - flagSetOperation // shared flags for operations (run, plan, etc) - flagSetNeedsApproval // adds the -y flag for commands that require approval to run - flagSetNomadClient // adds client config flags + flagSetNone flagSetBit = 1 << iota // nolint:deadcode,varcheck,unused + flagSetOperation // shared flags for operations (run, plan, etc) + flagSetNeedsApproval // adds the -y flag for commands that require approval to run + flagSetNomadClient // adds client config flags + flagSetExternalVarSources // adds --var-source; only for commands that compute a fresh deployment (run, plan, render) ) var ( diff --git a/internal/cli/generate_varfile.go b/internal/cli/generate_varfile.go index 632286c7..b69861b2 100644 --- a/internal/cli/generate_varfile.go +++ b/internal/cli/generate_varfile.go @@ -142,7 +142,11 @@ func (c *generateVarFileCommand) Run(args []string) int { // until something sets them, like the var file we're trying to create!) c.allowUnsetVars = true - packManager := generatePackManager(c.baseCommand, nil, c.packConfig) + packManager, err := generatePackManager(c.baseCommand, nil, c.packConfig) + if err != nil { + c.ui.ErrorWithContext(err, "failed to generate pack manager", errorContext.GetAll()...) + return 1 + } renderOutput, err := renderVariableOverrideFile(packManager, c.ui, errorContext) if err != nil { return 1 diff --git a/internal/cli/helpers.go b/internal/cli/helpers.go index c10ec5e6..81d47565 100644 --- a/internal/cli/helpers.go +++ b/internal/cli/helpers.go @@ -18,6 +18,7 @@ import ( "github.com/hashicorp/nomad-pack/internal/pkg/manager" "github.com/hashicorp/nomad-pack/internal/pkg/renderer" "github.com/hashicorp/nomad-pack/internal/pkg/variable/parser" + "github.com/hashicorp/nomad-pack/internal/pkg/variable/source" "github.com/hashicorp/nomad-pack/internal/runner" "github.com/hashicorp/nomad-pack/internal/runner/job" "github.com/hashicorp/nomad-pack/sdk/pack/variables" @@ -40,17 +41,28 @@ func initPackCommand(cfg *caching.PackConfig) (errorContext *errors.UIErrorConte } // generatePackManager is used to generate the pack manager for this Nomad Pack run. -func generatePackManager(c *baseCommand, client *api.Client, packCfg *caching.PackConfig) *manager.PackManager { +func generatePackManager(c *baseCommand, client *api.Client, packCfg *caching.PackConfig) (*manager.PackManager, error) { + // Parse external variable source configurations if provided. + var externalSourceConfigs []source.SourceConfig + if len(c.varSources) > 0 { + configs, err := parseVarSourceConfigs(c.varSources) + if err != nil { + return nil, err + } + externalSourceConfigs = configs + } + // TODO: Refactor to have manager use cache. cfg := manager.Config{ - Path: packCfg.Path, - VariableFiles: c.varFiles, - VariableCLIArgs: c.vars, - VariableEnvVars: c.envVars, - AllowUnsetVars: c.allowUnsetVars, - UseParserV1: c.useParserV1, - } - return manager.NewPackManager(&cfg, client) + Path: packCfg.Path, + VariableFiles: c.varFiles, + VariableCLIArgs: c.vars, + VariableEnvVars: c.envVars, + AllowUnsetVars: c.allowUnsetVars, + UseParserV1: c.useParserV1, + ExternalSourceConfigs: externalSourceConfigs, + } + return manager.NewPackManager(&cfg, client), nil } // predictPackName is a complete.Predictor that suggests cached pack names. diff --git a/internal/cli/plan.go b/internal/cli/plan.go index ad258e9b..e995fdde 100644 --- a/internal/cli/plan.go +++ b/internal/cli/plan.go @@ -54,7 +54,11 @@ func (c *PlanCommand) Run(args []string) int { return c.exitCodeError } - packManager := generatePackManager(c.baseCommand, client, c.packConfig) + packManager, err := generatePackManager(c.baseCommand, client, c.packConfig) + if err != nil { + c.ui.ErrorWithContext(err, "failed to generate pack manager", errorContext.GetAll()...) + return c.exitCodeError + } // load pack r, err := renderPack( @@ -142,7 +146,7 @@ func (c *PlanCommand) Run(args []string) int { func (c *PlanCommand) Flags() *flag.Sets { c.packConfig = &caching.PackConfig{} - return c.flagSet(flagSetOperation|flagSetNomadClient, func(set *flag.Sets) { + return c.flagSet(flagSetOperation|flagSetNomadClient|flagSetExternalVarSources, func(set *flag.Sets) { f := set.NewSet("Plan Options") c.jobConfig = &job.CLIConfig{ diff --git a/internal/cli/render.go b/internal/cli/render.go index e28234d5..d0388155 100644 --- a/internal/cli/render.go +++ b/internal/cli/render.go @@ -280,7 +280,11 @@ func (c *RenderCommand) Run(args []string) int { c.ui.Error(err.Error()) return 1 } - packManager := generatePackManager(c.baseCommand, client, c.packConfig) + packManager, err := generatePackManager(c.baseCommand, client, c.packConfig) + if err != nil { + c.ui.ErrorWithContext(err, "failed to generate pack manager", errorContext.GetAll()...) + return 1 + } renderOutput, err := renderPack( packManager, @@ -344,7 +348,7 @@ func (c *RenderCommand) Run(args []string) int { } func (c *RenderCommand) Flags() *flag.Sets { - return c.flagSet(flagSetOperation|flagSetNeedsApproval, func(set *flag.Sets) { + return c.flagSet(flagSetOperation|flagSetNeedsApproval|flagSetExternalVarSources, func(set *flag.Sets) { c.packConfig = &caching.PackConfig{} f := set.NewSet("Render Options") diff --git a/internal/cli/run.go b/internal/cli/run.go index 82dc3773..db5bf4c5 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -64,7 +64,11 @@ func (c *RunCommand) run() int { return 1 } - packManager := generatePackManager(c.baseCommand, client, c.packConfig) + packManager, err := generatePackManager(c.baseCommand, client, c.packConfig) + if err != nil { + c.ui.ErrorWithContext(err, "failed to generate pack manager", errorContext.GetAll()...) + return 1 + } // Render the pack now, before creating the deployer. If we get an error // we won't make it to the deployer. @@ -191,7 +195,7 @@ func (c *RunCommand) run() int { // Flags defines the flag.Sets for the operation. func (c *RunCommand) Flags() *flag.Sets { - return c.flagSet(flagSetOperation|flagSetNomadClient, func(set *flag.Sets) { + return c.flagSet(flagSetOperation|flagSetNomadClient|flagSetExternalVarSources, func(set *flag.Sets) { f := set.NewSet("Run Options") c.packConfig = &caching.PackConfig{} diff --git a/internal/cli/stop.go b/internal/cli/stop.go index 04505b1d..72cdd90e 100644 --- a/internal/cli/stop.go +++ b/internal/cli/stop.go @@ -81,7 +81,11 @@ func (c *StopCommand) Run(args []string) int { var jobs []*api.Job - packManager := generatePackManager(c.baseCommand, client, c.packConfig) + packManager, err := generatePackManager(c.baseCommand, client, c.packConfig) + if err != nil { + c.ui.ErrorWithContext(err, "failed to generate pack manager", errorContext.GetAll()...) + return 1 + } var r *renderer.Rendered diff --git a/internal/cli/varsource_config.go b/internal/cli/varsource_config.go new file mode 100644 index 00000000..644bbd65 --- /dev/null +++ b/internal/cli/varsource_config.go @@ -0,0 +1,76 @@ +// Copyright IBM Corp. 2023, 2026 +// SPDX-License-Identifier: MPL-2.0 + +package cli + +import ( + "fmt" + "net/url" + "strings" + + "github.com/hashicorp/nomad-pack/internal/pkg/variable/source" +) + +// parseVarSourceConfigs parses variable source URLs into typed source configs. +// Only the configuration is parsed here; no remote connections are made. The +// returned configs are built into live sources lazily, at render time, by the +// variable parser. +// +// Supported URL formats: +// - consul:///path (uses the Consul environment address) +// - consul://host:port/path (uses the specified Consul address) +func parseVarSourceConfigs(urls []string) ([]source.SourceConfig, error) { + if len(urls) == 0 { + return nil, nil + } + + configs := make([]source.SourceConfig, 0, len(urls)) + + for _, urlStr := range urls { + cfg, err := parseVarSourceConfig(urlStr) + if err != nil { + return nil, fmt.Errorf("var-source %q: %w", urlStr, err) + } + configs = append(configs, cfg) + } + + return configs, nil +} + +// parseVarSourceConfig parses a single variable source URL into a typed config. +func parseVarSourceConfig(urlStr string) (source.SourceConfig, error) { + u, err := url.Parse(urlStr) + if err != nil { + return nil, fmt.Errorf("invalid URL: %w", err) + } + + host := u.Host + path := strings.Trim(u.Path, "/") + + switch u.Scheme { + case "consul": + return parseConsulSourceConfig(host, path) + default: + return nil, fmt.Errorf("unsupported scheme %q (supported: consul)", u.Scheme) + } +} + +// parseConsulSourceConfig builds a Consul KV source config from the host and +// path of a consul:// URL. Each variable is read from /. +// A non-empty host overrides the Consul environment address; the rest of the +// Consul configuration, including the ACL token, comes from the standard Consul +// environment configuration (CONSUL_HTTP_ADDR, CONSUL_HTTP_TOKEN, and so on) +// when the source is built. +// - consul:///path/to/vars -> host="", path="path/to/vars" +// - consul://localhost:8500/path -> host="localhost:8500", path="path" +func parseConsulSourceConfig(host, path string) (source.SourceConfig, error) { + if path == "" { + return nil, fmt.Errorf("consul URL must include a path (e.g., consul:///nomad-pack)") + } + + return source.ConsulSourceConfig{ + Priority: source.PriorityConsul, + Address: host, + Path: path, + }, nil +} diff --git a/internal/pkg/manager/manager.go b/internal/pkg/manager/manager.go index f2d0a664..b513aea8 100644 --- a/internal/pkg/manager/manager.go +++ b/internal/pkg/manager/manager.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/nomad-pack/internal/pkg/renderer" "github.com/hashicorp/nomad-pack/internal/pkg/variable/parser" "github.com/hashicorp/nomad-pack/internal/pkg/variable/parser/config" + "github.com/hashicorp/nomad-pack/internal/pkg/variable/source" "github.com/hashicorp/nomad-pack/sdk/pack" "github.com/hashicorp/nomad/api" ) @@ -21,12 +22,13 @@ import ( // Config contains all the user specified parameters needed to correctly run // the pack manager. type Config struct { - Path string - VariableFiles []string - VariableCLIArgs map[string]string - VariableEnvVars map[string]string - UseParserV1 bool - AllowUnsetVars bool + Path string + VariableFiles []string + VariableCLIArgs map[string]string + VariableEnvVars map[string]string + UseParserV1 bool + AllowUnsetVars bool + ExternalSourceConfigs []source.SourceConfig // Lazily-built configs for external sources (Consul, Vault, Nomad) } // PackManager is responsible for loading, parsing, and rendering a Pack and @@ -67,12 +69,13 @@ func (pm *PackManager) ProcessVariableFiles() (*parser.ParsedVariables, []*error parentName, _, _ := strings.Cut(path.Base(pm.cfg.Path), "@") pCfg := &config.ParserConfig{ - Version: config.V2, - ParentPack: pm.loadedPack, - RootVariableFiles: loadedPack.RootVariableFiles(), - EnvOverrides: pm.cfg.VariableEnvVars, - FileOverrides: pm.cfg.VariableFiles, - FlagOverrides: pm.cfg.VariableCLIArgs, + Version: config.V2, + ParentPack: pm.loadedPack, + RootVariableFiles: loadedPack.RootVariableFiles(), + EnvOverrides: pm.cfg.VariableEnvVars, + FileOverrides: pm.cfg.VariableFiles, + FlagOverrides: pm.cfg.VariableCLIArgs, + ExternalSourceConfigs: pm.cfg.ExternalSourceConfigs, } if pm.cfg.UseParserV1 { diff --git a/internal/pkg/variable/parser/config/config.go b/internal/pkg/variable/parser/config/config.go index 0a6980bf..fb7f43e4 100644 --- a/internal/pkg/variable/parser/config/config.go +++ b/internal/pkg/variable/parser/config/config.go @@ -4,6 +4,7 @@ package config import ( + "github.com/hashicorp/nomad-pack/internal/pkg/variable/source" "github.com/hashicorp/nomad-pack/sdk/pack" ) @@ -47,6 +48,10 @@ type ParserConfig struct { // all sources. If the same key is supplied twice, the last wins. FlagOverrides map[string]string + // ExternalSourceConfigs are parsed, lazily-built configurations for + // external variable sources (Consul, Vault, Nomad). + ExternalSourceConfigs []source.SourceConfig + // IgnoreMissingVars determines whether we error or not on variable overrides // that don't have corresponding vars in the pack. IgnoreMissingVars bool diff --git a/internal/pkg/variable/parser/parser_v2.go b/internal/pkg/variable/parser/parser_v2.go index 6c26f918..439b797c 100644 --- a/internal/pkg/variable/parser/parser_v2.go +++ b/internal/pkg/variable/parser/parser_v2.go @@ -27,6 +27,13 @@ import ( "github.com/zclconf/go-cty/cty" ) +const ( + // externalSourceTimeout is the maximum time allowed for fetching variables + // from external sources (Consul, Vault, Nomad). This prevents hanging on + // slow or unresponsive external services. + externalSourceTimeout = 30 * time.Second +) + type ParserV2 struct { fs afero.Afero cfg *config.ParserConfig @@ -108,36 +115,36 @@ func (p *ParserV2) Parse() (*ParsedVariables, hcl.Diagnostics) { return nil, diags } - // Register sources with the registry (priority: env=10, file=20, cli=30) - if err := p.sourceRegistry.Register(source.NewEnvSource(10, p.envOverrideVars)); err != nil { - return nil, diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Failed to register environment source", - Detail: err.Error(), - }) - } - if err := p.sourceRegistry.Register(source.NewFileSource(20, p.fileOverrideVars)); err != nil { - return nil, diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Failed to register file source", - Detail: err.Error(), - }) - } - if err := p.sourceRegistry.Register(source.NewCLISource(30, p.flagOverrideVars)); err != nil { - return nil, diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Failed to register CLI source", - Detail: err.Error(), - }) + // Register sources with the registry in priority order + p.sourceRegistry.Register(source.NewEnvSource(source.PriorityEnv, p.envOverrideVars)) + p.sourceRegistry.Register(source.NewFileSource(source.PriorityFile, p.fileOverrideVars)) + + // Create and register external sources (Consul, Vault, Nomad) from configs if provided. + // This is where we lazily build the actual sources from their parsed + // configs, avoiding remote connections for commands that never resolve. + for _, sc := range p.cfg.ExternalSourceConfigs { + src, err := sc.Build() + if err != nil { + return nil, diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Failed to create external source", + Detail: err.Error(), + }) + } + + p.sourceRegistry.Register(src) } + p.sourceRegistry.Register(source.NewCLISource(source.PriorityCLI, p.flagOverrideVars)) + // Use context with timeout to prevent hanging on external sources - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), externalSourceTimeout) defer cancel() // Resolve and merge variables from all sources using the registry - for packName := range p.rootVars { - resolvedVars, resolveErr := p.sourceRegistry.Resolve(ctx, packName) + for packName, packSchema := range p.rootVars { + // Pass the pack's schema to enable schema-aware type conversion + resolvedVars, resolveErr := p.sourceRegistry.Resolve(ctx, packName, packSchema) if resolveErr != nil { diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, diff --git a/internal/pkg/variable/source/base_source.go b/internal/pkg/variable/source/base_source.go index 332759f9..a26758b0 100644 --- a/internal/pkg/variable/source/base_source.go +++ b/internal/pkg/variable/source/base_source.go @@ -39,11 +39,9 @@ func (b *BaseSource) Priority() int { // Fetch retrieves variables for the given pack from the wrapped map. // Returns an empty slice if the pack is not found or vars is nil. -func (b *BaseSource) Fetch(ctx context.Context, packID pack.ID) ([]*variables.Variable, error) { - if err := ctx.Err(); err != nil { - return nil, err - } - +// +// Unlike external sources, BaseSource does not filter by schema. External sources (e.g. Consul) do their own schema filtering. +func (b *BaseSource) Fetch(ctx context.Context, packID pack.ID, schema map[variables.ID]*variables.Variable) ([]*variables.Variable, error) { if b.vars == nil { return make([]*variables.Variable, 0), nil } diff --git a/internal/pkg/variable/source/config.go b/internal/pkg/variable/source/config.go new file mode 100644 index 00000000..d5bed08f --- /dev/null +++ b/internal/pkg/variable/source/config.go @@ -0,0 +1,50 @@ +// Copyright IBM Corp. 2023, 2026 +// SPDX-License-Identifier: MPL-2.0 + +package source + +import ( + "github.com/hashicorp/consul/api" +) + +// SourceConfig is a parsed, lazily-evaluated configuration for a variable +// source. Parsing a config (for example from a --var-source URL) is kept +// separate from Build, which constructs the concrete VariableSource. Build does +// only local work, such as building API client structs; it opens no connection. +// No network I/O happens until variables are actually fetched for rendering, so +// callers can parse and validate source configuration up front for free. +type SourceConfig interface { + // Build constructs the concrete VariableSource described by this config. + // Implementations may construct API clients here, but constructing a client + // must not open a connection; no remote reads happen until + // VariableSource.Fetch. + Build() (VariableSource, error) +} + +// ConsulSourceConfig holds the parsed configuration for a Consul KV variable +// source. It is a plain value type with no live connections, making it safe to +// pass across package boundaries without import cycles. +type ConsulSourceConfig struct { + // Priority is the precedence level applied to the built source. + Priority int + + // Address is the Consul HTTP address. When empty, the address from the + // standard Consul environment configuration is used. + Address string + + // Path is the Consul KV path under which variables are stored. Each + // variable is read from /. + Path string +} + +// Build implements SourceConfig by constructing a ConsulSource. It builds the +// Consul API client struct but opens no connection and performs no remote +// reads; those happen later in Fetch. +func (c ConsulSourceConfig) Build() (VariableSource, error) { + apiCfg := api.DefaultConfig() + if c.Address != "" { + apiCfg.Address = c.Address + } + + return NewConsulSource(c.Priority, apiCfg, c.Path) +} diff --git a/internal/pkg/variable/source/consul_source.go b/internal/pkg/variable/source/consul_source.go new file mode 100644 index 00000000..d792b277 --- /dev/null +++ b/internal/pkg/variable/source/consul_source.go @@ -0,0 +1,174 @@ +// Copyright IBM Corp. 2023, 2026 +// SPDX-License-Identifier: MPL-2.0 + +package source + +import ( + "context" + "fmt" + "strings" + + "github.com/hashicorp/consul/api" + "github.com/hashicorp/nomad-pack/sdk/pack" + "github.com/hashicorp/nomad-pack/sdk/pack/variables" + "github.com/zclconf/go-cty/cty" + ctyjson "github.com/zclconf/go-cty/cty/json" +) + +// ConsulSource fetches variables from Consul KV store. Each variable is read +// from /, where is the user-supplied KV path. +// Callers that want per-pack namespacing include it in the path +// themselves (for example, consul:///myapp/config). +type ConsulSource struct { + name string + priority int + client *api.Client + path string // KV path under which variables are stored +} + +// NewConsulSource creates a new Consul KV variable source. +// The config parameter can be nil to use default Consul configuration +// (which reads from CONSUL_HTTP_ADDR and CONSUL_HTTP_TOKEN env vars). +// The path parameter is the KV path where variables are stored; each variable +// is read from /. It must not be empty. +func NewConsulSource(priority int, config *api.Config, path string) (*ConsulSource, error) { + // Variables are read from /, so an empty path would + // list the entire KV store. Require an explicit path. + trimmedPath := strings.Trim(path, "/") + if trimmedPath == "" { + return nil, fmt.Errorf("consul source requires a non-empty KV path") + } + + if config == nil { + config = api.DefaultConfig() + } + + client, err := api.NewClient(config) + if err != nil { + return nil, fmt.Errorf("failed to create Consul client: %w", err) + } + + // Make the name unique by including the address and path. + // This allows multiple Consul sources with different configurations + // and eliminates the possibility of duplicate names. + name := fmt.Sprintf("consul(%s:%s)", config.Address, trimmedPath) + + return &ConsulSource{ + name: name, + priority: priority, + client: client, + path: trimmedPath, + }, nil +} + +// Name returns the unique identifier for this source. +func (c *ConsulSource) Name() string { + return c.name +} + +// Priority returns the precedence level (higher = higher priority). +func (c *ConsulSource) Priority() int { + return c.priority +} + +// Fetch retrieves variables for the given pack from Consul KV. Each variable is +// read from / and decoded into the type the pack schema +// declares for it. +// +// Type Conversion Rules: +// - If schema expects string: always return as string (even if valid JSON) +// - If schema expects number: decode the value as a JSON number +// - If schema expects bool: decode the value as a JSON boolean +// - If schema expects object/list: decode the value as JSON into that type +// - Variables not in schema are skipped (not returned) +// +// Edge Cases: +// - Returns nil (not error) if no keys found at path +// - Skips directory entries (keys ending with /) +// - Skips variables not defined in the pack schema +// - Returns an error for an empty value on a non-string variable; empty +// values for string variables are kept as "" +// +// The parser wraps Fetch in a timeout context, so a slow or unreachable Consul +// fails the resolve instead of hanging. +func (c *ConsulSource) Fetch(ctx context.Context, _ pack.ID, schema map[variables.ID]*variables.Variable) ([]*variables.Variable, error) { + // c.path was trimmed of slashes when the source was built; re-add a single + // trailing slash to scope the KV list to keys under this path and to strip + // each key down to its variable name. The pack ID is intentionally unused — + // any per-pack grouping lives in the path itself. + path := c.path + "/" + + // List all keys under this path + opts := api.QueryOptions{RequireConsistent: true} + pairs, _, err := c.client.KV().List(path, (&opts).WithContext(ctx)) + + if err != nil { + return nil, fmt.Errorf("failed to list Consul KV at %s: %w", path, err) + } + + // If no keys found, return nil (not an error) + if len(pairs) == 0 { + return nil, nil + } + + vars := make([]*variables.Variable, 0, len(pairs)) + for _, pair := range pairs { + // Skip directory entries (keys ending in /) before stripping the prefix + if strings.HasSuffix(pair.Key, "/") { + continue + } + + varName := strings.TrimPrefix(pair.Key, path) + + // Check if this variable exists in the schema + schemaVar, inSchema := schema[variables.ID(varName)] + if !inSchema { + // Skip variables not defined in the pack schema + continue + } + + // A non-string variable has no meaningful empty form (there is no "empty" + // number or bool), so an empty value almost always means the Consul key + // was misconfigured. Empty values for string variables are valid and kept as "". + if len(pair.Value) == 0 && schemaVar.Type != cty.String { + return nil, fmt.Errorf("empty Consul value for %s at %s: a %s value is required", varName, pair.Key, schemaVar.Type.FriendlyName()) + } + + // Convert using the variable's constraint type. ConstraintType preserves + // optional() attributes. + expectedType := schemaVar.ConstraintType + if expectedType == cty.NilType { + expectedType = schemaVar.Type + } + + // Convert value using schema-aware conversion + value, err := c.convertValueWithSchema(pair.Value, expectedType) + if err != nil { + return nil, fmt.Errorf("failed to convert value for %s: %w", varName, err) + } + + vars = append(vars, &variables.Variable{ + Name: variables.ID(varName), + Value: value, + Type: value.Type(), + }) + } + + return vars, nil +} + +// convertValueWithSchema converts raw Consul KV bytes into a cty.Value of the +// expected type. +func (c *ConsulSource) convertValueWithSchema(data []byte, expectedType cty.Type) (cty.Value, error) { + if expectedType == cty.String { + return cty.StringVal(string(data)), nil + } + + // For every other type, let cty decode the JSON directly into the expected type. + val, err := ctyjson.Unmarshal(data, expectedType) + if err != nil { + return cty.NilVal, fmt.Errorf("decoding Consul value as %s: %w", expectedType.FriendlyName(), err) + } + + return val, nil +} diff --git a/internal/pkg/variable/source/registry.go b/internal/pkg/variable/source/registry.go index 31d53f43..5840de05 100644 --- a/internal/pkg/variable/source/registry.go +++ b/internal/pkg/variable/source/registry.go @@ -29,7 +29,7 @@ import ( // registry.Register(source.NewEnvSource(10, envVars)) // registry.Register(source.NewFileSource(20, fileVars)) // registry.Register(source.NewCLISource(30, cliVars)) -// vars, err := registry.Resolve(ctx, packID) +// vars, err := registry.Resolve(ctx, packID, schema) type Registry struct { sources []VariableSource } @@ -41,36 +41,25 @@ func NewRegistry() *Registry { } } -// Register adds a source to the registry. Returns an error if the source -// has an empty name, or if a source with the same name is already registered. +// Register adds a source to the registry. // Sources are automatically sorted by priority after registration. -func (r *Registry) Register(source VariableSource) error { - if source.Name() == "" { - return fmt.Errorf("source name cannot be empty") - } - - // Check for duplicate names - for _, existing := range r.sources { - if existing.Name() == source.Name() { - return fmt.Errorf("source with name %q already registered", source.Name()) - } - } - +// Source names are expected to be unique (enforced by source constructors). +func (r *Registry) Register(source VariableSource) { r.sources = append(r.sources, source) // Sort by priority immediately after adding (lower first, so higher priority overwrites) slices.SortFunc(r.sources, func(a, b VariableSource) int { return cmp.Compare(a.Priority(), b.Priority()) }) - - return nil } // Resolve fetches and merges variables from all registered sources. // Sources are processed in priority order (lowest to highest), with // higher priority sources overwriting variables from lower priority sources. +// The schema parameter provides the expected type for each variable, allowing +// sources to perform schema-aware type conversion. // Returns an error if context is cancelled or if any source fails to fetch. -func (r *Registry) Resolve(ctx context.Context, packID pack.ID) ([]*variables.Variable, error) { +func (r *Registry) Resolve(ctx context.Context, packID pack.ID, schema map[variables.ID]*variables.Variable) ([]*variables.Variable, error) { // Check context before starting if err := ctx.Err(); err != nil { return nil, fmt.Errorf("context cancelled before resolution: %w", err) @@ -87,7 +76,7 @@ func (r *Registry) Resolve(ctx context.Context, packID pack.ID) ([]*variables.Va return nil, fmt.Errorf("context cancelled during resolution: %w", err) } - vars, err := source.Fetch(ctx, packID) + vars, err := source.Fetch(ctx, packID, schema) if err != nil { return nil, fmt.Errorf("failed to fetch from %s: %w", source.Name(), err) } diff --git a/internal/pkg/variable/source/registry_test.go b/internal/pkg/variable/source/registry_test.go index 111a1e39..a5cd1e44 100644 --- a/internal/pkg/variable/source/registry_test.go +++ b/internal/pkg/variable/source/registry_test.go @@ -14,59 +14,100 @@ import ( ) func TestRegistry_Resolve(t *testing.T) { - packID := pack.ID("test") - - testCases := []struct { - name string - sources []VariableSource - expected cty.Value - }{ - { - name: "priority resolution", - sources: []VariableSource{ - NewBaseSource("low", 1, map[pack.ID][]*variables.Variable{ - packID: {{Name: "var", Value: cty.StringVal("low")}}, - }), - NewBaseSource("high", 10, map[pack.ID][]*variables.Variable{ - packID: {{Name: "var", Value: cty.StringVal("high")}}, - }), - }, - expected: cty.StringVal("high"), + packID := pack.ID("webapp") + + schema := map[variables.ID]*variables.Variable{ + "app_name": { + Name: "app_name", + Type: cty.String, + }, + "replicas": { + Name: "replicas", + Type: cty.Number, }, } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - registry := NewRegistry() - for _, s := range tc.sources { - must.NoError(t, registry.Register(s)) + t.Run("cli overrides consul", func(t *testing.T) { + registry := NewRegistry() + + // Consul has lower priority + consulVars := map[pack.ID][]*variables.Variable{ + packID: { + {Name: "app_name", Value: cty.StringVal("consul-app")}, + {Name: "replicas", Value: cty.NumberIntVal(3)}, + }, + } + registry.Register(NewBaseSource("consul", PriorityConsul, consulVars)) + + // CLI has higher priority + cliVars := map[pack.ID][]*variables.Variable{ + packID: { + {Name: "app_name", Value: cty.StringVal("cli-app")}, + }, + } + registry.Register(NewBaseSource("cli", PriorityCLI, cliVars)) + + result, err := registry.Resolve(t.Context(), packID, schema) + must.NoError(t, err) + must.Len(t, 2, result) + + // CLI value should win for app_name + var appName, replicas *variables.Variable + for _, v := range result { + switch v.Name { + case "app_name": + appName = v + case "replicas": + replicas = v } + } - result, err := registry.Resolve(t.Context(), packID) - must.NoError(t, err) - must.Len(t, 1, result) - must.True(t, tc.expected.Equals(result[0].Value).True()) - }) - } + must.Eq(t, "cli-app", appName.Value.AsString()) + replicasInt, _ := replicas.Value.AsBigFloat().Int64() + must.Eq(t, int64(3), replicasInt) + }) + + t.Run("multiple sources merge correctly", func(t *testing.T) { + registry := NewRegistry() + + // File source provides base config + fileVars := map[pack.ID][]*variables.Variable{ + packID: {{Name: "replicas", Value: cty.NumberIntVal(1)}}, + } + registry.Register(NewBaseSource("file", PriorityFile, fileVars)) - t.Run("empty registry", func(t *testing.T) { + // Consul provides app name + consulVars := map[pack.ID][]*variables.Variable{ + packID: {{Name: "app_name", Value: cty.StringVal("prod-app")}}, + } + registry.Register(NewBaseSource("consul", PriorityConsul, consulVars)) + + result, err := registry.Resolve(t.Context(), packID, schema) + must.NoError(t, err) + must.Len(t, 2, result) + }) + + t.Run("no variables for pack", func(t *testing.T) { registry := NewRegistry() - result, err := registry.Resolve(t.Context(), pack.ID("test")) + registry.Register(NewBaseSource("empty", PriorityFile, nil)) + + result, err := registry.Resolve(t.Context(), pack.ID("nonexistent"), schema) must.NoError(t, err) must.Len(t, 0, result) }) - t.Run("context cancellation", func(t *testing.T) { + t.Run("cancelled context fails fast", func(t *testing.T) { registry := NewRegistry() - s := NewBaseSource("test", 1, map[pack.ID][]*variables.Variable{ - pack.ID("test"): {{Name: "var", Value: cty.StringVal("val")}}, - }) - must.NoError(t, registry.Register(s)) + vars := map[pack.ID][]*variables.Variable{ + packID: {{Name: "app_name", Value: cty.StringVal("test")}}, + } + registry.Register(NewBaseSource("test", PriorityFile, vars)) - ctx, cancel := context.WithCancel(context.Background()) - cancel() + ctx, cancel := context.WithCancel(t.Context()) + t.Cleanup(cancel) + cancel() // Cancel immediately - _, err := registry.Resolve(ctx, pack.ID("test")) - must.Error(t, err) + _, err := registry.Resolve(ctx, packID, schema) + must.ErrorContains(t, err, "context canceled") }) } diff --git a/internal/pkg/variable/source/source.go b/internal/pkg/variable/source/source.go index b48d7618..b1b9c7c6 100644 --- a/internal/pkg/variable/source/source.go +++ b/internal/pkg/variable/source/source.go @@ -10,6 +10,25 @@ import ( "github.com/hashicorp/nomad-pack/sdk/pack/variables" ) +// Priority constants define the precedence order for variable sources. +// Higher values take precedence over lower values when variables conflict. +// +// Priority Order (lowest to highest): +// - Environment variables (10) +// - Variable files (20) +// - Nomad Variables (23) +// - Vault KV (24) +// - Consul KV (25) +// - CLI flags (30) +const ( + PriorityEnv = 10 + PriorityFile = 20 + PriorityNomad = 23 + PriorityVault = 24 + PriorityConsul = 25 + PriorityCLI = 30 +) + // VariableSource represents a source of variables (CLI, file, env, external) type VariableSource interface { // Name returns the unique identifier for this source @@ -18,6 +37,7 @@ type VariableSource interface { // Priority returns the precedence level (higher = higher priority) Priority() int - // Fetch retrieves variables for the given pack - Fetch(ctx context.Context, packID pack.ID) ([]*variables.Variable, error) + // Fetch retrieves variables for the given pack. + // If a variable is not in the schema, it will be skipped (not returned). + Fetch(ctx context.Context, packID pack.ID, schema map[variables.ID]*variables.Variable) ([]*variables.Variable, error) }