feat: Multiple postinstall symlinks pointing to the same thing by pmengelbert · Pull Request #495 · project-dalec/dalec

pmengelbert · 2025-01-09T22:40:13Z

What this PR does / why we need it:
Currently, you can't create multiple symlink newpaths that point to the same oldpath. This is due to the fact that map keys have to be unique.

For more information, see #489

This should be merged before #494

cpuguy83 · 2025-01-10T00:17:56Z

spec.go

 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"`


We should mark this is deprecated and have people use paths instead.
We can also make it so when we load the spec we migrate Path to Paths then there's only one field to deal with.

How do we mark it as deprecated?

https://go.dev/wiki/Deprecated

done, but I'm going to add tests for it.

tests added.

cpuguy83 · 2025-01-10T00:19:53Z

target.go

 		}
 	}

+	if t.Image != nil {


Let's add a validate to t.Image, which itself could return nil if *Image is nil (note: this is what I did in #492

pmengelbert · 2025-01-13T18:27:08Z

@cpuguy83 This should be ready for another round of review.

cpuguy83 · 2025-01-13T19:25:36Z

load.go

+
+	var errs []error
+	if err := i.Post.validate(); err != nil {
+		errs = append(errs, errors.Wrap(err, "postinstall"))


Nit, just "post"

cpuguy83 · 2025-01-13T19:28:38Z

load.go

+
+		// 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 {


Validations should not be modifying anything.
We can also iterate on Paths without having to have it be non-nil.

cpuguy83 · 2025-01-13T19:28:57Z

load.go

+			cfg.Paths = []string{}
+		}
+
+		newpaths := slices.Clone(cfg.Paths)


Why the clone? Should not need to modify anything here.
If we want to make sure that cfg.Path is not also somewhere in cfg.Paths you can check that directly.

for _, link := range symlinks { if link.Path == cfg.Path { // error } }

I could have sworn I put this in fillDefaults. I need to move this code there. The clone from moving the old code around. I'll take another pass

Now I remember. Clone so we don't modify the original, but stick the cfg.Path field on the end so we can just loop through. It's weird, I'll change it.

Why the clone? Should not need to modify anything here. If we want to make sure that cfg.Path is not also somewhere in cfg.Paths you can check that directly.

for _, link := range symlinks { if link.Path == cfg.Path { // error } }

And the goal is to make sure the there are no duplicate oldpaths in the whole map[string]SymlinkTarget. The desired behavior is unclear (follow the symlink that has already been created? Or overwrite the existing symlink?) Map iteration is not reliably ordered (and we sort the keys to avoid cache misses), so the user is unlikely to get what they want anyway. Better to just not allow it.

done. This exhibits the desired behavior, doesn't modify the spec, is tested properly, and only allocates the map when needed.

cpuguy83 · 2025-01-13T19:34:41Z

load.go

+
+func validateSymlinks(symlinks map[string]SymlinkTarget) error {
+	var errs []error
+	dests := make(map[string]string, len(symlinks)<<1)


Maybe lets call this seen and only allocate the map if its needed.

cpuguy83 · 2025-01-18T00:33:58Z

frontend/windows/handle_container.go

-			s = s.File(llb.Mkdir(path.Dir(dstPath), 0755, llb.WithParents(true)))
-			s = s.File(llb.Copy(s, srcPath, dstPath))
+
+		for oldpath, newpaths := range post.Symlinks {


I think we still need to sort this.

cpuguy83 · 2025-01-18T00:34:30Z

helpers.go

-		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 oldpath, newpaths := range post.Symlinks {


Should also sort this

cpuguy83 · 2025-01-18T00:38:43Z

load_test.go

+				Post: &PostInstall{
+					Symlinks: map[string]SymlinkTarget{
+						"oldpath": {
+							Path:  "/newpath3",


I don't think we need to test this since its not a valid case.

Good catch. Fixed it so the test code will reject invalid tests.

cpuguy83 · 2025-01-18T00:40:53Z

target.go

+			cfg.Path = ""
+		}
+
+		sort.Strings(cfg.Paths)


I don't think this should be sorted since this should be left to the user.

Allow for multiple links to point to the same target. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Upon loading the spec, if `Path` is nonempty, move it to `Paths`. This commit also reworks the tests. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

* 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 <pmengelbert@gmail.com>

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 <pmengelbert@gmail.com>

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 <pmengelbert@gmail.com>

pmengelbert · 2025-02-06T15:16:22Z

This has been rebased and is ready for another round of review @cpuguy83

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

image.go

load.go

image.go

pmengelbert requested a review from a team as a code owner January 9, 2025 22:40

cpuguy83 reviewed Jan 10, 2025

View reviewed changes

cpuguy83 requested changes Jan 10, 2025

View reviewed changes

pmengelbert requested a review from cpuguy83 January 13, 2025 18:27

cpuguy83 requested changes Jan 13, 2025

View reviewed changes

pmengelbert force-pushed the pmengelbert/multiple_postinstall_symlinks_pointing_to_the_same_thing/1 branch 2 times, most recently from e4a8974 to cf1cf76 Compare January 16, 2025 16:48

cpuguy83 requested changes Jan 18, 2025

View reviewed changes

cpuguy83 added this to the v0.12.0 milestone Jan 29, 2025

pmengelbert force-pushed the pmengelbert/multiple_postinstall_symlinks_pointing_to_the_same_thing/1 branch from a21b267 to 6fd5303 Compare February 5, 2025 16:57

pmengelbert added 8 commits February 5, 2025 12:18

Update PostInstall symlink spec

49a3de1

Allow for multiple links to point to the same target. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Add tests, refine validation, add optimizations

858111a

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Move Path entry to Paths in SymlinkTarget

7e6cf20

Upon loading the spec, if `Path` is nonempty, move it to `Paths`. This commit also reworks the tests. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Add doc comment deprecating Path in SymlinkTarget

4547f8a

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Add tests for ImageConfig.fillDefaults()

1aed48b

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

pmengelbert force-pushed the pmengelbert/multiple_postinstall_symlinks_pointing_to_the_same_thing/1 branch from 6fd5303 to 7a63ab6 Compare February 6, 2025 15:14

pmengelbert requested a review from cpuguy83 February 6, 2025 15:16

Address linter comments

fc06e2d

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

cpuguy83 requested changes Feb 12, 2025

View reviewed changes

image.go Outdated Show resolved Hide resolved

load.go Outdated Show resolved Hide resolved

Apply suggestions from code review

bb833d6

cpuguy83 reviewed Feb 12, 2025

View reviewed changes

image.go Outdated Show resolved Hide resolved

Update image.go

45f8962

cpuguy83 approved these changes Feb 12, 2025

View reviewed changes

cpuguy83 merged commit 5f5936c into project-dalec:main Feb 12, 2025
18 checks passed

pmengelbert mentioned this pull request Feb 26, 2025

[BUG] Multiple symlinks cannot be made to point to the same destination #489

Closed

1 task

Conversation

pmengelbert commented Jan 9, 2025

Uh oh!

cpuguy83 Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmengelbert commented Jan 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmengelbert commented Feb 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cpuguy83 Jan 10, 2025 •

edited

Loading