Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: clean paths after reading config #341

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

Zebradil
Copy link
Member

@Zebradil Zebradil commented Dec 24, 2023

This PR adds mandatory processing of directory and content paths with the filepath.Clean function. It allows to avoid problems with paths containing .. and .. This, in turn, fixes the lazy functionality. At the moment, paths foo and foo/ (or even foo//) are treated as different destinations and are re-downloaded.

Fixes #347
Fixes #348

@kumaritanushree
Copy link
Contributor

@Zebradil added one comment in issue #348, can you please check that once?

Copy link
Contributor

@100mik 100mik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reducing paths to their most minimal form before processing them makes sense too me 🚀
This is something we could throw a test in for. Maybe a vendir with something like files/...

This looks otherwise good to me!

@@ -278,3 +280,12 @@ func (c Config) checkOverlappingPaths() error {

return nil
}

func (c *Config) cleanPaths() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are change the configuration directly will this have any impact on the lazy feature?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the question.

Paths are clean up right after reading the config, so after that any operation uses so to say "canonical" path, including the lock file and lazy functionality.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if the lock file already exists, the path might be different correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if the lock file already exists, the path might be different correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the current vendir behavior is to copy paths from the config to the lock file literally. With the cleanup logic, paths can differ between the config and the lock file. It is basically the same as changing vendor/ to vendor in the config and run sync. The lock file will be updated with the new path.

So, yes, if this is merged, the next sync with the new version of vendir will invalidate the "lazy cache" in some cases. But I don't think that can be an issue.

@Zebradil
Copy link
Member Author

It seems that in the community meeting we agreed to proceed with this idea. I'll add some e2e tests tomorrow.

@Zebradil
Copy link
Member Author

Zebradil commented Jan 11, 2024

The test has landed, please, review.

NOTE: I intentionally did not add test cases with destructive or insecure paths (e.g. .., /root, vendor/..) because they would become obsolete after implementing a better path validation logic.

@Zebradil Zebradil requested a review from 100mik January 11, 2024 21:54
Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kumaritanushree
Copy link
Contributor

LGTM

@kumaritanushree kumaritanushree merged commit 727be59 into carvel-dev:develop Jan 30, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Vendir allows self-destructing configuration Lazy sync doesn't work for the same path in different forms
4 participants