VAULT-15547 First pass at agent/proxy decoupling #20548
Conversation
|
|
||
| // Internal-only flags to follow. | ||
| // | ||
| // Why hello there little source code reader! Welcome to the Vault source |
There was a problem hiding this comment.
For comments structured as a question, it could be explained away or maybe I wasn't sure enough to explicitly call for a change. I remember leaving a couple of suggestions to use testing.T.Setenv() instead of os.Setenv. Those are just a suggestion to consider, I'll approve one way or the other on that.
command/proxy.go
Outdated
|
|
||
| serverHealth, err := client.Sys().Health() | ||
| if err == nil { | ||
| // We don't exit on error here, as this is not worth stopping Proxy over |
There was a problem hiding this comment.
This comment is confusing to me. The use of on error here makes me think that it relates to the check on the above line, but then that doesn't make any sense. Should the comment read something like We don't exit if the versions don't match...?
There was a problem hiding this comment.
Yeah, I can see how this can be confusing. Let me reword, and thank you for the suggestion! I'll steal your wording :)
There was a problem hiding this comment.
I've updated this, hopefully it's a bit clearer!
|
|
||
| // Unset the environment variable so that proxy picks up the right test | ||
| // cluster address | ||
| defer os.Setenv(api.EnvVaultAddress, os.Getenv(api.EnvVaultAddress)) |
There was a problem hiding this comment.
Have you considered using https://pkg.go.dev/testing#T.Setenv here instead? It essentially does the same thing but in a single tidy function call.
There was a problem hiding this comment.
I'm not sure it does do the same? What we're doing here is deferring setting the environment variable until after the test has finished, essentially so that the proxy command doesn't pick up the environment variable, but so that we set it back afterwards so we don't mess with the CI environment, for example.
I think that t.Setenv does a set then a defer unset, which is the opposite of what we'd need
There was a problem hiding this comment.
The documentation indicates that it does a set of the environment variable and once the test is finished, it restores the environment variable to the previous value. What I was thinking of was something like t.Setenv(api.EnvVaultAddress, ""). So that during the test execution, the environment variable would be set to an empty string. But like I said, that's merely a suggestion, no need to pursue this thread.
There was a problem hiding this comment.
For comments structured as a question, it could be explained away or maybe I wasn't sure enough to explicitly call for a change. I remember leaving a couple of suggestions to use testing.T.Setenv() instead of os.Setenv. Those are just a suggestion to consider, I'll approve one way or the other on that.
command/agent.go
Outdated
There was a problem hiding this comment.
Whilst a lot of the code isn't new, it might be nice to unify the casing for the import aliases while you're working in here (ctconfig, token_file, agentConfig). I think a few files have the varying casing issue.
| roleIDPath := makeTempFile(t, "role_id.txt", roleID+"\n") | ||
| secretIDPath := makeTempFile(t, "secret_id.txt", secretID+"\n") |
There was a problem hiding this comment.
populateTempFile does the same thing as makeTempFile but uses t.TempDir I think I added it rather than changing all the existing calls to makeTempFile.
Now I think I should have just changed them all or changed makeTempDir to call populateTempFile until I got the courage to refactor. 😞
|
One thing I didn't catch was if this was implemented yet:
If someone uses |
Not yet! That'll be coming soon. I mentioned in the description that this was pending. I wanted to make sure the message had scrutiny, and it's easy to miss things or gloss over them when they're in a larger change request. |
Yes you totally did, sorry! |
| @@ -0,0 +1,3 @@ | |||
| ```release-note:feature | |||
There was a problem hiding this comment.
@VioletHynes could you please update this changelog file with 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.
Will do! Good catch, sorry for missing this!
Sorry this is such a big PR. It didn't make sense to piecemeal some of this stuff and I wouldn't have felt good adding untested code (e.g. if I saved testing for later PR). I've still made an effort to keep this relatively contained, with more to come later for hopefully easier and better review.
Most of the code changes in this PR specific to the proxy command is copied from Agent, with template-related bits deleted. Hopefully, a lot of the code is familiar and therefore less daunting to review.
While the diff is large, a lot of it is just moving some of the shared things from the agent folder to a new, shared folder,
agentproxyshared. While still large, the less-large salient subset of these changes are contained within the following files:command/proxy.gocommand/proxy_test.gocommand/proxy/*There's still more to come, after this PR, but I wanted to keep this functional, but as small as possible while remaining fully functional (since it's still really big). What's coming in later PRs: