OtlpExporter: Integration test testing https support #1795

alanwest · 2021-02-05T02:48:25Z

Based on the conversation in #1778, I think it may be safe to re-enable TLS support for the simple case.

We use two gRPC libraries

1. Google gRPC library (used for apps prior to .NET Core 3.0)

If the scheme is https then the channel is constructed with new SslCredentials().

Documentation of SslCredentials states:

Creates client-side SSL credentials loaded from disk file pointed to by the GRPC_DEFAULT_SSL_ROOTS_FILE_PATH environment variable. If that fails, gets the roots certificates from a well known place on disk.

2. gRPC .NET (used for apps supporting netstandard2.1)

gRPC channel can be constructed from a Uri with an https scheme, so I think TLS will just work.

I think this is enough to get us basic support for TLS over http. The spec still has some in-flight changes to configuration options that may allow us to provide a more flexible solution in the future. Also, support for unix sockets could come later too.

I'm going to try to expand the integration tests we have for the OtlpExporter to vet out these changes to ensure TLS works as I expect.

codecov · 2021-02-05T03:03:44Z

Codecov Report

Merging #1795 (9d812d0) into main (90d2906) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1795      +/-   ##
==========================================
- Coverage   79.71%   79.70%   -0.02%     
==========================================
  Files         254      254              
  Lines        8404     8404              
==========================================
- Hits         6699     6698       -1     
- Misses       1705     1706       +1

Impacted Files	Coverage Δ
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs	`71.96% <0.00%> (-1.87%)`	⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs	`96.85% <0.00%> (-0.79%)`	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`82.35% <0.00%> (+5.88%)`	⬆️

cijothomas · 2021-02-05T17:19:34Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporter.cs

                {
-                    throw new NotSupportedException("Https Endpoint is not supported.");
+                    throw new NotSupportedException($"Endpoint URI scheme ({options.Endpoint.Scheme}) is not supported.");
                }

 #if NETSTANDARD2_1
                this.channel = GrpcChannel.ForAddress(options.Endpoint);


From https://docs.microsoft.com/en-us/aspnet/core/grpc/client?view=aspnetcore-5.0#configure-tls
To configure a gRPC channel to use TLS, ensure the server address starts with https. For example, GrpcChannel.ForAddress("https://localhost:5001") uses HTTPS protocol. The gRPC channel automatically negotiates a connection secured by TLS and uses a secure connection to make gRPC calls.

cijothomas

This looks good.
Not explicitly approving, as we are trying to add integration test for secure connection

cijothomas · 2021-02-05T18:46:51Z

@pjanotti @CodeBlanch @reyang @pcwiese Please help review.

reyang

LGTM.

pcwiese · 2021-02-05T18:56:03Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporter.cs

+                ChannelCredentials channelCredentials;
+                if (options.Endpoint.Scheme == Uri.UriSchemeHttps)
+                {
+                    channelCredentials = new SslCredentials();


So those that want a secure channel I guess would specify the cert path via the environment variable? If so this seems like it would work for the simple case. I hope the additional tests prove this out and this gets into 1.0!

LGTM also.

https://grpc.github.io/grpc/csharp/api/Grpc.Core.SslCredentials.html#Grpc_Core_SslCredentials__ctor
We shouldn't recommend customers to rely on this env variable. Its an implementation detail that it works, so they shouldn't bet on it.

Instead, the actual environment variable, as per otel spec should be added in 1.1.

Make sense.

pjanotti

LGTM.

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporter.cs

Co-authored-by: Paulo Janotti <pjanotti@splunk.com>

…emetry-dotnet into alanwest/otlp-allow-tls

test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/Dockerfile

alanwest · 2021-02-06T00:08:08Z

test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/IntegrationTests.cs

+                // TODO: Should test both http and https
+                Endpoint = new System.Uri($"https://{CollectorEndpoint}"),


Just to vet out https support, I just changed the test to use https. I plan to follow up and have it test both.

If we use a self-signed cert might need to manually pass validation somehow?

If we use a self-signed cert might need to manually pass validation somehow?

The cert gets installed as a trusted cert here, so no need to manually pass validation:

opentelemetry-dotnet/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/Dockerfile

Lines 18 to 20 in 21bed21

RUN apt-get update && apt-get install ca-certificates

COPY --from=build /repo/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/otel-collector-config/otel-collector.crt /usr/local/share/ca-certificates/

RUN update-ca-certificates --verbose

I had actually tried this first:

ServicePointManager.ServerCertificateValidationCallback = (sender, certificate, chain, policyErrors) => true;

For some reason this did not work... I've used this kind of thing in test applications before, but for some reason the callback was not getting invoked by the test.

ServicePointManager.ServerCertificateValidationCallback doesn't work for the Google library or .NET or both? I'm not too surprised if it doesn't work in the Google library. .NET would be more of a surprise 😄

…emetry-dotnet into alanwest/otlp-allow-tls

.../OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/otel-collector-config/otel-collector.crt

cijothomas · 2021-02-12T01:46:41Z

Related to this area in spec: open-telemetry/opentelemetry-specification#1426

CodeBlanch

LGTM

…-tls

alanwest · 2021-03-31T00:08:33Z

@cijothomas I think this is good to go. I had one comment that could be followed up on about expanding this test to test both http and https.

github-actions · 2022-01-28T03:17:58Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2022-02-05T03:17:23Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Enable support for both http and https

a1b43ff

alanwest requested a review from a team as a code owner February 5, 2021 02:48

Logic is hard

323ed14

Merge branch 'main' into alanwest/otlp-allow-tls

d5f6cce

cijothomas reviewed Feb 5, 2021

View reviewed changes

Merge branch 'main' into alanwest/otlp-allow-tls

11a8e85

cijothomas reviewed Feb 5, 2021

View reviewed changes

reyang approved these changes Feb 5, 2021

View reviewed changes

pcwiese reviewed Feb 5, 2021

View reviewed changes

pjanotti approved these changes Feb 5, 2021

View reviewed changes

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporter.cs Outdated Show resolved Hide resolved

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporter.cs Outdated Show resolved Hide resolved

cijothomas mentioned this pull request Feb 5, 2021

OpenTelemetry.Exporter.OpenTelemetryProtocol support #1791

Closed

alanwest and others added 5 commits February 5, 2021 15:33

https test that works for netstandard2.1 gRPC library

852e091

Apply suggestions from code review

9029055

Co-authored-by: Paulo Janotti <pjanotti@splunk.com>

Fix https test for Google gRPC library

b161bda

Merge branch 'alanwest/otlp-allow-tls' of github.com:alanwest/opentel…

4e595e9

…emetry-dotnet into alanwest/otlp-allow-tls

Add some comments

66e844a

alanwest commented Feb 6, 2021

View reviewed changes

test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/Dockerfile Outdated Show resolved Hide resolved

alanwest commented Feb 6, 2021

View reviewed changes

alanwest added 3 commits February 5, 2021 16:33

Merge branch 'main' into alanwest/otlp-allow-tls

a63867a

Update changelog

bc729c1

Merge branch 'alanwest/otlp-allow-tls' of github.com:alanwest/opentel…

21bed21

…emetry-dotnet into alanwest/otlp-allow-tls

CodeBlanch reviewed Feb 6, 2021

View reviewed changes

.../OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/otel-collector-config/otel-collector.crt Outdated Show resolved Hide resolved

alanwest mentioned this pull request Feb 8, 2021

OtlpExporter: Enable support for both http and https - no tests #1804

Merged

Merge branch 'main' into alanwest/otlp-allow-tls

008a9c5

alanwest added 3 commits February 11, 2021 20:56

Remove cert from repo

e906d8e

Create certificate each test run

af7ce26

Update workflow

09b1189

CodeBlanch approved these changes Feb 12, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into alanwest/otlp-allow…

6107e05

…-tls

alanwest changed the title ~~OtlpExporter: Enable support for both http and https~~ OtlpExporter: Integration test testing https support Mar 6, 2021

Merge branch 'main' into alanwest/otlp-allow-tls

fe51556

cijothomas added 5 commits April 6, 2021 18:37

Merge branch 'main' into alanwest/otlp-allow-tls

443470e

Merge branch 'main' into alanwest/otlp-allow-tls

fccd624

Merge branch 'main' into alanwest/otlp-allow-tls

1db8833

Merge branch 'main' into alanwest/otlp-allow-tls

db3ed09

Merge branch 'main' into alanwest/otlp-allow-tls

9d812d0

github-actions bot added the Stale label Jan 28, 2022

github-actions bot closed this Feb 5, 2022

alanwest deleted the alanwest/otlp-allow-tls branch February 23, 2022 19:40

alanwest restored the alanwest/otlp-allow-tls branch July 31, 2023 17:42

alanwest mentioned this pull request Jul 31, 2023

Add OTLP exporter TLS integration test #4732

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OtlpExporter: Integration test testing https support #1795

OtlpExporter: Integration test testing https support #1795

alanwest commented Feb 5, 2021

codecov bot commented Feb 5, 2021 •

edited

cijothomas Feb 5, 2021

cijothomas left a comment

cijothomas commented Feb 5, 2021

reyang left a comment

pcwiese Feb 5, 2021 •

edited

cijothomas Feb 5, 2021

pcwiese Feb 5, 2021

pjanotti left a comment

alanwest Feb 6, 2021

CodeBlanch Feb 6, 2021

alanwest Feb 6, 2021

CodeBlanch Feb 12, 2021

cijothomas commented Feb 12, 2021

CodeBlanch left a comment

alanwest commented Mar 31, 2021

github-actions bot commented Jan 28, 2022

github-actions bot commented Feb 5, 2022

		// TODO: Should test both http and https
		Endpoint = new System.Uri($"https://{CollectorEndpoint}"),

	RUN apt-get update && apt-get install ca-certificates
	COPY --from=build /repo/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/otel-collector-config/otel-collector.crt /usr/local/share/ca-certificates/
	RUN update-ca-certificates --verbose

OtlpExporter: Integration test testing https support #1795

OtlpExporter: Integration test testing https support #1795

Conversation

alanwest commented Feb 5, 2021

1. Google gRPC library (used for apps prior to .NET Core 3.0)

2. gRPC .NET (used for apps supporting netstandard2.1)

codecov bot commented Feb 5, 2021 • edited

Codecov Report

cijothomas Feb 5, 2021

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

cijothomas commented Feb 5, 2021

reyang left a comment

Choose a reason for hiding this comment

pcwiese Feb 5, 2021 • edited

Choose a reason for hiding this comment

cijothomas Feb 5, 2021

Choose a reason for hiding this comment

pcwiese Feb 5, 2021

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment

alanwest Feb 6, 2021

Choose a reason for hiding this comment

CodeBlanch Feb 6, 2021

Choose a reason for hiding this comment

alanwest Feb 6, 2021

Choose a reason for hiding this comment

CodeBlanch Feb 12, 2021

Choose a reason for hiding this comment

cijothomas commented Feb 12, 2021

CodeBlanch left a comment

Choose a reason for hiding this comment

alanwest commented Mar 31, 2021

github-actions bot commented Jan 28, 2022

github-actions bot commented Feb 5, 2022

codecov bot commented Feb 5, 2021 •

edited

pcwiese Feb 5, 2021 •

edited