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

state/remote/swift: Support Openstack request logging #13583

Merged
merged 4 commits into from
Apr 15, 2017

Conversation

fatmcgav
Copy link
Contributor

PR #12089 added support for logging request and response bodies to Openstack resource requests.
This PR extends that to the swift remote state backend.

In order to support this, had to modify the LogRoundTripper struct to use uppercase field names so that they are exposed externally of the package.

Also extended LogRoundTripper to log the request/response headers, as they are useful when working with Swift and container archiving/expiration which uses headers to specify, rather than the request body.

@fatmcgav
Copy link
Contributor Author

@jtopjian Comments welcome.

@jtopjian
Copy link
Contributor

Nice! I did a cursory review and it looks good. I'll take a look in more detail either today or tomorrow.

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Apr 13, 2017

Cheers... I just had a thought aswell.

Should LogRoundTripper be in util.go, rather than types.go?

@jtopjian
Copy link
Contributor

@fatmcgav It could. The reason I have it in types.go is because it's a method to a type and it keeps everything bundled in one file. utils.go, IMO, is for standalone functions.

Thanks for this feature. Printing the headers is definitely useful, especially for Swift, but we should mask some of the sensitive headers printed. For example, when building an instance, this is now printed:

x-subject-token: gAAAAABY8DEPwc...
...
x-auth-token: gAAAAABY8DEPwc5AEjh...

I think masking the token with *** would be a better option.

Other than that, looks great 😄

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Apr 14, 2017

Cool, makes sense on location.

Masking sensitive headers makes sense. I'll refactor the printing of the headers and add some redaction logic...

Edit: having a look for a sensible list of headers to redact, I found:

Looks like a sensible default list to me...

Refactor `logRequest` and `logResponse` to use `RedactHeaders` func.
@fatmcgav
Copy link
Contributor Author

@jtopjian I just pushed b3da653 which adds a RedactHeaders function.

Thoughts?

@jtopjian
Copy link
Contributor

Awesome! Nice work :)

@stack72 This looks good to me.

@fatmcgav
Copy link
Contributor Author

No worries... Happy to squash down the latest commit if cleaner...

@stack72 stack72 merged commit c63ad9c into hashicorp:master Apr 15, 2017
@ghost
Copy link

ghost commented Apr 14, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants