From 84df8684eb2a95a6ffa08629e218fe7d674e226d Mon Sep 17 00:00:00 2001 From: Ryan Fitzpatrick Date: Thu, 15 Dec 2022 17:34:11 +0000 Subject: [PATCH] Incorporate obsoleted overwritepropertiesconverter https://github.com/open-telemetry/opentelemetry-collector/pull/6656 removed the overwrite properties config converter that this distribution uses. --- go.mod | 2 +- .../configconverter/overwrite_properties.go | 65 ++++++++ .../overwrite_properties_test.go | 58 +++++++ internal/settings/settings.go | 4 +- internal/settings/settings_test.go | 10 +- tests/general/set_properties_test.go | 152 ++++++++++++++++++ .../testdata/set_properties_config.yaml | 28 ++++ tests/testutils/collector_container.go | 15 +- 8 files changed, 321 insertions(+), 13 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/go.mod b/go.mod index f579c3a8e9..c7a3eed19d 100644 --- a/go.mod +++ b/go.mod @@ -307,7 +307,7 @@ require ( github.com/linkedin/goavro/v2 v2.9.8 // indirect github.com/linode/linodego v1.9.3 // indirect github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0 // indirect - github.com/magiconair/properties v1.8.6 // indirect + github.com/magiconair/properties v1.8.6 github.com/mailru/easyjson v0.7.7 // indirect github.com/mattn/go-colorable v0.1.12 // indirect github.com/mattn/go-ieproxy v0.0.9 // indirect diff --git a/internal/configconverter/overwrite_properties.go b/internal/configconverter/overwrite_properties.go new file mode 100644 index 0000000000..99e5e958ab --- /dev/null +++ b/internal/configconverter/overwrite_properties.go @@ -0,0 +1,65 @@ +// 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..0593176684 --- /dev/null +++ b/internal/configconverter/overwrite_properties_test.go @@ -0,0 +1,58 @@ +// 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 prevent 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 4be72d7319..6f4461a9fb 100644 --- a/internal/settings/settings.go +++ b/internal/settings/settings.go @@ -138,10 +138,8 @@ func getConfigDir(f *Settings) string { // ConfMapConverters returns confmap.Converters for the collector core service. func (s *Settings) ConfMapConverters() []confmap.Converter { confMapConverters := []confmap.Converter{ - // nolint: staticcheck - // overwritepropertiesconverter.New(s.setProperties.value), // support until there's an actual replacement + configconverter.NewOverwritePropertiesConverter(s.setProperties.value), } - if !s.noConvertConfig { confMapConverters = append( confMapConverters, diff --git a/internal/settings/settings_test.go b/internal/settings/settings_test.go index db6b6a231a..a5d139cf1e 100644 --- a/internal/settings/settings_test.go +++ b/internal/settings/settings_test.go @@ -89,7 +89,7 @@ func TestNewSettingsNoConvertConfig(t *testing.T) { "--config", anotherConfigPath, "--set", "foo", "--set", "bar", - "--set", "baz", + "--set=baz", "--feature-gates", "foo", "--feature-gates", "-bar", }) @@ -102,8 +102,7 @@ func TestNewSettingsNoConvertConfig(t *testing.T) { require.Equal(t, []string{configPath, anotherConfigPath}, settings.ResolverURIs()) require.Equal(t, []confmap.Converter{ - // nolint: staticcheck - // overwritepropertiesconverter.New(settings.setProperties.value), // support until there's an actual replacement + configconverter.NewOverwritePropertiesConverter(settings.setProperties.value), }, settings.ConfMapConverters()) require.Equal(t, []string{"--feature-gates", "foo", "--feature-gates", "-bar"}, settings.ColCoreArgs()) } @@ -114,7 +113,7 @@ func TestNewSettingsConvertConfig(t *testing.T) { "--config", configPath, "--config", anotherConfigPath, "--set", "foo", - "--set", "bar", + "--set=bar", "--set", "baz", "--feature-gates", "foo", "--feature-gates", "-bar", @@ -129,8 +128,7 @@ func TestNewSettingsConvertConfig(t *testing.T) { require.Equal(t, []string{configPath, anotherConfigPath}, settings.ResolverURIs()) require.Equal(t, []confmap.Converter{ - // nolint: staticcheck - // overwritepropertiesconverter.New(settings.setProperties.value), // support until there's an actual replacement + configconverter.NewOverwritePropertiesConverter(settings.setProperties.value), configconverter.RemoveBallastKey{}, configconverter.MoveOTLPInsecureKey{}, configconverter.MoveHecTLS{}, 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)