diff --git a/.chloggen/jaegerremotesampling-port-validation.yaml b/.chloggen/jaegerremotesampling-port-validation.yaml new file mode 100644 index 0000000000000..c8d09e7ed0460 --- /dev/null +++ b/.chloggen/jaegerremotesampling-port-validation.yaml @@ -0,0 +1,16 @@ +change_type: breaking + +component: jaegerremotesampling + +note: Require explicit `grpc:` configuration to keep the default gRPC sampling endpoint and add HTTP/gRPC port conflict validation +issues: [45944] +subtext: | + The jaegerremotesampling extension now validates that HTTP and gRPC servers + are configured on different ports during configuration loading, preventing + runtime port binding failures and providing clear, actionable error messages + for misconfigurations. + + This change is breaking: the extension no longer starts a gRPC server on + port 14250 by default when `grpc:` is omitted from the YAML configuration. + Users who relied on the previous default must now configure `grpc:` + explicitly to keep the gRPC sampling endpoint. diff --git a/extension/jaegerremotesampling/config.go b/extension/jaegerremotesampling/config.go index ae7520aaca155..1ffb57c6a2c10 100644 --- a/extension/jaegerremotesampling/config.go +++ b/extension/jaegerremotesampling/config.go @@ -10,6 +10,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/config/configgrpc" "go.opentelemetry.io/collector/config/confighttp" + "go.opentelemetry.io/collector/config/configoptional" ) var ( @@ -21,8 +22,8 @@ var ( // Config has the configuration for the extension enabling the health check // extension, used to report the health status of the service. type Config struct { - HTTPServerConfig *confighttp.ServerConfig `mapstructure:"http"` - GRPCServerConfig *configgrpc.ServerConfig `mapstructure:"grpc"` + HTTPServerConfig *confighttp.ServerConfig `mapstructure:"http"` + GRPCServerConfig configoptional.Optional[configgrpc.ServerConfig] `mapstructure:"grpc"` // Source configures the source for the strategies file. One of `remote` or `file` has to be specified. Source Source `mapstructure:"source"` @@ -43,10 +44,12 @@ var _ component.Config = (*Config)(nil) // Validate checks if the extension configuration is valid func (cfg *Config) Validate() error { - if cfg.HTTPServerConfig == nil && cfg.GRPCServerConfig == nil { + // Validate the protocol configuration. At least one protocol should be configured to serve the strategies. + if cfg.HTTPServerConfig == nil && !cfg.GRPCServerConfig.HasValue() { return errAtLeastOneProtocol } + // Validate source configuration if cfg.Source.File != "" && cfg.Source.Remote != nil { return errTooManySources } diff --git a/extension/jaegerremotesampling/config_test.go b/extension/jaegerremotesampling/config_test.go index a2082fd840aad..88cbd024aff56 100644 --- a/extension/jaegerremotesampling/config_test.go +++ b/extension/jaegerremotesampling/config_test.go @@ -14,6 +14,7 @@ import ( "go.opentelemetry.io/collector/config/configgrpc" "go.opentelemetry.io/collector/config/confighttp" "go.opentelemetry.io/collector/config/confignet" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/confmap/confmaptest" "go.opentelemetry.io/collector/confmap/xconfmap" @@ -30,14 +31,13 @@ func TestLoadConfig(t *testing.T) { { id: component.NewID(metadata.Type), expected: &Config{ - HTTPServerConfig: &confighttp.ServerConfig{NetAddr: confignet.AddrConfig{ - Endpoint: "localhost:5778", - Transport: confignet.TransportTypeTCP, - }}, - GRPCServerConfig: &configgrpc.ServerConfig{NetAddr: confignet.AddrConfig{ - Endpoint: "localhost:14250", - Transport: confignet.TransportTypeTCP, - }}, + HTTPServerConfig: &confighttp.ServerConfig{ + NetAddr: confignet.AddrConfig{ + Endpoint: "localhost:5778", + Transport: confignet.TransportTypeTCP, + }, + }, + GRPCServerConfig: configoptional.None[configgrpc.ServerConfig](), Source: Source{ Remote: &configgrpc.ClientConfig{ Endpoint: "jaeger-collector:14250", @@ -48,14 +48,13 @@ func TestLoadConfig(t *testing.T) { { id: component.NewIDWithName(metadata.Type, "1"), expected: &Config{ - HTTPServerConfig: &confighttp.ServerConfig{NetAddr: confignet.AddrConfig{ - Endpoint: "localhost:5778", - Transport: confignet.TransportTypeTCP, - }}, - GRPCServerConfig: &configgrpc.ServerConfig{NetAddr: confignet.AddrConfig{ - Endpoint: "localhost:14250", - Transport: confignet.TransportTypeTCP, - }}, + HTTPServerConfig: &confighttp.ServerConfig{ + NetAddr: confignet.AddrConfig{ + Endpoint: "localhost:5778", + Transport: confignet.TransportTypeTCP, + }, + }, + GRPCServerConfig: configoptional.None[configgrpc.ServerConfig](), Source: Source{ ReloadInterval: time.Second, File: "/etc/otelcol/sampling_strategies.json", @@ -92,14 +91,14 @@ func TestValidate(t *testing.T) { { desc: "no sources", cfg: Config{ - GRPCServerConfig: &configgrpc.ServerConfig{}, + GRPCServerConfig: configoptional.Some(configgrpc.ServerConfig{}), }, expected: errNoSources, }, { desc: "too many sources", cfg: Config{ - GRPCServerConfig: &configgrpc.ServerConfig{}, + GRPCServerConfig: configoptional.Some(configgrpc.ServerConfig{}), Source: Source{ Remote: &configgrpc.ClientConfig{}, File: "/tmp/some-file", @@ -115,3 +114,80 @@ func TestValidate(t *testing.T) { }) } } + +func TestConfigValidate(t *testing.T) { + tests := []struct { + name string + config Config + expectedErr error + }{ + { + name: "valid config with both protocols", + config: Config{ + HTTPServerConfig: &confighttp.ServerConfig{ + NetAddr: confignet.AddrConfig{ + Endpoint: "0.0.0.0:5778", + }, + }, + GRPCServerConfig: configoptional.Some(configgrpc.ServerConfig{ + NetAddr: confignet.AddrConfig{ + Endpoint: "0.0.0.0:14250", + }, + }), + Source: Source{ + File: "/etc/strategies.json", + }, + }, + expectedErr: nil, + }, + { + name: "valid config with only gRPC", + config: Config{ + GRPCServerConfig: configoptional.Some(configgrpc.ServerConfig{ + NetAddr: confignet.AddrConfig{ + Endpoint: "0.0.0.0:14250", + }, + }), + Source: Source{ + Remote: &configgrpc.ClientConfig{}, + }, + }, + expectedErr: nil, + }, + { + name: "valid config with only HTTP", + config: Config{ + HTTPServerConfig: &confighttp.ServerConfig{ + NetAddr: confignet.AddrConfig{ + Endpoint: "0.0.0.0:5778", + }, + }, + Source: Source{ + File: "/etc/strategies.json", + }, + }, + expectedErr: nil, + }, + { + name: "invalid: no protocols configured", + config: Config{ + Source: Source{ + File: "/etc/strategies.json", + }, + }, + expectedErr: errAtLeastOneProtocol, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.config.Validate() + if tt.expectedErr != nil { + require.Error(t, err) + assert.ErrorIs(t, err, tt.expectedErr) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/extension/jaegerremotesampling/extension.go b/extension/jaegerremotesampling/extension.go index ae0bf23785197..bbb0e9179d910 100644 --- a/extension/jaegerremotesampling/extension.go +++ b/extension/jaegerremotesampling/extension.go @@ -87,11 +87,13 @@ func (jrse *jrsExtension) Start(ctx context.Context, host component.Host) error } } - if jrse.cfg.GRPCServerConfig != nil { - grpcServer, err := grpc.NewGRPC(jrse.telemetry, *jrse.cfg.GRPCServerConfig, jrse.samplingStore) + if jrse.cfg.GRPCServerConfig.HasValue() { + grpcCfg := jrse.cfg.GRPCServerConfig.Get() + grpcServer, err := grpc.NewGRPC(jrse.telemetry, *grpcCfg, jrse.samplingStore) if err != nil { return fmt.Errorf("error while creating the gRPC server: %w", err) } + jrse.grpcServer = grpcServer // start our gRPC server interface if err := jrse.grpcServer.Start(ctx, host); err != nil { diff --git a/extension/jaegerremotesampling/extension_test.go b/extension/jaegerremotesampling/extension_test.go index f78398bac5e71..747ba84c01f40 100644 --- a/extension/jaegerremotesampling/extension_test.go +++ b/extension/jaegerremotesampling/extension_test.go @@ -17,7 +17,9 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/component/componenttest" "go.opentelemetry.io/collector/config/configgrpc" + "go.opentelemetry.io/collector/config/confignet" "go.opentelemetry.io/collector/config/configopaque" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/config/configtls" "google.golang.org/grpc" "google.golang.org/grpc/metadata" @@ -98,7 +100,12 @@ func TestRemote(t *testing.T) { // create the config, pointing to the mock server cfg := testConfig() - cfg.GRPCServerConfig.NetAddr.Endpoint = "127.0.0.1:0" + cfg.GRPCServerConfig = configoptional.Some(configgrpc.ServerConfig{ + NetAddr: confignet.AddrConfig{ + Endpoint: "127.0.0.1:0", + Transport: confignet.TransportTypeTCP, + }, + }) cfg.Source.ReloadInterval = tc.reloadInterval cfg.Source.Remote = &configgrpc.ClientConfig{ Endpoint: fmt.Sprintf("127.0.0.1:%d", lis.Addr().(*net.TCPAddr).Port), @@ -162,10 +169,15 @@ func (s *samplingServer) GetSamplingStrategy(ctx context.Context, params *api_v2 StrategyType: api_v2.SamplingStrategyType_PROBABILISTIC, }, nil } - func testConfig() *Config { cfg := createDefaultConfig().(*Config) cfg.HTTPServerConfig.NetAddr.Endpoint = "127.0.0.1:5778" - cfg.GRPCServerConfig.NetAddr.Endpoint = "127.0.0.1:14250" + cfg.GRPCServerConfig = configoptional.Some(configgrpc.ServerConfig{ + NetAddr: confignet.AddrConfig{ + Endpoint: "127.0.0.1:14250", + Transport: confignet.TransportTypeTCP, + }, + }) + return cfg } diff --git a/extension/jaegerremotesampling/factory.go b/extension/jaegerremotesampling/factory.go index 55fc78eadde53..860d327f40bee 100644 --- a/extension/jaegerremotesampling/factory.go +++ b/extension/jaegerremotesampling/factory.go @@ -11,6 +11,7 @@ import ( "go.opentelemetry.io/collector/config/configgrpc" "go.opentelemetry.io/collector/config/confighttp" "go.opentelemetry.io/collector/config/confignet" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/extension" "go.uber.org/zap" @@ -36,13 +37,8 @@ func createDefaultConfig() component.Config { Transport: confignet.TransportTypeTCP, }, }, - GRPCServerConfig: &configgrpc.ServerConfig{ - NetAddr: confignet.AddrConfig{ - Endpoint: testutil.EndpointForPort(14250), - Transport: confignet.TransportTypeTCP, - }, - }, - Source: Source{}, + GRPCServerConfig: configoptional.None[configgrpc.ServerConfig](), + Source: Source{}, } } diff --git a/extension/jaegerremotesampling/factory_test.go b/extension/jaegerremotesampling/factory_test.go index 918eb9623fba8..f3c7aa1754167 100644 --- a/extension/jaegerremotesampling/factory_test.go +++ b/extension/jaegerremotesampling/factory_test.go @@ -11,6 +11,7 @@ import ( "go.opentelemetry.io/collector/config/configgrpc" "go.opentelemetry.io/collector/config/confighttp" "go.opentelemetry.io/collector/config/confignet" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/extension/extensiontest" ) @@ -21,10 +22,7 @@ func TestCreateDefaultConfig(t *testing.T) { Endpoint: "localhost:5778", Transport: confignet.TransportTypeTCP, }}, - GRPCServerConfig: &configgrpc.ServerConfig{NetAddr: confignet.AddrConfig{ - Endpoint: "localhost:14250", - Transport: confignet.TransportTypeTCP, - }}, + GRPCServerConfig: configoptional.None[configgrpc.ServerConfig](), } // test