VAULT-12095 Support multiple config files for Vault Agent #18403

VioletHynes · 2022-12-15T17:10:45Z

This PR adds support for the following:

Ability to specify -config=./a-directory/, like Vault
Ability to specify multiple -config=a.hcl -config=b.hcl

Merge strategy and file choosing strategy is identical to Vault itself.

I used the same language for the docs that we did here: https://developer.hashicorp.com/vault/tutorials/operations/configure-vault

If you can't review this before the holiday break, don't worry! There's no rush.

VioletHynes · 2022-12-15T17:11:38Z

command/agent/config/config.go

+// isTemporaryFile returns true or false depending on whether the
+// provided file name is a temporary file for the following editors:
+// emacs or vim.
+func isTemporaryFile(name string) bool {


This is a little weird to me, but we use it for Vault's config parsing so I thought I'd keep it for consistency.

VioletHynes · 2022-12-15T17:12:02Z

command/agent/config/config.go

@@ -236,16 +473,6 @@ func LoadConfig(path string) (*Config, error) {
 		return nil, fmt.Errorf("error parsing 'api_proxy':%w", err)
 	}

-	if result.APIProxy != nil && result.Cache != nil {


These checks have been moved to a new check stage after all of the config has been added and merged.

VioletHynes · 2022-12-15T17:12:39Z

command/agent/config/config.go

 	err = parseVault(result, list)
 	if err != nil {
 		return nil, fmt.Errorf("error parsing 'vault':%w", err)
 	}

-	if result.Vault == nil {


We no longer set a default Vault config retries, we simply use the default if it's not set.

VioletHynes · 2022-12-15T17:14:40Z

command/agent_test.go

@@ -61,191 +61,6 @@ func testAgentCommand(tb testing.TB, logger hclog.Logger) (*cli.MockUi, *AgentCo
 	}
 }

-/*
-func TestAgent_Cache_UnixListener(t *testing.T) {


I'm not sure what this is, so I deleted it as it was commented out. We can always fish it out of git if we need it again, but I don't like having a ~200 line comment block in files like this.

command/agent.go

command/agent/config/config.go

ncabatoff · 2023-01-03T15:37:44Z

command/agent/config/config.go

+		result.TemplateConfig = c2.TemplateConfig
+	}
+
+	for _, l := range c.Templates {


There is a more succinct syntax (...) you could use in place of a for loop if you wanted.

command/agent/config/config.go

ncabatoff · 2023-01-03T15:40:44Z

command/agent/config/config.go

+			if c.APIProxy.UseAutoAuthTokenRaw != nil {
+				return fmt.Errorf("use_auto_auth_token defined in both api_proxy and cache config. Please remove this configuration from the cache block")
+			} else {
+				c.APIProxy.ForceAutoAuthToken = c.Cache.ForceAutoAuthToken


I'd prefer if a Validate method didn't mutate its inputs.

This is an unfortunate consequence of supporting the API Proxy config values in Cache, too. It made the most sense to me to keep this logic together as opposed to having two separate blocks e.g.:

// in Validate if c.APIProxy != nil && c.Cache != nil { if c.Cache.UseAutoAuthTokenRaw != nil { if c.APIProxy.UseAutoAuthTokenRaw != nil { return fmt.Errorf("use_auto_auth_token defined in both api_proxy and cache config. Please remove this configuration from the cache block") } } }

and

// elsewhere if c.APIProxy != nil && c.Cache != nil { if c.Cache.UseAutoAuthTokenRaw != nil { if c.APIProxy.UseAutoAuthTokenRaw == nil { c.APIProxy.ForceAutoAuthToken = c.Cache.ForceAutoAuthToken } } }

I don't really like mutation like this either, but in this case it feels like the lesser of two evils to me, with regards to code cleanliness.

ncabatoff · 2023-01-03T15:50:00Z

command/agent/config/config.go

+
+	var files []string
+	err = nil
+	for err != io.EOF {


It's fine as you have it, though I think it might be a little more readable if you used filepath.WalkDir instead of Readdir here.

ncabatoff · 2023-01-03T16:01:22Z

website/content/docs/agent/index.mdx

+As with Vault, the `-config` flag can be used in three different ways:
+
+- Use the flag once to name the path to a single specific configuration file.
+- Use the flag multiple times to name multiple configuration files, which will be composed at runtime.


I don't normally see composed used in this sense, maybe it's a British-ism? I would use merged or combined instead.

I actually took this wording from one of our existing learn docs: https://developer.hashicorp.com/vault/tutorials/operations/configure-vault

I don't think that's proper English either. You can say "this cake is composed of the following ingredients", but I've never seen it used in the form "I composed these ingredients to make a cake". My English grammar isn't so hot but I think it comes down to transitive vs intransitive use. @tjperry07 wdyt?

For the record, it's definitely a grammatical form I recognise and the sentence does make 100% sense to me, so it could be a British English thing maybe? To give another tech (or, maybe more maths) example, it's used in this way regularly when talking about composing functions (e.g. for a composition f . g, f and g are composed).

I don't really have qualms with changing it, but we should probably update it in both places if we do.

Composed can be used, but it's reading strange to Americans because we usually use it when talking about art and feelings. She composed the painting beautifully. They composed themselves before walking into the room.

In this case merged, combined, even compiled (if appropriate) is better. Just because we follow US English and all US English quirks. But composed is grammatically correct.

command/agent/config/config_test.go

ncabatoff · 2023-01-03T16:24:21Z

command/agent/config/config_test.go

+		t.Fatal(diff)
+	}
+
+	config, err = LoadConfigFile("./test-fixtures/config-dir-auto-auth-and-listener/config1.hcl")


Why these extra checks? They seem redundant after the deep.Equal above.

Essentially, we are doing two checks:

Check that the config matches if we load the directory

Check that the config matches if we load and merge the two files independently

In essence, we're confirming there is no difference between:
vault agent -config=dir1
and
vault agent -config=dir1/file1.hcl -config=dir1/file2.hcl

I guess I'm always dubious of the value of tests that reproduce the code they're testing.

I think I'd like to keep it as is, to ensure that the behaviour doesn't diverge in the future. Anecdotally, this was quite useful during development. We could factor out the test to two tests that check the multi-file approach in a different test to the dir approach, but we'd lose the validation that the two produce the same resultant config.

* VAULT-12095 Code changes for multi-config * VAULT-12095 typo * VAULT-12095 make vault non-nil during update * VAULT-12095 docs * VAULT-12095 small refactor * VAULT-12095 typos

VioletHynes added 3 commits December 15, 2022 11:10

VAULT-12095 Code changes for multi-config

366bc4a

VAULT-12095 typo

cdca5c4

VAULT-12095 make vault non-nil during update

f1acca0

VioletHynes commented Dec 15, 2022

View reviewed changes

VAULT-12095 docs

edeaef7

VioletHynes requested review from peteski22 and ncabatoff December 15, 2022 17:23

VioletHynes marked this pull request as ready for review December 15, 2022 17:23

VioletHynes requested a review from tjperry07 as a code owner December 15, 2022 17:23

vercel bot deployed to Preview December 15, 2022 17:27 View deployment

VioletHynes added this to the 1.13.0-rc1 milestone Dec 16, 2022