From c313985ecb02d55f30251023b5be97b7f39e0f61 Mon Sep 17 00:00:00 2001 From: Ryan Fitzpatrick Date: Wed, 14 Dec 2022 22:25:33 +0000 Subject: [PATCH] Incorporate obsoleted overwritepropertiesconverter and strip --set args for otelcol service https://github.com/open-telemetry/opentelemetry-collector/pull/6656 removed the overwrite properties config converter that this distribution uses. These changes migrate its functionality to our library and strip --set options to prevent further utilization by the otelcol service. They also add an integration tests to vet its usage. --- .../configconverter/overwrite_properties.go | 66 ++++++++ .../overwrite_properties_test.go | 59 +++++++ internal/settings/settings.go | 32 ++-- internal/settings/settings_test.go | 24 ++- tests/general/set_properties_test.go | 152 ++++++++++++++++++ .../testdata/set_properties_config.yaml | 28 ++++ tests/testutils/collector_container.go | 15 +- 7 files changed, 347 insertions(+), 29 deletions(-) create mode 100644 internal/configconverter/overwrite_properties.go create mode 100644 internal/configconverter/overwrite_properties_test.go create mode 100644 tests/general/set_properties_test.go create mode 100644 tests/general/testdata/set_properties_config.yaml diff --git a/internal/configconverter/overwrite_properties.go b/internal/configconverter/overwrite_properties.go new file mode 100644 index 0000000000..0301a769c5 --- /dev/null +++ b/internal/configconverter/overwrite_properties.go @@ -0,0 +1,66 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Taken from https://github.com/open-telemetry/opentelemetry-collector/blob/v0.66.0/confmap/converter/overwritepropertiesconverter/properties.go +// to prevent breaking changes. +// "Deprecated: [v0.63.0] this converter will not be supported anymore because of dot separation limitation. +// See https://github.com/open-telemetry/opentelemetry-collector/issues/6294 for more details." +package configconverter + +import ( + "bytes" + "context" + "strings" + + "github.com/knadh/koanf/maps" + "github.com/magiconair/properties" + + "go.opentelemetry.io/collector/confmap" +) + +type converter struct { + properties []string +} + +func NewOverwritePropertiesConverter(properties []string) confmap.Converter { + return &converter{properties: properties} +} + +func (c *converter) Convert(_ context.Context, conf *confmap.Conf) error { + if len(c.properties) == 0 { + return nil + } + + b := &bytes.Buffer{} + for _, property := range c.properties { + property = strings.TrimSpace(property) + b.WriteString(property) + b.WriteString("\n") + } + + var props *properties.Properties + var err error + if props, err = properties.Load(b.Bytes(), properties.UTF8); err != nil { + return err + } + + // Create a map manually instead of using properties.Map() to not expand the env vars. + parsed := make(map[string]interface{}, props.Len()) + for _, key := range props.Keys() { + value, _ := props.Get(key) + parsed[key] = value + } + prop := maps.Unflatten(parsed, ".") + return conf.Merge(confmap.NewFromStringMap(prop)) +} diff --git a/internal/configconverter/overwrite_properties_test.go b/internal/configconverter/overwrite_properties_test.go new file mode 100644 index 0000000000..66b0ae74eb --- /dev/null +++ b/internal/configconverter/overwrite_properties_test.go @@ -0,0 +1,59 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Taken from https://github.com/open-telemetry/opentelemetry-collector/blob/v0.66.0/confmap/converter/overwritepropertiesconverter/properties_test.go +// to brevent breaking changes. +package configconverter + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "go.opentelemetry.io/collector/confmap" +) + +func TestOverwritePropertiesConverter_Empty(t *testing.T) { + pmp := NewOverwritePropertiesConverter(nil) + conf := confmap.NewFromStringMap(map[string]interface{}{"foo": "bar"}) + assert.NoError(t, pmp.Convert(context.Background(), conf)) + assert.Equal(t, map[string]interface{}{"foo": "bar"}, conf.ToStringMap()) +} + +func TestOverwritePropertiesConverter(t *testing.T) { + props := []string{ + "processors.batch.timeout=2s", + "processors.batch/foo.timeout=3s", + "receivers.otlp.protocols.grpc.endpoint=localhost:1818", + "exporters.kafka.brokers=foo:9200,foo2:9200", + } + + pmp := NewOverwritePropertiesConverter(props) + conf := confmap.New() + require.NoError(t, pmp.Convert(context.Background(), conf)) + keys := conf.AllKeys() + assert.Len(t, keys, 4) + assert.Equal(t, "2s", conf.Get("processors::batch::timeout")) + assert.Equal(t, "3s", conf.Get("processors::batch/foo::timeout")) + assert.Equal(t, "foo:9200,foo2:9200", conf.Get("exporters::kafka::brokers")) + assert.Equal(t, "localhost:1818", conf.Get("receivers::otlp::protocols::grpc::endpoint")) +} + +func TestOverwritePropertiesConverter_InvalidProperty(t *testing.T) { + pmp := NewOverwritePropertiesConverter([]string{"=2s"}) + conf := confmap.New() + assert.Error(t, pmp.Convert(context.Background(), conf)) +} diff --git a/internal/settings/settings.go b/internal/settings/settings.go index e983a5859f..0b4179429d 100644 --- a/internal/settings/settings.go +++ b/internal/settings/settings.go @@ -118,16 +118,16 @@ func (f *flags) ResolverURIs() []string { configDir := getConfigDir(f) if f.dryRun { - removeFlag(&f.serviceArgs, "--dry-run") + removeFlag(&f.serviceArgs, "--dry-run", false) } if f.configD { - removeFlag(&f.serviceArgs, "--configd") + removeFlag(&f.serviceArgs, "--configd", false) configPaths = append(configPaths, fmt.Sprintf("%s:%s", ConfigDScheme, configDir)) } if f.discoveryMode { - removeFlag(&f.serviceArgs, "--discovery") + removeFlag(&f.serviceArgs, "--discovery", false) // discovery uri must come last to successfully merge w/ other config content configPaths = append(configPaths, fmt.Sprintf("%s:%s", DiscoveryModeScheme, f.configDir)) } @@ -151,24 +151,24 @@ func getConfigDir(f *flags) string { } if f.configDir.value != nil { - removeFlag(&f.serviceArgs, "--config-dir") + removeFlag(&f.serviceArgs, "--config-dir", true) configDir = f.configDir.String() - removeFlag(&f.serviceArgs, configDir) } return configDir } func (f *flags) ConfMapConverters() []confmap.Converter { - var confMapConverters []confmap.Converter - // nolint: staticcheck - // overwritepropertiesconverter.New(f.setProperties.value), // support until there's an actual replacement + confMapConverters := []confmap.Converter{ + configconverter.NewOverwritePropertiesConverter(f.setProperties.value), + } + removeFlag(&f.serviceArgs, "--set", true) if f.noConvertConfig { // the collector complains about this flag if we don't remove it. Unfortunately, // this must be done manually since the flag library has no functionality to remove // args - removeFlag(&f.serviceArgs, "--no-convert-config") + removeFlag(&f.serviceArgs, "--no-convert-config", false) } else { confMapConverters = append( confMapConverters, @@ -190,7 +190,7 @@ func (f *flags) IsDryRun() bool { } func newFlags(args []string) (*flags, error) { - flagSet := flag.FlagSet{} + flagSet := new(flag.FlagSet) // we don't want to be responsible for tracking all supported collector service // flags, so allow any we don't use and defer parsing to the actual service flagSet.ParseErrorsWhitelist.UnknownFlags = true @@ -228,10 +228,16 @@ func newFlags(args []string) (*flags, error) { return settings, nil } -func removeFlag(flags *[]string, flag string) { +func removeFlag(flags *[]string, flag string, takesArg bool) { var out []string - for _, s := range *flags { - if s != flag { + + for i := 0; i < len(*flags); i++ { + s := (*flags)[i] + if s == flag { + if takesArg { + i++ + } + } else if !strings.HasPrefix(s, fmt.Sprintf("%s=", flag)) { out = append(out, s) } } diff --git a/internal/settings/settings_test.go b/internal/settings/settings_test.go index 95ba01a1e5..af6ca7cbd4 100644 --- a/internal/settings/settings_test.go +++ b/internal/settings/settings_test.go @@ -93,7 +93,7 @@ func TestNewSettingsNoConvertConfig(t *testing.T) { "--config", anotherConfigPath, "--set", "foo", "--set", "bar", - "--set", "baz", + "--set=baz", "--feature-gates", "foo", "--feature-gates", "-bar", }) @@ -106,13 +106,12 @@ func TestNewSettingsNoConvertConfig(t *testing.T) { require.Equal(t, []string{"foo", "bar", "baz"}, f.setProperties.value) require.Equal(t, []string{configPath, anotherConfigPath}, settings.ResolverURIs()) - require.Empty(t, settings.ConfMapConverters()) - // nolint: staticcheck - // overwritepropertiesconverter.New(f.setProperties.value), // support until there's an actual replacement + require.Equal(t, []confmap.Converter{ + configconverter.NewOverwritePropertiesConverter(f.setProperties.value), + }, settings.ConfMapConverters()) require.Equal(t, []string{ "--config", configPath, "--config", anotherConfigPath, - "--set", "foo", "--set", "bar", "--set", "baz", "--feature-gates", "foo", "--feature-gates", "-bar", }, settings.ServiceArgs()) } @@ -123,7 +122,7 @@ func TestNewSettingsConvertConfig(t *testing.T) { "--config", configPath, "--config", anotherConfigPath, "--set", "foo", - "--set", "bar", + "--set=bar", "--set", "baz", "--feature-gates", "foo", "--feature-gates", "-bar", @@ -140,8 +139,7 @@ func TestNewSettingsConvertConfig(t *testing.T) { require.Equal(t, []string{configPath, anotherConfigPath}, settings.ResolverURIs()) require.Equal(t, []confmap.Converter{ - // nolint: staticcheck - // overwritepropertiesconverter.New(f.setProperties.value), // support until there's an actual replacement + configconverter.NewOverwritePropertiesConverter(f.setProperties.value), configconverter.RemoveBallastKey{}, configconverter.MoveOTLPInsecureKey{}, configconverter.MoveHecTLS{}, @@ -150,7 +148,6 @@ func TestNewSettingsConvertConfig(t *testing.T) { require.Equal(t, []string{ "--config", configPath, "--config", anotherConfigPath, - "--set", "foo", "--set", "bar", "--set", "baz", "--feature-gates", "foo", "--feature-gates", "-bar", }, settings.ServiceArgs()) } @@ -417,12 +414,13 @@ service: } func TestRemoveFlag(t *testing.T) { - args := []string{"--aaa", "--bbb", "--ccc"} - removeFlag(&args, "--bbb") + args := []string{"--aaa", "--bbb", "bbbArg", "--ccc"} + removeFlag(&args, "--bbb", true) require.Equal(t, []string{"--aaa", "--ccc"}, args) - removeFlag(&args, "--ccc") + removeFlag(&args, "--ccc", false) require.Equal(t, []string{"--aaa"}, args) - removeFlag(&args, "--aaa") + // confirm overstepping acceptable + removeFlag(&args, "--aaa", true) require.Empty(t, args) } diff --git a/tests/general/set_properties_test.go b/tests/general/set_properties_test.go new file mode 100644 index 0000000000..a2629666a5 --- /dev/null +++ b/tests/general/set_properties_test.go @@ -0,0 +1,152 @@ +// Copyright Splunk, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build integration + +package tests + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/signalfx/splunk-otel-collector/tests/testutils" +) + +func TestSetProperties(t *testing.T) { + tc := testutils.NewTestcase(t) + defer tc.PrintLogsOnFailure() + defer tc.ShutdownOTLPReceiverSink() + + configServerPort := testutils.GetAvailablePort(t) + cc, shutdown := tc.SplunkOtelCollectorContainer( + "set_properties_config.yaml", + func(collector testutils.Collector) testutils.Collector { + return collector. + WithArgs( + "--config", "/etc/config.yaml", + "--set", "receivers.otlp.protocols.grpc.max_recv_msg_size_mib=1", + "--set=receivers.otlp.protocols.http.endpoint=localhost:0", + "--set", "processors.filter/one.metrics.include.match_type=regexp", + "--set=processors.filter/one.metrics.include.metric_names=[a.name]", + "--set", "processors.filter/two.metrics.include.match_type=strict", + "--set=processors.filter/two.metrics.include.metric_names=[another.name]", + ). + WithEnv( + map[string]string{ + "SPLUNK_DEBUG_CONFIG_SERVER_PORT": fmt.Sprintf("%d", configServerPort), + }, + ) + }, + ) + defer shutdown() + + expectedInitial := map[string]any{ + "file": map[string]any{ + "receivers": map[string]any{ + "otlp": map[string]any{"protocols": "overwritten"}, + }, + "processors": map[string]any{ + "filter/one": map[string]any{ + "metrics": map[string]any{ + "include": map[string]any{ + "match_type": "overwritten", + "metric_names": "overwritten", + }, + }, + }, + "filter/two": map[string]any{ + "metrics": map[string]any{ + "include": map[string]any{ + "match_type": "overwritten", + "metric_names": "overwritten", + }, + }, + }, + }, + "exporters": map[string]any{ + "otlp": map[string]any{ + "endpoint": "${OTLP_ENDPOINT}", + "tls": map[string]any{ + "insecure": true, + }, + }, + }, + "service": map[string]any{ + "pipelines": map[string]any{ + "metrics": map[string]any{ + "receivers": []any{"otlp"}, + "processors": []any{"filter/one", "filter/two"}, + "exporters": []any{"otlp"}, + }, + }, + }, + }, + } + + require.Equal(t, expectedInitial, cc.InitialConfig(t, configServerPort)) + + expectedEffective := map[string]any{ + "receivers": map[string]any{ + "otlp": map[string]any{ + "protocols": map[string]any{ + "grpc": map[string]any{ + "max_recv_msg_size_mib": "1", + }, + "http": map[string]any{ + "endpoint": "localhost:0", + }, + }, + }, + }, + "processors": map[string]any{ + "filter/one": map[string]any{ + "metrics": map[string]any{ + "include": map[string]any{ + "match_type": "regexp", + "metric_names": "[a.name]", + }, + }, + }, + "filter/two": map[string]any{ + "metrics": map[string]any{ + "include": map[string]any{ + "match_type": "strict", + "metric_names": "[another.name]", + }, + }, + }, + }, + "exporters": map[string]any{ + "otlp": map[string]any{ + "endpoint": tc.OTLPEndpoint, + "tls": map[string]any{ + "insecure": true, + }, + }, + }, + "service": map[string]any{ + "pipelines": map[string]any{ + "metrics": map[string]any{ + "receivers": []any{"otlp"}, + "processors": []any{"filter/one", "filter/two"}, + "exporters": []any{"otlp"}, + }, + }, + }, + } + + require.Equal(t, expectedEffective, cc.EffectiveConfig(t, configServerPort)) +} diff --git a/tests/general/testdata/set_properties_config.yaml b/tests/general/testdata/set_properties_config.yaml new file mode 100644 index 0000000000..f7cbd7cd3d --- /dev/null +++ b/tests/general/testdata/set_properties_config.yaml @@ -0,0 +1,28 @@ +receivers: + otlp: + protocols: overwritten + +processors: + filter/one: + metrics: + include: + match_type: overwritten + metric_names: overwritten + filter/two: + metrics: + include: + match_type: overwritten + metric_names: overwritten + +exporters: + otlp: + endpoint: "${OTLP_ENDPOINT}" + tls: + insecure: true + +service: + pipelines: + metrics: + receivers: [otlp] + processors: [filter/one, filter/two] + exporters: [otlp] \ No newline at end of file diff --git a/tests/testutils/collector_container.go b/tests/testutils/collector_container.go index a1aa9032b0..8eb0840cb5 100644 --- a/tests/testutils/collector_container.go +++ b/tests/testutils/collector_container.go @@ -263,9 +263,18 @@ func (collector *CollectorContainer) EffectiveConfig(t testing.TB, port uint16) } func (collector *CollectorContainer) execConfigRequest(t testing.TB, uri string) map[string]any { - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - n, r, err := collector.Container.Exec(ctx, []string{"curl", "-s", uri}) + var n int + var r io.Reader + var err error + for i := 0; i < 3; i++ { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + n, r, err = collector.Container.Exec(ctx, []string{"curl", "-s", uri}) + cancel() + if err == nil && n == 0 { + break + } + time.Sleep(time.Second) + } require.NoError(t, err) require.Zero(t, n) out, err := io.ReadAll(r)