Change debug tags and make sure sni is printed on certain logs #7673
Conversation
|
[approve ci] |
iocore/net/SSLClientUtils.cc
Outdated
| if (is_debug_tag_set("ssl_verify")) { | ||
| Debug("ssl_verify", "Core server certificate verification failed for (%s). Action=%s Error=%s server=%s(%s) depth=%d", | ||
| sni_name, enforce_mode ? "Terminate" : "Continue", X509_verify_cert_error_string(err), | ||
| netvc->options.ssl_servername.get(), buff, depth); | ||
| } |
There was a problem hiding this comment.
Are you wrapping these in the conditional because the computation of the parts of the message are expensive?
maskit
left a comment
maskit
left a comment
There was a problem hiding this comment.
Changing tag is probably ok, but changing Warning to Debug may be not. Warnings are printed regardless of diags.debug.enabled and diags.debug.tags, and those would be disappeared on prod. If those warnings are not really important on prod then I'm ok with making them debug log, but users watching the warnings would be surprised. We might want to mark this incompatible. I didn't check how SSLError works but I have the same concern on it as well.
I thought about that too, but the message logged in Diags also seems reasonable to be in debug logs as well. We will have our ops team test this out for a few days and see what they prefers, I'll make adjustments according to their comments. |
|
Oh, I didn't catch that the Warning messages were going away. I thought you were just adding Debug messages in parallel so in the more detailed case all would show up in traffic.out. I know that @djcarlin relies on the certificate verification messages always showing up. He uses that to check when to yell at origins for having bad certs. He doesn't run with debug enabled enough for that to be equivalent. |
duke8253
force-pushed
the
master-hs_log
branch
2 times, most recently
from
6d90fe9 to
f3c1b92
Compare
|
@randall is going to review this |
|
[approve ci autest] |
…e#7673) (apache#498) (cherry picked from commit 09b8e52)
* asf/master: (763 commits) rate_limit: Add a global hook to rate limit concurrent connections based on SNI (apache#8021) Fix uri_signing unit test for out of source builds (apache#8040) tests: Add conditions for BoringSSL and OpenSSL (apache#8045) change debug tags and make sure sni is printed on certain logs (apache#7673) Doc build in CI: build English docs with -W (apache#8039) When loading async SSL configuration file fails, log SSL error (apache#8036) Doc build: treat warnings as errors only by default (apache#8038) For test async_engine, export all symbols (apache#8037) Fix the server cert reload (apache#8030) Treat Sphinx doc build warnings as errors. (apache#8033) Stablize trace curl test in good_request_after_bad (apache#8032) Doc: Update documentation to build cleanly in Sphinx 3. Require Sphinx 3 or better. (apache#7978) Docs: Fix pre-formatting for ratelimit plugin (apache#7986) Make it slightly harder to dump private keys to logs (apache#8029) tls_bad_alpn: Add an openssl version skip check (apache#8026) per thread jemalloc arena for MADV_DONTDUMP (apache#7501) Adds a new rm-destination, this lets you specify either QUERY or PATH, and be able to drop them from the incoming request (apache#8025) Fix HPACK eviction iterator manipulation (apache#8004) Do not invalidate cached resources upon error responses to unsafe methods (apache#7999) Cleanup SSLUtils (apache#8007) ...
8 participants
No description provided.