Skip to content

Commit

Permalink
chore: remove config Validate call in docker (#34699)
Browse files Browse the repository at this point in the history
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Configuration validation is done during collector's startup, making it
redundant when being called inside component's logic. This PR removes
the Validate call done during Dockers's receiver start function.

**Link to tracking Issue:** <Issue number if applicable>

#33498

**Testing:** <Describe what testing was performed and which tests were
added.>
No testing needed to be modified as the `Validate` functionality is
already covered.

**Documentation:** <Describe the documentation added.>

---------

Co-authored-by: Sean Marciniak <[email protected]>
  • Loading branch information
rogercoll and MovieStoreGuy authored Aug 28, 2024
1 parent ddd8bbc commit ee7be8b
Show file tree
Hide file tree
Showing 12 changed files with 134 additions and 116 deletions.
32 changes: 7 additions & 25 deletions extension/observer/dockerobserver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
package dockerobserver // import "github.com/open-telemetry/opentelemetry-collector-contrib/extension/observer/dockerobserver"

import (
"errors"
"fmt"
"time"

Expand All @@ -15,16 +14,7 @@ import (

// Config defines configuration for docker observer
type Config struct {

// The URL of the docker server. Default is "unix:///var/run/docker.sock" on non-Windows
// and "npipe:////./pipe/docker_engine" on Windows
Endpoint string `mapstructure:"endpoint"`

// The maximum amount of time to wait for docker API responses. Default is 5s
Timeout time.Duration `mapstructure:"timeout"`

// A list of filters whose matching images are to be excluded. Supports literals, globs, and regex.
ExcludedImages []string `mapstructure:"excluded_images"`
docker.Config `mapstructure:",squash"`

// If true, the "Config.Hostname" field (if present) of the docker
// container will be used as the discovered host that is used to configure
Expand All @@ -47,15 +37,9 @@ type Config struct {
// through the docker event listener example: cache_sync_interval: "20m"
// Default: "60m"
CacheSyncInterval time.Duration `mapstructure:"cache_sync_interval"`

// Docker client API version. Default is 1.22
DockerAPIVersion string `mapstructure:"api_version"`
}

func (config Config) Validate() error {
if config.Endpoint == "" {
return errors.New("endpoint must be specified")
}
if err := docker.VersionIsValidAndGTE(config.DockerAPIVersion, minimumRequiredDockerAPIVersion); err != nil {
return err
}
Expand All @@ -71,14 +55,12 @@ func (config Config) Validate() error {
func (config *Config) Unmarshal(conf *confmap.Conf) error {
err := conf.Unmarshal(config)
if err != nil {
if floatAPIVersion, ok := conf.Get("api_version").(float64); ok {
return fmt.Errorf(
"%w.\n\nHint: You may want to wrap the 'api_version' value in quotes (api_version: \"%1.2f\")",
err,
floatAPIVersion,
)
}
return err
}
return nil

if len(config.ExcludedImages) == 0 {
config.ExcludedImages = nil
}

return err
}
21 changes: 12 additions & 9 deletions extension/observer/dockerobserver/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"go.opentelemetry.io/collector/confmap/confmaptest"

"github.com/open-telemetry/opentelemetry-collector-contrib/extension/observer/dockerobserver/internal/metadata"
"github.com/open-telemetry/opentelemetry-collector-contrib/internal/docker"
)

var version = "1.40"
Expand All @@ -34,14 +35,16 @@ func TestLoadConfig(t *testing.T) {
{
id: component.NewIDWithName(metadata.Type, "all_settings"),
expected: &Config{
Endpoint: "unix:///var/run/docker.sock",
Config: docker.Config{
Endpoint: "unix:///var/run/docker.sock",
Timeout: 20 * time.Second,
ExcludedImages: []string{"excluded", "image"},
DockerAPIVersion: version,
},
CacheSyncInterval: 5 * time.Minute,
Timeout: 20 * time.Second,
ExcludedImages: []string{"excluded", "image"},
UseHostnameIfPresent: true,
UseHostBindings: true,
IgnoreNonHostBindings: true,
DockerAPIVersion: version,
},
},
}
Expand All @@ -59,19 +62,19 @@ func TestLoadConfig(t *testing.T) {
}

func TestValidateConfig(t *testing.T) {
cfg := &Config{}
cfg := &Config{Config: docker.Config{DockerAPIVersion: "1.24", Timeout: 5 * time.Second}, CacheSyncInterval: 5 * time.Second}
assert.Equal(t, "endpoint must be specified", component.ValidateConfig(cfg).Error())

cfg = &Config{Endpoint: "someEndpoint", DockerAPIVersion: "1.23"}
cfg = &Config{Config: docker.Config{Endpoint: "someEndpoint", DockerAPIVersion: "1.23"}}
assert.Equal(t, `"api_version" 1.23 must be at least 1.24`, component.ValidateConfig(cfg).Error())

cfg = &Config{Endpoint: "someEndpoint", DockerAPIVersion: version}
cfg = &Config{Config: docker.Config{Endpoint: "someEndpoint", DockerAPIVersion: version}}
assert.Equal(t, "timeout must be specified", component.ValidateConfig(cfg).Error())

cfg = &Config{Endpoint: "someEndpoint", DockerAPIVersion: version, Timeout: 5 * time.Minute}
cfg = &Config{Config: docker.Config{Endpoint: "someEndpoint", DockerAPIVersion: version, Timeout: 5 * time.Minute}}
assert.Equal(t, "cache_sync_interval must be specified", component.ValidateConfig(cfg).Error())

cfg = &Config{Endpoint: "someEndpoint", DockerAPIVersion: version, Timeout: 5 * time.Minute, CacheSyncInterval: 5 * time.Minute}
cfg = &Config{Config: docker.Config{Endpoint: "someEndpoint", DockerAPIVersion: version, Timeout: 5 * time.Minute}, CacheSyncInterval: 5 * time.Minute}
assert.Nil(t, component.ValidateConfig(cfg))
}

Expand Down
15 changes: 7 additions & 8 deletions extension/observer/dockerobserver/extension.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@ var (
minimumRequiredDockerAPIVersion = docker.MustNewAPIVersion(defaultDockerAPIVersion)
)

var _ extension.Extension = (*dockerObserver)(nil)
var _ observer.EndpointsLister = (*dockerObserver)(nil)
var _ observer.Observable = (*dockerObserver)(nil)
var (
_ extension.Extension = (*dockerObserver)(nil)
_ observer.EndpointsLister = (*dockerObserver)(nil)
_ observer.Observable = (*dockerObserver)(nil)
)

type dockerObserver struct {
*observer.EndpointsWatcher
Expand Down Expand Up @@ -60,11 +62,9 @@ func (d *dockerObserver) Start(ctx context.Context, _ component.Host) error {
d.ctx = dCtx

// Create new Docker client
dConfig, err := docker.NewConfig(d.config.Endpoint, d.config.Timeout, d.config.ExcludedImages, d.config.DockerAPIVersion)
if err != nil {
return err
}
dConfig := docker.NewConfig(d.config.Endpoint, d.config.Timeout, d.config.ExcludedImages, d.config.DockerAPIVersion)

var err error
d.dClient, err = docker.NewDockerClient(dConfig, d.logger)
if err != nil {
return fmt.Errorf("could not create docker client: %w", err)
Expand Down Expand Up @@ -119,7 +119,6 @@ func (d *dockerObserver) ListEndpoints() []observer.Endpoint {
// containerEndpoints generates a list of observer.Endpoint given a Docker ContainerJSON.
// This function will only generate endpoints if a container is in the Running state and not Paused.
func (d *dockerObserver) containerEndpoints(c *dtypes.ContainerJSON) []observer.Endpoint {

if !c.State.Running || c.State.Running && c.State.Paused {
return nil
}
Expand Down
9 changes: 6 additions & 3 deletions extension/observer/dockerobserver/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"go.opentelemetry.io/collector/extension"

"github.com/open-telemetry/opentelemetry-collector-contrib/extension/observer/dockerobserver/internal/metadata"
"github.com/open-telemetry/opentelemetry-collector-contrib/internal/docker"
)

// NewFactory should be called to create a factory with default values.
Expand All @@ -26,10 +27,12 @@ func NewFactory() extension.Factory {

func createDefaultConfig() component.Config {
return &Config{
Endpoint: client.DefaultDockerHost,
Timeout: 5 * time.Second,
Config: docker.Config{
Endpoint: client.DefaultDockerHost,
Timeout: 5 * time.Second,
DockerAPIVersion: defaultDockerAPIVersion,
},
CacheSyncInterval: 60 * time.Minute,
DockerAPIVersion: defaultDockerAPIVersion,
}
}

Expand Down
40 changes: 26 additions & 14 deletions internal/docker/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/docker/docker/api/types/versions"
"github.com/docker/docker/client"
"go.opentelemetry.io/collector/confmap"
)

type Config struct {
Expand All @@ -29,16 +30,39 @@ type Config struct {
DockerAPIVersion string `mapstructure:"api_version"`
}

func (config *Config) Unmarshal(conf *confmap.Conf) error {
// WithIgonreUnused needed because this configuration is embedded inside other configurations
err := conf.Unmarshal(config, confmap.WithIgnoreUnused())
if err != nil {
if floatAPIVersion, ok := conf.Get("api_version").(float64); ok {
return fmt.Errorf(
"%w.\n\nHint: You may want to wrap the 'api_version' value in quotes (api_version: \"%1.2f\")",
err,
floatAPIVersion,
)
}
return err
}
return nil
}

func (config Config) Validate() error {
if config.Endpoint == "" {
return errors.New("endpoint must be specified")
}
return nil
}

// NewConfig creates a new config to be used when creating
// a docker client
func NewConfig(endpoint string, timeout time.Duration, excludedImages []string, apiVersion string) (*Config, error) {
func NewConfig(endpoint string, timeout time.Duration, excludedImages []string, apiVersion string) *Config {
cfg := &Config{
Endpoint: endpoint,
Timeout: timeout,
ExcludedImages: excludedImages,
DockerAPIVersion: apiVersion,
}
return cfg, cfg.validate()
return cfg
}

// NewDefaultConfig creates a new config with default values
Expand All @@ -53,18 +77,6 @@ func NewDefaultConfig() *Config {
return cfg
}

// validate asserts that an endpoint field is set
// on the config struct
func (config Config) validate() error {
if config.Endpoint == "" {
return errors.New("config.Endpoint must be specified")
}
if err := VersionIsValidAndGTE(config.DockerAPIVersion, minimumRequiredDockerAPIVersion); err != nil {
return err
}
return nil
}

type apiVersion struct {
major int
minor int
Expand Down
9 changes: 7 additions & 2 deletions internal/docker/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ require (
github.com/docker/docker v26.1.5+incompatible
github.com/gobwas/glob v0.2.3
github.com/stretchr/testify v1.9.0
go.opentelemetry.io/collector/confmap v1.14.1
go.uber.org/goleak v1.3.0
go.uber.org/zap v1.27.0
)
Expand All @@ -20,8 +21,13 @@ require (
github.com/felixge/httpsnoop v1.0.4 // indirect
github.com/go-logr/logr v1.4.1 // indirect
github.com/go-logr/stdr v1.2.2 // indirect
github.com/go-viper/mapstructure/v2 v2.1.0 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/kr/pretty v0.2.1 // indirect
github.com/knadh/koanf/maps v0.1.1 // indirect
github.com/knadh/koanf/providers/confmap v0.1.0 // indirect
github.com/knadh/koanf/v2 v2.1.1 // indirect
github.com/mitchellh/copystructure v1.2.0 // indirect
github.com/mitchellh/reflectwalk v1.0.2 // indirect
github.com/moby/docker-image-spec v1.3.1 // indirect
github.com/moby/term v0.0.0-20210619224110-3f7ff695adc6 // indirect
github.com/morikuni/aec v1.0.0 // indirect
Expand All @@ -39,7 +45,6 @@ require (
golang.org/x/net v0.23.0 // indirect
golang.org/x/sys v0.18.0 // indirect
golang.org/x/time v0.4.0 // indirect
gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
gotest.tools/v3 v3.0.3 // indirect
)
Expand Down
26 changes: 20 additions & 6 deletions internal/docker/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit ee7be8b

Please sign in to comment.