diff --git a/cli/options.go b/cli/options.go index b03942f0..34fc848d 100644 --- a/cli/options.go +++ b/cli/options.go @@ -629,6 +629,7 @@ func (o *ProjectOptions) prepare(ctx context.Context) (*types.ConfigDetails, err } // ProjectFromOptions load a compose project based on command line options +// // Deprecated: use ProjectOptions.LoadProject or ProjectOptions.LoadModel func ProjectFromOptions(ctx context.Context, options *ProjectOptions) (*types.Project, error) { return options.LoadProject(ctx) diff --git a/loader/mapstructure.go b/loader/mapstructure.go index e5b902ab..3082bd2e 100644 --- a/loader/mapstructure.go +++ b/loader/mapstructure.go @@ -44,7 +44,7 @@ func decoderHook(from reflect.Value, to reflect.Value) (interface{}, error) { } } // If it is nil and a pointer, create and assign the target value first - if to.Type().Kind() == reflect.Ptr && to.IsNil() { + if to.Type().Kind() == reflect.Pointer && to.IsNil() { to.Set(reflect.New(to.Type().Elem())) u = to.Interface().(decoder) } diff --git a/loader/tests/labels_test.go b/loader/tests/labels_test.go index 6935af76..756d7eb2 100644 --- a/loader/tests/labels_test.go +++ b/loader/tests/labels_test.go @@ -92,3 +92,88 @@ jobs: assert.DeepEqual(t, p.Services["foo"].Labels, expected) assert.DeepEqual(t, p.Jobs["foo"].Labels, expected) } + +// In a KEY=VALUE list the last entry for a key takes effect, so an override +// file repeating a base entry after another value for the same key must still +// win, while an entry repeated as is stays a single one. It holds for the +// labels of every resource, not only of services. +func TestLabelsLastEntryWinsAcrossFiles(t *testing.T) { + const labels = ` + labels: + - mode=release + - keep=1 +` + overridden := ` + labels: + - mode=debug + - mode=release + - keep=1 +` + base := ` +name: test +services: + foo: + image: alpine +` + labels + ` +jobs: + foo: + image: alpine + triggers: + manual: true +` + labels + ` +networks: + foo: +` + labels + ` +volumes: + foo: +` + labels + ` +secrets: + foo: + environment: FOO +` + labels + ` +configs: + foo: + environment: FOO +` + labels + override := ` +services: + foo:` + overridden + ` +jobs: + foo:` + overridden + ` +networks: + foo:` + overridden + ` +volumes: + foo:` + overridden + ` +secrets: + foo:` + overridden + ` +configs: + foo:` + overridden + loadFilesAs(t, []string{base, override}, ` +name: test +services: + foo: + image: alpine + labels: {mode: release, keep: "1"} +jobs: + foo: + name: foo + image: alpine + triggers: + manual: true + labels: {mode: release, keep: "1"} +networks: + foo: + labels: {mode: release, keep: "1"} +volumes: + foo: + labels: {mode: release, keep: "1"} +secrets: + foo: + environment: FOO + labels: {mode: release, keep: "1"} +configs: + foo: + environment: FOO + labels: {mode: release, keep: "1"} +`) +} diff --git a/override/merge.go b/override/merge.go index 69ebc57d..60806b3e 100644 --- a/override/merge.go +++ b/override/merge.go @@ -41,15 +41,17 @@ var mergeSpecials = map[tree.Path]merger{} func init() { mergeSpecials["networks.*.ipam.config"] = mergeIPAMConfig - mergeSpecials["networks.*.labels"] = mergeToSequence - mergeSpecials["volumes.*.labels"] = mergeToSequence + mergeSpecials["networks.*.labels"] = mergeKeyValueSequence + mergeSpecials["volumes.*.labels"] = mergeKeyValueSequence + mergeSpecials["secrets.*.labels"] = mergeKeyValueSequence + mergeSpecials["configs.*.labels"] = mergeKeyValueSequence for _, prefix := range []tree.Path{"services", "jobs"} { - mergeSpecials[prefix+".*.annotations"] = mergeToSequence + mergeSpecials[prefix+".*.annotations"] = mergeKeyValueSequence mergeSpecials[prefix+".*.build"] = mergeBuild - mergeSpecials[prefix+".*.build.args"] = mergeToSequence - mergeSpecials[prefix+".*.build.additional_contexts"] = mergeToSequence + mergeSpecials[prefix+".*.build.args"] = mergeKeyValueSequence + mergeSpecials[prefix+".*.build.additional_contexts"] = mergeKeyValueSequence mergeSpecials[prefix+".*.build.extra_hosts"] = mergeToSequence - mergeSpecials[prefix+".*.build.labels"] = mergeToSequence + mergeSpecials[prefix+".*.build.labels"] = mergeKeyValueSequence mergeSpecials[prefix+".*.command"] = override mergeSpecials[prefix+".*.depends_on"] = mergeDependsOn mergeSpecials[prefix+".*.dns"] = mergeToSequence @@ -58,19 +60,19 @@ func init() { mergeSpecials[prefix+".*.entrypoint"] = override mergeSpecials[prefix+".*.env_file"] = mergeToSequence mergeSpecials[prefix+".*.label_file"] = mergeToSequence - mergeSpecials[prefix+".*.environment"] = mergeToSequence + mergeSpecials[prefix+".*.environment"] = mergeKeyValueSequence mergeSpecials[prefix+".*.extra_hosts"] = mergeToSequence mergeSpecials[prefix+".*.healthcheck.test"] = override - mergeSpecials[prefix+".*.labels"] = mergeToSequence - mergeSpecials[prefix+".*.volumes.*.volume.labels"] = mergeToSequence + mergeSpecials[prefix+".*.labels"] = mergeKeyValueSequence + mergeSpecials[prefix+".*.volumes.*.volume.labels"] = mergeKeyValueSequence mergeSpecials[prefix+".*.logging"] = mergeLogging mergeSpecials[prefix+".*.models"] = mergeModels mergeSpecials[prefix+".*.networks"] = mergeNetworks - mergeSpecials[prefix+".*.sysctls"] = mergeToSequence + mergeSpecials[prefix+".*.sysctls"] = mergeKeyValueSequence mergeSpecials[prefix+".*.tmpfs"] = mergeToSequence mergeSpecials[prefix+".*.ulimits.*"] = mergeUlimit } - mergeSpecials["services.*.deploy.labels"] = mergeToSequence + mergeSpecials["services.*.deploy.labels"] = mergeKeyValueSequence } // MergeYaml merges map[string]any yaml trees handling special rules @@ -172,11 +174,11 @@ func mergeNetworks(config any, other any, path tree.Path) (any, error) { } func mergeAsMapping(config, other any, defaults map[string]any, path tree.Path) (any, error) { - right, err := convertIntoMapping(config, defaults, path) + right, err := convertIntoMapping(nullAsEmpty(config), defaults, path) if err != nil { return nil, err } - left, err := convertIntoMapping(other, defaults, path) + left, err := convertIntoMapping(nullAsEmpty(other), defaults, path) if err != nil { return nil, err } @@ -207,6 +209,17 @@ func appendWithoutDuplicates(base []any, override []any) []any { return merged } +// mergeKeyValueSequence merges KEY=VALUE lists by key: an override entry +// replaces the base entry of the same key in place, new keys are appended. The +// last entry for a key takes effect, so an override repeating a base entry +// after another value for the same key (`[MODE=debug, MODE=release]` over +// `[MODE=release]`) must end on the repeated one, not drop it as a duplicate. +// Keys repeated in the base collapse the same way, so that the override never +// leaves two identical entries behind. +func mergeKeyValueSequence(config any, other any, path tree.Path) (any, error) { + return uniqueEntries(slices.Concat(convertIntoSequence(config), convertIntoSequence(other)), keyValueIndexer, path) +} + func convertIntoSequence(value any) []any { switch v := value.(type) { case map[string]any: @@ -295,6 +308,15 @@ func mergeIPAMConfig(config any, other any, path tree.Path) (any, error) { return ipamConfigs, nil } +// nullAsEmpty reads a null value as an empty mapping. An empty one rather than +// nil, as merging into a nil map panics. +func nullAsEmpty(value any) any { + if value == nil { + return map[string]any{} + } + return value +} + func convertIntoMapping(a any, defaultValue map[string]any, path tree.Path) (map[string]any, error) { switch v := a.(type) { case map[string]any: diff --git a/override/merge_dedup_test.go b/override/merge_dedup_test.go index d1e4aff1..ed0558ec 100644 --- a/override/merge_dedup_test.go +++ b/override/merge_dedup_test.go @@ -128,3 +128,142 @@ services: assert.NilError(t, err) assert.DeepEqual(t, once, twice) } + +// In a KEY=VALUE list the last entry for a key takes effect, so an override +// that repeats a base entry after another value for the same key must still +// win: the repeated entry is not a duplicate to drop, it is the final word. +// Regression of v2.15.0, reported by docker/buildx#4117. +func Test_mergeYamlKeyValueSequenceLastEntryWins(t *testing.T) { + tests := []struct { + name string + path string + }{ + {name: "environment", path: "environment"}, + {name: "labels", path: "labels"}, + {name: "build args", path: "build:\n args"}, + {name: "build labels", path: "build:\n labels"}, + {name: "annotations", path: "annotations"}, + {name: "sysctls", path: "sysctls"}, + {name: "build additional contexts", path: "build:\n additional_contexts"}, + {name: "deploy labels", path: "deploy:\n labels"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assertMergeYaml(t, ` +services: + test: + image: foo + `+tt.path+`: + - MODE=release +`, ` +services: + test: + `+tt.path+`: + - MODE=debug + - MODE=release +`, ` +services: + test: + image: foo + `+tt.path+`: + - MODE=release +`) + }) + } +} + +// A file may repeat a key with different values (`[MODE=debug, MODE=release]` +// is a valid list, the last entry takes effect). Merging must compact those +// repeats before applying the override, otherwise the override replaces only +// the last one and leaves two identical entries, which the schema rejects. +// Jobs are used because EnforceUnicity does not cover them and would hide it. +func Test_mergeYamlKeyValueSequenceCompactsRepeatedBaseKeys(t *testing.T) { + assertMergeYaml(t, ` +jobs: + test: + image: foo + labels: + - MODE=debug + - MODE=release +`, ` +jobs: + test: + labels: + - MODE=debug +`, ` +jobs: + test: + image: foo + labels: + - MODE=debug +`) +} + +// Merging two empty KEY=VALUE lists yields an empty list, not a nil one: a nil +// list is not a valid `labels` value for the schema. Secrets are used because +// EnforceUnicity does not cover them and would rebuild the list, hiding it. +func Test_mergeYamlKeyValueSequenceOfEmptyLists(t *testing.T) { + tests := []struct { + name string + base string + override string + }{ + {name: "empty list over empty list", base: "[]", override: "[]"}, + {name: "empty mapping over empty mapping", base: "{}", override: "{}"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assertMergeYaml(t, ` +secrets: + test: + labels: `+tt.base+` +`, ` +secrets: + test: + labels: `+tt.override+` +`, ` +secrets: + test: + labels: [] +`) + }) + } +} + +// With interpolation disabled the merge sees raw strings, and keys are read up +// to the first `=` as EnforceUnicity does: entries with the same text collapse, +// at the position of the first one. Entries that could only resolve to the same +// key once interpolated are not reconciled and stay side by side, so which of +// them wins after interpolation depends on their position, as it already does +// for services. Jobs are used because EnforceUnicity does not cover them. +func Test_mergeYamlKeyValueSequenceOfUninterpolatedEntries(t *testing.T) { + tests := []struct { + name string + base string + override string + want string + }{ + {name: "same variable key", base: `"${K}=1"`, override: `"${K}=2"`, want: `"${K}=2"`}, + {name: "default containing an equal sign", base: `"${A:-x=1}"`, override: `"${A:-x=2}"`, want: `"${A:-x=2}"`}, + {name: "different variable keys", base: `"${K1}=1"`, override: `"${K2}=2"`, want: `"${K1}=1", "${K2}=2"`}, + {name: "entry that is a bare variable", base: `"${ENTRY}"`, override: `"K=2"`, want: `"${ENTRY}", "K=2"`}, + {name: "replaced at the position of the first entry", base: `"${K1}=1", "${K2}=2"`, override: `"${K1}=3"`, want: `"${K1}=3", "${K2}=2"`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assertMergeYaml(t, ` +jobs: + test: + labels: [`+tt.base+`] +`, ` +jobs: + test: + labels: [`+tt.override+`] +`, ` +jobs: + test: + labels: [`+tt.want+`] +`) + }) + } +} diff --git a/override/merge_null_test.go b/override/merge_null_test.go new file mode 100644 index 00000000..2d9c2abb --- /dev/null +++ b/override/merge_null_test.go @@ -0,0 +1,111 @@ +/* + Copyright 2020 The Compose Specification Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package override + +import ( + "testing" + + "gotest.tools/v3/assert" +) + +// An override setting a list-or-mapping attribute to null is a valid document +// and must merge without error, leaving the base value in place: consumers +// such as buildx bake only read build attributes and must keep loading files +// whose runtime attributes are nulled by an override. +// Regression of v2.15.0, reported by docker/buildx#4117. +func Test_mergeYamlNullOverrideOfMappingLikeAttribute(t *testing.T) { + tests := []struct { + attribute string + want string + }{ + {attribute: "networks", want: "runtime: null"}, + {attribute: "models", want: "runtime: null"}, + {attribute: "depends_on", want: "runtime: {condition: service_started, required: true}"}, + } + for _, tt := range tests { + t.Run(tt.attribute, func(t *testing.T) { + assertMergeYaml(t, ` +services: + test: + image: foo + `+tt.attribute+`: [runtime] +`, ` +services: + test: + `+tt.attribute+`: null +`, ` +services: + test: + image: foo + `+tt.attribute+`: + `+tt.want+` +`) + }) + } +} + +// A base setting the attribute to null and an override providing entries is +// equally valid: the override's entries are the result. +func Test_mergeYamlNullBaseWithOverrideOfMappingLikeAttribute(t *testing.T) { + tests := []struct { + attribute string + want string + }{ + {attribute: "networks", want: "runtime: null"}, + {attribute: "models", want: "runtime: null"}, + {attribute: "depends_on", want: "runtime: {condition: service_started, required: true}"}, + } + for _, tt := range tests { + t.Run(tt.attribute, func(t *testing.T) { + assertMergeYaml(t, ` +services: + test: + image: foo + `+tt.attribute+`: null +`, ` +services: + test: + `+tt.attribute+`: [runtime] +`, ` +services: + test: + image: foo + `+tt.attribute+`: + `+tt.want+` +`) + }) + } +} + +// A null item of ipam.config is invalid and must be rejected, not read as an +// empty item that would silently drop the subnet of the base: null only stands +// for an empty value on networks, models and depends_on. +func Test_mergeYamlNullIPAMConfigItem(t *testing.T) { + _, err := Merge(unmarshal(t, ` +networks: + test: + ipam: + config: + - subnet: 10.0.0.0/24 +`), unmarshal(t, ` +networks: + test: + ipam: + config: [null] +`)) + assert.ErrorContains(t, err, "networks.test.ipam.config: cannot convert into a mapping") +} diff --git a/override/uncity.go b/override/uncity.go index 3b0c63d3..b71c9e95 100644 --- a/override/uncity.go +++ b/override/uncity.go @@ -86,27 +86,33 @@ func enforceUnicity(value any, p tree.Path) (any, error) { case []any: for pattern, indexer := range unique { if p.Matches(pattern) { - seq := []any{} - keys := map[string]int{} - for i, entry := range v { - key, err := indexer(entry, p.Next(fmt.Sprintf("[%d]", i))) - if err != nil { - return nil, err - } - if j, ok := keys[key]; ok { - seq[j] = entry - } else { - seq = append(seq, entry) - keys[key] = len(seq) - 1 - } - } - return seq, nil + return uniqueEntries(v, indexer, p) } } } return value, nil } +// uniqueEntries keeps one entry per key: the last entry for a key takes effect, +// at the position of the first one. +func uniqueEntries(entries []any, indexer indexer, p tree.Path) ([]any, error) { + seq := []any{} + keys := map[string]int{} + for i, entry := range entries { + key, err := indexer(entry, p.Next(fmt.Sprintf("[%d]", i))) + if err != nil { + return nil, err + } + if j, ok := keys[key]; ok { + seq[j] = entry + } else { + seq = append(seq, entry) + keys[key] = len(seq) - 1 + } + } + return seq, nil +} + func keyValueIndexer(v any, p tree.Path) (string, error) { switch value := v.(type) { case string: diff --git a/tree/matcher_test.go b/tree/matcher_test.go index 3f7e0086..e7d24174 100644 --- a/tree/matcher_test.go +++ b/tree/matcher_test.go @@ -33,9 +33,9 @@ func TestMatcherMatches(t *testing.T) { assert.Check(t, m.Matches("services.web.ports.[].target")) assert.Check(t, m.Matches("services.web.environment.DEBUG")) - assert.Check(t, !(m.Matches("services.web.command")), "undeclared sibling") - assert.Check(t, !(m.Matches("services.web.image.tag")), "matching is exact, not subtree") - assert.Check(t, !(m.Matches("services.web")), "ancestor of a pattern is not a match") + assert.Check(t, !m.Matches("services.web.command"), "undeclared sibling") + assert.Check(t, !m.Matches("services.web.image.tag"), "matching is exact, not subtree") + assert.Check(t, !m.Matches("services.web"), "ancestor of a pattern is not a match") } func TestMatcherMayContain(t *testing.T) { @@ -45,6 +45,6 @@ func TestMatcherMayContain(t *testing.T) { assert.Check(t, m.MayContain("services.web.deploy")) assert.Check(t, m.MayContain("services.web.deploy.update_config")) - assert.Check(t, !(m.MayContain("services.web.deploy.update_config.delay")), "a full match is not a strict ancestor") - assert.Check(t, !(m.MayContain("services.web.build"))) + assert.Check(t, !m.MayContain("services.web.deploy.update_config.delay"), "a full match is not a strict ancestor") + assert.Check(t, !m.MayContain("services.web.build")) }