Fix container name filters in docker input (#3331)

(cherry picked from commit fa25e123d8)

filter/filter.go

@@ -77,3 +77,40 @@ func compileFilterNoGlob(filters []string) Filter {
 	}
 	return &out
 }
+
+type IncludeExcludeFilter struct {
+	include Filter
+	exclude Filter
+}
+
+func NewIncludeExcludeFilter(
+	include []string,
+	exclude []string,
+) (Filter, error) {
+	in, err := Compile(include)
+	if err != nil {
+		return nil, err
+	}
+
+	ex, err := Compile(exclude)
+	if err != nil {
+		return nil, err
+	}
+
+	return &IncludeExcludeFilter{in, ex}, nil
+}
+
+func (f *IncludeExcludeFilter) Match(s string) bool {
+	if f.include != nil {
+		if !f.include.Match(s) {
+			return false
+		}
+	}
+
+	if f.exclude != nil {
+		if f.exclude.Match(s) {
+			return false
+		}
+	}
+	return true
+}

plugins/inputs/docker/docker.go

@@ -20,16 +20,6 @@ import (
 	"github.com/influxdata/telegraf/plugins/inputs"
 )
 
-type DockerLabelFilter struct {
-	labelInclude filter.Filter
-	labelExclude filter.Filter
-}
-
-type DockerContainerFilter struct {
-	containerInclude filter.Filter
-	containerExclude filter.Filter
-}
-
 // Docker object
 type Docker struct {
 	Endpoint       string
@@ -41,11 +31,9 @@ type Docker struct {
 	TagEnvironment []string `toml:"tag_env"`
 	LabelInclude   []string `toml:"docker_label_include"`
 	LabelExclude   []string `toml:"docker_label_exclude"`
-	LabelFilter    DockerLabelFilter
 
 	ContainerInclude []string `toml:"container_name_include"`
 	ContainerExclude []string `toml:"container_name_exclude"`
-	ContainerFilter  DockerContainerFilter
 
 	SSLCA              string `toml:"ssl_ca"`
 	SSLCert            string `toml:"ssl_cert"`
@@ -55,10 +43,12 @@ type Docker struct {
 	newEnvClient func() (Client, error)
 	newClient    func(string, *tls.Config) (Client, error)
 
-	client         Client
-	httpClient     *http.Client
-	engine_host    string
-	filtersCreated bool
+	client          Client
+	httpClient      *http.Client
+	engine_host     string
+	filtersCreated  bool
+	labelFilter     filter.Filter
+	containerFilter filter.Filter
 }
 
 // KB, MB, GB, TB, PB...human friendly
@@ -291,12 +281,8 @@ func (d *Docker) gatherContainer(
 		"container_version": imageVersion,
 	}
 
-	if len(d.ContainerInclude) > 0 || len(d.ContainerExclude) > 0 {
-		if len(d.ContainerInclude) == 0 || !d.ContainerFilter.containerInclude.Match(cname) {
-			if len(d.ContainerExclude) == 0 || d.ContainerFilter.containerExclude.Match(cname) {
-				return nil
-			}
-		}
+	if !d.containerFilter.Match(cname) {
+		return nil
 	}
 
 	ctx, cancel := context.WithTimeout(context.Background(), d.Timeout.Duration)
@@ -317,10 +303,8 @@ func (d *Docker) gatherContainer(
 
 	// Add labels to tags
 	for k, label := range container.Labels {
-		if len(d.LabelInclude) == 0 || d.LabelFilter.labelInclude.Match(k) {
-			if len(d.LabelExclude) == 0 || !d.LabelFilter.labelExclude.Match(k) {
-				tags[k] = label
-			}
+		if d.labelFilter.Match(k) {
+			tags[k] = label
 		}
 	}
 
@@ -666,46 +650,25 @@ func parseSize(sizeStr string) (int64, error) {
 }
 
 func (d *Docker) createContainerFilters() error {
+	// Backwards compatibility for deprecated `container_names` parameter.
 	if len(d.ContainerNames) > 0 {
 		d.ContainerInclude = append(d.ContainerInclude, d.ContainerNames...)
 	}
 
-	if len(d.ContainerInclude) != 0 {
-		var err error
-		d.ContainerFilter.containerInclude, err = filter.Compile(d.ContainerInclude)
-		if err != nil {
-			return err
-		}
+	filter, err := filter.NewIncludeExcludeFilter(d.ContainerInclude, d.ContainerExclude)
+	if err != nil {
+		return err
 	}
-
-	if len(d.ContainerExclude) != 0 {
-		var err error
-		d.ContainerFilter.containerExclude, err = filter.Compile(d.ContainerExclude)
-		if err != nil {
-			return err
-		}
-	}
-
+	d.containerFilter = filter
 	return nil
 }
 
 func (d *Docker) createLabelFilters() error {
-	if len(d.LabelInclude) != 0 {
-		var err error
-		d.LabelFilter.labelInclude, err = filter.Compile(d.LabelInclude)
-		if err != nil {
-			return err
-		}
+	filter, err := filter.NewIncludeExcludeFilter(d.LabelInclude, d.LabelExclude)
+	if err != nil {
+		return err
 	}
-
-	if len(d.LabelExclude) != 0 {
-		var err error
-		d.LabelFilter.labelExclude, err = filter.Compile(d.LabelExclude)
-		if err != nil {
-			return err
-		}
-	}
-
+	d.labelFilter = filter
 	return nil
 }
 

plugins/inputs/docker/docker_test.go

@@ -44,21 +44,23 @@ func (c *MockClient) ContainerInspect(
 	return c.ContainerInspectF(ctx, containerID)
 }
 
+var baseClient = MockClient{
+	InfoF: func(context.Context) (types.Info, error) {
+		return info, nil
+	},
+	ContainerListF: func(context.Context, types.ContainerListOptions) ([]types.Container, error) {
+		return containerList, nil
+	},
+	ContainerStatsF: func(context.Context, string, bool) (types.ContainerStats, error) {
+		return containerStats(), nil
+	},
+	ContainerInspectF: func(context.Context, string) (types.ContainerJSON, error) {
+		return containerInspect, nil
+	},
+}
+
 func newClient(host string, tlsConfig *tls.Config) (Client, error) {
-	return &MockClient{
-		InfoF: func(context.Context) (types.Info, error) {
-			return info, nil
-		},
-		ContainerListF: func(context.Context, types.ContainerListOptions) ([]types.Container, error) {
-			return containerList, nil
-		},
-		ContainerStatsF: func(context.Context, string, bool) (types.ContainerStats, error) {
-			return containerStats(), nil
-		},
-		ContainerInspectF: func(context.Context, string) (types.ContainerJSON, error) {
-			return containerInspect, nil
-		},
-	}, nil
+	return &baseClient, nil
 }
 
 func TestDockerGatherContainerStats(t *testing.T) {
@@ -234,82 +236,291 @@ func TestDocker_WindowsMemoryContainerStats(t *testing.T) {
 	require.NoError(t, err)
 }
 
-func TestDockerGatherLabels(t *testing.T) {
-	var gatherLabelsTests = []struct {
-		include     []string
-		exclude     []string
-		expected    []string
-		notexpected []string
+func TestContainerLabels(t *testing.T) {
+	var tests = []struct {
+		name      string
+		container types.Container
+		include   []string
+		exclude   []string
+		expected  map[string]string
 	}{
-		{[]string{}, []string{}, []string{"label1", "label2"}, []string{}},
-		{[]string{"*"}, []string{}, []string{"label1", "label2"}, []string{}},
-		{[]string{"lab*"}, []string{}, []string{"label1", "label2"}, []string{}},
-		{[]string{"label1"}, []string{}, []string{"label1"}, []string{"label2"}},
-		{[]string{"label1*"}, []string{}, []string{"label1"}, []string{"label2"}},
-		{[]string{}, []string{"*"}, []string{}, []string{"label1", "label2"}},
-		{[]string{}, []string{"lab*"}, []string{}, []string{"label1", "label2"}},
-		{[]string{}, []string{"label1"}, []string{"label2"}, []string{"label1"}},
-		{[]string{"*"}, []string{"*"}, []string{}, []string{"label1", "label2"}},
+		{
+			name: "Nil filters matches all",
+			container: types.Container{
+				Labels: map[string]string{
+					"a": "x",
+				},
+			},
+			include: nil,
+			exclude: nil,
+			expected: map[string]string{
+				"a": "x",
+			},
+		},
+		{
+			name: "Empty filters matches all",
+			container: types.Container{
+				Labels: map[string]string{
+					"a": "x",
+				},
+			},
+			include: []string{},
+			exclude: []string{},
+			expected: map[string]string{
+				"a": "x",
+			},
+		},
+		{
+			name: "Must match include",
+			container: types.Container{
+				Labels: map[string]string{
+					"a": "x",
+					"b": "y",
+				},
+			},
+			include: []string{"a"},
+			exclude: []string{},
+			expected: map[string]string{
+				"a": "x",
+			},
+		},
+		{
+			name: "Must not match exclude",
+			container: types.Container{
+				Labels: map[string]string{
+					"a": "x",
+					"b": "y",
+				},
+			},
+			include: []string{},
+			exclude: []string{"b"},
+			expected: map[string]string{
+				"a": "x",
+			},
+		},
+		{
+			name: "Include Glob",
+			container: types.Container{
+				Labels: map[string]string{
+					"aa": "x",
+					"ab": "y",
+					"bb": "z",
+				},
+			},
+			include: []string{"a*"},
+			exclude: []string{},
+			expected: map[string]string{
+				"aa": "x",
+				"ab": "y",
+			},
+		},
+		{
+			name: "Exclude Glob",
+			container: types.Container{
+				Labels: map[string]string{
+					"aa": "x",
+					"ab": "y",
+					"bb": "z",
+				},
+			},
+			include: []string{},
+			exclude: []string{"a*"},
+			expected: map[string]string{
+				"bb": "z",
+			},
+		},
+		{
+			name: "Excluded Includes",
+			container: types.Container{
+				Labels: map[string]string{
+					"aa": "x",
+					"ab": "y",
+					"bb": "z",
+				},
+			},
+			include: []string{"a*"},
+			exclude: []string{"*b"},
+			expected: map[string]string{
+				"aa": "x",
+			},
+		},
 	}
-
-	for _, tt := range gatherLabelsTests {
-		t.Run("", func(t *testing.T) {
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
 			var acc testutil.Accumulator
-			d := Docker{
-				newClient: newClient,
+
+			newClientFunc := func(host string, tlsConfig *tls.Config) (Client, error) {
+				client := baseClient
+				client.ContainerListF = func(context.Context, types.ContainerListOptions) ([]types.Container, error) {
+					return []types.Container{tt.container}, nil
+				}
+				return &client, nil
 			}
 
-			for _, label := range tt.include {
-				d.LabelInclude = append(d.LabelInclude, label)
-			}
-			for _, label := range tt.exclude {
-				d.LabelExclude = append(d.LabelExclude, label)
+			d := Docker{
+				newClient:    newClientFunc,
+				LabelInclude: tt.include,
+				LabelExclude: tt.exclude,
 			}
 
 			err := d.Gather(&acc)
 			require.NoError(t, err)
 
-			for _, label := range tt.expected {
-				if !acc.HasTag("docker_container_cpu", label) {
-					t.Errorf("Didn't get expected label of %s.  Test was:  Include: %s  Exclude %s",
-						label, tt.include, tt.exclude)
+			// Grab tags from a container metric
+			var actual map[string]string
+			for _, metric := range acc.Metrics {
+				if metric.Measurement == "docker_container_cpu" {
+					actual = metric.Tags
 				}
 			}
 
-			for _, label := range tt.notexpected {
-				if acc.HasTag("docker_container_cpu", label) {
-					t.Errorf("Got unexpected label of %s.  Test was:  Include: %s  Exclude %s",
-						label, tt.include, tt.exclude)
-				}
+			for k, v := range tt.expected {
+				require.Equal(t, v, actual[k])
 			}
 		})
 	}
 }
 
 func TestContainerNames(t *testing.T) {
-	var gatherContainerNames = []struct {
-		include     []string
-		exclude     []string
-		expected    []string
-		notexpected []string
+	var tests = []struct {
+		name       string
+		containers [][]string
+		include    []string
+		exclude    []string
+		expected   []string
 	}{
-		{[]string{}, []string{}, []string{"etcd", "etcd2"}, []string{}},
-		{[]string{"*"}, []string{}, []string{"etcd", "etcd2"}, []string{}},
-		{[]string{"etc*"}, []string{}, []string{"etcd", "etcd2"}, []string{}},
-		{[]string{"etcd"}, []string{}, []string{"etcd"}, []string{"etcd2"}},
-		{[]string{"etcd2*"}, []string{}, []string{"etcd2"}, []string{"etcd"}},
-		{[]string{}, []string{"etc*"}, []string{}, []string{"etcd", "etcd2"}},
-		{[]string{}, []string{"etcd"}, []string{"etcd2"}, []string{"etcd"}},
-		{[]string{"*"}, []string{"*"}, []string{"etcd", "etcd2"}, []string{}},
-		{[]string{}, []string{"*"}, []string{""}, []string{"etcd", "etcd2"}},
+		{
+			name: "Nil filters matches all",
+			containers: [][]string{
+				{"/etcd"},
+				{"/etcd2"},
+			},
+			include:  nil,
+			exclude:  nil,
+			expected: []string{"etcd", "etcd2"},
+		},
+		{
+			name: "Empty filters matches all",
+			containers: [][]string{
+				{"/etcd"},
+				{"/etcd2"},
+			},
+			include:  []string{},
+			exclude:  []string{},
+			expected: []string{"etcd", "etcd2"},
+		},
+		{
+			name: "Match all containers",
+			containers: [][]string{
+				{"/etcd"},
+				{"/etcd2"},
+			},
+			include:  []string{"*"},
+			exclude:  []string{},
+			expected: []string{"etcd", "etcd2"},
+		},
+		{
+			name: "Include prefix match",
+			containers: [][]string{
+				{"/etcd"},
+				{"/etcd2"},
+			},
+			include:  []string{"etc*"},
+			exclude:  []string{},
+			expected: []string{"etcd", "etcd2"},
+		},
+		{
+			name: "Exact match",
+			containers: [][]string{
+				{"/etcd"},
+				{"/etcd2"},
+			},
+			include:  []string{"etcd"},
+			exclude:  []string{},
+			expected: []string{"etcd"},
+		},
+		{
+			name: "Star matches zero length",
+			containers: [][]string{
+				{"/etcd"},
+				{"/etcd2"},
+			},
+			include:  []string{"etcd2*"},
+			exclude:  []string{},
+			expected: []string{"etcd2"},
+		},
+		{
+			name: "Exclude matches all",
+			containers: [][]string{
+				{"/etcd"},
+				{"/etcd2"},
+			},
+			include:  []string{},
+			exclude:  []string{"etc*"},
+			expected: []string{},
+		},
+		{
+			name: "Exclude single",
+			containers: [][]string{
+				{"/etcd"},
+				{"/etcd2"},
+			},
+			include:  []string{},
+			exclude:  []string{"etcd"},
+			expected: []string{"etcd2"},
+		},
+		{
+			name: "Exclude all",
+			containers: [][]string{
+				{"/etcd"},
+				{"/etcd2"},
+			},
+			include:  []string{"*"},
+			exclude:  []string{"*"},
+			expected: []string{},
+		},
+		{
+			name: "Exclude item matching include",
+			containers: [][]string{
+				{"acme"},
+				{"foo"},
+				{"acme-test"},
+			},
+			include:  []string{"acme*"},
+			exclude:  []string{"*test*"},
+			expected: []string{"acme"},
+		},
+		{
+			name: "Exclude item no wildcards",
+			containers: [][]string{
+				{"acme"},
+				{"acme-test"},
+			},
+			include:  []string{"acme*"},
+			exclude:  []string{"test"},
+			expected: []string{"acme", "acme-test"},
+		},
 	}
-
-	for _, tt := range gatherContainerNames {
-		t.Run("", func(t *testing.T) {
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
 			var acc testutil.Accumulator
 
+			newClientFunc := func(host string, tlsConfig *tls.Config) (Client, error) {
+				client := baseClient
+				client.ContainerListF = func(context.Context, types.ContainerListOptions) ([]types.Container, error) {
+					var containers []types.Container
+					for _, names := range tt.containers {
+						containers = append(containers, types.Container{
+							Names: names,
+						})
+					}
+					return containers, nil
+				}
+				return &client, nil
+			}
+
 			d := Docker{
-				newClient:        newClient,
+				newClient:        newClientFunc,
 				ContainerInclude: tt.include,
 				ContainerExclude: tt.exclude,
 			}
@@ -317,39 +528,21 @@ func TestContainerNames(t *testing.T) {
 			err := d.Gather(&acc)
 			require.NoError(t, err)
 
+			// Set of expected names
+			var expected = make(map[string]bool)
+			for _, v := range tt.expected {
+				expected[v] = true
+			}
+
+			// Set of actual names
+			var actual = make(map[string]bool)
 			for _, metric := range acc.Metrics {
-				if metric.Measurement == "docker_container_cpu" {
-					if val, ok := metric.Tags["container_name"]; ok {
-						var found bool = false
-						for _, cname := range tt.expected {
-							if val == cname {
-								found = true
-								break
-							}
-						}
-						if !found {
-							t.Errorf("Got unexpected container of %s. Test was -> Include: %s, Exclude: %s", val, tt.include, tt.exclude)
-						}
-					}
+				if name, ok := metric.Tags["container_name"]; ok {
+					actual[name] = true
 				}
 			}
 
-			for _, metric := range acc.Metrics {
-				if metric.Measurement == "docker_container_cpu" {
-					if val, ok := metric.Tags["container_name"]; ok {
-						var found bool = false
-						for _, cname := range tt.notexpected {
-							if val == cname {
-								found = true
-								break
-							}
-						}
-						if found {
-							t.Errorf("Got unexpected container of %s. Test was -> Include: %s, Exclude: %s", val, tt.include, tt.exclude)
-						}
-					}
-				}
-			}
+			require.Equal(t, expected, actual)
 		})
 	}
 }