From 49a3de15adde576cb100e77026c93f1b9e9b3892 Mon Sep 17 00:00:00 2001 From: Peter Engelbert Date: Thu, 9 Jan 2025 17:10:01 -0500 Subject: [PATCH 01/11] Update PostInstall symlink spec Allow for multiple links to point to the same target. Signed-off-by: Peter Engelbert --- frontend/azlinux/handle_container.go | 2 +- frontend/deb/distro/container.go | 3 +- frontend/windows/handle_container.go | 37 +++++-- helpers.go | 41 ++++++- helpers_test.go | 160 +++++++++++++++++++++++++++ load.go | 36 ++++++ spec.go | 3 +- target.go | 6 + test/azlinux_test.go | 7 +- test/windows_test.go | 13 ++- 10 files changed, 286 insertions(+), 22 deletions(-) create mode 100644 helpers_test.go diff --git a/frontend/azlinux/handle_container.go b/frontend/azlinux/handle_container.go index d6f6bf2ab..005d46c35 100644 --- a/frontend/azlinux/handle_container.go +++ b/frontend/azlinux/handle_container.go @@ -79,7 +79,7 @@ func specToContainerLLB(w worker, spec *dalec.Spec, targetKey string, rpmDir llb dalec.WithConstraints(opts...), ).AddMount(workPath, rootfs) - if post := spec.GetImagePost(targetKey); post != nil && len(post.Symlinks) > 0 { + if post := spec.GetImagePost(targetKey); post != nil { rootfs = builderImg. Run(dalec.WithConstraints(opts...), dalec.InstallPostSymlinks(post, workPath)). AddMount(workPath, rootfs) diff --git a/frontend/deb/distro/container.go b/frontend/deb/distro/container.go index b332f33d6..1bd0445b6 100644 --- a/frontend/deb/distro/container.go +++ b/frontend/deb/distro/container.go @@ -72,7 +72,8 @@ func (c *Config) createSymlinks(worker llb.State, spec *dalec.Spec, targetKey st return in } - if len(post.Symlinks) == 0 { + symlinks := post.GetSymlinks() + if len(symlinks) == 0 { return in } diff --git a/frontend/windows/handle_container.go b/frontend/windows/handle_container.go index 88d56a204..60842150b 100644 --- a/frontend/windows/handle_container.go +++ b/frontend/windows/handle_container.go @@ -198,18 +198,39 @@ func copySymlinks(post *dalec.PostInstall) llb.StateOption { return s } - lm := post.Symlinks - if len(lm) == 0 { + symlinks := post.GetSymlinks() + if len(symlinks) == 0 { return s } - keys := dalec.SortMapKeys(lm) - for _, srcPath := range keys { - l := lm[srcPath] - dstPath := l.Path - s = s.File(llb.Mkdir(path.Dir(dstPath), 0755, llb.WithParents(true))) - s = s.File(llb.Copy(s, srcPath, dstPath)) + + for _, sl := range symlinks { + s = s.File(llb.Mkdir(path.Dir(sl.Dest), 0755, llb.WithParents(true))) + s = s.File(llb.Copy(s, sl.Source, sl.Dest)) } return s } } + +func getTargetPlatform(bc *dockerui.Client) (ocispecs.Platform, error) { + platform := defaultPlatform + + switch len(bc.TargetPlatforms) { + case 0: + case 1: + platform = bc.TargetPlatforms[0] + default: + return ocispecs.Platform{}, + fmt.Errorf("multiple target supplied for build: %v. note: only amd64 is supported for windows outputs", bc.TargetPlatforms) + } + + return platform, nil +} + +func getBaseOutputImage(spec *dalec.Spec, target, defaultBase string) string { + baseRef := defaultBase + if spec.Targets[target].Image != nil && spec.Targets[target].Image.Base != "" { + baseRef = spec.Targets[target].Image.Base + } + return baseRef +} diff --git a/helpers.go b/helpers.go index 1891beb15..1d60fe992 100644 --- a/helpers.go +++ b/helpers.go @@ -357,6 +357,38 @@ func (s *Spec) GetImagePost(target string) *PostInstall { return nil } +func (p *PostInstall) GetSymlinks() []ArtifactSymlinkConfig { + if p == nil { + return []ArtifactSymlinkConfig{} + } + + ret := make([]ArtifactSymlinkConfig, 0, len(p.Symlinks)<<1) + keys := SortMapKeys(p.Symlinks) + for _, oldpath := range keys { + cfg := p.Symlinks[oldpath] + + if cfg.Path != "" { + ret = append(ret, ArtifactSymlinkConfig{ + Source: oldpath, + Dest: cfg.Path, + }) + + continue + } + + paths := slices.Clone(cfg.Paths) + sort.Strings(paths) + for _, newpath := range paths { + ret = append(ret, ArtifactSymlinkConfig{ + Source: oldpath, + Dest: newpath, + }) + } + } + + return ret +} + // ShArgs returns a RunOption that runs the given command in a shell. func ShArgs(args string) llb.RunOption { return llb.Args(append([]string{"sh", "-c"}, args)) @@ -374,7 +406,8 @@ func InstallPostSymlinks(post *PostInstall, rootfsPath string) llb.RunOption { return } - if len(post.Symlinks) == 0 { + symlinks := post.GetSymlinks() + if len(symlinks) == 0 { return } @@ -383,9 +416,9 @@ func InstallPostSymlinks(post *PostInstall, rootfsPath string) llb.RunOption { buf := bytes.NewBuffer(nil) buf.WriteString("set -ex\n") - for src, tgt := range post.Symlinks { - fmt.Fprintf(buf, "mkdir -p %q\n", filepath.Join(rootfsPath, filepath.Dir(tgt.Path))) - fmt.Fprintf(buf, "ln -s %q %q\n", src, filepath.Join(rootfsPath, tgt.Path)) + for _, sl := range symlinks { + fmt.Fprintf(buf, "mkdir -p %q\n", filepath.Join(rootfsPath, filepath.Dir(sl.Dest))) + fmt.Fprintf(buf, "ln -s %q %q\n", sl.Source, filepath.Join(rootfsPath, sl.Dest)) } const name = "tmp.dalec.symlink.sh" diff --git a/helpers_test.go b/helpers_test.go new file mode 100644 index 000000000..faed50d99 --- /dev/null +++ b/helpers_test.go @@ -0,0 +1,160 @@ +package dalec + +import "testing" + +type tableEntry struct { + PostInstall + shouldFailVaildation bool + desc string + expectedOutput []ArtifactSymlinkConfig + expectedNumLinks int +} + +func TestGetSymlinks(t *testing.T) { + table := []tableEntry{ + { + desc: "invalid SymlinkTarget should fail validation: empty target", + PostInstall: PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": {}, + }, + }, + shouldFailVaildation: true, + }, + { + desc: "invalid SymlinkTarget should fail validation: empty key, valid target(paths)", + PostInstall: PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "": { + Paths: []string{"/newpath_z", "/newpath_a"}, + }, + }, + }, + shouldFailVaildation: true, + }, + { + desc: "invalid SymlinkTarget should fail validation: empty key: valid target(path)", + PostInstall: PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "": { + Path: "/newpath_z", + }, + }, + }, + shouldFailVaildation: true, + }, + { + desc: "should be able to create multiple symlinks to the same target, with the correct ordering", + PostInstall: PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Paths: []string{"/newpath_z", "/newpath_a"}, + }, + }, + }, + expectedOutput: []ArtifactSymlinkConfig{ + { + Source: "oldpath", + Dest: "/newpath_a", + }, + { + Source: "oldpath", + Dest: "/newpath_z", + }, + }, + expectedNumLinks: 2, + }, + { + desc: "combine Path and Paths with correct ordering", + PostInstall: PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath2": { + Paths: []string{"/newpath_z", "/newpath_a"}, + }, + "oldpath1": { + Path: "/newpath1", + }, + }, + }, + expectedOutput: []ArtifactSymlinkConfig{ + { + Source: "oldpath1", + Dest: "/newpath1", + }, + { + Source: "oldpath2", + Dest: "/newpath_a", + }, + { + Source: "oldpath2", + Dest: "/newpath_z", + }, + }, + expectedNumLinks: 3, + }, + { + desc: "just Path", + PostInstall: PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath3": { + Path: "/newpath3", + }, + "oldpath2": { + Path: "/newpath2", + }, + "oldpath1": { + Path: "/newpath1", + }, + }, + }, + expectedOutput: []ArtifactSymlinkConfig{ + { + Source: "oldpath1", + Dest: "/newpath1", + }, + { + Source: "oldpath2", + Dest: "/newpath2", + }, + { + Source: "oldpath3", + Dest: "/newpath3", + }, + }, + expectedNumLinks: 3, + }, + } + + for _, test := range table { + if passed := t.Run(test.desc, func(t *testing.T) { + p := test.PostInstall + + if err := p.validate(); err != nil { + if test.shouldFailVaildation { + return + } + + t.Log("input failed validation") + t.Fail() + } + + actualSymlinks := p.GetSymlinks() + if len(actualSymlinks) != test.expectedNumLinks { + t.Logf("expected %d links, but there were %d", test.expectedNumLinks, len(actualSymlinks)) + t.Fail() + } + + for i := range actualSymlinks { + actual := actualSymlinks[i] + expected := test.expectedOutput[i] + + if actual != expected { + t.Logf("expected:\n%#v\n================found:\n%#v\n", expected, actual) + t.Fail() + } + } + }); !passed { + t.Fail() + } + } +} diff --git a/load.go b/load.go index 2be576713..c42cc8ac0 100644 --- a/load.go +++ b/load.go @@ -386,8 +386,15 @@ func (s Spec) Validate() error { } } + if s.Image != nil { + if err := s.Image.Post.validate(); err != nil { + errs = append(errs, err) + } + } + return goerrors.Join(errs...) } + func validatePatch(patch PatchSpec, patchSrc Source) error { if SourceIsDir(patchSrc) { // Patch sources that use directory-backed sources require a subpath in the @@ -452,3 +459,32 @@ func (b *ArtifactBuild) processBuildArgs(lex *shell.Lex, args map[string]string, return goerrors.Join(errs...) } + +func (p *PostInstall) validate() error { + if p == nil { + return nil + } + + var errs []error + symlinks := p.GetSymlinks() + if err := validateSymlinks(symlinks); err != nil { + errs = append(errs, err) + } + + return goerrors.Join(errs...) +} + +func validateSymlinks(symlinks []ArtifactSymlinkConfig) error { + var errs []error + for _, l := range symlinks { + if l.Dest == "" { + errs = append(errs, fmt.Errorf("invalid symlink destination")) + } + + if l.Source == "" { + errs = append(errs, fmt.Errorf("invalid symlink source")) + } + } + + return goerrors.Join(errs...) +} diff --git a/spec.go b/spec.go index da4251fca..895796c5e 100644 --- a/spec.go +++ b/spec.go @@ -130,7 +130,8 @@ type PostInstall struct { // SymlinkTarget specifies the properties of a symlink type SymlinkTarget struct { // Path is the path where the symlink should be placed - Path string `yaml:"path" json:"path" jsonschema:"required"` + Path string `yaml:"path" json:"path" jsonschema:"oneof_required=path"` + Paths []string `yaml:"paths" json:"paths" jsonschema:"oneof_required=paths"` } type SourceDockerImage struct { diff --git a/target.go b/target.go index bc4b7df30..f6084b9ce 100644 --- a/target.go +++ b/target.go @@ -46,6 +46,12 @@ func (t *Target) validate() error { } } + if t.Image != nil { + if err := t.Image.Post.validate(); err != nil { + errs = append(errs, errors.Wrap(err, "postinsall")) + } + } + return goerrors.Join(errs...) } diff --git a/test/azlinux_test.go b/test/azlinux_test.go index 8d7b55faf..a878b5c94 100644 --- a/test/azlinux_test.go +++ b/test/azlinux_test.go @@ -466,7 +466,7 @@ echo "$BAR" > bar.txt Post: &dalec.PostInstall{ Symlinks: map[string]dalec.SymlinkTarget{ "/usr/bin/src1": {Path: "/src1"}, - "/usr/bin/src3": {Path: "/non/existing/dir/src3"}, + "/usr/bin/src3": {Paths: []string{"/non/existing/dir/src3", "/non/existing/dir2/src3"}}, }, }, }, @@ -588,8 +588,13 @@ echo "$BAR" > bar.txt Steps: []dalec.TestStep{ {Command: "/bin/bash -c 'test -L /src1'"}, {Command: "/bin/bash -c 'test \"$(readlink /src1)\" = \"/usr/bin/src1\"'"}, + {Command: "/bin/bash -c 'test -L /non/existing/dir/src3'"}, + {Command: "/bin/bash -c 'test \"$(readlink /non/existing/dir/src3)\" = \"/usr/bin/src3\"'"}, + {Command: "/bin/bash -c 'test -L /non/existing/dir2/src3'"}, + {Command: "/bin/bash -c 'test \"$(readlink /non/existing/dir2/src3)\" = \"/usr/bin/src3\"'"}, {Command: "/src1", Stdout: dalec.CheckOutput{Equals: "hello world\n"}, Stderr: dalec.CheckOutput{Empty: true}}, {Command: "/non/existing/dir/src3", Stdout: dalec.CheckOutput{Equals: "goodbye\n"}, Stderr: dalec.CheckOutput{Empty: true}}, + {Command: "/non/existing/dir2/src3", Stdout: dalec.CheckOutput{Equals: "goodbye\n"}, Stderr: dalec.CheckOutput{Empty: true}}, }, }, { diff --git a/test/windows_test.go b/test/windows_test.go index 33fd89a62..3c3f79578 100644 --- a/test/windows_test.go +++ b/test/windows_test.go @@ -306,7 +306,7 @@ echo "$BAR" > bar.txt Post: &dalec.PostInstall{ Symlinks: map[string]dalec.SymlinkTarget{ "/Windows/System32/src1": {Path: "/src1"}, - "/Windows/System32/src3": {Path: "/non/existing/dir/src3"}, + "/Windows/System32/src3": {Paths: []string{"/non/existing/dir/src3", "/non/existing/dir2/src3"}}, }, }, }, @@ -328,19 +328,20 @@ echo "$BAR" > bar.txt validateSymlinks := func(ctx context.Context, t *testing.T, ref gwclient.Reference, spec dalec.Spec) { post := spec.GetImagePost("windowscross") - for srcPath, l := range post.Symlinks { + symlinks := post.GetSymlinks() + for _, sl := range symlinks { b1, err := ref.ReadFile(ctx, gwclient.ReadRequest{ - Filename: srcPath, + Filename: sl.Source, }) if err != nil { - t.Fatalf("couldn't find Windows \"symlink\" target %q: %v", srcPath, err) + t.Fatalf("couldn't find Windows \"symlink\" target %q: %v", sl.Source, err) } b2, err := ref.ReadFile(ctx, gwclient.ReadRequest{ - Filename: l.Path, + Filename: sl.Dest, }) if err != nil { - t.Fatalf("couldn't find Windows \"symlink\" at destination %q: %v", l.Path, err) + t.Fatalf("couldn't find Windows \"symlink\" at destination %q: %v", sl.Dest, err) } if len(b1) != len(b2) { From 858111ac52113d5fee496ca2fc31424b976ce3c5 Mon Sep 17 00:00:00 2001 From: Peter Engelbert Date: Thu, 9 Jan 2025 17:37:09 -0500 Subject: [PATCH 02/11] Add tests, refine validation, add optimizations Signed-off-by: Peter Engelbert --- helpers_test.go | 38 +++++++++++++++++++++++++++++++++++--- load.go | 31 +++++++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/helpers_test.go b/helpers_test.go index faed50d99..dbef46d64 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -43,6 +43,32 @@ func TestGetSymlinks(t *testing.T) { }, shouldFailVaildation: true, }, + { + desc: "invalid SymlinkTarget should fail validation: all symlink 'newpaths' should be unique", + PostInstall: PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "perfectly_valid": { + Path: "/also_valid", + }, + "also_perfectly_valid": { + Paths: []string{"/also_valid"}, + }, + }, + }, + shouldFailVaildation: true, + }, + { + desc: "invalid SymlinkTarget should fail validation: path and paths are mutually exclusive", + PostInstall: PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "perfectly_valid": { + Path: "/also_valid", + Paths: []string{"/also_valid_too", "also_valid_too,_also"}, + }, + }, + }, + shouldFailVaildation: true, + }, { desc: "should be able to create multiple symlinks to the same target, with the correct ordering", PostInstall: PostInstall{ @@ -130,12 +156,18 @@ func TestGetSymlinks(t *testing.T) { p := test.PostInstall if err := p.validate(); err != nil { - if test.shouldFailVaildation { - return + if !test.shouldFailVaildation { + t.Logf("input failed validation: %s", err) + t.Fail() } - t.Log("input failed validation") + return + } + + if test.shouldFailVaildation { // err was nil, but shouldn't have been + t.Logf("input should have failed validation, but succeeded:\n%#v", test.PostInstall) t.Fail() + return } actualSymlinks := p.GetSymlinks() diff --git a/load.go b/load.go index c42cc8ac0..61cb0d5ef 100644 --- a/load.go +++ b/load.go @@ -466,6 +466,19 @@ func (p *PostInstall) validate() error { } var errs []error + for k, sl := range p.Symlinks { + var err error + if k == "" { + err = fmt.Errorf("symlink source is empty") + errs = append(errs, err) + } + + if sl.Path != "" && len(sl.Paths) != 0 || sl.Path == "" && len(sl.Paths) == 0 { + err = fmt.Errorf("'path' and 'paths' fields are mutually exclusive, and at least one is required") + errs = append(errs, err) + } + } + symlinks := p.GetSymlinks() if err := validateSymlinks(symlinks); err != nil { errs = append(errs, err) @@ -476,14 +489,28 @@ func (p *PostInstall) validate() error { func validateSymlinks(symlinks []ArtifactSymlinkConfig) error { var errs []error + dests := make(map[string]string, len(symlinks)<<1) + for _, l := range symlinks { + var err error if l.Dest == "" { - errs = append(errs, fmt.Errorf("invalid symlink destination")) + err = fmt.Errorf("invalid symlink destination") + errs = append(errs, err) } if l.Source == "" { - errs = append(errs, fmt.Errorf("invalid symlink source")) + err = fmt.Errorf("invalid symlink source") + errs = append(errs, err) + } + + if err != nil { + continue + } + + if other_oldpath, ok := dests[l.Dest]; ok { + errs = append(errs, fmt.Errorf("symlink 'newpaths' must be unique: %q points to both %q and %q", l.Dest, l.Source, other_oldpath)) } + dests[l.Dest] = l.Source } return goerrors.Join(errs...) From 7e6cf205de4b85200c065d0fe1ca7d0b04ee3bab Mon Sep 17 00:00:00 2001 From: Peter Engelbert Date: Mon, 13 Jan 2025 11:25:40 -0500 Subject: [PATCH 03/11] Move `Path` entry to `Paths` in SymlinkTarget Upon loading the spec, if `Path` is nonempty, move it to `Paths`. This commit also reworks the tests. Signed-off-by: Peter Engelbert --- frontend/deb/distro/container.go | 3 +- frontend/windows/handle_container.go | 11 +- helpers.go | 43 +----- helpers_test.go | 192 --------------------------- image.go | 63 +++++++-- load.go | 68 ++++------ load_test.go | 119 +++++++++++++++++ target.go | 6 +- test/windows_test.go | 34 ++--- 9 files changed, 233 insertions(+), 306 deletions(-) delete mode 100644 helpers_test.go diff --git a/frontend/deb/distro/container.go b/frontend/deb/distro/container.go index 1bd0445b6..b332f33d6 100644 --- a/frontend/deb/distro/container.go +++ b/frontend/deb/distro/container.go @@ -72,8 +72,7 @@ func (c *Config) createSymlinks(worker llb.State, spec *dalec.Spec, targetKey st return in } - symlinks := post.GetSymlinks() - if len(symlinks) == 0 { + if len(post.Symlinks) == 0 { return in } diff --git a/frontend/windows/handle_container.go b/frontend/windows/handle_container.go index 60842150b..9283ba7fd 100644 --- a/frontend/windows/handle_container.go +++ b/frontend/windows/handle_container.go @@ -198,14 +198,15 @@ func copySymlinks(post *dalec.PostInstall) llb.StateOption { return s } - symlinks := post.GetSymlinks() - if len(symlinks) == 0 { + if len(post.Symlinks) == 0 { return s } - for _, sl := range symlinks { - s = s.File(llb.Mkdir(path.Dir(sl.Dest), 0755, llb.WithParents(true))) - s = s.File(llb.Copy(s, sl.Source, sl.Dest)) + for oldpath, newpaths := range post.Symlinks { + for _, newpath := range newpaths.Paths { + s = s.File(llb.Mkdir(path.Dir(newpath), 0755, llb.WithParents(true))) + s = s.File(llb.Copy(s, oldpath, newpath)) + } } return s diff --git a/helpers.go b/helpers.go index 1d60fe992..bf5582ce2 100644 --- a/helpers.go +++ b/helpers.go @@ -357,38 +357,6 @@ func (s *Spec) GetImagePost(target string) *PostInstall { return nil } -func (p *PostInstall) GetSymlinks() []ArtifactSymlinkConfig { - if p == nil { - return []ArtifactSymlinkConfig{} - } - - ret := make([]ArtifactSymlinkConfig, 0, len(p.Symlinks)<<1) - keys := SortMapKeys(p.Symlinks) - for _, oldpath := range keys { - cfg := p.Symlinks[oldpath] - - if cfg.Path != "" { - ret = append(ret, ArtifactSymlinkConfig{ - Source: oldpath, - Dest: cfg.Path, - }) - - continue - } - - paths := slices.Clone(cfg.Paths) - sort.Strings(paths) - for _, newpath := range paths { - ret = append(ret, ArtifactSymlinkConfig{ - Source: oldpath, - Dest: newpath, - }) - } - } - - return ret -} - // ShArgs returns a RunOption that runs the given command in a shell. func ShArgs(args string) llb.RunOption { return llb.Args(append([]string{"sh", "-c"}, args)) @@ -406,8 +374,7 @@ func InstallPostSymlinks(post *PostInstall, rootfsPath string) llb.RunOption { return } - symlinks := post.GetSymlinks() - if len(symlinks) == 0 { + if len(post.Symlinks) == 0 { return } @@ -416,9 +383,11 @@ func InstallPostSymlinks(post *PostInstall, rootfsPath string) llb.RunOption { buf := bytes.NewBuffer(nil) buf.WriteString("set -ex\n") - for _, sl := range symlinks { - fmt.Fprintf(buf, "mkdir -p %q\n", filepath.Join(rootfsPath, filepath.Dir(sl.Dest))) - fmt.Fprintf(buf, "ln -s %q %q\n", sl.Source, filepath.Join(rootfsPath, sl.Dest)) + for oldpath, newpaths := range post.Symlinks { + for _, newpath := range newpaths.Paths { + fmt.Fprintf(buf, "mkdir -p %q\n", filepath.Join(rootfsPath, filepath.Dir(newpath))) + fmt.Fprintf(buf, "ln -s %q %q\n", oldpath, filepath.Join(rootfsPath, newpath)) + } } const name = "tmp.dalec.symlink.sh" diff --git a/helpers_test.go b/helpers_test.go deleted file mode 100644 index dbef46d64..000000000 --- a/helpers_test.go +++ /dev/null @@ -1,192 +0,0 @@ -package dalec - -import "testing" - -type tableEntry struct { - PostInstall - shouldFailVaildation bool - desc string - expectedOutput []ArtifactSymlinkConfig - expectedNumLinks int -} - -func TestGetSymlinks(t *testing.T) { - table := []tableEntry{ - { - desc: "invalid SymlinkTarget should fail validation: empty target", - PostInstall: PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "oldpath": {}, - }, - }, - shouldFailVaildation: true, - }, - { - desc: "invalid SymlinkTarget should fail validation: empty key, valid target(paths)", - PostInstall: PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "": { - Paths: []string{"/newpath_z", "/newpath_a"}, - }, - }, - }, - shouldFailVaildation: true, - }, - { - desc: "invalid SymlinkTarget should fail validation: empty key: valid target(path)", - PostInstall: PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "": { - Path: "/newpath_z", - }, - }, - }, - shouldFailVaildation: true, - }, - { - desc: "invalid SymlinkTarget should fail validation: all symlink 'newpaths' should be unique", - PostInstall: PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "perfectly_valid": { - Path: "/also_valid", - }, - "also_perfectly_valid": { - Paths: []string{"/also_valid"}, - }, - }, - }, - shouldFailVaildation: true, - }, - { - desc: "invalid SymlinkTarget should fail validation: path and paths are mutually exclusive", - PostInstall: PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "perfectly_valid": { - Path: "/also_valid", - Paths: []string{"/also_valid_too", "also_valid_too,_also"}, - }, - }, - }, - shouldFailVaildation: true, - }, - { - desc: "should be able to create multiple symlinks to the same target, with the correct ordering", - PostInstall: PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "oldpath": { - Paths: []string{"/newpath_z", "/newpath_a"}, - }, - }, - }, - expectedOutput: []ArtifactSymlinkConfig{ - { - Source: "oldpath", - Dest: "/newpath_a", - }, - { - Source: "oldpath", - Dest: "/newpath_z", - }, - }, - expectedNumLinks: 2, - }, - { - desc: "combine Path and Paths with correct ordering", - PostInstall: PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "oldpath2": { - Paths: []string{"/newpath_z", "/newpath_a"}, - }, - "oldpath1": { - Path: "/newpath1", - }, - }, - }, - expectedOutput: []ArtifactSymlinkConfig{ - { - Source: "oldpath1", - Dest: "/newpath1", - }, - { - Source: "oldpath2", - Dest: "/newpath_a", - }, - { - Source: "oldpath2", - Dest: "/newpath_z", - }, - }, - expectedNumLinks: 3, - }, - { - desc: "just Path", - PostInstall: PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "oldpath3": { - Path: "/newpath3", - }, - "oldpath2": { - Path: "/newpath2", - }, - "oldpath1": { - Path: "/newpath1", - }, - }, - }, - expectedOutput: []ArtifactSymlinkConfig{ - { - Source: "oldpath1", - Dest: "/newpath1", - }, - { - Source: "oldpath2", - Dest: "/newpath2", - }, - { - Source: "oldpath3", - Dest: "/newpath3", - }, - }, - expectedNumLinks: 3, - }, - } - - for _, test := range table { - if passed := t.Run(test.desc, func(t *testing.T) { - p := test.PostInstall - - if err := p.validate(); err != nil { - if !test.shouldFailVaildation { - t.Logf("input failed validation: %s", err) - t.Fail() - } - - return - } - - if test.shouldFailVaildation { // err was nil, but shouldn't have been - t.Logf("input should have failed validation, but succeeded:\n%#v", test.PostInstall) - t.Fail() - return - } - - actualSymlinks := p.GetSymlinks() - if len(actualSymlinks) != test.expectedNumLinks { - t.Logf("expected %d links, but there were %d", test.expectedNumLinks, len(actualSymlinks)) - t.Fail() - } - - for i := range actualSymlinks { - actual := actualSymlinks[i] - expected := test.expectedOutput[i] - - if actual != expected { - t.Logf("expected:\n%#v\n================found:\n%#v\n", expected, actual) - t.Fail() - } - } - }); !passed { - t.Fail() - } - } -} diff --git a/image.go b/image.go index f2e483fea..f089e6bee 100644 --- a/image.go +++ b/image.go @@ -3,6 +3,7 @@ package dalec import ( "context" goerrors "errors" + "sort" "github.com/google/shlex" "github.com/moby/buildkit/client/llb" @@ -145,46 +146,53 @@ func MergeImageConfig(dst *DockerImageConfig, src *ImageConfig) error { return nil } -func (s *ImageConfig) validate() error { - if s == nil { +func (i *ImageConfig) validate() error { + if i == nil { return nil } var errs []error - if s.Base != "" && len(s.Bases) > 0 { + if i.Base != "" && len(i.Bases) > 0 { errs = append(errs, errors.New("cannot specify both image.base and image.bases")) } - for i, base := range s.Bases { + for i, base := range i.Bases { if err := base.validate(); err != nil { errs = append(errs, errors.Wrapf(err, "bases[%d]", i)) } } + + if err := i.Post.validate(); err != nil { + errs = append(errs, errors.Wrap(err, "postinstall")) + } + return goerrors.Join(errs...) } -func (s *ImageConfig) fillDefaults() { - if s == nil { +func (i *ImageConfig) fillDefaults() { + if i == nil { return } // s.Bases is a superset of s.Base, so migrate s.Base to s.Bases - if s.Base != "" { - s.Bases = append(s.Bases, BaseImage{ + if i.Base != "" { + i.Bases = append(i.Bases, BaseImage{ Rootfs: Source{ DockerImage: &SourceDockerImage{ - Ref: s.Base, + Ref: i.Base, }, }, }) - s.Base = "" + i.Base = "" } - for _, bi := range s.Bases { + for _, bi := range i.Bases { bi.fillDefaults() } + + i.Post.fillDefaults() } func (s *BaseImage) validate() error { @@ -199,10 +207,43 @@ func (s *BaseImage) validate() error { return nil } +func (p *PostInstall) validate() error { + if p == nil { + return nil + } + + var errs []error + + if err := validateSymlinks(p.Symlinks); err != nil { + errs = append(errs, err) + } + + return errors.Wrap(goerrors.Join(errs...), "symlink") +} + func (s *BaseImage) fillDefaults() { fillDefaults(&s.Rootfs) } +func (p *PostInstall) fillDefaults() { + if p == nil { + return + } + + // validation has already taken place + for oldpath := range p.Symlinks { + cfg := p.Symlinks[oldpath] + if cfg.Path == "" { + continue + } + + cfg.Paths = append(cfg.Paths, cfg.Path) + cfg.Path = "" + sort.Strings(cfg.Paths) + p.Symlinks[oldpath] = cfg + } +} + func (bi *BaseImage) ResolveImageConfig(ctx context.Context, sOpt SourceOpts, opt sourceresolver.Opt) ([]byte, error) { // In the future, *BaseImage may support other source types, but for now it only supports Docker images. // diff --git a/load.go b/load.go index 61cb0d5ef..e0c1ffcff 100644 --- a/load.go +++ b/load.go @@ -4,6 +4,7 @@ import ( goerrors "errors" "fmt" "os" + "slices" "strings" "github.com/goccy/go-yaml" @@ -324,6 +325,8 @@ func (s *Spec) FillDefaults() { t.fillDefaults() s.Targets[k] = t } + + s.Image.fillDefaults() } func (s Spec) Validate() error { @@ -386,10 +389,8 @@ func (s Spec) Validate() error { } } - if s.Image != nil { - if err := s.Image.Post.validate(); err != nil { - errs = append(errs, err) - } + if err := s.Image.validate(); err != nil { + errs = append(errs, errors.Wrap(err, "image")) } return goerrors.Join(errs...) @@ -460,57 +461,46 @@ func (b *ArtifactBuild) processBuildArgs(lex *shell.Lex, args map[string]string, return goerrors.Join(errs...) } -func (p *PostInstall) validate() error { - if p == nil { - return nil - } - +func validateSymlinks(symlinks map[string]SymlinkTarget) error { var errs []error - for k, sl := range p.Symlinks { + dests := make(map[string]string, len(symlinks)<<1) + + for oldpath, cfg := range symlinks { var err error - if k == "" { + if oldpath == "" { err = fmt.Errorf("symlink source is empty") errs = append(errs, err) } - if sl.Path != "" && len(sl.Paths) != 0 || sl.Path == "" && len(sl.Paths) == 0 { + if cfg.Path != "" && len(cfg.Paths) != 0 || cfg.Path == "" && len(cfg.Paths) == 0 { err = fmt.Errorf("'path' and 'paths' fields are mutually exclusive, and at least one is required") errs = append(errs, err) } - } - - symlinks := p.GetSymlinks() - if err := validateSymlinks(symlinks); err != nil { - errs = append(errs, err) - } - - return goerrors.Join(errs...) -} -func validateSymlinks(symlinks []ArtifactSymlinkConfig) error { - var errs []error - dests := make(map[string]string, len(symlinks)<<1) - - for _, l := range symlinks { - var err error - if l.Dest == "" { - err = fmt.Errorf("invalid symlink destination") - errs = append(errs, err) + if err != nil { + continue } - if l.Source == "" { - err = fmt.Errorf("invalid symlink source") - errs = append(errs, err) + // By this point, both the oldpath and the SymlinkTarget are + // well-formed. We still need to make sure each 'newpath' is unique. + if cfg.Paths == nil { + cfg.Paths = []string{} } - if err != nil { - continue - } + newpaths := slices.Clone(cfg.Paths) + newpaths = append(newpaths, cfg.Path) + + for _, newpath := range newpaths { + if newpath == "" { + continue + } + + if other_oldpath, ok := dests[newpath]; ok { + errs = append(errs, fmt.Errorf("symlink 'newpaths' must be unique: %q points to both %q and %q", newpath, oldpath, other_oldpath)) + } - if other_oldpath, ok := dests[l.Dest]; ok { - errs = append(errs, fmt.Errorf("symlink 'newpaths' must be unique: %q points to both %q and %q", l.Dest, l.Source, other_oldpath)) + dests[newpath] = oldpath } - dests[l.Dest] = l.Source } return goerrors.Join(errs...) diff --git a/load_test.go b/load_test.go index 4b8104586..da0b38d51 100644 --- a/load_test.go +++ b/load_test.go @@ -1147,6 +1147,125 @@ func TestImage_validate(t *testing.T) { }, }, }, + { + Name: "valid SymlinkTarget should pass validation (path)", + Image: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "newpath", + }, + }, + }, + }, + }, + { + Name: "valid SymlinkTarget should pass validation (paths, single)", + Image: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Paths: []string{"newpath"}, + }, + }, + }, + }, + }, + { + Name: "valid SymlinkTarget should pass validation (paths, multiple)", + Image: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Paths: []string{"newpath1", "newpath2"}, + }, + }, + }, + }, + }, + { + Name: "invalid SymlinkTarget should fail validation: empty target", + Image: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": {}, + }, + }, + }, + expectErr: "'path' and 'paths' fields are mutually exclusive, and at least one is required", + }, + { + Name: "invalid SymlinkTarget should fail validation: empty key, valid target(paths)", + Image: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "": { + Paths: []string{"/newpath_z", "/newpath_a"}, + }, + }, + }, + }, + expectErr: "symlink source is empty", + }, + { + Name: "invalid SymlinkTarget should fail validation: empty key: valid target(path)", + Image: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "": { + Path: "/newpath_z", + }, + }, + }, + }, + expectErr: "symlink source is empty", + }, + { + Name: "invalid SymlinkTarget should fail validation: all symlink 'newpaths' should be unique(paths)", + Image: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "perfectly_valid": { + Path: "/also_valid", + }, + "also_perfectly_valid": { + Paths: []string{"/also_valid"}, + }, + }, + }, + }, + expectErr: "symlink 'newpaths' must be unique:", + }, + { + Name: "invalid SymlinkTarget should fail validation: all symlink 'newpaths' should be unique(path)", + Image: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "perfectly_valid": { + Path: "/also_valid", + }, + "also_perfectly_valid": { + Path: "/also_valid", + }, + }, + }, + }, + expectErr: "symlink 'newpaths' must be unique:", + }, + { + Name: "invalid SymlinkTarget should fail validation: path and paths are mutually exclusive", + Image: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "perfectly_valid": { + Path: "/also_valid", + Paths: []string{"/also_valid_too", "also_valid_too,_also"}, + }, + }, + }, + }, + expectErr: "'path' and 'paths' fields are mutually exclusive, and at least one is required", + }, } for _, tc := range cases { diff --git a/target.go b/target.go index f6084b9ce..654bbc309 100644 --- a/target.go +++ b/target.go @@ -46,10 +46,8 @@ func (t *Target) validate() error { } } - if t.Image != nil { - if err := t.Image.Post.validate(); err != nil { - errs = append(errs, errors.Wrap(err, "postinsall")) - } + if err := t.Image.validate(); err != nil { + errs = append(errs, errors.Wrap(err, "postinstall")) } return goerrors.Join(errs...) diff --git a/test/windows_test.go b/test/windows_test.go index 3c3f79578..470d4302a 100644 --- a/test/windows_test.go +++ b/test/windows_test.go @@ -328,31 +328,33 @@ echo "$BAR" > bar.txt validateSymlinks := func(ctx context.Context, t *testing.T, ref gwclient.Reference, spec dalec.Spec) { post := spec.GetImagePost("windowscross") - symlinks := post.GetSymlinks() - for _, sl := range symlinks { + for oldpath, newpaths := range post.Symlinks { b1, err := ref.ReadFile(ctx, gwclient.ReadRequest{ - Filename: sl.Source, + Filename: oldpath, }) if err != nil { - t.Fatalf("couldn't find Windows \"symlink\" target %q: %v", sl.Source, err) + t.Fatalf("couldn't find Windows \"symlink\" target %q: %v", oldpath, err) } - b2, err := ref.ReadFile(ctx, gwclient.ReadRequest{ - Filename: sl.Dest, - }) - if err != nil { - t.Fatalf("couldn't find Windows \"symlink\" at destination %q: %v", sl.Dest, err) - } - - if len(b1) != len(b2) { - t.Fatalf("Windows \"symlink\" not identical to target file") - } + for _, newpath := range newpaths.Paths { + b2, err := ref.ReadFile(ctx, gwclient.ReadRequest{ + Filename: newpath, + }) + if err != nil { + t.Fatalf("couldn't find Windows \"symlink\" at destination %q: %v", newpath, err) + } - for i := range b1 { - if b1[i] != b2[i] { + if len(b1) != len(b2) { t.Fatalf("Windows \"symlink\" not identical to target file") } + + for i := range b1 { + if b1[i] != b2[i] { + t.Fatalf("Windows \"symlink\" not identical to target file") + } + } } + } } From 4547f8a31eba661353ecfe289c875191be410398 Mon Sep 17 00:00:00 2001 From: Peter Engelbert Date: Mon, 13 Jan 2025 11:30:46 -0500 Subject: [PATCH 04/11] Add doc comment deprecating `Path` in `SymlinkTarget` Signed-off-by: Peter Engelbert --- docs/spec.schema.json | 26 ++++++++++++++++++++++++-- spec.go | 5 ++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/docs/spec.schema.json b/docs/spec.schema.json index 48c2a24c7..2a484de44 100644 --- a/docs/spec.schema.json +++ b/docs/spec.schema.json @@ -1106,16 +1106,38 @@ "description": "Spec is the specification for a package build." }, "SymlinkTarget": { + "oneOf": [ + { + "required": [ + "path" + ], + "title": "path" + }, + { + "required": [ + "paths" + ], + "title": "paths" + } + ], "properties": { "path": { "type": "string", - "description": "Path is the path where the symlink should be placed" + "description": "Path is the path where the symlink should be placed\n\nDeprecated: This is here for backward compatibility. Use `Paths` instead." + }, + "paths": { + "items": { + "type": "string" + }, + "type": "array", + "description": "Path is a list of `newpath`s that will all point to the same `oldpath`." } }, "additionalProperties": false, "type": "object", "required": [ - "path" + "path", + "paths" ], "description": "SymlinkTarget specifies the properties of a symlink" }, diff --git a/spec.go b/spec.go index 895796c5e..ec35c6e0e 100644 --- a/spec.go +++ b/spec.go @@ -130,7 +130,10 @@ type PostInstall struct { // SymlinkTarget specifies the properties of a symlink type SymlinkTarget struct { // Path is the path where the symlink should be placed - Path string `yaml:"path" json:"path" jsonschema:"oneof_required=path"` + // + // Deprecated: This is here for backward compatibility. Use `Paths` instead. + Path string `yaml:"path" json:"path" jsonschema:"oneof_required=path"` + // Path is a list of `newpath`s that will all point to the same `oldpath`. Paths []string `yaml:"paths" json:"paths" jsonschema:"oneof_required=paths"` } From 1aed48b7537f2c10927943bc1c66ae5af78f1c97 Mon Sep 17 00:00:00 2001 From: Peter Engelbert Date: Mon, 13 Jan 2025 13:26:13 -0500 Subject: [PATCH 05/11] Add tests for ImageConfig.fillDefaults() Signed-off-by: Peter Engelbert --- load_test.go | 199 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 199 insertions(+) diff --git a/load_test.go b/load_test.go index da0b38d51..9cc9e0ad9 100644 --- a/load_test.go +++ b/load_test.go @@ -8,6 +8,7 @@ import ( "maps" "os" "reflect" + "slices" "testing" "github.com/moby/buildkit/frontend/dockerui" @@ -1279,3 +1280,201 @@ func TestImage_validate(t *testing.T) { }) } } + +func TestImageFillDefaults(t *testing.T) { + t.Run("postinstall", testPostInstallFillDefaults) +} + +func testPostInstallFillDefaults(t *testing.T) { + t.Run("symlinks", testSymlinkFillDefaults) +} + +func testSymlinkFillDefaults(t *testing.T) { + type tableEntry struct { + desc string + input ImageConfig + output ImageConfig + } + + // note: fillDefaults is run after validation, so input is assumed to be + // valid + + table := []tableEntry{ + { + desc: "empty Path and single Paths should remain untouched", + input: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "", + Paths: []string{"/newpath"}, + }, + }, + }, + }, + output: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "", + Paths: []string{"/newpath"}, + }, + }, + }, + }, + }, + { + desc: "path should be moved to Paths", + input: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "/newpath2", + Paths: []string{"/newpath1"}, + }, + }, + }, + }, + output: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "", + Paths: []string{"/newpath1", "/newpath2"}, + }, + }, + }, + }, + }, + { + desc: "Path should be appended to Paths", + input: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "/newpath3", + Paths: []string{"/newpath1", "/newpath2"}, + }, + }, + }, + }, + output: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "", + Paths: []string{"/newpath1", "/newpath2", "/newpath3"}, + }, + }, + }, + }, + }, + { + desc: "should work if Paths is nil", + input: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "/newpath", + Paths: nil, + }, + }, + }, + }, + output: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "", + Paths: []string{"/newpath"}, + }, + }, + }, + }, + }, + { + desc: "should work if Paths is empty", + input: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "/newpath", + Paths: nil, + }, + }, + }, + }, + output: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "", + Paths: []string{"/newpath"}, + }, + }, + }, + }, + }, + { + desc: "empty Path and multimple Paths should remain untouched", + input: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "", + Paths: []string{"/newpath1", "/newpath2"}, + }, + }, + }, + }, + output: ImageConfig{ + Post: &PostInstall{ + Symlinks: map[string]SymlinkTarget{ + "oldpath": { + Path: "", + Paths: []string{"/newpath1", "/newpath2"}, + }, + }, + }, + }, + }, + } + + for _, test := range table { + t.Run(test.desc, func(t *testing.T) { + cmp := func(v1 SymlinkTarget, v2 SymlinkTarget) bool { + if v1.Path != v2.Path { + return false + } + + if !slices.Equal(v1.Paths, v2.Paths) { + return false + } + + return true + } + + test.input.fillDefaults() + + in := test.input.Post.Symlinks + out := test.output.Post.Symlinks + if err := validateSymlinks(in); err != nil { + t.Errorf("you wrote a bad test. the input must be valid for the defaults to be filled.") + return + } + + if err := validateSymlinks(out); err != nil { + t.Errorf("you wrote a bad test. the output specified fails validation") + return + } + + if !maps.EqualFunc(in, out, cmp) { + in, _ := json.MarshalIndent(in, "", "\t") + out, _ := json.MarshalIndent(out, "", "\t") + + t.Errorf("input and output are not matched:\nexpected: %s\n=======\nactual:%s\n", string(out), string(in)) + } + }) + } + +} From a1ac6aff6a1956bb7a072c468ccc057d3ab04bc9 Mon Sep 17 00:00:00 2001 From: Peter Engelbert Date: Thu, 16 Jan 2025 11:32:15 -0500 Subject: [PATCH 06/11] Fix validation and fillDefaults for ImageConfig * no longer modifies anything in `.validate()` * only allocates map when it's actually needed * stipulates in the test that `.Paths` should end up sorted (this is for llb caching) * s/postinstall/post Signed-off-by: Peter Engelbert --- load.go | 63 +++++++++++++++++++++++++++++++++++++++------------- load_test.go | 4 ++-- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/load.go b/load.go index e0c1ffcff..f8ebb207d 100644 --- a/load.go +++ b/load.go @@ -4,7 +4,6 @@ import ( goerrors "errors" "fmt" "os" - "slices" "strings" "github.com/goccy/go-yaml" @@ -462,8 +461,10 @@ func (b *ArtifactBuild) processBuildArgs(lex *shell.Lex, args map[string]string, } func validateSymlinks(symlinks map[string]SymlinkTarget) error { - var errs []error - dests := make(map[string]string, len(symlinks)<<1) + var ( + errs []error + numPairs int + ) for oldpath, cfg := range symlinks { var err error @@ -473,7 +474,9 @@ func validateSymlinks(symlinks map[string]SymlinkTarget) error { } if cfg.Path != "" && len(cfg.Paths) != 0 || cfg.Path == "" && len(cfg.Paths) == 0 { - err = fmt.Errorf("'path' and 'paths' fields are mutually exclusive, and at least one is required") + err = fmt.Errorf("'path' and 'paths' fields are mutually exclusive, and at least one is required: "+ + "symlink to %s", oldpath) + errs = append(errs, err) } @@ -481,25 +484,53 @@ func validateSymlinks(symlinks map[string]SymlinkTarget) error { continue } - // By this point, both the oldpath and the SymlinkTarget are - // well-formed. We still need to make sure each 'newpath' is unique. - if cfg.Paths == nil { - cfg.Paths = []string{} + if cfg.Path != "" { // this means .Paths is empty + numPairs++ + continue } - newpaths := slices.Clone(cfg.Paths) - newpaths = append(newpaths, cfg.Path) - - for _, newpath := range newpaths { + for _, newpath := range cfg.Paths { // this means .Path is empty + numPairs++ if newpath == "" { + errs = append(errs, fmt.Errorf("symlink newpath should not be empty")) continue } + } + } - if other_oldpath, ok := dests[newpath]; ok { - errs = append(errs, fmt.Errorf("symlink 'newpaths' must be unique: %q points to both %q and %q", newpath, oldpath, other_oldpath)) - } + // The remainder of this function checks for duplicate `newpath`s in the + // symlink pairs. This is not allowed: neither the ordering of the + // `oldpath` map keys, nor that of the `.Paths` values can be trusted. We + // also sort both to avoid cache misses, so we would end up with + // inconsistent behavior -- regardless of whether the inputs are the same. + if numPairs < 2 { + return goerrors.Join(errs...) + } + + var ( + oldpath string + cfg SymlinkTarget + ) + + seen := make(map[string]string, numPairs) + checkDuplicateNewpath := func(newpath string) { + if newpath == "" { + return + } + + if seenPath, found := seen[newpath]; found { + errs = append(errs, fmt.Errorf("symlink 'newpaths' must be unique: %q points to both %q and %q", + newpath, oldpath, seenPath)) + } + + seen[newpath] = oldpath + } + + for oldpath, cfg = range symlinks { + checkDuplicateNewpath(cfg.Path) - dests[newpath] = oldpath + for _, newpath := range cfg.Paths { + checkDuplicateNewpath(newpath) } } diff --git a/load_test.go b/load_test.go index 9cc9e0ad9..9a0e32a5b 100644 --- a/load_test.go +++ b/load_test.go @@ -1416,13 +1416,13 @@ func testSymlinkFillDefaults(t *testing.T) { }, }, { - desc: "empty Path and multimple Paths should remain untouched", + desc: "empty Path and multimple Paths should have Paths sorted", input: ImageConfig{ Post: &PostInstall{ Symlinks: map[string]SymlinkTarget{ "oldpath": { Path: "", - Paths: []string{"/newpath1", "/newpath2"}, + Paths: []string{"/newpath2", "/newpath1"}, }, }, }, From f1e5639b951f1818e1c752eaf592dc871e0562f0 Mon Sep 17 00:00:00 2001 From: Peter Engelbert Date: Wed, 5 Feb 2025 11:06:19 -0500 Subject: [PATCH 07/11] Remove invalid tests Some of the `fillDefaults` tests were testing invalid situations. To prevent this, validation is run on the test ins and outs before calling `fillDefaults`. The invalid tests have been removed. This commit also removes the sorting from `fillDefaults`, this will be left to the user instead. Signed-off-by: Peter Engelbert --- load_test.go | 51 ++------------------------------------------------- 1 file changed, 2 insertions(+), 49 deletions(-) diff --git a/load_test.go b/load_test.go index 9a0e32a5b..0074981bc 100644 --- a/load_test.go +++ b/load_test.go @@ -1329,8 +1329,7 @@ func testSymlinkFillDefaults(t *testing.T) { Post: &PostInstall{ Symlinks: map[string]SymlinkTarget{ "oldpath": { - Path: "/newpath2", - Paths: []string{"/newpath1"}, + Path: "/newpath1", }, }, }, @@ -1340,30 +1339,7 @@ func testSymlinkFillDefaults(t *testing.T) { Symlinks: map[string]SymlinkTarget{ "oldpath": { Path: "", - Paths: []string{"/newpath1", "/newpath2"}, - }, - }, - }, - }, - }, - { - desc: "Path should be appended to Paths", - input: ImageConfig{ - Post: &PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "oldpath": { - Path: "/newpath3", - Paths: []string{"/newpath1", "/newpath2"}, - }, - }, - }, - }, - output: ImageConfig{ - Post: &PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "oldpath": { - Path: "", - Paths: []string{"/newpath1", "/newpath2", "/newpath3"}, + Paths: []string{"/newpath1"}, }, }, }, @@ -1392,29 +1368,6 @@ func testSymlinkFillDefaults(t *testing.T) { }, }, }, - { - desc: "should work if Paths is empty", - input: ImageConfig{ - Post: &PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "oldpath": { - Path: "/newpath", - Paths: nil, - }, - }, - }, - }, - output: ImageConfig{ - Post: &PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "oldpath": { - Path: "", - Paths: []string{"/newpath"}, - }, - }, - }, - }, - }, { desc: "empty Path and multimple Paths should have Paths sorted", input: ImageConfig{ From 7a63ab6e17699bb383a4492a505d6291ebb77f88 Mon Sep 17 00:00:00 2001 From: Peter Engelbert Date: Wed, 5 Feb 2025 11:52:55 -0500 Subject: [PATCH 08/11] Defer sorting of symlinks to user Instead of sorting the list of symlinks, leave it for the user to do. This commit removes the sorting from `fillDefaults` and implements it in the handlers. The map keys of the symlinks are now sorted as well. Signed-off-by: Peter Engelbert --- frontend/windows/handle_container.go | 9 +++++++-- helpers.go | 8 ++++++-- image.go | 4 ++-- load_test.go | 29 ++-------------------------- 4 files changed, 17 insertions(+), 33 deletions(-) diff --git a/frontend/windows/handle_container.go b/frontend/windows/handle_container.go index 9283ba7fd..6b0d4a321 100644 --- a/frontend/windows/handle_container.go +++ b/frontend/windows/handle_container.go @@ -6,6 +6,7 @@ import ( "fmt" "path" "runtime" + "sort" "sync" "github.com/Azure/dalec" @@ -202,8 +203,12 @@ func copySymlinks(post *dalec.PostInstall) llb.StateOption { return s } - for oldpath, newpaths := range post.Symlinks { - for _, newpath := range newpaths.Paths { + sortedKeys := dalec.SortMapKeys(post.Symlinks) + for _, oldpath := range sortedKeys { + newpaths := post.Symlinks[oldpath].Paths + sort.Strings(newpaths) + + for _, newpath := range newpaths { s = s.File(llb.Mkdir(path.Dir(newpath), 0755, llb.WithParents(true))) s = s.File(llb.Copy(s, oldpath, newpath)) } diff --git a/helpers.go b/helpers.go index bf5582ce2..6e0a84ab8 100644 --- a/helpers.go +++ b/helpers.go @@ -383,8 +383,12 @@ func InstallPostSymlinks(post *PostInstall, rootfsPath string) llb.RunOption { buf := bytes.NewBuffer(nil) buf.WriteString("set -ex\n") - for oldpath, newpaths := range post.Symlinks { - for _, newpath := range newpaths.Paths { + sortedKeys := SortMapKeys(post.Symlinks) + for _, oldpath := range sortedKeys { + newpaths := post.Symlinks[oldpath].Paths + sort.Strings(newpaths) + + for _, newpath := range newpaths { fmt.Fprintf(buf, "mkdir -p %q\n", filepath.Join(rootfsPath, filepath.Dir(newpath))) fmt.Fprintf(buf, "ln -s %q %q\n", oldpath, filepath.Join(rootfsPath, newpath)) } diff --git a/image.go b/image.go index f089e6bee..c9c856318 100644 --- a/image.go +++ b/image.go @@ -192,7 +192,7 @@ func (i *ImageConfig) fillDefaults() { bi.fillDefaults() } - i.Post.fillDefaults() + i.Post.normalizeSymlinks() } func (s *BaseImage) validate() error { @@ -225,7 +225,7 @@ func (s *BaseImage) fillDefaults() { fillDefaults(&s.Rootfs) } -func (p *PostInstall) fillDefaults() { +func (p *PostInstall) normalizeSymlinks() { if p == nil { return } diff --git a/load_test.go b/load_test.go index 0074981bc..c6ea41c07 100644 --- a/load_test.go +++ b/load_test.go @@ -1100,6 +1100,8 @@ targets: assert.Check(t, spec.Targets["foo"].Image.Bases[0].Rootfs.DockerImage != nil) assert.Check(t, cmp.Equal(spec.Targets["foo"].Image.Bases[0].Rootfs.DockerImage.Ref, "busybox:latest")) }) + + t.Run("postinstall", testPostInstallFillDefaults) } func TestImage_validate(t *testing.T) { @@ -1281,10 +1283,6 @@ func TestImage_validate(t *testing.T) { } } -func TestImageFillDefaults(t *testing.T) { - t.Run("postinstall", testPostInstallFillDefaults) -} - func testPostInstallFillDefaults(t *testing.T) { t.Run("symlinks", testSymlinkFillDefaults) } @@ -1368,29 +1366,6 @@ func testSymlinkFillDefaults(t *testing.T) { }, }, }, - { - desc: "empty Path and multimple Paths should have Paths sorted", - input: ImageConfig{ - Post: &PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "oldpath": { - Path: "", - Paths: []string{"/newpath2", "/newpath1"}, - }, - }, - }, - }, - output: ImageConfig{ - Post: &PostInstall{ - Symlinks: map[string]SymlinkTarget{ - "oldpath": { - Path: "", - Paths: []string{"/newpath1", "/newpath2"}, - }, - }, - }, - }, - }, } for _, test := range table { From fc06e2ddec22b279bb86c306479a9689a35eef80 Mon Sep 17 00:00:00 2001 From: Peter Engelbert Date: Thu, 6 Feb 2025 16:15:45 -0500 Subject: [PATCH 09/11] Address linter comments Signed-off-by: Peter Engelbert --- frontend/windows/handle_container.go | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/frontend/windows/handle_container.go b/frontend/windows/handle_container.go index 6b0d4a321..17bd1cc58 100644 --- a/frontend/windows/handle_container.go +++ b/frontend/windows/handle_container.go @@ -217,26 +217,3 @@ func copySymlinks(post *dalec.PostInstall) llb.StateOption { return s } } - -func getTargetPlatform(bc *dockerui.Client) (ocispecs.Platform, error) { - platform := defaultPlatform - - switch len(bc.TargetPlatforms) { - case 0: - case 1: - platform = bc.TargetPlatforms[0] - default: - return ocispecs.Platform{}, - fmt.Errorf("multiple target supplied for build: %v. note: only amd64 is supported for windows outputs", bc.TargetPlatforms) - } - - return platform, nil -} - -func getBaseOutputImage(spec *dalec.Spec, target, defaultBase string) string { - baseRef := defaultBase - if spec.Targets[target].Image != nil && spec.Targets[target].Image.Base != "" { - baseRef = spec.Targets[target].Image.Base - } - return baseRef -} From bb833d6a5c26fb1b2b6e9880bbecf22ed653782e Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 12 Feb 2025 14:46:24 -0800 Subject: [PATCH 10/11] Apply suggestions from code review --- image.go | 1 - load.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/image.go b/image.go index c9c856318..27a49962c 100644 --- a/image.go +++ b/image.go @@ -239,7 +239,6 @@ func (p *PostInstall) normalizeSymlinks() { cfg.Paths = append(cfg.Paths, cfg.Path) cfg.Path = "" - sort.Strings(cfg.Paths) p.Symlinks[oldpath] = cfg } } diff --git a/load.go b/load.go index f8ebb207d..23f4d3f7e 100644 --- a/load.go +++ b/load.go @@ -473,7 +473,7 @@ func validateSymlinks(symlinks map[string]SymlinkTarget) error { errs = append(errs, err) } - if cfg.Path != "" && len(cfg.Paths) != 0 || cfg.Path == "" && len(cfg.Paths) == 0 { + if (cfg.Path != "" && len(cfg.Paths) > 0) || (cfg.Path == "" && len(cfg.Paths) == 0 ) { err = fmt.Errorf("'path' and 'paths' fields are mutually exclusive, and at least one is required: "+ "symlink to %s", oldpath) From 45f8962fdc6278ea6de2c122575f537e38f1d717 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 12 Feb 2025 14:49:08 -0800 Subject: [PATCH 11/11] Update image.go --- image.go | 1 - 1 file changed, 1 deletion(-) diff --git a/image.go b/image.go index 27a49962c..ae710d6b0 100644 --- a/image.go +++ b/image.go @@ -3,7 +3,6 @@ package dalec import ( "context" goerrors "errors" - "sort" "github.com/google/shlex" "github.com/moby/buildkit/client/llb"