Skip to content
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

[receiver/otlp, internal, featuregate] Add featuregate to switch to localhost defaults for server-like components #8622

Merged
merged 17 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 26 additions & 0 deletions .chloggen/mx-psi_featuregate-localhost-2.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: component

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Add `component.UseLocalHostAsDefaultHost` feature gate that changes default endpoints from 0.0.0.0 to localhost"

# One or more tracking issues or pull requests related to the change
issues: [8510]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext: |
The only component in this repository affected by this is the OTLP receiver.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: []

26 changes: 26 additions & 0 deletions .chloggen/mx-psi_featuregate-localhost.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: featuregate

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Add the `featuregate.ErrAlreadyRegistered` error, which is returned by `featuregate.Registry`'s `Register` when adding a feature gate that is already registered."

# One or more tracking issues or pull requests related to the change
issues: [8622]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext: |
Use `errors.Is` to check for this error.

# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [api]
8 changes: 7 additions & 1 deletion featuregate/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package featuregate // import "go.opentelemetry.io/collector/featuregate"

import (
"errors"
"fmt"
"sort"
"sync"
Expand All @@ -12,6 +13,11 @@ import (

var globalRegistry = NewRegistry()

var (
// ErrAlreadyRegistered is returned when adding a Gate that is already registered.
ErrAlreadyRegistered = errors.New("gate is already registered")
)

// GlobalRegistry returns the global Registry.
func GlobalRegistry() *Registry {
return globalRegistry
Expand Down Expand Up @@ -100,7 +106,7 @@ func (r *Registry) Register(id string, stage Stage, opts ...RegisterOption) (*Ga
return nil, fmt.Errorf("no removal version set for %v gate %q", g.stage.String(), id)
}
if _, loaded := r.gates.LoadOrStore(id, g); loaded {
return nil, fmt.Errorf("attempted to add pre-existing gate %q", id)
return nil, fmt.Errorf("failed to register %q: %w", id, ErrAlreadyRegistered)
}
return g, nil
}
Expand Down
2 changes: 1 addition & 1 deletion featuregate/registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func TestRegistry(t *testing.T) {
assert.False(t, g.IsEnabled())

_, err = r.Register(id, StageBeta)
assert.Error(t, err)
assert.ErrorIs(t, err, ErrAlreadyRegistered)
assert.Panics(t, func() {
r.MustRegister(id, StageBeta)
})
Expand Down
80 changes: 80 additions & 0 deletions internal/localhostgate/featuregate.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

// package localhostgate defines a feature gate that controls whether server-like receivers and extensions use localhost as the default host for their endpoints.
// This package is duplicated across core and contrib to avoid exposing the feature gate as part of the public API.
// To do this we define a `registerOrLoad` helper and try to register the gate in both modules.
// IMPORTANT NOTE: ANY CHANGES TO THIS PACKAGE MUST BE MIRRORED IN THE CONTRIB COUNTERPART.
package localhostgate // import "go.opentelemetry.io/collector/internal/localhostgate"

import (
"errors"
"fmt"

"go.uber.org/zap"

"go.opentelemetry.io/collector/featuregate"
)

const useLocalHostAsDefaultHostID = "component.UseLocalHostAsDefaultHost"

// UseLocalHostAsDefaultHostfeatureGate is the feature gate that controls whether
// server-like receivers and extensions such as the OTLP receiver use localhost as the default host for their endpoints.
var UseLocalHostAsDefaultHostfeatureGate *featuregate.Gate

// registerOrLoad tries to register the feature gate and loads it if it already exists.
func registerOrLoad(reg *featuregate.Registry, id string, stage featuregate.Stage, opts ...featuregate.RegisterOption) (*featuregate.Gate, error) {
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
gate, err := reg.Register(id, stage, opts...)

if err != nil {
switch {
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
case errors.Is(err, featuregate.ErrAlreadyRegistered):
// Gate is already registered; find it.
// Only a handful of feature gates are registered, so it's fine to iterate over all of them.
reg.VisitAll(func(g *featuregate.Gate) {
if g.ID() == id {
gate = g
return
}

Check warning on line 38 in internal/localhostgate/featuregate.go

View check run for this annotation

Codecov / codecov/patch

internal/localhostgate/featuregate.go#L30-L38

Added lines #L30 - L38 were not covered by tests
})
default:
// Propagate the error otherwise.
return nil, err

Check warning on line 42 in internal/localhostgate/featuregate.go

View check run for this annotation

Codecov / codecov/patch

internal/localhostgate/featuregate.go#L40-L42

Added lines #L40 - L42 were not covered by tests
}
}

return gate, nil
}

func init() {
var err error
UseLocalHostAsDefaultHostfeatureGate, err = registerOrLoad(
featuregate.GlobalRegistry(),
useLocalHostAsDefaultHostID,
featuregate.StageAlpha,
featuregate.WithRegisterDescription("controls whether server-like receivers and extensions such as the OTLP receiver use localhost as the default host for their endpoints"),
)

if err != nil {
panic(err)

Check warning on line 59 in internal/localhostgate/featuregate.go

View check run for this annotation

Codecov / codecov/patch

internal/localhostgate/featuregate.go#L59

Added line #L59 was not covered by tests
}
}

// EndpointForPort gets the endpoint for a given port using localhost or 0.0.0.0 depending on the feature gate.
func EndpointForPort(port int) string {
host := "localhost"
if !UseLocalHostAsDefaultHostfeatureGate.IsEnabled() {
host = "0.0.0.0"
}
return fmt.Sprintf("%s:%d", host, port)
}

// LogAboutUseLocalHostAsDefault logs about the upcoming change from 0.0.0.0 to localhost on server-like components.
func LogAboutUseLocalHostAsDefault(logger *zap.Logger) {
if !UseLocalHostAsDefaultHostfeatureGate.IsEnabled() {
logger.Info(
"The default endpoint(s) for this component will change in a future version to use localhost instead of 0.0.0.0. Use the feature gate to preview the new default.",
zap.String("feature gate ID", useLocalHostAsDefaultHostID),
)
}

Check warning on line 79 in internal/localhostgate/featuregate.go

View check run for this annotation

Codecov / codecov/patch

internal/localhostgate/featuregate.go#L73-L79

Added lines #L73 - L79 were not covered by tests
}
57 changes: 57 additions & 0 deletions internal/localhostgate/featuregate_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

package localhostgate

import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"go.opentelemetry.io/collector/featuregate"
)

func setFeatureGateForTest(t testing.TB, gate *featuregate.Gate, enabled bool) func() {
originalValue := gate.IsEnabled()
require.NoError(t, featuregate.GlobalRegistry().Set(gate.ID(), enabled))
return func() {
require.NoError(t, featuregate.GlobalRegistry().Set(gate.ID(), originalValue))
}
}

func TestEndpointForPort(t *testing.T) {
tests := []struct {
port int
enabled bool
endpoint string
}{
{
port: 4317,
enabled: false,
endpoint: "0.0.0.0:4317",
},
{
port: 4317,
enabled: true,
endpoint: "localhost:4317",
},
{
port: 0,
enabled: false,
endpoint: "0.0.0.0:0",
},
{
port: 0,
enabled: true,
endpoint: "localhost:0",
},
}

for _, tt := range tests {
t.Run(tt.endpoint, func(t *testing.T) {
defer setFeatureGateForTest(t, UseLocalHostAsDefaultHostfeatureGate, tt.enabled)()
assert.Equal(t, EndpointForPort(tt.port), tt.endpoint)
})
}
}
4 changes: 3 additions & 1 deletion receiver/otlpreceiver/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ The following settings are configurable:

- `endpoint` (default = 0.0.0.0:4317 for grpc protocol, 0.0.0.0:4318 http protocol):
host:port to which the receiver is going to receive data. The valid syntax is
described at https://github.com/grpc/grpc/blob/master/doc/naming.md.
described at https://github.com/grpc/grpc/blob/master/doc/naming.md. The
`component.UseLocalHostAsDefaultHost` feature gate changes these to localhost:4317 and
localhost:4318 respectively. This will become the default in a future release.

## Advanced Configuration

Expand Down
9 changes: 9 additions & 0 deletions receiver/otlpreceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ type Protocols struct {
type Config struct {
// Protocols is the configuration for the supported protocols, currently gRPC and HTTP (Proto and JSON).
Protocols `mapstructure:"protocols"`

// logLocalHostWarning is used to log a warning if the default unspecified endpoint is used.
// Can be removed once component.UseLocalHostAsDefaultHost moves to stable.
logLocalHostWarning bool
}

var _ component.Config = (*Config)(nil)
Expand All @@ -67,11 +71,16 @@ func (cfg *Config) Unmarshal(conf *confmap.Conf) error {

if !conf.IsSet(protoGRPC) {
cfg.GRPC = nil
cfg.logLocalHostWarning = true // default endpoint used
} else {
cfg.logLocalHostWarning = cfg.logLocalHostWarning || !conf.IsSet(protoGRPC+confmap.KeyDelimiter+"endpoint")
}

if !conf.IsSet(protoHTTP) {
cfg.HTTP = nil
cfg.logLocalHostWarning = true // default endpoint used
} else {
cfg.logLocalHostWarning = cfg.logLocalHostWarning || !conf.IsSet(protoHTTP+confmap.KeyDelimiter+"endpoint")
var err error

if cfg.HTTP.TracesURLPath, err = sanitizeURLPath(cfg.HTTP.TracesURLPath); err != nil {
Expand Down
18 changes: 13 additions & 5 deletions receiver/otlpreceiver/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,20 @@ import (
"go.opentelemetry.io/collector/confmap/confmaptest"
)

// defaultCfgWithLog creates default config with warning log enabled.
func defaultCfgWithLog() component.Config {
cfg := NewFactory().CreateDefaultConfig()
cfg.(*Config).logLocalHostWarning = true
return cfg
}

func TestUnmarshalDefaultConfig(t *testing.T) {
cm, err := confmaptest.LoadConf(filepath.Join("testdata", "default.yaml"))
require.NoError(t, err)
factory := NewFactory()
cfg := factory.CreateDefaultConfig()
assert.NoError(t, component.UnmarshalConfig(cm, cfg))
assert.Equal(t, factory.CreateDefaultConfig(), cfg)
assert.Equal(t, defaultCfgWithLog(), cfg)
}

func TestUnmarshalConfigOnlyGRPC(t *testing.T) {
Expand All @@ -36,7 +43,7 @@ func TestUnmarshalConfigOnlyGRPC(t *testing.T) {
cfg := factory.CreateDefaultConfig()
assert.NoError(t, component.UnmarshalConfig(cm, cfg))

defaultOnlyGRPC := factory.CreateDefaultConfig().(*Config)
defaultOnlyGRPC := defaultCfgWithLog().(*Config)
defaultOnlyGRPC.HTTP = nil
assert.Equal(t, defaultOnlyGRPC, cfg)
}
Expand All @@ -48,7 +55,7 @@ func TestUnmarshalConfigOnlyHTTP(t *testing.T) {
cfg := factory.CreateDefaultConfig()
assert.NoError(t, component.UnmarshalConfig(cm, cfg))

defaultOnlyHTTP := factory.CreateDefaultConfig().(*Config)
defaultOnlyHTTP := defaultCfgWithLog().(*Config)
defaultOnlyHTTP.GRPC = nil
assert.Equal(t, defaultOnlyHTTP, cfg)
}
Expand All @@ -60,7 +67,7 @@ func TestUnmarshalConfigOnlyHTTPNull(t *testing.T) {
cfg := factory.CreateDefaultConfig()
assert.NoError(t, component.UnmarshalConfig(cm, cfg))

defaultOnlyHTTP := factory.CreateDefaultConfig().(*Config)
defaultOnlyHTTP := defaultCfgWithLog().(*Config)
defaultOnlyHTTP.GRPC = nil
assert.Equal(t, defaultOnlyHTTP, cfg)
}
Expand All @@ -72,7 +79,7 @@ func TestUnmarshalConfigOnlyHTTPEmptyMap(t *testing.T) {
cfg := factory.CreateDefaultConfig()
assert.NoError(t, component.UnmarshalConfig(cm, cfg))

defaultOnlyHTTP := factory.CreateDefaultConfig().(*Config)
defaultOnlyHTTP := defaultCfgWithLog().(*Config)
defaultOnlyHTTP.GRPC = nil
assert.Equal(t, defaultOnlyHTTP, cfg)
}
Expand Down Expand Up @@ -134,6 +141,7 @@ func TestUnmarshalConfig(t *testing.T) {
LogsURLPath: "/log/ingest",
},
},
logLocalHostWarning: true,
}, cfg)

}
Expand Down
9 changes: 5 additions & 4 deletions receiver/otlpreceiver/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,16 @@ import (
"go.opentelemetry.io/collector/config/confighttp"
"go.opentelemetry.io/collector/config/confignet"
"go.opentelemetry.io/collector/consumer"
"go.opentelemetry.io/collector/internal/localhostgate"
"go.opentelemetry.io/collector/internal/sharedcomponent"
"go.opentelemetry.io/collector/receiver"
)

const (
typeStr = "otlp"

defaultGRPCEndpoint = "0.0.0.0:4317"
defaultHTTPEndpoint = "0.0.0.0:4318"
grpcPort = 4317
httpPort = 4318

defaultTracesURLPath = "/v1/traces"
defaultMetricsURLPath = "/v1/metrics"
Expand All @@ -42,15 +43,15 @@ func createDefaultConfig() component.Config {
Protocols: Protocols{
GRPC: &configgrpc.GRPCServerSettings{
NetAddr: confignet.NetAddr{
Endpoint: defaultGRPCEndpoint,
Endpoint: localhostgate.EndpointForPort(grpcPort),
Transport: "tcp",
},
// We almost write 0 bytes, so no need to tune WriteBufferSize.
ReadBufferSize: 512 * 1024,
},
HTTP: &HTTPConfig{
HTTPServerSettings: &confighttp.HTTPServerSettings{
Endpoint: defaultHTTPEndpoint,
Endpoint: localhostgate.EndpointForPort(httpPort),
},
TracesURLPath: defaultTracesURLPath,
MetricsURLPath: defaultMetricsURLPath,
Expand Down