opentelemetry-exporter-otlp-proto-http: fix certificate_file=False being ignored#5379
opentelemetry-exporter-otlp-proto-http: fix certificate_file=False being ignored#5379SFernandoS wants to merge 3 commits into
Conversation
|
|
ba7aa4d to
57b8fbf
Compare
|
It should be noted that the type signature for the constructor explicitly disallows passing in a certificate_file: str | None = None,That being said, I think it's ok to allow a bool to be passed here. Make sure to update the type signature. |
57b8fbf to
12eaf08
Compare
|
Good catch! I've updated the type signature to |
7707eb4 to
7188a42
Compare
…ing ignored The constructor used which caused an explicitly passed False value to be discarded due to Python's falsy evaluation. Use check instead so that False (disable SSL verification) is respected.
7188a42 to
2ea6f46
Compare
|
Updated with the type signature fix as requested. Ready for another look. |
lzchen
left a comment
There was a problem hiding this comment.
Any reason why we need to make this change? A bool is not a valid type so passing in False should be undocumented/unsupported behavior.
There is currently no way to disable TLS verification on the exporters. I suppose we could add another argument, but it's also pretty easy to support this way. |
|
The motivation is that there is currently no supported way to disable TLS verification for the OTLP HTTP exporter. I originally chose False because it required a minimal change while preserving backward compatibility. That said, I agree overloading certificate_file with a boolean may not be the cleanest API. If the preferred direction is to introduce an explicit parameter (e.g. verify_ssl or insecure_skip_verify), I'm happy to rework the PR accordingly. |
|
I've opened #5416 as an alternative approach based on the API design discussion here. If maintainers prefer the explicit insecure parameter, I'm happy to continue with that proposal and close this PR. |
Description
The
OTLPSpanExporterconstructor usedcertificate_file or environ.get(...)to resolvethe certificate configuration. Because
Falseis falsy in Python, passingcertificate_file=Falseexplicitly (to disable SSL verification) was silently ignored,falling back to the environment variable or the default
True.This PR changes the check to
certificate_file if certificate_file is not None else environ.get(...)so that only an unset parameter triggers the fallback.
Type of change
How Has This Been Tested?
tox -e py312-test-opentelemetry-exporter-otlp-proto-http)OTLPSpanExporter(certificate_file=False)now correctlystores
Falseinstead of falling through to the defaultDoes This PR Require a Contrib Repo Change?
Checklist: