feat(autodiscovery): report ignored keys when AD formats are mixed

comp/core/autodiscovery/common/utils/annotations.go

@@ -10,6 +10,7 @@ import (
 	"errors"
 	"fmt"
 	"strconv"
+	"strings"
 
 	"github.com/DataDog/datadog-agent/comp/core/autodiscovery/integration"
 )
@@ -25,6 +26,23 @@ const (
 	checkTagCardinality     = "check_tag_cardinality"
 )
 
+// v1CheckKeys holds the suffixes of the keys defining a v1 check
+// configuration. ignore_autodiscovery_tags and logs are left out on purpose:
+// they can be read next to a v2 check configuration, so setting them alongside
+// one does not mean they are ignored. For instance, ignore_autodiscovery_tags is
+// applied to v2 configurations when
+// cluster_checks.support_hybrid_ignore_ad_tags is enabled.
+//
+// check_tag_cardinality is also left out, for a different reason: a standalone
+// one is dropped next to a v2 configuration, but that happens whether or not v1
+// keys are set, so it is a separate gap rather than a consequence of mixing
+// formats.
+var v1CheckKeys = []string{
+	checkNamePath,
+	initConfigPath,
+	instancePath,
+}
+
 // ExtractTemplatesFromMap looks for autodiscovery configurations in a given
 // map and returns them if found.
 func ExtractTemplatesFromMap(key string, input map[string]string, prefix string) ([]integration.Config, []error) {
@@ -275,6 +293,13 @@ func extractTemplatesFromMapWithV2(entityName string, annotations map[string]str
 		} else {
 			configs = append(configs, c...)
 		}
+
+		// v1 and legacy check configurations are not applied when a v2 one
+		// is set on the same entity. Report them, as they would otherwise
+		// be dropped without any feedback.
+		if ignored := findIgnoredCheckKeys(annotations, prefixCandidates); len(ignored) > 0 {
+			errors = append(errors, ignoredCheckKeysError(prefix+checksPath, ignored))
+		}
 	} else {
 		// AD annotations v1: "ad.datadoghq.com/redis.check_names"
 		// AD annotations legacy: "service-discovery.datadoghq.com/redis.check_names"
@@ -287,6 +312,12 @@ func extractTemplatesFromMapWithV2(entityName string, annotations map[string]str
 			} else {
 				configs = append(configs, c...)
 			}
+
+			// Same as above for the formats with a lower priority than the one that was found.
+			lowerPriority := lowerPriorityPrefixes(prefixCandidates, actualPrefix)
+			if ignored := findIgnoredCheckKeys(annotations, lowerPriority); len(ignored) > 0 {
+				errors = append(errors, ignoredCheckKeysError(actualPrefix+checkNamePath, ignored))
+			}
 		}
 	}
 
@@ -321,3 +352,40 @@ func findPrefix(annotations map[string]string, prefixes []string, suffix string)
 
 	return ""
 }
+
+// lowerPriorityPrefixes returns the prefixes that come after usedPrefix in
+// prefixes, which is ordered by decreasing priority.
+func lowerPriorityPrefixes(prefixes []string, usedPrefix string) []string {
+	for idx, prefix := range prefixes {
+		if prefix == usedPrefix {
+			return prefixes[idx+1:]
+		}
+	}
+
+	return nil
+}
+
+// findIgnoredCheckKeys returns the keys holding a check configuration that is
+// not applied because a configuration with a higher priority was found on the
+// same entity. Autodiscovery only applies one check configuration format per
+// entity, so mixing them drops part of the configuration.
+func findIgnoredCheckKeys(input map[string]string, ignoredPrefixes []string) []string {
+	var ignored []string
+
+	for _, prefix := range ignoredPrefixes {
+		for _, suffix := range v1CheckKeys {
+			key := prefix + suffix
+			if _, found := input[key]; found {
+				ignored = append(ignored, key)
+			}
+		}
+	}
+
+	return ignored
+}
+
+// ignoredCheckKeysError builds the error reported when several check
+// configuration formats are set on the same entity.
+func ignoredCheckKeysError(usedKey string, ignoredKeys []string) error {
+	return fmt.Errorf("%s takes precedence, ignoring %s: Autodiscovery only applies the check configuration with the highest priority (v2, then v1, then legacy)", usedKey, strings.Join(ignoredKeys, ", "))
+}

comp/core/autodiscovery/common/utils/container_labels_test.go

@@ -195,6 +195,9 @@ func TestExtractTemplatesFromContainerLabels(t *testing.T) {
 					ADIdentifiers: []string{adID},
 				},
 			},
+			errs: []error{
+				errors.New("com.datadoghq.ad.checks takes precedence, ignoring com.datadoghq.ad.check_names: Autodiscovery only applies the check configuration with the highest priority (v2, then v1, then legacy)"),
+			},
 		},
 	}
 

comp/core/autodiscovery/common/utils/pod_annotations_test.go

@@ -356,6 +356,9 @@ func TestExtractTemplatesFromAnnotations(t *testing.T) {
 					ADIdentifiers: []string{adID},
 				},
 			},
+			errs: []error{
+				errors.New("ad.datadoghq.com/foobar.checks takes precedence, ignoring ad.datadoghq.com/foobar.check_names, service-discovery.datadoghq.com/foobar.check_names: Autodiscovery only applies the check configuration with the highest priority (v2, then v1, then legacy)"),
+			},
 		},
 		{
 			name: "v2 annotations with ignore_ad_tags",
@@ -381,6 +384,9 @@ func TestExtractTemplatesFromAnnotations(t *testing.T) {
 					IgnoreAutodiscoveryTags: true,
 				},
 			},
+			errs: []error{
+				errors.New("ad.datadoghq.com/foobar.checks takes precedence, ignoring ad.datadoghq.com/foobar.check_names, service-discovery.datadoghq.com/foobar.check_names: Autodiscovery only applies the check configuration with the highest priority (v2, then v1, then legacy)"),
+			},
 		},
 		{
 			name: "v2 annotations with adv1 ignore_ad_tags",
@@ -406,6 +412,9 @@ func TestExtractTemplatesFromAnnotations(t *testing.T) {
 					IgnoreAutodiscoveryTags: false,
 				},
 			},
+			errs: []error{
+				errors.New("ad.datadoghq.com/foobar.checks takes precedence, ignoring ad.datadoghq.com/foobar.check_names, service-discovery.datadoghq.com/foobar.check_names: Autodiscovery only applies the check configuration with the highest priority (v2, then v1, then legacy)"),
+			},
 		},
 		{
 			name: "v2 annotations with init_config",
@@ -473,6 +482,56 @@ func TestExtractTemplatesFromAnnotations(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "complete v1 and v2 check configurations on the same identifier",
+			annotations: map[string]string{
+				"ad.datadoghq.com/foobar.checks": `{
+					"apache": {
+						"instances": [
+							{"apache_status_url":"http://%%host%%/server-status?auto"}
+						]
+					}
+				}`,
+				"ad.datadoghq.com/foobar.check_names":  `["http_check"]`,
+				"ad.datadoghq.com/foobar.init_configs": `[{}]`,
+				"ad.datadoghq.com/foobar.instances":    `[{"name":"My service","url":"http://%%host%%"}]`,
+			},
+			adIdentifier: "foobar",
+			output: []integration.Config{
+				{
+					Name:          "apache",
+					Instances:     []integration.Data{integration.Data(`{"apache_status_url":"http://%%host%%/server-status?auto"}`)},
+					InitConfig:    integration.Data("{}"),
+					ADIdentifiers: []string{adID},
+				},
+			},
+			errs: []error{
+				errors.New("ad.datadoghq.com/foobar.checks takes precedence, ignoring ad.datadoghq.com/foobar.check_names, ad.datadoghq.com/foobar.init_configs, ad.datadoghq.com/foobar.instances: Autodiscovery only applies the check configuration with the highest priority (v2, then v1, then legacy)"),
+			},
+		},
+		{
+			name: "complete v1 and legacy check configurations on the same identifier",
+			annotations: map[string]string{
+				"ad.datadoghq.com/foobar.check_names":                 `["apache"]`,
+				"ad.datadoghq.com/foobar.init_configs":                `[{}]`,
+				"ad.datadoghq.com/foobar.instances":                   `[{"apache_status_url":"http://%%host%%/server-status?auto"}]`,
+				"service-discovery.datadoghq.com/foobar.check_names":  `["http_check"]`,
+				"service-discovery.datadoghq.com/foobar.init_configs": `[{}]`,
+				"service-discovery.datadoghq.com/foobar.instances":    `[{"name":"My service","url":"http://%%host%%"}]`,
+			},
+			adIdentifier: "foobar",
+			output: []integration.Config{
+				{
+					Name:          "apache",
+					Instances:     []integration.Data{integration.Data(`{"apache_status_url":"http://%%host%%/server-status?auto"}`)},
+					InitConfig:    integration.Data("{}"),
+					ADIdentifiers: []string{adID},
+				},
+			},
+			errs: []error{
+				errors.New("ad.datadoghq.com/foobar.check_names takes precedence, ignoring service-discovery.datadoghq.com/foobar.check_names, service-discovery.datadoghq.com/foobar.init_configs, service-discovery.datadoghq.com/foobar.instances: Autodiscovery only applies the check configuration with the highest priority (v2, then v1, then legacy)"),
+			},
+		},
 	}
 
 	for _, tt := range tests {

comp/core/autodiscovery/providers/container_test.go

@@ -164,7 +164,9 @@ func TestGenerateConfig(t *testing.T) {
 					Source:        "container:docker://3b8efe0c50e8",
 				},
 			},
-			expectedErr: nil,
+			expectedErr: types.ErrorMsgSet{
+				"ad.datadoghq.com/apache.checks takes precedence, ignoring ad.datadoghq.com/apache.check_names, ad.datadoghq.com/apache.init_configs, ad.datadoghq.com/apache.instances: Autodiscovery only applies the check configuration with the highest priority (v2, then v1, then legacy)": {},
+			},
 		},
 		{
 			name: "New + old, new takes over",
@@ -195,7 +197,9 @@ func TestGenerateConfig(t *testing.T) {
 					Source:        "container:docker://3b8efe0c50e8",
 				},
 			},
-			expectedErr: nil,
+			expectedErr: types.ErrorMsgSet{
+				"ad.datadoghq.com/apache.check_names takes precedence, ignoring service-discovery.datadoghq.com/apache.check_names, service-discovery.datadoghq.com/apache.init_configs, service-discovery.datadoghq.com/apache.instances: Autodiscovery only applies the check configuration with the highest priority (v2, then v1, then legacy)": {},
+			},
 		},
 		{
 			name: "New annotation prefix, two templates",

releasenotes/notes/report-ignored-ad-check-configs-34903b7a393295f0.yaml

@@ -0,0 +1,17 @@
+# Each section from every release note are combined when the
+# CHANGELOG.rst is rendered. So the text needs to be worded so that
+# it does not depend on any information only available in another
+# section. This may mean repeating some details, but each section
+# must be readable independently of the other.
+#
+# Each section note must be formatted as reStructuredText.
+---
+enhancements:
+  - |
+    Autodiscovery now reports the check configuration keys it ignores when
+    several annotation or label formats are set on the same entity. Only the
+    format with the highest priority is applied (``checks``, then
+    ``check_names`` with ``init_configs`` and ``instances``, then the legacy
+    ``service-discovery.datadoghq.com`` prefix), and the others used to be
+    discarded silently. The ignored keys are now listed in the Autodiscovery
+    section of the ``agent status`` output.