Redact API key from debug logging #3116
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: codefromthecrypt The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
This redacts the API key, which ends up in Authorization header in debug logs. This helps both reduce the size and also protect secrets. As a side effect, this adds a mockwebserver test, matching version with the okhttp client used in openapi generated code. This part is important as we improve the debug experience for watch events later. Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt
force-pushed
the
redact-api-key
branch
from
d4622b5
to
568386a
Compare
| } | ||
|
|
||
| // Verify the server saw the API key | ||
| assertThat(server.takeRequest().getHeader("authorization"), equalTo(TEST_API_KEY)); |
There was a problem hiding this comment.
ps I would personally love to switch everything off hamcrest to assertj, something that is mostly automatable with openrewrite. However, for this change I left it alone. Let me know and I'll happily fix everything to latest assertj instead.
See https://www.moderne.io/blog/migrating-from-hamcrest-to-assertj-with-openrewrite-and-moderne
|
Thanks for the PR! I'm ok moving to assert4j if you prefer, a number of these choices were old and updates are good. We have typically used Wiremock for mocking HTTP calls instead of mockwebserver. I'd prefer to stick with a single way of mocking HTTP unless there is a strong reason to use it for this use case. Thanks! |
|
TL;DR; I'll take the okhttp mock server out for now, but it would be my strong preference for the follow-up change that would be less annoying to test if I can use this (the debug with streaming thing)
Sure, indeed this warrants explanation. So, the client is written in okhttp, and the most natural, most popular and longest living way to test okhttp clients is with their own mock framework. Besides functional reasons, you have only one version to manage, and less build CVEs to worry about. I recently worked on some code that uses wire mock and found some tests commenting difficulty with redirect tests from one thing to another. I have personally never had a problem with okhttp and mainly put the infrastructure here as it has straight-forward code to setup tests for web sockets and streaming responses. I would offer "freedom and responsibility" on this one and offer to redo all the other tests to okhttp's mock server and then you can see how clean or not it is. Meanwhile, I'll revert this to wire mock as agree personally I don't like "winky changes" where some parts do one thing and another something else. |
|
Changed to use wire mock (updating its version as it was quite old). Note that this dropped test performance time from .2ms to over 5 seconds. This was running it several times in the same IDE.
|
|
Thanks for switching to WireMock. One nit, can you revert the version update and send as a separate PR? I'd prefer to keep PRs clean and focused on a single change. If you are willing to write the PR to move to mock okhttp server I'm happy to review it, but I also don't guarantee it will merge, so I'd hate to waste your time. I haven't really had any problems with WireMock and it feels unnecessary to change, but if you're motivated to do the work, I'm not going to stop you, my main requirement is that we only have one. @yue9944882 wdyt? |
Signed-off-by: Adrian Cole <adrian@tetrate.io>
|
@brendandburns agree some are fine with wiremock, just in reflection it was the odd choice to begin with. It also has a lot of dependency interference, so trips up things like snyk etc due to the vast amount of dependencies it brings in. Regardless, I'll keep this topic to a separate PR and you both can decide. |
|
note: the part about wiremock -> okhttp is really low priority for me, so I won't likely do this soon. I'm more interested in the other in-flight changes. |
|
@codefromthecrypt btw, someone else may also send the update to the wiremock dependency. |
Signed-off-by: Adrian Cole <adrian@tetrate.io>
|
Updated wiremock in PR #3126 |
|
Sorry to make more trouble, I realized that this was in a generated file (ApiClient.java) we will need to add this as a patch here: https://github.com/kubernetes-client/java/tree/master/scripts/patches So that it persists after a new code generation. |
This redacts the "authorization" header by default, and makes the logging interceptor testable. This also adds the generated annotation, as at first we forgot this was generated code. See kubernetes-client/java#3116 cc @brendandburns @yue9944882 Signed-off-by: Adrian Cole <adrian@tetrate.io>
|
raised upstream here OpenAPITools/openapi-generator#18010 I'm out of time for the day, so I'll look into how to create and verify the diff tomorrow. |
|
@codefromthecrypt fwiw, the openapi project has been really good about accepting fixes, so if you'd rather submit a PR to that project, we can then regenerate the code. It is a little slower, but it does have benefits for more than just this project. Either way is fine with me. |
|
good idea. I'll close this one out and we can track OpenAPITools/openapi-generator#18010 instead and just apply it once done. |
|
@brendandburns I would really like to remove the tech debt caused by wiremock. If you ack #3146 I'll give free labor on it. |
This redacts the API key, which ends up in Authorization header in debug logs. This helps both reduce the size and also protect secrets.
As a side effect, this adds a mockwebserver test, matching version with the okhttp client used in openapi generated code. This part is important as we improve the debug experience for watch events later.