[VC-36032] Set User-Agent header containing the agent version in all HTTP requests #631
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… with private key authentication Signed-off-by: Richard Wall <[email protected]>
This will make it easier to diagnose problems by allowing platform teams to parse HTTP server logs or intermediate HTTP proxy logs and know which version of the agent has made the request. Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
wallrj
commented
Nov 29, 2024
| @@ -265,13 +266,14 @@ func (c *VenafiCloudClient) Post(ctx context.Context, path string, body io.Reade | |||
| return nil, err | |||
| } | |||
|
|
|||
| req, err := http.NewRequest(http.MethodPost, fullURL(c.baseURL, path), body) | |||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, fullURL(c.baseURL, path), body) | |||
There was a problem hiding this comment.
This is a drive-by fix. I missed this in #627
| @@ -178,7 +178,7 @@ func Test_ValidateAndCombineConfig(t *testing.T) { | |||
|
|
|||
| // The log line printed by pflag is not captured by the log recorder. | |||
| assert.Equal(t, testutil.Undent(` | |||
| INFO Using the Jetstack Secure OAuth auth mode since --credentials-file was specified without --venafi-cloud. | |||
| INFO Authentication mode mode="Jetstack Secure OAuth" reason="--credentials-file was specified without --venafi-cloud" | |||
There was a problem hiding this comment.
The tests are not being run on the master branch and the green badge on the README file does not reflect the status of the current branch:
- https://github.com/jetstack/jetstack-secure/actions/workflows/tests.yaml?query=branch%3Amaster
- https://github.com/jetstack/jetstack-secure/blob/master/README.md
Opened an issue about that: #632
wallrj
changed the title
maelvls
approved these changes
Nov 29, 2024
There was a problem hiding this comment.
Thanks for fixing this, I wish we had done it sooner. I've realized the importance of the user agent header when responding to last weeks venctl/agent outage.
I haven't tested the PR, but I'm very confident that nothing will break. Nothing in the PR could break anything, I'd say.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In VC-36032 we aim to provide:
By adding user-agent headers to all HTTP requests, it will allow the Venafi platform team to know which version of the agent is making requests to the resource and auth APIs, by analysing the request headers among the server logs.
I tried writing tests for this, but the it would have required some significant refactoring of the client code,
for example there's no easy way to intercept the Auth0 client request made by the Jetstack-Secure client in client_oauth.go.
That's a bit of a lame excuse, but I would like to propose that we refactor the Venafi client code in a followup PR and at the same time add tests which verify that all the flavours of Venafi client do include user agent headers.