Skip to content

Commit

Permalink
Incorporate obsoleted overwritepropertiesconverter and strip --set ar…
Browse files Browse the repository at this point in the history
…gs for otelcol service

open-telemetry/opentelemetry-collector#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.
  • Loading branch information
rmfitzpatrick committed Dec 14, 2022
1 parent fde0a08 commit c313985
Show file tree
Hide file tree
Showing 7 changed files with 347 additions and 29 deletions.
66 changes: 66 additions & 0 deletions internal/configconverter/overwrite_properties.go
Original file line number Diff line number Diff line change
@@ -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))
}
59 changes: 59 additions & 0 deletions internal/configconverter/overwrite_properties_test.go
Original file line number Diff line number Diff line change
@@ -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))
}
32 changes: 19 additions & 13 deletions internal/settings/settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand All @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
}
Expand Down
24 changes: 11 additions & 13 deletions internal/settings/settings_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
})
Expand All @@ -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())
}
Expand All @@ -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",
Expand All @@ -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{},
Expand All @@ -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())
}
Expand Down Expand Up @@ -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)
}

Expand Down
Loading

0 comments on commit c313985

Please sign in to comment.