Symlinks aren't properly copied

[benmoss]

What steps did you take:
vendir sync with this config:

apiVersion: vendir.k14s.io/v1alpha1
kind: Config
minimumRequiredVersion: 0.12.0
directories:
- path: config/upstream
  contents:
  - path: serving
    git:
      url: https://github.com/knative/serving
      ref: v0.21.0
    includePaths:
    - config/core/**/*

What happened:
config/upstream/serving/config/core/300-imagecache.yaml is an invalid symlink, pointing to a file that doesn't exist.

What did you expect:
I'm not 100% sure, probably it should collapse the symlink into a regular file with the contents of the file it pointed to. Maybe this needs to be configurable.

Anything else you would like to add:
I looked into this a little and found it was broken in the library that vendir depends on: otiai10/copy#32

Environment:

• vendir version (execute vendir --version): 0.16.0
• OS (e.g. from /etc/os-release): ubuntu focal

[github-actions]

This issue is being marked as stale due to a long period of inactivity and will be closed in 5 days if there is no response.

[gberche-orange]

I'm still reproducing the problem with vendir version 0.42.0

See reproduction repo at https://github.com/gberche-orange/renovate-repro-vendir-symlinks

@cppforlife Any chance to reopen this issue ?

[joaopapereira]

The problem here is that the includePaths are processed before we check for symlinks (which we can argue if it makes sense or not).
When the validation is done the folder that the symlinks point to, no longer is present.
Out of my head, I think we can try to solve this in a couple of ways:

1. Do nothing. This will keep the current behavior and the error will happen whenever the user has includePaths
2. Validate the symlinks presence before we filter the folders out. This is a change in behavior and I am not 100% sure how impactful it is for current users. At first look, it does not sound very problematic but maybe we need to do a deeper dive
3. Warn that the symlink is not valid and remove it

Not sure, @Zebradil any idea/suggestion?

[Zebradil]

Sorry, I somehow missed this thread.

I'm not sure either, to be honest.

In the current state, the configuration from the starter post produces the following error (together with non-zero exit code):

vendir: Error: Unable to resolve symlink: lstat config/upstream/serving/vendor: no such file or directory

Vendir creates the target directory with files (and the broken symlinks) in it, but it doesn't create the lock file. This is definitely a bug: it should either finish successfully with warnings or keep the target directory as is.
The error is produced by the symlink validation code added in #138. The code is intended to restrict creating symlinks that point outside of the working directory. This can be circumvented by using the --dangerous-allow-all-symlink-destinations flag, which switches the validation off and let's vendir to finish successfully (i.e.: no errors printed, the lock file created).

My proposal is:

1. Required: fix the current behaviour by validating symlinks in the resulted bundle before moving it to the target destination.
2. Optional: add a new flag that'll populate files instead of symlinks. This will fix the current use case in a way the user expects it to be (if I undrestood correctly).
3. Optional: split validation of out-of-working-directory symlinks and the other symlinks. This will allow to apply different rules to them.

[joaopapereira]

I agree with your proposal 1.
Regarding 2 I am not sure what do you mean by "populate files instead of symlinks"
In terms of the optional 3 not sure what is the difference between the 2 type of symlink you are mentioning can you give an example?

[Zebradil]

I'll prepare a PR for the first point, then.

Regarding 2 I am not sure what do you mean by "populate files instead of symlinks"

Means to create actual files instead of symlinks pointing to them.
For example, instead of

300-imagecache.yaml -> ../../vendor/knative.dev/caching/config/image.yaml

we'd have just

300-imagecache.yaml

which is copied as is from ../../vendor/knative.dev/caching/config/image.yaml.

In terms of the optional 3 not sure what is the difference between the 2 type of symlink you are mentioning can you give an example?

I thought that maybe it makes sense to separate handling of the following situations:

1. When symlinks point outside of the initial bundle. For example, a link that points to /usr/lib/something or foo.txt -> ../../../../etc/passwd — paths that aren't created by the current vendir sync process.
2. When symlinks point inside of the initial bundle but outside of the filtered bundle (e.g. by includePaths), like the situation this issue begun with.

At first, I thought that the --dangerous-allow-all-symlink-destinations flag is meant for the first case, but now I think that maybe it's enough for both cases.

We can think of a configuration option to allow symlink destinations to allow users to encode --dangerous-allow-all-symlink-destinations in a config of a particular directory or (better) a particular source.