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

kpt doesn't work with symlinks #1544

Closed
morgante opened this issue Mar 12, 2021 · 12 comments · Fixed by #2473
Closed

kpt doesn't work with symlinks #1544

morgante opened this issue Mar 12, 2021 · 12 comments · Fixed by #2473
Assignees
Labels
area/pkg p1 size/L 4 day triaged Issue has been triaged by adding an `area/` label
Milestone

Comments

@morgante
Copy link
Contributor

Expected behavior

When working in a symlinked directory, kpt should behave normally and have commands process properly.

Actual behavior

kpt commands fail unpredictably when working in symlinked directories.

Information

v0.38.1

Steps to reproduce the behavior

  1. Get a package: kpt pkg get https://github.com/GoogleContainerTools/kpt/package-examples/helloworld-set hello2
  2. Create a symlink to the package: ln -s hello2/ hello3
  3. Try various kpt commands and see they return no output. (Ex. kpt cfg tree hello3/)
@mikebz mikebz added the p2 label Mar 17, 2021
@mikebz mikebz added area/pkg triaged Issue has been triaged by adding an `area/` label labels Mar 17, 2021
@mikebz
Copy link
Contributor

mikebz commented Mar 17, 2021

We will see what it takes to get this to work but setting the priority to p2 for now since I have not seen this widely reported and I don't know if customers have mentioned this.

@frankfarzan frankfarzan added this to the v1.0 final milestone Mar 30, 2021
@mortent mortent added the size/L 4 day label May 25, 2021
@mortent mortent modified the milestones: v1.0 m3, v1.0 m4 May 26, 2021
@stephenjlevy
Copy link

I am seeing this issue as well, and it has become a blocker for us when trying to use kpt with an existing monorepo.

kpt pkg get /Users/me/src/myproj.git/test/abc@v0.1
Package "v0.1":
Fetching /Users/me/src/myproj@master
From /Users/me/src/myproj
 * branch            master     -> FETCH_HEAD
 + e2c7f6c...23411f2 master     -> origin/master  (forced update)
Error: error copying package: read /Users/me/.kpt/repos/f5axgwlsomxxg4dfoz2s693smmxw64dt2qryzwcpacbbj3mabgmozbncpy======/bbb/xyz: is a directory

Addressing this TODO would fix the issue in our particular case, as the directory containing the symlink is external to where the kpt package is located.
https://github.com/GoogleContainerTools/kpt/blob/cbd342d350b88677e522bf0d9faa0675edb8bbc1/internal/util/fetch/fetch.go#L195-L196

@mikebz mikebz removed this from the v1.0 m4 milestone Jul 14, 2021
@morgante
Copy link
Contributor Author

@mikebz Can we increase priority on this. Another customer has run into it.

@droot droot assigned phanimarupaka and unassigned mortent Aug 25, 2021
@phanimarupaka phanimarupaka added p1 and removed p2 labels Aug 26, 2021
@phanimarupaka phanimarupaka added this to the Q3-2021 milestone Aug 26, 2021
@phanimarupaka
Copy link
Contributor

phanimarupaka commented Aug 26, 2021

This essentially needs us to unify the path parsing logic all across kpt. As it stands, only few of the workflows leverage central path parsing logic.

e.g kpt pkg update hello3 works fine.

I am working on identifying and fixing the leaks by unifying the path logic.

@phanimarupaka
Copy link
Contributor

Found a serious bug while working on this issue. If we trigger kpt live apply on symlink, it is pruning all resources

➜  foo git:(main) ✗ kpt live apply wordpress
deployment.apps/wordpress-mysql unchanged
persistentvolumeclaim/mysql-pv-claim unchanged
persistentvolumeclaim/wp-pv-claim unchanged
service/wordpress unchanged
service/wordpress-mysql unchanged
deployment.apps/wordpress unchanged
6 resource(s) applied. 0 created, 6 unchanged, 0 configured, 0 failed
➜  foo git:(main) ✗ kpt live apply wordpress-link
deployment.apps/wordpress-mysql pruned
deployment.apps/wordpress pruned
service/wordpress-mysql pruned
service/wordpress pruned
persistentvolumeclaim/wp-pv-claim pruned
persistentvolumeclaim/mysql-pv-claim pruned
6 resource(s) pruned, 0 skipped, 0 failed

I am investigating and working on the fix as part of this issue. Overall, this issue/feature will be included in the next kpt release.

@mortent
Copy link
Contributor

mortent commented Aug 27, 2021

So I think this feature requires more than just making sure kpt follows symlinks. Some questions we should answer:

  • Are there any constraints on following symlinks and what are the security implications? Symlinks can point to anywhere on the users filesystem, so it is not clear to me if we should blindly follow all symlinks. In particular since users might fetch a package with symlinks from someone else and might not be aware that there are symlinks pointing outside the package.
  • How should we handle broken symlinks?
  • What does this mean for support on Windows and other filesystems that doesn't support symlinks?
  • Git treatment of symlinks is actually dependent on a configuration option: https://git-scm.com/docs/git-config#Documentation/git-config.txt-coresymlinks. Is this something kpt should check if encountering symlinks?

@phanimarupaka
Copy link
Contributor

phanimarupaka commented Aug 30, 2021

@mortent @droot

So I think this feature requires more than just making sure kpt follows symlinks. Some questions we should answer:

  • Are there any constraints on following symlinks and what are the security implications? Symlinks can point to anywhere on the users filesystem, so it is not clear to me if we should blindly follow all symlinks. In particular since users might fetch a package with symlinks from someone else and might not be aware that there are symlinks pointing outside the package.

I see that the current use-case is only for the symlinks for the root package path. So we need not parse the symlinks for the underlying files/directories. This will avoid the security implications where directories/files within the package are pointing to the files outside the package. We just don't parse them. If needed we can develop those features incrementally based on the concrete use-cases we see in the future.

  • How should we handle broken symlinks?

Since we are only concerned about the symlink to the root package directory, we can error out if the input symlink is broken.

  • What does this mean for support on Windows and other filesystems that doesn't support symlinks?

We just don't support them.

I don't think this should be tied to git configs as there might be commands like kpt pkg init which doesn't need a git context at all.

@droot
Copy link
Contributor

droot commented Aug 31, 2021

@phanimarupaka as @mortent has already mentioned the reasons that makes this problem lot harder.
Resolving the arg for the top level pkg dir (if its a symlink) in kpt commands is the best we can do at this point.

something related: in future, we want to support other location targets for packages such as OCI artifacts, GCS storage buckets and use of symlinks in the package makes it less portable.

@phanimarupaka phanimarupaka linked a pull request Sep 4, 2021 that will close this issue
@phanimarupaka
Copy link
Contributor

This feature will be supported in beta.5 release of kpt.

@chasecaleb
Copy link

@phanimarupaka Are you sure this is supported now? I'm on kpt 1.0.0-beta.14 and symlinks still appear to be ignored. Here's an example:

caleb@work-vmware> kpt pkg tree
Package "test1"
└── [Kptfile]  Kptfile kptfile
caleb@work-vmware> ln -s ../../base/ ./base
caleb@work-vmware> kpt pkg tree            
Package "test1"
└── [Kptfile]  Kptfile kptfile
caleb@work-vmware> rm base
caleb@work-vmware> cp -r ../../base/ base
caleb@work-vmware> kpt pkg tree          
Package "test1"
├── [Kptfile]  Kptfile kptfile
└── Package "base"
    ├── [Kptfile]  Kptfile kptfile
    ├── [cluster-autoscaler.yaml]  ClusterRole cluster-autoscaler
    ├── [cluster-autoscaler.yaml]  ClusterRoleBinding cluster-autoscaler
    ├── [cluster-autoscaler.yaml]  Deployment kube-system/cluster-autoscaler
    ├── [cluster-autoscaler.yaml]  Role kube-system/cluster-autoscaler
    ├── [cluster-autoscaler.yaml]  RoleBinding kube-system/cluster-autoscaler
    ├── [kpt-annotations.yaml]  ConfigMap kpt-annotations
    └── [kpt-labels.yaml]  ConfigMap kpt-labels
caleb@work-vmware> kpt version
1.0.0-beta.14

@phanimarupaka
Copy link
Contributor

cc @droot

@rahuldhir
Copy link

@phanimarupaka @droot any update on support for this? The lack of symlink support makes it difficult to have different Kptfiles that rely on a common base.

If you do not plan to support symlinks, how do you advise setting up environment-specific Kptfiles (e.g. separate Kptfiles for dev/stg/prd) that each rely on a common base configuration?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/pkg p1 size/L 4 day triaged Issue has been triaged by adding an `area/` label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants