-
Notifications
You must be signed in to change notification settings - Fork 11
feat(otlp): added tls server config option for http and grpc #2220
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,7 +71,10 @@ use std::{num::NonZeroUsize, time::Duration}; | |
|
|
||
| use agent_data_plane_config::control::ListenAddress; | ||
| use agent_data_plane_config::defaults::{DEFAULT_STRING_INTERNER_SIZE_BYTES, MAX_STRING_INTERNER_SIZE_BYTES}; | ||
| use agent_data_plane_config::domains::traces::{OttlErrorMode, OttlFilter, OttlTransform}; | ||
| use agent_data_plane_config::domains::{ | ||
| otlp::TlsConfig, | ||
| traces::{OttlErrorMode, OttlFilter, OttlTransform}, | ||
| }; | ||
| use agent_data_plane_config::SalukiConfiguration; | ||
| use bytesize::ByteSize; | ||
| use saluki_config::DurationString; | ||
|
|
@@ -311,16 +314,28 @@ pub struct OtlpConfigReceiver { | |
| #[derive(Clone, Debug, Default, Deserialize)] | ||
| #[serde(default)] | ||
| pub struct OtlpConfigReceiverProtocols { | ||
| /// OTLP gRPC receiver TLS knobs (`otlp_config.receiver.protocols.grpc.tls.*`). | ||
| pub grpc: OtlpConfigReceiverProtocolsGrpc, | ||
| /// OTLP HTTP receiver knobs (`otlp_config.receiver.protocols.http.*`). | ||
| pub http: OtlpConfigReceiverProtocolsHttp, | ||
| } | ||
|
|
||
| /// `otlp_config.receiver.protocols.grpc.*` values absent from the Datadog schema. | ||
| #[derive(Clone, Debug, Default, Deserialize)] | ||
| #[serde(default)] | ||
| pub struct OtlpConfigReceiverProtocolsGrpc { | ||
| /// OTLP gRPC receiver TLS configuration. | ||
| pub tls: TlsConfig, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fact that these are being added as saluki_only keys is suspicious. If these are not part of the Agent's configurable surface, i.e. if they are part of the OTEL collector but not configurable when running OTEL Ingest via the Agent, then they would be out of scope for the current project. |
||
| } | ||
|
|
||
| /// `otlp_config.receiver.protocols.http.*`. | ||
| #[derive(Clone, Debug, Default, Deserialize)] | ||
| #[serde(default)] | ||
| pub struct OtlpConfigReceiverProtocolsHttp { | ||
| /// OTLP HTTP receiver transport (`otlp_config.receiver.protocols.http.transport`). | ||
| pub transport: Option<String>, | ||
| /// OTLP HTTP receiver TLS configuration. | ||
| pub tls: TlsConfig, | ||
| } | ||
|
|
||
| fn deserialize_string_interner_size<'de, D>(deserializer: D) -> Result<NonZeroUsize, D::Error> | ||
|
|
@@ -513,6 +528,8 @@ impl SalukiOnly { | |
| if let Some(v) = self.otlp_config.receiver.protocols.http.transport.clone() { | ||
| otlp.receiver.http.transport = v; | ||
| } | ||
| otlp.receiver.grpc.tls = self.otlp_config.receiver.protocols.grpc.tls.clone(); | ||
| otlp.receiver.http.tls = self.otlp_config.receiver.protocols.http.tls.clone(); | ||
| otlp.traces.string_interner_size = self.otlp_config.traces.string_interner_size; | ||
| if let Some(v) = self.otlp_config.traces.enable_otlp_compute_top_level_by_span_kind { | ||
| otlp.traces.enable_compute_top_level_by_span_kind = v; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
|
|
||
| use std::{num::NonZeroUsize, str::FromStr}; | ||
|
|
||
| use serde::Serialize; | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| use crate::defaults::DEFAULT_STRING_INTERNER_SIZE_BYTES; | ||
| use crate::Error; | ||
|
|
@@ -81,6 +81,55 @@ impl FromStr for HistogramMode { | |
| } | ||
| } | ||
|
|
||
| /// TLS settings for an inbound OTLP receiver. | ||
| #[derive(Clone, Debug, Default, Deserialize, PartialEq, Eq, Serialize)] | ||
| #[serde(default)] | ||
| pub struct TlsConfig { | ||
|
Comment on lines
+85
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a user supplies the normal TLS configuration containing only Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in e4ef0f4 |
||
| /// Path to PEM-encoded certificate authorities loaded for Collector configuration compatibility. | ||
| /// | ||
| /// For an inbound receiver, this does not affect client authentication. Use [`Self::client_ca_file`] to configure | ||
| /// mutual TLS. | ||
| pub ca_file: String, | ||
|
|
||
| /// Path to the PEM-encoded certificate chain the receiver presents to clients. | ||
| pub cert_file: String, | ||
|
|
||
| /// Path to the PEM-encoded private key matching [`Self::cert_file`]. | ||
| pub key_file: String, | ||
|
|
||
| /// Path to PEM-encoded certificate authorities trusted for client certificates. | ||
| /// | ||
| /// When set, the receiver requires and verifies a client certificate, enabling mutual TLS. | ||
| pub client_ca_file: String, | ||
| } | ||
|
|
||
| impl TlsConfig { | ||
| /// Returns whether TLS is configured for the receiver. | ||
| /// | ||
| /// TLS is disabled when no paths are configured. A configured server identity requires both `cert_file` and | ||
| /// `key_file`; configuring only one, or configuring either CA path without an identity, is an error. | ||
| pub fn is_configured(&self) -> Result<bool, Error> { | ||
| match ( | ||
| self.cert_file.is_empty(), | ||
| self.key_file.is_empty(), | ||
| self.ca_file.is_empty(), | ||
| self.client_ca_file.is_empty(), | ||
| ) { | ||
| (true, true, true, true) => Ok(false), | ||
| (false, false, _, _) => Ok(true), | ||
| (true, true, false, _) => Err(Error::new_without_source( | ||
| "`otlp_config.receiver.protocols.*.tls.ca_file` requires `cert_file` and `key_file`", | ||
| )), | ||
| (true, true, _, false) => Err(Error::new_without_source( | ||
| "`otlp_config.receiver.protocols.*.tls.client_ca_file` requires `cert_file` and `key_file`", | ||
| )), | ||
| _ => Err(Error::new_without_source( | ||
| "`otlp_config.receiver.protocols.*.tls.cert_file` and `key_file` must be configured together", | ||
| )), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// How cumulative monotonic sums are reported. | ||
| #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize)] | ||
| pub enum CumulativeMonotonicMode { | ||
|
|
@@ -220,6 +269,9 @@ pub struct GrpcReceiver { | |
|
|
||
| /// Transport the gRPC receiver binds (for example, `tcp` or `unix`). | ||
| pub transport: String, | ||
|
|
||
| /// TLS settings for this inbound receiver. | ||
| pub tls: TlsConfig, | ||
| } | ||
|
|
||
| /// OTLP HTTP receiver. | ||
|
|
@@ -231,6 +283,9 @@ pub struct HttpReceiver { | |
| /// Transport the HTTP receiver binds (for example, `tcp` or `unix`). (not in Datadog Agent | ||
| /// config schema) | ||
| pub transport: String, | ||
|
|
||
| /// TLS settings for this inbound receiver. | ||
| pub tls: TlsConfig, | ||
| } | ||
|
|
||
| impl Default for HttpReceiver { | ||
|
|
@@ -239,6 +294,7 @@ impl Default for HttpReceiver { | |
| // Witnessed; overwritten during drive. | ||
| endpoint: String::new(), | ||
| transport: "tcp".to_string(), | ||
| tls: TlsConfig::default(), | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -331,7 +387,28 @@ impl Default for Contexts { | |
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::{CumulativeMonotonicMode, InitialCumulativeMonotonicValue}; | ||
| use super::{CumulativeMonotonicMode, InitialCumulativeMonotonicValue, TlsConfig}; | ||
|
|
||
| #[test] | ||
| fn tls_config_requires_a_complete_server_identity() { | ||
| let mut config = TlsConfig::default(); | ||
| assert!(!config.is_configured().expect("an empty TLS config should disable TLS")); | ||
|
|
||
| config.cert_file = "server.pem".to_string(); | ||
| assert!(config.is_configured().is_err()); | ||
|
|
||
| config.key_file = "server.key".to_string(); | ||
| assert!(config.is_configured().expect("a certificate and key should enable TLS")); | ||
|
|
||
| config.cert_file.clear(); | ||
| config.key_file.clear(); | ||
| config.client_ca_file = "clients.pem".to_string(); | ||
| assert!(config.is_configured().is_err()); | ||
|
|
||
| config.client_ca_file.clear(); | ||
| config.ca_file = "compatibility-ca.pem".to_string(); | ||
| assert!(config.is_configured().is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn cumulative_monotonic_mode_parses_known_values() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the goal to fully implement the options from the OTEL side? Seems like we're missing quite a bit if so? https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configtls/README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so. I scoped this PR to the options that are present here, but I am confused about what the authoritative and comprehensive list is.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that your link is outdated and that the
.schema.yamlfiles strewn about are authoritative, but maybe we should check with someone on the OTEL side. Here's the schema for TLS: https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configtls/config.schema.yamlIs our actual goal to 100% support all OTEL config options?