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

[JENKINS-56114] Correct behavior for Windows Server 2016 with Docker #3914

Merged

Conversation

Wadeck
Copy link
Contributor

@Wadeck Wadeck commented Feb 26, 2019

  • also correct the isIllegalSymlink that was using the full isDescendant, that is not required there

See JENKINS-56114.

Proposed changelog entries

  • Correct an issue occuring on Windows Server 2016 with MSFT Docker that prevent the workspace and the artifact to be browse as expected

The changelogs will be integrated by the core maintainers after the merge. See the changelog examples here: https://jenkins.io/changelog/ -->

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • [n/a] Appropriate autotests or explanation to why this change has no tests
  • [n/a] For dependency updates: links to external changelogs and, if possible, full diffs

Copy link
Contributor

@jeffret-b jeffret-b left a comment

Choose a reason for hiding this comment

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

If it works, it seems fine. I haven't been able to figure out any further issues this might cause.

It's such a specific environment that it's hard to make any specific, meaningful test. If manual and usage testing demonstrate it works for this environment, then it makes sense to include. I don't see any way it should disrupt other or more common environments.

core/src/main/java/hudson/FilePath.java Outdated Show resolved Hide resolved
@Wadeck
Copy link
Contributor Author

Wadeck commented Mar 1, 2019

Tested by Jeremy Kam (initial reporter) and it works fine!

return false;
}
} catch (NoSuchFileException e) {
// nonexistent file
// nonexistent file / Windows Server 2016 + MSFT docker
Copy link

Choose a reason for hiding this comment

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

I guess this is not the case anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

toRealPath will never work under Windows Server 2016 + MSFT Docker (perhaps other env) when the root is a mounted folder.

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Mar 2, 2019
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

I do not see anything wrong here for JENKINS-56114, but I would highly recommend explicitly checking the platform before trying to apply a workaround. Otherwise relaxedToRealPath() may suppress real exceptions and cause unexpected behavior on other platforms.

@oleg-nenashev oleg-nenashev removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Mar 2, 2019
@Wadeck
Copy link
Contributor Author

Wadeck commented Mar 4, 2019

@oleg-nenashev I added a condition on Windows for the specific code.

core/src/main/java/hudson/FilePath.java Outdated Show resolved Hide resolved
@daniel-beck daniel-beck added the regression-fix Pull request that fixes a regression in one of the previous Jenkins releases label Mar 5, 2019
@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Mar 6, 2019
@oleg-nenashev
Copy link
Member

Will merge it tomorrow if there is no negative feedback

@oleg-nenashev oleg-nenashev merged commit 5562b2f into jenkinsci:master Mar 8, 2019
olivergondza pushed a commit that referenced this pull request Mar 18, 2019
…3914)

* [JENKINS-56114] Correct behavior for Windows Server 2016 with Docker
- also adjust the symlink escape hatch

* Correct missing space + retrigger build

* Improve performance with a check on the logger level

* Run the specific code only under Windows

* Rename the method as it's used only for Windows

(cherry picked from commit 5562b2f)
ad22 pushed a commit to ad22/jenkins that referenced this pull request Mar 21, 2019
…enkinsci#3914)

* [JENKINS-56114] Correct behavior for Windows Server 2016 with Docker
- also adjust the symlink escape hatch

* Correct missing space + retrigger build

* Improve performance with a check on the logger level

* Run the specific code only under Windows

* Rename the method as it's used only for Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback regression-fix Pull request that fixes a regression in one of the previous Jenkins releases
Projects
None yet
7 participants