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

Redact headers in Retry logging #2178

Merged
merged 4 commits into from Oct 18, 2018

Conversation

@insdami
Copy link
Contributor

@insdami insdami commented Oct 16, 2018

It'd be nice to have a little bit more control on what's logged in Retry, specially in the headers.

This is not nice, but it captures the intention. Ideally I'd like to use the Logger.logMessage that's in master so I can format the string representation of the request. Would we possible to have in in .18?

@insdami insdami force-pushed the insdami:redact-headers branch from 5fc9629 to df8b5c3 Oct 16, 2018
Copy link
Member

@rossabaker rossabaker left a comment

Does any redaction happen by default now? Is this about configurability, or redacting it in the first place?

Adding this parameter is going to break binary compatibility. We could proceed with this, and a hardcoded redactHeaderWhen, in 0.18, and then add the parameter in 0.19. Or we could add this in 0.18 by creating a new function named something other than apply, and then deprecate that and move back to apply in 0.19.

@@ -19,7 +20,7 @@ object Retry {

private[this] val logger = getLogger

def apply[F[_]](policy: RetryPolicy[F])(client: Client[F])(
def apply[F[_]](policy: RetryPolicy[F])(client: Client[F])(redactHeaderWhen: CaseInsensitiveString => Boolean = Headers.SensitiveHeaders.contains)(

This comment has been minimized.

@rossabaker

rossabaker Oct 16, 2018
Member

I'd make this the second arg in the first list. It's nice for middleware composition when the client arg stands alone in the last list.

@insdami
Copy link
Contributor Author

@insdami insdami commented Oct 17, 2018

The need comes from redacting information that might be sensitive to keep in the logs.
I think the logging is fixed here which makes harder for the user to control what's logged and the logging level.
It'd be cool if we can somehow control that, I looked at https://github.com/http4s/http4s/blob/master/client/src/main/scala/org/http4s/client/middleware/Logger.scala#L24 which in someway enables those things.

Thanks for the feedback!

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Oct 17, 2018

I'm going to release 0.18.20 tomorrow morning (UTC-4) with the related #2182. If you want to add a function that takes a redactHeaders without breaking binary compatibility, we can try to get this in, too.

@insdami insdami force-pushed the insdami:redact-headers branch from df8b5c3 to 611df87 Oct 18, 2018
@insdami
Copy link
Contributor Author

@insdami insdami commented Oct 18, 2018

Cool, I've moved it to default argument and it takes the default sensitive headers as redactHeadersWhen. I think, that won't break bin compatibility

@insdami insdami force-pushed the insdami:redact-headers branch from a2e225d to 9702aee Oct 18, 2018
Copy link
Member

@rossabaker rossabaker left a comment

👍 on green.

@ChristopherDavenport ChristopherDavenport merged commit a39f009 into http4s:release-0.18.x Oct 18, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants