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

Add support for TLS insecure option in opentelemetry oltp exporter #31941

Closed
vsevel opened this issue Mar 17, 2023 · 27 comments · Fixed by #35062
Closed

Add support for TLS insecure option in opentelemetry oltp exporter #31941

vsevel opened this issue Mar 17, 2023 · 27 comments · Fixed by #35062
Assignees
Labels
area/tracing kind/enhancement New feature or request
Milestone

Comments

@vsevel
Copy link
Contributor

vsevel commented Mar 17, 2023

Description

It would be convenient to support for development:

quarkus.opentelemetry.tracer.exporter.otlp.tls.insecure=true

Implementation ideas

No response

@vsevel vsevel added the kind/enhancement New feature or request label Mar 17, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 17, 2023

/cc @brunobat (opentelemetry), @radcortez (opentelemetry)

@brunobat
Copy link
Contributor

What would be the expected behaviour.
Could you please provide an example?

@vsevel
Copy link
Contributor Author

vsevel commented Mar 20, 2023

hello, the expected behavior would be to support https when exporting spans to the apm server, but not check the certificate presented by the apm.

currently, if I do not add the target certificate into the truststore, I get:

2023-03-20 09:56:14,505 ERROR [io.opentelemetry.exporter.internal.grpc.OkHttpGrpcExporter] (OkHttp https://the-apm-server....com:8200/...) Failed to export spans. The request could not be executed. Full error message: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target

the behavior is the same even if I set:

quarkus.tls.trust-all=true

the only way to get the spans to get sent to the apm is to configure the truststore -Djavax.net.ssl.trustStore=...
for dev it would be convenient to connect to the apm and not check its certificate.

@brunobat
Copy link
Contributor

I understand now. Makes sense to me.
We need to work on the exporter, in order to remove OkHttp, for multiple reasons.
Will consider this issue then.

@brunobat
Copy link
Contributor

@geoand would you like to pick this one up?

@geoand
Copy link
Contributor

geoand commented Jul 27, 2023

I'll have a look next week

@geoand
Copy link
Contributor

geoand commented Jul 27, 2023

I think it will be pretty easy as we completely control the export process now (since we have removed the OkHttp exporter)

@geoand
Copy link
Contributor

geoand commented Jul 27, 2023

@vsevel would you like to give #35062 a try?

@vsevel
Copy link
Contributor Author

vsevel commented Jul 27, 2023

sure. give me a bit of time

@geoand
Copy link
Contributor

geoand commented Jul 27, 2023

👌

geoand added a commit that referenced this issue Jul 28, 2023
Take `quarkus.tls.trust-all` into account in Otlp export
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Jul 28, 2023
@vsevel
Copy link
Contributor Author

vsevel commented Aug 17, 2023

@geoand I confirm this works, however quarkus.tls.trust-all is a build time config property. correct?
this is not very useful, as we typically want to disable tls on a local test, but not in the other envs.
it would be nice to have a dedicated oltp tls property.

@geoand
Copy link
Contributor

geoand commented Aug 17, 2023

Thanks for checking!

You can override test specific properties, no?

@vsevel
Copy link
Contributor Author

vsevel commented Aug 17, 2023

when I start the app I get:

2023-08-17 13:41:01,056 WARN  [io.qua.run.con.ConfigRecorder] (main) Build time property cannot be changed at runtime:
 - quarkus.tls.trust-all is set to 'true' but it is build time fixed to 'false'. Did you change the property quarkus.tls.trust-all after building the application?
...
2023-08-17 13:41:12,135 SEVERE [io.qua.ope.run.exp.otl.VertxGrpcExporter] (vert.x-eventloop-thread-1) Failed to export spans. The request could not be executed. Full error message: Failed to create SSL connection

when I add the trust-all to the src/main/resources/application.properties , the SEVERE disappears.
am I missing something?

@geoand
Copy link
Contributor

geoand commented Aug 17, 2023

How are you setting the property in the first case?

@vsevel
Copy link
Contributor Author

vsevel commented Aug 17, 2023

on the disk, specifically in ./config/base-application.properties with QUARKUS_CONFIG_LOCATIONS=./config/base-application.properties

@geoand
Copy link
Contributor

geoand commented Aug 17, 2023

Ah okay, I see.

You would need to use a TestProfile to set the build time properties you need

@vsevel
Copy link
Contributor Author

vsevel commented Aug 17, 2023

I added this to src/main/resources/application.yaml:

"%mytest":
  quarkus:
    tls:
      trust-all: true

I start the app with:

 java -Dquarkus.profile=mytest ...

I see:

2023-08-17 14:47:17,030 WARN  [io.qua.run.con.ConfigRecorder] (main) Build time property cannot be changed at runtime:
 - quarkus.tls.trust-all is set to 'true' but it is build time fixed to 'false'. Did you change the property quarkus.tls.trust-all after building the application?
...
2023-08-17 14:47:18,327 INFO  [io.quarkus] (main) Profile mytest activated. 
...
2023-08-17 14:47:43,110 SEVERE [io.qua.ope.run.exp.otl.VertxGrpcExporter] (vert.x-eventloop-thread-1) Failed to export spans. The request could not be executed. Full error message: Failed to create SSL connection

is that what you meant by TestProfile?

@geoand
Copy link
Contributor

geoand commented Aug 17, 2023 via email

@vsevel
Copy link
Contributor Author

vsevel commented Aug 17, 2023

hmmm
but then I only have access to this from with a test.
whereas the goal was to be able to run the app in profile prod on dev machines (where we do not necessarily have all certs deployed and updated), and deactivate tls for oltp, just like quarkus.vault.tls.skip-verify.
I will try to find the time to provide an evolution if you are ok.

@geoand
Copy link
Contributor

geoand commented Aug 17, 2023

Sure yeah, we can have a dedicated property

@geoand
Copy link
Contributor

geoand commented Aug 17, 2023

I can do that next week when I can get from PTO

@geoand
Copy link
Contributor

geoand commented Aug 21, 2023

One question for @cescoffier though: What was the reason we chose to make quarkus.tls.trust-all runtime fixed instead of overridable at runtime?

@cescoffier
Copy link
Member

Good question. I think it can be revisited.

@cescoffier
Copy link
Member

Note that you need to know whether you plan to use ssl at build time as the native compilation paths are very different.

@geoand
Copy link
Contributor

geoand commented Aug 21, 2023

That makes sense, but does the trust-all property make any difference to whethere TLS will be enabled or not?

@vsevel
Copy link
Contributor Author

vsevel commented Aug 21, 2023

trust-all as a build time property removes a lot of usefulness.
the usual use case is when running in dev and you do not have the appropriate certs locally.
but when going to test/uat/prod you want to be strict about it.

@geoand
Copy link
Contributor

geoand commented Aug 21, 2023

Right, that's why I asked before adding a specific property for OTel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tracing kind/enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants