Support mTLS in gRPC exporters #927
Conversation
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
pavolloffay
requested review from
bogdandrutu,
dmitryax,
flands,
owais,
pjanotti,
rghetia,
songy23 and
tigrannajaryan
as code owners
| type TLSConfig struct { | ||
| // Root CA certificate file for TLS credentials of gRPC client. Should | ||
| // only be used if `secure` is set to true. | ||
| CaCert string `mapstructure:"cert_pem_file"` |
There was a problem hiding this comment.
The config property name hasn't been changed.
Can we make a breaking change and change it to include ca in name? e.g ca_cert_pem_file? Or just ca_cert?
There was a problem hiding this comment.
We can probably do that in a separate PR.
This is a breaking change when it comes to the code (not configuration) so you will need to followup with a PR in the collector-contrib where you update otel version (using make, see the rule there) and update all the code. Also need to make a quick announcement on the gitter, so if there are others who depend on this they know about this change.
There was a problem hiding this comment.
@dmitryax updated OTEL collector in the contrib in PR https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/220/files#diff-993db3ecf423549b9fcb5b7e4d195efeR10.
That corresponds to 80ee563 that landed to master after this PR.
There was a problem hiding this comment.
@bogdandrutu ping me if there is anything I can do more. The refactoring/unifying of the TLS spec will be done in #933
| type TLSConfig struct { | ||
| // Root CA certificate file for TLS credentials of gRPC client. Should | ||
| // only be used if `secure` is set to true. | ||
| CaCert string `mapstructure:"cert_pem_file"` |
There was a problem hiding this comment.
We can probably do that in a separate PR.
This is a breaking change when it comes to the code (not configuration) so you will need to followup with a PR in the collector-contrib where you update otel version (using make, see the rule there) and update all the code. Also need to make a quick announcement on the gitter, so if there are others who depend on this they know about this change.
config/configgrpc/configgrpc.go
Outdated
| @@ -110,3 +125,56 @@ func GrpcSettingsToDialOptions(settings GRPCSettings) ([]grpc.DialOption, error) | |||
|
|
|||
| return opts, nil | |||
| } | |||
|
|
|||
| // Config loads TLS certificates and returns a TLS Config. | |||
| func (c TLSConfig) Config() (*tls.Config, error) { | |||
There was a problem hiding this comment.
Maybe LoadConfig? or ToCryptoTlsConfig
There was a problem hiding this comment.
I like more load as it really loads it.
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
|
@bogdandrutu PR updated. Once this is merged I will open a PR to the contrib to update the code there and announce on the gitter channel. |
| @@ -62,6 +57,26 @@ type GRPCSettings struct { | |||
| WaitForReady bool `mapstructure:"wait_for_ready"` | |||
| } | |||
|
|
|||
| // TLSConfig exposes client TLS configuration. | |||
| type TLSConfig struct { | |||
There was a problem hiding this comment.
QQ: should this be moved in a package configtls in order to be shared with other protocols that support tls?
There was a problem hiding this comment.
Also I feel that we are inconsistent between calling the structs settings vs config. May be done in a separate PR
There was a problem hiding this comment.
+1 to this request - I'm working on adding mTLS for receivers/exporters that use HTTP.
|
One structural question - There are TLS config options under https://github.com/open-telemetry/opentelemetry-collector/blob/master/receiver/securereceiverconfig.go Should all TLS option then fall under config/... |
|
Going to merge this since we have a separate issue to track the merging of configs. #933 |
* Support mTLS in gRPC exporters Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Rename to CaCert Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Add tests Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fix lint Signed-off-by: Pavol Loffay <ploffay@redhat.com> * rename load func Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Description:
Support mutual TLS in gRPC exporters.
Other notable changes:
CertPemFilerenamed toCaCert, the config property wasn't changed.Link to tracking Issue:
Testing: < Describe what testing was performed and which tests were added.>
Documentation: < Describe the documentation added.>