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

docker: adopt api_version as string to correct float limitations #30480

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

Filter by extension

Filter by extension

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

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

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

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Adopt api_version as strings to correct invalid float truncation

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [24025]

# (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:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# 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: [user, api]
6 changes: 6 additions & 0 deletions extension/observer/dockerobserver/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,12 @@ The maximum amount of time to wait for docker API responses.

default: `5s`

### `api_version`

The client API version. If using one with a terminating zero, input as a string to prevent undesired truncation (e.g. `"1.40"` instead of `1.40`, which is parsed as `1.4`).

default: `1.22`

### `excluded_images`

A list of filters whose matching images are to be excluded. Supports literals, globs, and regex.
Expand Down
8 changes: 5 additions & 3 deletions extension/observer/dockerobserver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"errors"
"fmt"
"time"

"github.com/open-telemetry/opentelemetry-collector-contrib/internal/docker"
)

// Config defines configuration for docker observer
Expand Down Expand Up @@ -44,15 +46,15 @@ type Config struct {
CacheSyncInterval time.Duration `mapstructure:"cache_sync_interval"`

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

func (config Config) Validate() error {
if config.Endpoint == "" {
return errors.New("endpoint must be specified")
}
if config.DockerAPIVersion < minimalRequiredDockerAPIVersion {
return fmt.Errorf("api_version must be at least %v", minimalRequiredDockerAPIVersion)
if err := docker.VersionIsValidAndGTE(config.DockerAPIVersion, minimumRequiredDockerAPIVersion); err != nil {
return err
}
if config.Timeout == 0 {
return fmt.Errorf("timeout must be specified")
Expand Down
35 changes: 26 additions & 9 deletions extension/observer/dockerobserver/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@ import (
"github.com/open-telemetry/opentelemetry-collector-contrib/extension/observer/dockerobserver/internal/metadata"
)

var version = "1.40"

func TestLoadConfig(t *testing.T) {
t.Parallel()

tests := []struct {
id component.ID
expected component.Config
id component.ID
expected component.Config
expectedError string
}{
{
id: component.NewID(metadata.Type),
Expand All @@ -37,14 +40,28 @@ func TestLoadConfig(t *testing.T) {
UseHostnameIfPresent: true,
UseHostBindings: true,
IgnoreNonHostBindings: true,
DockerAPIVersion: 1.22,
DockerAPIVersion: version,
},
},
{
id: component.NewIDWithName(metadata.Type, "unsupported_api_version"),
expected: &Config{
Endpoint: "unix:///var/run/docker.sock",
CacheSyncInterval: time.Hour,
Timeout: 5 * time.Second,
DockerAPIVersion: "1.4",
},
expectedError: `"api_version" 1.4 must be at least 1.22`,
},
}
for _, tt := range tests {
t.Run(tt.id.String(), func(t *testing.T) {
cfg := loadConfig(t, tt.id)
assert.NoError(t, component.ValidateConfig(cfg))
if tt.expectedError != "" {
assert.EqualError(t, component.ValidateConfig(cfg), tt.expectedError)
} else {
assert.NoError(t, component.ValidateConfig(cfg))
}
assert.Equal(t, tt.expected, cfg)
})
}
Expand All @@ -54,16 +71,16 @@ func TestValidateConfig(t *testing.T) {
cfg := &Config{}
assert.Equal(t, "endpoint must be specified", component.ValidateConfig(cfg).Error())

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

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

cfg = &Config{Endpoint: "someEndpoint", DockerAPIVersion: 1.22, Timeout: 5 * time.Minute}
cfg = &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: 1.22, Timeout: 5 * time.Minute, CacheSyncInterval: 5 * time.Minute}
cfg = &Config{Endpoint: "someEndpoint", DockerAPIVersion: version, Timeout: 5 * time.Minute, CacheSyncInterval: 5 * time.Minute}
assert.Nil(t, component.ValidateConfig(cfg))
}

Expand Down
6 changes: 3 additions & 3 deletions extension/observer/dockerobserver/extension.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import (
"github.com/open-telemetry/opentelemetry-collector-contrib/internal/docker"
)

const (
defaultDockerAPIVersion = 1.22
minimalRequiredDockerAPIVersion = 1.22
var (
defaultDockerAPIVersion = "1.22"
minimumRequiredDockerAPIVersion = docker.MustNewAPIVersion(defaultDockerAPIVersion)
)

var _ extension.Extension = (*dockerObserver)(nil)
Expand Down
4 changes: 4 additions & 0 deletions extension/observer/dockerobserver/testdata/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ docker_observer/all_settings:
use_host_bindings: true
ignore_non_host_bindings: true
cache_sync_interval: 5m
api_version: '1.40'
docker_observer/use_hostname_if_present:
use_hostname_if_present: true
docker_observer/use_host_bindings:
Expand All @@ -15,3 +16,6 @@ docker_observer/ignore_non_host_bindings:
ignore_non_host_bindings: true
docker_observer/exclude_nginx:
excluded_images: ["nginx"]
docker_observer/unsupported_api_version:
# intentionally a float since it will be parsed as 1.4
api_version: 1.40
71 changes: 63 additions & 8 deletions internal/docker/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ package docker // import "github.com/open-telemetry/opentelemetry-collector-cont
import (
"errors"
"fmt"
"strconv"
"strings"
"time"

"github.com/docker/docker/api/types/versions"
)

type Config struct {
Expand All @@ -20,21 +24,19 @@ type Config struct {
ExcludedImages []string `mapstructure:"excluded_images"`

// Docker client API version.
DockerAPIVersion float64 `mapstructure:"api_version"`
DockerAPIVersion string `mapstructure:"api_version"`
}

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

err := cfg.validate()
return cfg, err
return cfg, cfg.validate()
}

// NewDefaultConfig creates a new config with default values
Expand All @@ -43,7 +45,7 @@ func NewDefaultConfig() *Config {
cfg := &Config{
Endpoint: "unix:///var/run/docker.sock",
Timeout: 5 * time.Second,
DockerAPIVersion: minimalRequiredDockerAPIVersion,
DockerAPIVersion: minimumRequiredDockerAPIVersion,
}

return cfg
Expand All @@ -55,8 +57,61 @@ func (config Config) validate() error {
if config.Endpoint == "" {
return errors.New("config.Endpoint must be specified")
}
if config.DockerAPIVersion < minimalRequiredDockerAPIVersion {
return fmt.Errorf("Docker API version must be at least %v", minimalRequiredDockerAPIVersion)
if err := VersionIsValidAndGTE(config.DockerAPIVersion, minimumRequiredDockerAPIVersion); err != nil {
return err
}
return nil
}

type apiVersion struct {
major int
minor int
}

func NewAPIVersion(version string) (string, error) {
s := strings.TrimSpace(version)
split := strings.Split(s, ".")

invalidVersion := "invalid version %q"

nParts := len(split)
if s == "" || nParts < 1 || nParts > 2 {
return "", fmt.Errorf(invalidVersion, s)
}

apiVer := new(apiVersion)
var err error
target := map[int]*int{0: &apiVer.major, 1: &apiVer.minor}
for i, part := range split {
part = strings.TrimSpace(part)
if part != "" {
if *target[i], err = strconv.Atoi(part); err != nil {
return "", fmt.Errorf(invalidVersion+": %w", s, err)
}
}
}

return fmt.Sprintf("%d.%d", apiVer.major, apiVer.minor), nil
}

// MustNewAPIVersion evaluates version as a client api version and panics if invalid.
func MustNewAPIVersion(version string) string {
v, err := NewAPIVersion(version)
if err != nil {
panic(err)
}
return v
}

// VersionIsValidAndGTE evalutes version as a client api version and returns an error if invalid or less than gte.
// gte is assumed to be valid (easiest if result of MustNewAPIVersion on initialization)
func VersionIsValidAndGTE(version, gte string) error {
v, err := NewAPIVersion(version)
if err != nil {
return err
}
if versions.LessThan(v, gte) {
return fmt.Errorf(`"api_version" %s must be at least %s`, version, gte)
}
return nil
}
80 changes: 80 additions & 0 deletions internal/docker/config_test.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 docker // import "github.com/open-telemetry/opentelemetry-collector-contrib/internal/docker"

import (
"testing"

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

func TestAPIVersion(t *testing.T) {
for _, test := range []struct {
input string
expectedVersion string
expectedRep string
expectedError string
}{
{
input: "1.2",
expectedVersion: MustNewAPIVersion("1.2"),
expectedRep: "1.2",
},
{
input: "1.40",
expectedVersion: MustNewAPIVersion("1.40"),
expectedRep: "1.40",
},
{
input: "10",
expectedVersion: MustNewAPIVersion("10.0"),
expectedRep: "10.0",
},
{
input: "0",
expectedVersion: MustNewAPIVersion("0.0"),
expectedRep: "0.0",
},
{
input: ".400",
expectedVersion: MustNewAPIVersion("0.400"),
expectedRep: "0.400",
},
{
input: "00000.400",
expectedVersion: MustNewAPIVersion("0.400"),
expectedRep: "0.400",
},
{
input: "0.1.",
expectedError: `invalid version "0.1."`,
},
{
input: "0.1.2.3",
expectedError: `invalid version "0.1.2.3"`,
},
{
input: "",
expectedError: `invalid version ""`,
},
{
input: "...",
expectedError: `invalid version "..."`,
},
} {
t.Run(test.input, func(t *testing.T) {
version, err := NewAPIVersion(test.input)
if test.expectedError != "" {
assert.EqualError(t, err, test.expectedError)
assert.Empty(t, version)
return
}
require.NoError(t, err)
require.Equal(t, test.expectedVersion, version)
require.Equal(t, test.expectedRep, version)
require.Equal(t, test.expectedRep, version)
})
}
}
16 changes: 11 additions & 5 deletions internal/docker/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,9 @@ import (
"go.uber.org/zap"
)

const (
minimalRequiredDockerAPIVersion = 1.22
userAgent = "OpenTelemetry-Collector Docker Stats Receiver/v0.0.1"
)
const userAgent = "OpenTelemetry-Collector Docker Stats Receiver/v0.0.1"

var minimumRequiredDockerAPIVersion = MustNewAPIVersion("1.22")

// Container is client.ContainerInspect() response container
// stats and translated environment string map for potential labels.
Expand All @@ -46,10 +45,17 @@ type Client struct {
}

func NewDockerClient(config *Config, logger *zap.Logger, opts ...docker.Opt) (*Client, error) {
version := minimumRequiredDockerAPIVersion
if config.DockerAPIVersion != "" {
var err error
if version, err = NewAPIVersion(config.DockerAPIVersion); err != nil {
return nil, err
}
}
client, err := docker.NewClientWithOpts(
append([]docker.Opt{
docker.WithHost(config.Endpoint),
docker.WithVersion(fmt.Sprintf("v%v", config.DockerAPIVersion)),
docker.WithVersion(version),
docker.WithHTTPHeaders(map[string]string{"User-Agent": userAgent}),
}, opts...)...,
)
Expand Down
Loading