[EXPORTER] OTLP GRPC mTLS support #2120

kylepl · 2023-04-25T17:59:13Z

Fixes #1785

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

linux-foundation-easycla · 2023-04-25T17:59:17Z

The committers listed above are authorized under a signed CLA.

✅ login: kylepl / name: Kyle L (355def3, 933e6c0, 6245fcd, 4a5e770, 3df0faa, b764668, 0dfacfc, 1eec89b)
✅ login: lalitb / name: Lalit Kumar Bhasin (ab7c455)

owent · 2023-04-26T03:04:01Z

exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_exporter_options.h

@@ -6,13 +6,44 @@
 #include "opentelemetry/exporters/otlp/otlp_environment.h"

 #include <memory>
+// TODO: These requires C++17, is that OK? It seems like we need to support C++0x?


We need support c++11. You can use absl::variant and absl::optional for alternative.

owent · 2023-04-26T03:07:14Z

exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_exporter_options.h

+
+// TODO: Check if this strong-typing is ok. It seems nice, and allows variant typing.
+struct FilePath {
+  std::string value;


I think we'd better use nostd::string_view here.

Just note that the existing options in OtlpGrpcExporterOptions all use std::string, hence why I used that. If this should be different, so be it - just checking before changing it.

owent · 2023-04-26T03:40:18Z

The trace is already stable. I think we need discuss this approch because it break the API and also we need it to support environment variables in specification.

lalitb · 2023-04-26T04:25:54Z

@kylepl Thanks for working on this. You can also go through #1793, as this provides support for client-side cert/key for OTLP HTTP exporter.

kylepl · 2023-04-26T05:07:02Z

Thanks for the comments!

Based on my understanding of Stable, compilation breakages are not allowed except for major version changes (reasonable). Is it reasonable to still make things compatible, but mark ssl_credentials_cacert_path and ssl_credentials_cacert_as_string as deprecated? It's not quite obvious to me what needs to ABI compatible (i.e. I don't know what gets shipped by itself).

Supporting the environment variables sounds good to me, can do so.

Also, a related thing related to what I'm interested in - is gRPC actually has experimental support refreshing certificates every X seconds from disk. If that seems reasonable, while it could be a separate PR, given it may effect the API, it may be worth discussing now.

And of course feel free to give me your opinions on style and such, once we agree on the high-level, I'll take another pass on the code:

If it is reasonable to have the ssl credentials held in a sub-struct (to group them together), and thus somehow mark ssl_credentials_cacert_path and ssl_credentials_cacert_as_string - while still supporting them.
If supporting auto-refresh of certificates seems reasonable (it is an experimental gRPC feature, we could add a field called "experimental_enable_auto_refresh" to signify that.

codecov · 2023-05-03T05:58:31Z

Codecov Report

Merging #2120 (1eec89b) into main (d0452f8) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 1eec89b differs from pull request most recent head ab7c455. Consider uploading reports for the commit ab7c455 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2120      +/-   ##
==========================================
- Coverage   87.53%   87.50%   -0.02%     
==========================================
  Files         169      169              
  Lines        4889     4888       -1     
==========================================
- Hits         4279     4277       -2     
- Misses        610      611       +1

see 2 files with indirect coverage changes

lalitb · 2023-05-03T06:03:19Z

@kylepl Why do we want to deprecate the existing members - ssl_credentials_cacert_path and ssl_credentials_cacert_as_string. These are for server-side auth, and can remain there. Can't we just add the new members to OtlpGrpcExporterOptions for client-side auth ? Sorry if I am missing something here.

kylepl · 2023-05-03T13:25:25Z

@lalitb Just to be precise, and is likely what you mean, ssl_credentials_cacert_{path|as_string} are specified by the client as the root certificates to validate the server.

There are few pieces to the change:

I think it is useful to group related options together into sub-structs to show what is related, and to avoid an unwieldy top-level struct. Thus the thinking was to group client-side cert, key and root certs (for authenticating the server). This is what gRPC does too.
I personally like more type-safe objects - hence using a single field as either a file or string contents. This avoids the need to just have a comment that only one is respected - there is no way to specify both.

But, there is a cost of changing what is already there even if we did agree on what to do if this was entirely new code. Thus my proposal is to support the new format and the old format, with a deprecation warning - but this is where I defer to everyone on what makes sense.

So if you want the two existing options to remain where they are, and potentially just have SSLOptions but without the root certs, that's also fine, just not quite as ideal IMO long-term - but everything is trade-offs. :)

kylepl · 2023-05-03T13:58:12Z

I've switched to using absl::variant/optional, thus the compilation should be fixed for earlier versions.

kylepl · 2023-05-04T04:09:25Z

exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_exporter_options.h

@@ -17,7 +16,7 @@ namespace otlp
 {

 // This type allows the variants to be default-constructed to neither type.
-// This avoids needing deeper template if std::optional<std::variant<>>.
+// This avoids needing deeper template if absl::optional<absl::variant<>>.
 struct Unset {};


Note to self: This is what nostd::monostate is for, remove this,

marcalff · 2023-05-22T15:36:54Z

@kylepl

First, thanks for picking this up, support for mTLS in the gRPC exporter is
long overdue.

Please find here some comments and guidance.

struct OtlpGrpcExporterOptions

The ca_cert is already implemented with ssl_credentials_cacert_path and
ssl_credentials_cacert_as_string, so these should not change unfortunately.

For the client cert and key, please use a flat structure, with different
members for the path or string options.

Using a std::variant is technically correct, and more type safe, but it
causes additional complexity to use the interface itself, and the pattern is
different from the HTTP exporter.

Please follow the pattern from struct OtlpHttpExporterOptions, for
consistency.

In the long term, configurations using environment variables is going to be
replaced by configuration using a yaml file, with a different structure, per
OTEP 225:

When this change is implemented for all opentelemetry repositories, the
options struct in opentelemetry-cpp will probably be revisited then.

For now, consistency with the existing HTTP pattern will avoid too many intermediate changes.

Environment variables

struct OtlpGrpcExporterOptions should by default read environment variables,
so that configuration options are honored.

Please call:

GetOtlpDefaultTracesSslClientKeyPath();
GetOtlpDefaultTracesSslClientKeyString();
GetOtlpDefaultTracesSslClientCertificatePath();
GetOtlpDefaultTracesSslClientCertificateString();

Likewise, see struct OtlpHttpExporterOptions.

CMake

This is a new feature, so it needs a CMake option, for optional adoption.

Something like:

CMake option WITH_OTLP_GRPC_SSL_MTLS_PREVIEW
define ENABLE_OTLP_GRPC_SSL_MTLS_PREVIEW

See existing options for the OTLP HTTP exporter.

In the implementation:

it is ok to require a recent gRPC version if mTLS support is required
it is not ok to require a recent gRPC version if mTLS support is not
required.

In other words, the code should still build with old gRPC libraries,
with the understanding that mTLS will not be available then.

See build failures in CI related to gcc 4.8 with a very old gRPC library, it
should still build.

C++11 support, and support for old libraries, will go away over time, but for
now these old builds should still be functional for existing features.

Unit tests

For unit tests for the option struct itself, see file
otlp_grpc_exporter_test.cc.

Examples of units tests for the HTTP exporter can be found in file
otlp_http_exporter_test.cc.

Functional tests

See functional/otlp for existing OTLP HTTP functional tests.

Either in the same PR, or in subsequent work, functional tests talking to an
opentelemetry collector using the OTLP GRPC exporter can be implemented the
same way.

marcalff · 2023-06-21T15:02:39Z

@kylepl

Please confirm if you are still working on this.

The following link contains comments and guidance about how to implement this change, in case you missed it.

Thanks.

TODO: Add tests. I have not run or written any tests, other than verifying it worked once in my production system.

kylepl · 2023-06-21T17:53:05Z

Apologies for the long delay. I've taken another pass - doing everything except for the tests - which I can do once this LGTY.

kylepl · 2023-06-21T22:27:06Z

There were a number of failures - I think because of the unnecessary variant/optional include. I've removed them, so hopefully the bazel versions pass now.

For formatting, is there a script I should run to do the auto-formatting?

Also, I wasn't sure if the thumbs-up meant review seemed reasonable, but I'll wait for an explicit comment before doing the tests. :)

"SSL" to the mTLS macros since evenetually there will be something named TLS on the gRPC side.

kylepl · 2023-06-22T14:48:43Z

And I've put in one more fix for Windows not liking initializer lists for structs.

Should just be the formatting errors to resolve (once I know what to run for it).

exporters/otlp/src/otlp_grpc_client.cc

marcalff

@kylepl

Thanks for the PR, this is looking much better already.

Right now, the build fails (to fix), and does not use the new MTLS_PREVIEW option anyway.

Please look for:

        -DWITH_OTLP_HTTP_SSL_PREVIEW=ON \
        -DWITH_OTLP_HTTP_SSL_TLS_PREVIEW=ON \

in file ci/do_ci.sh,

and add both WITH_OTLP_GRPC and WITH_OTLP_GRPC_SSL_MTLS_PREVIEW at the same places, to have some CI coverage of the new code.

marcalff · 2023-06-23T13:10:22Z

To fix format errors, run clang-format version 10.

See docs/using-clang-format.md

kylepl · 2023-06-23T14:32:54Z

I've done clang-format on the .cc/.h files, and enabled this path in the CI.

kylepl · 2023-06-23T22:44:34Z

I've done another pass of fixing format errors.

There are also now failures about gRPC inputs not being included, I think because more options must be specified - but I'm not sure what exactly needs to be copied over (seems like a lot?). e.g.

  grpc_cpp_plugin=`which grpc_cpp_plugin`
  proto_make_file="CMakeFiles/opentelemetry_proto.dir/build.make"
  sed -i "s~gRPC_CPP_PLUGIN_EXECUTABLE-NOTFOUND~$grpc_cpp_plugin~" ${proto_make_file} #fixme

marcalff · 2023-06-26T06:56:20Z

@kylepl

I've done another pass of fixing format errors.

There are also now failures about gRPC inputs not being included, I think because more options must be specified - but I'm not sure what exactly needs to be copied over (seems like a lot?). e.g.

Indeed, a lot more setup is needed beside just adding WITH_OTLP_GRPC.

This was bad guidance from me, sorry about that.

Let's revert this changes to do_ci.sh, and instead, add WITH_OTLP_GRPC_SSL_MTLS_PREVIEW to only cmake.exporter.otprotocol.test then.

This should be simpler as the GRPC setup is already done for this build.

arleady has GRPC enabled on it.

kylepl · 2023-06-26T14:28:18Z

No worries, done.

owent

LGTM and thanks.

lalitb

Looks good. Thanks for the contribution.
Perhaps as separate PR, we can add the functional tests for validating OTLP gRPC with client certificate chain + Server ca certificate.
Ref- https://github.com/open-telemetry/opentelemetry-cpp/tree/main/functional/otlp

marcalff

Approved, thanks for the contribution.

@kylepl

If you want to continue working in this area, feel free to pick

[TEST] Functional tests for OTLP GRPC MTLS export #2208

kylepl requested a review from a team April 25, 2023 17:59

owent reviewed Apr 26, 2023

View reviewed changes

kylepl commented May 4, 2023

View reviewed changes

marcalff changed the title ~~Add support for client-side cert/key to gRPC exports.~~ [EXPORTER] Add support for client-side cert/key to gRPC exports. May 26, 2023

esigo assigned marcalff Jun 19, 2023

kylepl force-pushed the support-client-certs branch from f55a83a to 8dc2ca9 Compare June 21, 2023 15:54

Add support for client-side cert/key to gRPC exports.

355def3

TODO: Add tests. I have not run or written any tests, other than verifying it worked once in my production system.

kylepl force-pushed the support-client-certs branch from 8dc2ca9 to 355def3 Compare June 21, 2023 17:51

Remove variant/optional includes, they are no longer used.

933e6c0

Switch away from initializer lists because Windows complains. Also add

6245fcd

"SSL" to the mTLS macros since evenetually there will be something named TLS on the gRPC side.

marcalff reviewed Jun 23, 2023

View reviewed changes

exporters/otlp/src/otlp_grpc_client.cc Outdated Show resolved Hide resolved

marcalff reviewed Jun 23, 2023

View reviewed changes

kylepl added 3 commits June 23, 2023 10:13

Fix compilation failure.

4a5e770

Update CI to try these options.

3df0faa

Apply clang-format.

b764668

Fix lint errors.

0dfacfc

Switch to just using WITH_OTLP_GRPC_SSL_MTLS_PREVIEW on one test that

1eec89b

arleady has GRPC enabled on it.

owent approved these changes Jun 29, 2023

View reviewed changes

Merge branch 'main' into support-client-certs

ab7c455

lalitb approved these changes Jun 29, 2023

View reviewed changes

marcalff mentioned this pull request Jun 29, 2023

[TEST] Functional tests for OTLP GRPC MTLS export #2208

Open

marcalff approved these changes Jun 29, 2023

View reviewed changes

marcalff changed the title ~~[EXPORTER] Add support for client-side cert/key to gRPC exports.~~ [EXPORTER] OTLP GRPC mTLS support Jun 29, 2023

marcalff merged commit 049ab63 into open-telemetry:main Jun 29, 2023
41 checks passed

chenrui333 mentioned this pull request Jul 14, 2023

opentelemetry-cpp 1.10.0 Homebrew/homebrew-core#136563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EXPORTER] OTLP GRPC mTLS support #2120

[EXPORTER] OTLP GRPC mTLS support #2120

kylepl commented Apr 25, 2023 •

edited by marcalff

Loading

linux-foundation-easycla bot commented Apr 25, 2023 •

edited

Loading

owent Apr 26, 2023

kylepl May 3, 2023

owent Apr 26, 2023

kylepl May 3, 2023

owent commented Apr 26, 2023

lalitb commented Apr 26, 2023

kylepl commented Apr 26, 2023

codecov bot commented May 3, 2023 •

edited

Loading

lalitb commented May 3, 2023

kylepl commented May 3, 2023

kylepl commented May 3, 2023

kylepl May 4, 2023

marcalff commented May 22, 2023

marcalff commented Jun 21, 2023

kylepl commented Jun 21, 2023

kylepl commented Jun 21, 2023

kylepl commented Jun 22, 2023

marcalff left a comment

marcalff commented Jun 23, 2023

kylepl commented Jun 23, 2023

kylepl commented Jun 23, 2023

marcalff commented Jun 26, 2023

kylepl commented Jun 26, 2023

owent left a comment

lalitb left a comment

marcalff left a comment

[EXPORTER] OTLP GRPC mTLS support #2120

[EXPORTER] OTLP GRPC mTLS support #2120

Conversation

kylepl commented Apr 25, 2023 • edited by marcalff Loading

Changes

linux-foundation-easycla bot commented Apr 25, 2023 • edited Loading

owent Apr 26, 2023

Choose a reason for hiding this comment

kylepl May 3, 2023

Choose a reason for hiding this comment

owent Apr 26, 2023

Choose a reason for hiding this comment

kylepl May 3, 2023

Choose a reason for hiding this comment

owent commented Apr 26, 2023

lalitb commented Apr 26, 2023

kylepl commented Apr 26, 2023

codecov bot commented May 3, 2023 • edited Loading

Codecov Report

lalitb commented May 3, 2023

kylepl commented May 3, 2023

kylepl commented May 3, 2023

kylepl May 4, 2023

Choose a reason for hiding this comment

marcalff commented May 22, 2023

struct OtlpGrpcExporterOptions

Environment variables

CMake

Unit tests

Functional tests

marcalff commented Jun 21, 2023

kylepl commented Jun 21, 2023

kylepl commented Jun 21, 2023

kylepl commented Jun 22, 2023

marcalff left a comment

Choose a reason for hiding this comment

marcalff commented Jun 23, 2023

kylepl commented Jun 23, 2023

kylepl commented Jun 23, 2023

marcalff commented Jun 26, 2023

kylepl commented Jun 26, 2023

owent left a comment

Choose a reason for hiding this comment

lalitb left a comment

Choose a reason for hiding this comment

marcalff left a comment

Choose a reason for hiding this comment

kylepl commented Apr 25, 2023 •

edited by marcalff

Loading

linux-foundation-easycla bot commented Apr 25, 2023 •

edited

Loading

codecov bot commented May 3, 2023 •

edited

Loading