Introduce NewLoggingHTTPTransport and deprecate NewTransport

[detro]

Related: hashicorp/terraform-provider-tfe#479
Related: #2
Closes: #546

Background

Current SDK offers an helper http.RoundTripper in helpers/logging, that is created via logging.NewTransport(). This http.RountTripper can then be used as .Transport in an http.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.

func NewLoggingHTTPTransport(t http.RoundTripper) *loggingHttpTransport {
	return &loggingHttpTransport{"", false, t}
}

func NewSubsystemLoggingHTTPTransport(subsystem string, t http.RoundTripper) *loggingHttpTransport {
	return &loggingHttpTransport{subsystem, true, t}
}

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 Request context.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:

// ctx == context.Context, provided by the SDK...

ctx = tflog.MaskFieldValuesWithFieldKeys(ctx, "MY_SENSITIVE_FIELD")
ctx = tflog.MaskMessageStrings(ctx, "MY_SECRET_STRING")

transport := logging.NewLoggingHTTPTransport(http.DefaultTransport)
client := http.Client{
  Transport: transport,
}

req, _ := http.NewRequest("GET", "https://www.terraform.io", nil)

res, err := client.Do(req.WithContext(ctx))

As a consequence, the existing NewTransport would be documented as deprecated.

Related

• How to setup log filtering with terraform-plugin-log
• Fast follow-up: Document the new NewLoggingHTTPTransport and NewSubsystemLoggingHTTPTransport #1007

[bflad]

Here are my initial thoughts. Please reach out with any questions.

[detro]

I have implemented all the feedback of the PR review so far.

I still need to:

• include examples that do log masking and omission
• update the PR proposal above with the actual implementation
• website doc -> will create fast-follow ticket -> Document the new NewLoggingHTTPTransport and NewSubsystemLoggingHTTPTransport #1007

[bflad]

Overall I think this is almost there -- just a few little things then should be good to go. 👍

[bflad]

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. 👍

[detro]

All right, I think this is addressing everything @bflad - Indeed I'm pretty happy we get to address #546 in its entirety. Also, got rid of the pointless copying around of the response buffer.

Even if you approved, I think I'll wait for a confirmation the UUID looks good before merging.

[bflad]

Looks good to me 🚀

[github-actions]

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.