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

Jetty Deploy scan / symlink behavior is broken #6114

Closed
joakime opened this issue Mar 29, 2021 · 7 comments · Fixed by #6317
Closed

Jetty Deploy scan / symlink behavior is broken #6114

joakime opened this issue Mar 29, 2021 · 7 comments · Fixed by #6317
Labels
Bug For general bugs on Jetty side

Comments

@joakime
Copy link
Contributor

joakime commented Mar 29, 2021

Jetty version
9.4.39.v20210325

Java version
Java 11

OS type/version
Linux / OSX

Description

Lets say we have a ${jetty.base} that has 3 entries in webapps/, all symlinks, pointing to the same exploded webapp.

[symlink-manyapps-base]$ tree -F
.
├── app/
│   └── WEB-INF/
│       └── classes/
│           └── org/
│               └── eclipse/
│                   └── jetty/
│                       └── demo/
│                           └── ContextInfoServlet.class
├── start.d/
│   ├── deploy.ini
│   └── http.ini
└── webapps/
    ├── bar -> ../app/
    ├── foo -> ../app/
    └── zed -> ../app/

12 directories, 3 files

When we try to deploy this setup, we see that the symlink directory names are not honored.
And it seems that it only detects 1 webapp, of the 3 desired to be deployed.

[symlink-manyapps-base]$ java -jar ~/jetty/distros/jetty-home-9.4.39.v20210325/start.jar 
2021-03-29 14:28:26.369:INFO::main: Logging initialized @356ms to org.eclipse.jetty.util.log.StdErrLog
2021-03-29 14:28:26.590:INFO:oejs.Server:main: jetty-9.4.39.v20210325; built: 2021-03-25T14:42:11.471Z; git: 9fc7ca5a922f2a37b84ec9dbc26a5168cee7e667; jvm 11.0.10+9
2021-03-29 14:28:26.602:INFO:oejdp.ScanningAppProvider:main: Deployment monitor [file:///home/joakim/code/jetty/distros/bases/symlink-manyapps-base/webapps/] at interval 1
2021-03-29 14:28:26.659:INFO:oejw.StandardDescriptorProcessor:main: NO JSP Support for /app, did not find org.eclipse.jetty.jsp.JettyJspServlet
2021-03-29 14:28:26.665:INFO:oejs.session:main: DefaultSessionIdManager workerName=node0
2021-03-29 14:28:26.665:INFO:oejs.session:main: No SessionScavenger set, using defaults
2021-03-29 14:28:26.665:INFO:oejs.session:main: node0 Scavenging every 600000ms
2021-03-29 14:28:26.683:INFO:oejsh.ContextHandler:main: Started o.e.j.w.WebAppContext@7c37508a{app,/app,file:///home/joakim/code/jetty/distros/bases/symlink-manyapps-base/app/,AVAILABLE}{/home/joakim/code/jetty/distros/bases/symlink-manyapps-base/app}
2021-03-29 14:28:26.700:INFO:oejs.AbstractConnector:main: Started ServerConnector@599a3b86{HTTP/1.1, (http/1.1)}{0.0.0.0:8080}
2021-03-29 14:28:26.701:INFO:oejs.Server:main: Started @689ms

Expected results are that there are 3 webapps deployed.

  • /bar
  • /foo
  • /zed
@joakime joakime added the Bug For general bugs on Jetty side label Mar 29, 2021
@gregw
Copy link
Contributor

gregw commented Mar 29, 2021

This is strange, because the PR that eventually got merged didn't canonicalize the discovered files and only used sameFile to compare them to the root, so the basic approach should work fine for symlinks like you've shown.

Perhaps a canonicalization was left in somewhere?

@gregw
Copy link
Contributor

gregw commented Mar 29, 2021

The scanner calls getCanonicalPath on all discovered files (after calling the filter that was recently fixed). This has been this way since at least Nov 2019. Changing that to getAbsolutePath appears to fix this problem, but I'm cautious about such changes. I suggest we change in 10, test, then only after a release that proves it is stable, do we consider cherry-picking back to 9

@joakime
Copy link
Contributor Author

joakime commented Apr 12, 2021

joakime added a commit that referenced this issue Apr 12, 2021
+ Using getAbsolutePath not canonical (too many corner cases with it to be reliable)

Signed-off-by: Joakim Erdfelt <[email protected]>
joakime added a commit that referenced this issue Apr 26, 2021
gregw added a commit that referenced this issue May 24, 2021
Use Path.toRealPath rather than getCanonicalPath in the Scanner
Make following symlinks configurable

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 24, 2021
fix from review

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 24, 2021
updates from review

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 24, 2021
updates from review

Signed-off-by: Greg Wilkins <[email protected]>
sbordet pushed a commit that referenced this issue Jun 4, 2021
* Fix #6114 Deploy symlink webapps

Use Path.toRealPath rather than getCanonicalPath in the Scanner
Make following symlinks configurable

Signed-off-by: Greg Wilkins <[email protected]>
@joakime
Copy link
Contributor Author

joakime commented Jun 4, 2021

Reopening, as the Scanner is still broken for symlink behavior.

@joakime joakime reopened this Jun 4, 2021
@gregw
Copy link
Contributor

gregw commented Jun 4, 2021

Can you explain why?

@joakime
Copy link
Contributor Author

joakime commented Jun 5, 2021

There's 2 fundamental issues:

  1. What's being tracked and reported isn't correct when working with symlinks. (it's easy to have entries linger that should not be tracked anymore)
    • The symlink target is changed, the old symlink is target is not removed from the tracked list and still reports as changed if it's changed (it should be removed and no longer tracked)
    • multi depth symlinks is another case. where the 2nd (or deeper) link was changed to a different location.
  2. There's situations that still crashes the Scanner to the point where it will no longer be running.
    • the scanner doesn't take into account invalid link / file not found cases (invalid data and unhandled exceptions), eg: where a link points to something that isn't valid or is outright missing. or a mount point is failing, or a network share is flaky, etc.
    • the scanner doesn't take into account directory permission (exceptions caused by file/dir permission issues)

@gregw
Copy link
Contributor

gregw commented Jun 21, 2021

I'm closing this because the PR has gone into a release and I think the problems raised are about how to fail nicer if there is a bad symlink, which is a different issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants