-
Notifications
You must be signed in to change notification settings - Fork 231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce NewLoggingHTTPTransport
and deprecate NewTransport
#1006
Conversation
3f468c0
to
96bbf89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are my initial thoughts. Please reach out with any questions.
1a34750
to
843d830
Compare
I have implemented all the feedback of the PR review so far. I still need to:
|
5aabf0a
to
d284a70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this is almost there -- just a few little things then should be good to go. 👍
…rigger GolangCI-Lint (what we use today)
This deprecated the `NewTransport` facility.
…ngci-lint executable
…og masking and filtering
… short (i.e. "Sending HTTP Request / Received HTTP Response")
309d200
to
cb6d577
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock and this looks good overall, however I do think we should do something for #546 specifically (or not close it yet) as part of this. 👍
This is a UUID, and it's specific to the pair of HTTP Req/Res that 1 HTTP Transaction executes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 🚀
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Related: hashicorp/terraform-provider-tfe#479
Related: #2
Closes: #546
Background
Current SDK offers an helper
http.RoundTripper
inhelpers/logging
, that is created vialogging.NewTransport()
. Thishttp.RountTripper
can then be used as.Transport
in anhttp.Client
.When logging level is debug or higher, it does a simple dump of the whole content of the request and of the following response.
This exposes any sensitive information that might be going over HTTP, and there is no way for developers using this code, to setup any filtering.
Proposal
This PR introduced a 2 new implementations of
http.RoundTripper
, one designed to log via the provider root logger, the other via a user-defined subsystem.This transport can be used exactly like the previous one, but it's built around the
tflog
package of terraform-plugin-log, and offers an opt-in hook to configure the HTTP Requestcontext.Context
.For example, if the user wants to setup log filtering against the provider root logger, and ensure that it's applied to each request handled by this transport:
As a consequence, the existing
NewTransport
would be documented as deprecated.Related
terraform-plugin-log
NewLoggingHTTPTransport
andNewSubsystemLoggingHTTPTransport
#1007