Skip to content
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

Merged
merged 16 commits into from Jul 26, 2022

Conversation

detro
Copy link
Contributor

@detro detro commented Jul 18, 2022

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

@detro detro requested a review from a team as a code owner Jul 18, 2022
@detro detro force-pushed the detro/improve_http_transport_logging branch 2 times, most recently from 3f468c0 to 96bbf89 Compare Jul 19, 2022
Copy link
Member

@bflad bflad left a comment

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

Makefile Show resolved Hide resolved
helper/logging/logging_http_transport.go Outdated Show resolved Hide resolved
helper/logging/logging_http_transport.go Outdated Show resolved Hide resolved
helper/logging/logging_http_transport.go Outdated Show resolved Hide resolved
helper/logging/logging_http_transport.go Outdated Show resolved Hide resolved
helper/logging/logging_http_transport.go Outdated Show resolved Hide resolved
helper/logging/transport.go Outdated Show resolved Hide resolved
@detro detro force-pushed the detro/improve_http_transport_logging branch from 1a34750 to 843d830 Compare Jul 19, 2022
@detro
Copy link
Contributor Author

detro commented Jul 19, 2022

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

I still need to:

@detro detro force-pushed the detro/improve_http_transport_logging branch from 5aabf0a to d284a70 Compare Jul 19, 2022
@detro detro added this to the v2.20.0 milestone Jul 20, 2022
@detro detro requested a review from bflad Jul 20, 2022
Copy link
Member

@bflad bflad left a comment

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

helper/logging/logging_http_transport.go Outdated Show resolved Hide resolved
helper/logging/logging_http_transport.go Outdated Show resolved Hide resolved
helper/logging/logging_http_transport.go Show resolved Hide resolved
helper/logging/logging_http_transport.go Show resolved Hide resolved
helper/logging/logging_http_transport_test.go Outdated Show resolved Hide resolved
helper/logging/logging_http_transport_test.go Outdated Show resolved Hide resolved
@detro detro force-pushed the detro/improve_http_transport_logging branch from 309d200 to cb6d577 Compare Jul 25, 2022
bflad
bflad approved these changes Jul 25, 2022
Copy link
Member

@bflad bflad left a comment

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

helper/logging/logging_http_transport.go Outdated Show resolved Hide resolved
helper/logging/logging_http_transport.go Show resolved Hide resolved
helper/logging/logging_http_transport.go Outdated Show resolved Hide resolved
detro added 2 commits Jul 26, 2022
This is a UUID, and it's specific to the pair of HTTP Req/Res that 1 HTTP Transaction executes
@detro detro added enhancement New feature or request subsystem/observability Issues and feature requests related to observability (traces, logging, etc.) inside of providers. labels Jul 26, 2022
@detro
Copy link
Contributor Author

detro commented Jul 26, 2022

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
bflad approved these changes Jul 26, 2022
Copy link
Member

@bflad bflad left a comment

Looks good to me 🚀

@detro detro merged commit 426ae64 into main Jul 26, 2022
6 checks passed
@detro detro deleted the detro/improve_http_transport_logging branch Jul 26, 2022
@github-actions
Copy link

github-actions bot commented Aug 26, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request subsystem/observability Issues and feature requests related to observability (traces, logging, etc.) inside of providers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

helper/logging/transport.go http.RoundTripper implementation allow correlating request and responses
2 participants