diff --git a/cmd/yaml-mapper/mapper/map_processors.go b/cmd/yaml-mapper/mapper/map_processors.go index dbdcd886b4..f0673b443c 100644 --- a/cmd/yaml-mapper/mapper/map_processors.go +++ b/cmd/yaml-mapper/mapper/map_processors.go @@ -34,6 +34,7 @@ func mapFuncRegistry() map[string]MappingRunFunc { mapAppendEnvVar, mapMergeEnvs, mapOverrideType, + mapCustomConfigFile, } { registry[p.name] = p.runFunc } @@ -296,6 +297,44 @@ var mapOverrideType = MappingProcessor{ }, } +// mapCustomConfigFile serializes an object-shaped Helm value (e.g. custom datadog.yaml settings) +// into a YAML string and stores it under CustomConfig's `configData` field, keyed by the given +// filename. The filename is passed via `args` instead of `newPath` since it can contain dots +// (e.g. "datadog.yaml") that shouldn't be split into nested path segments. +// args: +// - fileName: datadog.yaml +var mapCustomConfigFile = MappingProcessor{ + name: "mapCustomConfigFile", + runFunc: func(interim map[string]any, newPath string, pathVal any, args []any) { + if len(args) != 1 { + return + } + fileName, ok := utils.GetPathString(args[0], "fileName") + if !ok || fileName == "" { + return + } + + var configData string + switch v := pathVal.(type) { + case string: + configData = v + default: + out, err := yaml.Marshal(pathVal) + if err != nil { + slog.Error("failed to marshal custom config content", "path", newPath, "fileName", fileName, "error", err) + return + } + configData = string(out) + } + + utils.MergeOrSet(interim, newPath, map[string]any{ + fileName: map[string]any{ + "configData": configData, + }, + }) + }, +} + // hasDuplicateEnv checks if a given env var name is already present in the given list of env vars. func hasDuplicateEnv(existingEnvs []any, newEnvName string) bool { for _, existingEnv := range existingEnvs { diff --git a/cmd/yaml-mapper/mapper/mapper.go b/cmd/yaml-mapper/mapper/mapper.go index 0b1b4fa025..b1320f6d3e 100644 --- a/cmd/yaml-mapper/mapper/mapper.go +++ b/cmd/yaml-mapper/mapper/mapper.go @@ -197,20 +197,27 @@ func (m *Mapper) mapValues(sourceValues chartutil.Values, mappingValues chartuti // Map values.yaml => DDA for _, sourceKey := range mappingKeys { + destKey, _ := mappingValues[sourceKey] + // Only copy a whole table if none of its sub-keys are mapped separately + // (e.g. agents.podSecurity.seLinuxContext.rule has its own mapping entry, + // so seLinuxContext itself shouldn't be copied as one big table). + hasMappedDescendant := hasDescendantMappingKey(mappingKeys, sourceKey) + pathVal, _ := sourceValues.PathValue(sourceKey) if pathVal == nil { if mapVal, ok := utils.GetPathMap(sourceValues[sourceKey]); ok && mapVal != nil { pathVal = mapVal - } else if tableVal, err := sourceValues.Table(sourceKey); err == nil && len(tableVal) == 1 { + } else if tableVal, err := sourceValues.Table(sourceKey); err == nil && len(tableVal) > 0 && !hasMappedDescendant { pathVal = tableVal } else { continue } } - utils.MergeOrSet(sourceKeysRef, sourceKey, map[string]any{"visited": true}) + // Mark this key and everything nested under it as visited, since copying the + // whole table also covers all of its nested values. + markVisited(sourceKeysRef, sourceKey) - destKey, _ := mappingValues[sourceKey] if (destKey == "" || destKey == nil) && !shouldSkipMappingKey(sourceKey) { slog.Error("DDA destination key not found", "sourceKey", sourceKey) errorCount++ @@ -383,6 +390,33 @@ func (m *Mapper) updateMapping(sourceValues chartutil.Values, mappingValues char return nil } +// hasDescendantMappingKey returns true if mappingKeys contains an entry that is a strict +// descendant of sourceKey (i.e. prefixed with "sourceKey."). Such entries indicate that +// sub-fields of the table at sourceKey are mapped individually, so the table itself must +// not be copied wholesale. +func hasDescendantMappingKey(mappingKeys []string, sourceKey string) bool { + prefix := sourceKey + "." + for _, k := range mappingKeys { + if strings.HasPrefix(k, prefix) { + return true + } + } + return false +} + +// markVisited marks sourceKey, and every flattened key nested beneath it, as visited in +// sourceKeysRef. This is used when a table value is consumed as a whole (either copied +// directly or passed to a mapFunc), which implicitly consumes all of its nested values too. +func markVisited(sourceKeysRef map[string]any, sourceKey string) { + utils.MergeOrSet(sourceKeysRef, sourceKey, map[string]any{"visited": true}) + prefix := sourceKey + "." + for k := range sourceKeysRef { + if strings.HasPrefix(k, prefix) { + utils.MergeOrSet(sourceKeysRef, k, map[string]any{"visited": true}) + } + } +} + // flattenValues builds a mapping of dotted-key paths from a provided Values source. func flattenValues(sourceValues chartutil.Values, valuesMap map[string]any, prefix string) map[string]any { for key, value := range sourceValues { diff --git a/cmd/yaml-mapper/mapper/mapper_test.go b/cmd/yaml-mapper/mapper/mapper_test.go index ae0bb58970..9f49ef6865 100644 --- a/cmd/yaml-mapper/mapper/mapper_test.go +++ b/cmd/yaml-mapper/mapper/mapper_test.go @@ -136,6 +136,142 @@ datadog: } } +// TestMapValuesMultiKeyTable covers two real-world helm2dda bug reports: +// 1. clusterAgent.confd/datadog.confd with multiple files must map wholesale into +// their configDataMap destination, instead of being silently dropped. +// 2. agents.customAgentConfig/clusterAgent.datadog_cluster_yaml with nested fields +// must not report the flattened nested keys as "not found in mapping" errors, +// since they're already fully consumed by the mapCustomConfigFile mapFunc. +// +// It also guards against regressing the fix for agents.podSecurity.seLinuxContext, +// whose sub-fields (e.g. seLinuxOptions.level) have their own separate mapping +// entries and must therefore NOT be copied wholesale as a multi-key table. +// fileConfigDataCheck verifies the configData content of a single file entry within a +// MultiCustomConfig/customConfigurations-style map, whose filename key may itself +// contain literal dots (e.g. "datadog.yaml"), making PathValue/Table unusable for it. +type fileConfigDataCheck struct { + fileName string + want string +} + +func TestMapValuesMultiKeyTable(t *testing.T) { + tempDir := t.TempDir() + + tests := []struct { + name string + inputValues string + expectNoErrors bool + expectedDDA map[string]any + checkTables map[string]map[string]any + checkFileConfigData map[string]fileConfigDataCheck + missingPaths []string + }{ + { + name: "clusterAgent.confd with multiple files maps wholesale", + inputValues: `clusterAgent: + confd: + mysql.yaml: |- + cluster_check: true + kubernetes_state.yaml: |- + ad_identifiers: + - kube-state-metrics +`, + expectNoErrors: true, + checkTables: map[string]map[string]any{ + "spec.override.clusterAgent.extraConfd.configDataMap": { + "mysql.yaml": "cluster_check: true", + "kubernetes_state.yaml": "ad_identifiers:\n - kube-state-metrics", + }, + }, + }, + { + name: "customAgentConfig and datadog_cluster_yaml do not report spurious leaf errors", + inputValues: `agents: + customAgentConfig: + log_level: "debug" + jmx_use_container_support: true +clusterAgent: + datadog_cluster_yaml: + log_level: "debug" + cluster_checks: + enabled: true +`, + expectNoErrors: true, + checkFileConfigData: map[string]fileConfigDataCheck{ + "spec.override.nodeAgent.customConfigurations": { + fileName: "datadog.yaml", + want: "jmx_use_container_support: true\nlog_level: debug\n", + }, + "spec.override.clusterAgent.customConfigurations": { + fileName: "datadog-cluster.yaml", + want: "cluster_checks:\n enabled: true\nlog_level: debug\n", + }, + }, + }, + { + name: "seLinuxContext sub-fields stay individually mapped, not copied wholesale", + inputValues: `agents: + podSecurity: + seLinuxContext: + seLinuxOptions: + level: "s0:c123,c456" + role: "system_r" + type: "spc_t" + user: "system_u" +`, + expectNoErrors: true, + expectedDDA: map[string]any{ + "spec.override.nodeAgent.containers.agent.securityContext.seLinuxOptions.level": "s0:c123,c456", + "spec.override.nodeAgent.containers.agent.securityContext.seLinuxOptions.role": "system_r", + "spec.override.nodeAgent.containers.agent.securityContext.seLinuxOptions.type": "spc_t", + "spec.override.nodeAgent.containers.agent.securityContext.seLinuxOptions.user": "system_u", + }, + // seLinuxOptions itself must not also appear as a flat, wholesale-copied value. + missingPaths: []string{"spec.override.nodeAgent.containers.agent.securityContext.seLinuxOptions.rule"}, + }, + } + + for i, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + valuesPath := filepath.Join(tempDir, fmt.Sprintf("multikey-values-%d.yaml", i+1)) + ddaPath := filepath.Join(tempDir, fmt.Sprintf("multikey-dda-%d.yaml", i+1)) + writeTestFile(t, valuesPath, tt.inputValues) + + mapper := NewMapper(MapConfig{ + MappingPath: "mapping_datadog_helm_to_datadogagent_crd.yaml", + SourcePath: valuesPath, + DestPath: ddaPath, + }) + err := mapper.Run() + if tt.expectNoErrors { + require.NoError(t, err, "run %s failed", tt.name) + } + + dda, err := chartutil.ReadValuesFile(ddaPath) + require.NoError(t, err, "run %s failed to read output", tt.name) + + assertValues(t, dda, tt.expectedDDA) + for _, missingPath := range tt.missingPaths { + assertMissingPath(t, dda, missingPath, "run %s should not contain %s", tt.name, missingPath) + } + for path, want := range tt.checkTables { + got, tableErr := dda.Table(path) + require.NoError(t, tableErr, "run %s: expected table at path %q", tt.name, path) + for k, v := range want { + assert.Equal(t, v, got[k], "run %s: unexpected value at %q[%q]", tt.name, path, k) + } + } + for path, check := range tt.checkFileConfigData { + parent, tableErr := dda.Table(path) + require.NoError(t, tableErr, "run %s: expected table at path %q", tt.name, path) + fileEntry, ok := utils.GetPathMap(parent[check.fileName]) + require.True(t, ok, "run %s: expected %q[%q] to be a map", tt.name, path, check.fileName) + assert.Equal(t, check.want, fileEntry["configData"], "run %s: unexpected configData at %q[%q]", tt.name, path, check.fileName) + } + }) + } +} + func writeTestFile(t *testing.T, path, content string) { t.Helper() require.NoError(t, os.WriteFile(path, []byte(content), 0644)) @@ -1083,7 +1219,7 @@ func TestApplyDeprecationRules(t *testing.T) { func TestMappingProcessors(t *testing.T) { // Test that all mapping processors are properly registered t.Run("mapFuncRegistry_dict", func(t *testing.T) { - expectedFuncs := []string{"mapSecretKeyName", "mapSeccompProfile", "mapSystemProbeAppArmor", "mapLocalServiceName", "mapAppendEnvVar", "mapMergeEnvs", "mapOverrideType"} + expectedFuncs := []string{"mapSecretKeyName", "mapSeccompProfile", "mapSystemProbeAppArmor", "mapLocalServiceName", "mapAppendEnvVar", "mapMergeEnvs", "mapOverrideType", "mapCustomConfigFile"} mapFuncs := mapFuncRegistry() for _, funcName := range expectedFuncs { @@ -1721,6 +1857,139 @@ func TestMappingProcessors(t *testing.T) { "spec.features.foo.bar": 8080, }, }, + // mapCustomConfigFile tests + { + name: "mapCustomConfigFile_string_value", + funcName: "mapCustomConfigFile", + interim: map[string]any{}, + newPath: "spec.override.nodeAgent.customConfigurations", + pathVal: "log_level: debug\ntags:\n - foo:bar\n", + mapFuncArgs: []any{ + map[string]any{ + "fileName": "datadog.yaml", + }, + }, + expectedMap: map[string]any{ + "spec.override.nodeAgent.customConfigurations": map[string]any{ + "datadog.yaml": map[string]any{ + "configData": "log_level: debug\ntags:\n - foo:bar\n", + }, + }, + }, + }, + { + name: "mapCustomConfigFile_object_value_marshaled_to_yaml", + funcName: "mapCustomConfigFile", + interim: map[string]any{}, + newPath: "spec.override.clusterAgent.customConfigurations", + pathVal: map[string]any{ + "log_level": "debug", + "tags": []any{"foo:bar"}, + }, + mapFuncArgs: []any{ + map[string]any{ + "fileName": "datadog-cluster.yaml", + }, + }, + expectedMap: map[string]any{ + "spec.override.clusterAgent.customConfigurations": map[string]any{ + "datadog-cluster.yaml": map[string]any{ + "configData": "log_level: debug\ntags:\n- foo:bar\n", + }, + }, + }, + }, + { + name: "mapCustomConfigFile_merges_with_existing_customConfigurations", + funcName: "mapCustomConfigFile", + interim: map[string]any{ + "spec.override.nodeAgent.customConfigurations": map[string]any{ + "other-file.yaml": map[string]any{ + "configData": "foo: bar\n", + }, + }, + }, + newPath: "spec.override.nodeAgent.customConfigurations", + pathVal: "log_level: debug\n", + mapFuncArgs: []any{ + map[string]any{ + "fileName": "datadog.yaml", + }, + }, + expectedMap: map[string]any{ + "spec.override.nodeAgent.customConfigurations": map[string]any{ + "other-file.yaml": map[string]any{ + "configData": "foo: bar\n", + }, + "datadog.yaml": map[string]any{ + "configData": "log_level: debug\n", + }, + }, + }, + }, + { + name: "mapCustomConfigFile_no_args_is_noop", + funcName: "mapCustomConfigFile", + interim: map[string]any{ + "spec.global.site": "datadoghq.com", + }, + newPath: "spec.override.nodeAgent.customConfigurations", + pathVal: "log_level: debug\n", + mapFuncArgs: []any{}, + expectedMap: map[string]any{ + "spec.global.site": "datadoghq.com", + }, + }, + { + name: "mapCustomConfigFile_missing_fileName_is_noop", + funcName: "mapCustomConfigFile", + interim: map[string]any{ + "spec.global.site": "datadoghq.com", + }, + newPath: "spec.override.nodeAgent.customConfigurations", + pathVal: "log_level: debug\n", + mapFuncArgs: []any{ + map[string]any{}, + }, + expectedMap: map[string]any{ + "spec.global.site": "datadoghq.com", + }, + }, + { + name: "mapCustomConfigFile_empty_fileName_is_noop", + funcName: "mapCustomConfigFile", + interim: map[string]any{ + "spec.global.site": "datadoghq.com", + }, + newPath: "spec.override.nodeAgent.customConfigurations", + pathVal: "log_level: debug\n", + mapFuncArgs: []any{ + map[string]any{ + "fileName": "", + }, + }, + expectedMap: map[string]any{ + "spec.global.site": "datadoghq.com", + }, + }, + { + name: "mapCustomConfigFile_unmarshalable_value_is_noop", + funcName: "mapCustomConfigFile", + interim: map[string]any{ + "spec.global.site": "datadoghq.com", + }, + newPath: "spec.override.nodeAgent.customConfigurations", + // funcs can't be marshaled to YAML/JSON, exercising the yaml.Marshal error branch. + pathVal: func() {}, + mapFuncArgs: []any{ + map[string]any{ + "fileName": "datadog.yaml", + }, + }, + expectedMap: map[string]any{ + "spec.global.site": "datadoghq.com", + }, + }, } mapFuncs := mapFuncRegistry() diff --git a/cmd/yaml-mapper/mapper/mapping_datadog_helm_to_datadogagent_crd.yaml b/cmd/yaml-mapper/mapper/mapping_datadog_helm_to_datadogagent_crd.yaml index 908cc6248b..2c78c58cce 100644 --- a/cmd/yaml-mapper/mapper/mapping_datadog_helm_to_datadogagent_crd.yaml +++ b/cmd/yaml-mapper/mapper/mapping_datadog_helm_to_datadogagent_crd.yaml @@ -90,7 +90,11 @@ agents.containers.traceAgent.logLevel: spec.override.nodeAgent.containers.trace- agents.containers.traceAgent.ports: spec.override.nodeAgent.containers.trace-agent.ports agents.containers.traceAgent.resources: spec.override.nodeAgent.containers.trace-agent.resources agents.containers.traceAgent.securityContext: spec.override.nodeAgent.containers.trace-agent.securityContext -agents.customAgentConfig: spec.override.nodeAgent.customConfigurations.datadog.yaml.configData +agents.customAgentConfig: + mapFunc: mapCustomConfigFile + newPath: spec.override.nodeAgent.customConfigurations + args: + - fileName: datadog.yaml agents.daemonsetAnnotations: spec.override.nodeAgent.annotations agents.dnsConfig: spec.override.nodeAgent.dnsConfig agents.enabled: "" @@ -180,7 +184,7 @@ clusterAgent.admissionController.webhookName: spec.features.admissionController. clusterAgent.advancedConfd: spec.override.clusterAgent.extraConfd.configData clusterAgent.affinity: spec.override.clusterAgent.affinity clusterAgent.command: spec.override.clusterAgent.containers.cluster-agent.command -clusterAgent.confd: spec.override.clusterAgent.extraConfd +clusterAgent.confd: spec.override.clusterAgent.extraConfd.configDataMap clusterAgent.containerExclude: "" clusterAgent.containerInclude: "" clusterAgent.containers.clusterAgent.securityContext: spec.override.clusterAgent.containers.clusterAgent.securityContext @@ -188,7 +192,11 @@ clusterAgent.containers.clusterAgent.securityContext.allowPrivilegeEscalation: s clusterAgent.containers.clusterAgent.securityContext.readOnlyRootFilesystem: spec.override.clusterAgent.containers.clusterAgent.securityContext.readOnlyRootFilesystem clusterAgent.containers.initContainer.securityContext: spec.override.clusterAgent.containers.init-config.securityContext clusterAgent.createPodDisruptionBudget: "" -clusterAgent.datadog_cluster_yaml: spec.override.clusterAgent.customConfigurations.datadog-cluster.yaml.configData +clusterAgent.datadog_cluster_yaml: + mapFunc: mapCustomConfigFile + newPath: spec.override.clusterAgent.customConfigurations + args: + - fileName: datadog-cluster.yaml clusterAgent.deploymentAnnotations: spec.override.clusterAgent.annotations clusterAgent.dnsConfig: spec.override.clusterAgent.dnsConfig clusterAgent.enabled: "" @@ -484,10 +492,7 @@ datadog.orchestratorExplorer.customResources: spec.features.orchestratorExplorer datadog.orchestratorExplorer.enabled: spec.features.orchestratorExplorer.enabled datadog.originDetectionUnified.enabled: spec.global.originDetectionUnified.enabled datadog.osReleasePath: "" -datadog.otelCollector.config: -- spec.features.otelCollector.conf.configMap.items -- spec.features.otelCollector.conf.configMap.name -- spec.features.otelCollector.conf.configData +datadog.otelCollector.config: spec.features.otelCollector.conf.configData datadog.otelCollector.configMap.items: spec.features.otelCollector.conf.configMap.items datadog.otelCollector.configMap.key: "" datadog.otelCollector.configMap.name: spec.features.otelCollector.conf.configMap.name diff --git a/cmd/yaml-mapper/mapper/testdata/dda_no_errors.yaml b/cmd/yaml-mapper/mapper/testdata/dda_no_errors.yaml index 2b3d06e6bb..4ca3478ce6 100644 --- a/cmd/yaml-mapper/mapper/testdata/dda_no_errors.yaml +++ b/cmd/yaml-mapper/mapper/testdata/dda_no_errors.yaml @@ -29,11 +29,28 @@ spec: site: datadoghq.com override: clusterAgent: + customConfigurations: + datadog-cluster.yaml: + configData: | + cluster_checks: + enabled: true + log_level: debug + extraConfd: + configDataMap: + kubernetes_state.yaml: |- + ad_identifiers: + - kube-state-metrics + mysql.yaml: 'cluster_check: true' image: name: cluster-agent tag: 7.50.0 replicas: 2 nodeAgent: + customConfigurations: + datadog.yaml: + configData: | + jmx_use_container_support: true + log_level: debug image: name: agent tag: 7.50.0 diff --git a/cmd/yaml-mapper/mapper/testdata/values_no_errors.yaml b/cmd/yaml-mapper/mapper/testdata/values_no_errors.yaml index 6b8869c9b3..60ce2c4a79 100644 --- a/cmd/yaml-mapper/mapper/testdata/values_no_errors.yaml +++ b/cmd/yaml-mapper/mapper/testdata/values_no_errors.yaml @@ -27,9 +27,22 @@ agents: tolerations: - key: "node-role.kubernetes.io/master" effect: "NoSchedule" + customAgentConfig: + log_level: "debug" + jmx_use_container_support: true clusterAgent: replicas: 2 image: name: "cluster-agent" tag: "7.50.0" + confd: + mysql.yaml: |- + cluster_check: true + kubernetes_state.yaml: |- + ad_identifiers: + - kube-state-metrics + datadog_cluster_yaml: + log_level: "debug" + cluster_checks: + enabled: true