pkg/system: deprecate MkdirAll and remove custom volume GUID handling #49162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
commit 86d1223 introduced a custom version of `os.MkdirAll` for Windows to account for situations where the path to create would start with a Windows volume name (GUID path), for example, `"\\?\Volume{4c1b02c1-d990-11dc-99ae-806e6f6e6963}\`. At the time that patch was added we were using [go1.4.2], which did not have special handling for Windows in [MkdirAll], therefore would recognize such paths as regular paths, trying to create them, which would fail. This code was later updated in 46ec4c1 to provide ACL (DACL) support on Windows. Further updates were made in cfef1b1 and 55ceb50 to allow for an early return when detecting a volume GUID path, and the code was re-aligned with the latest (go1.19.2) implementation in f058afc, which brought in the platform-specific [fixRootDirectory] handling introduced in go1.11. While that enhancement detected UNC volume-paths (`\\?c\`, `//?/c:`), it did not yet support volume GUID paths. go1.22, through [golang.org/cl/86295] added support for this, and `os.MkdirAll` now natively detects volume GUID paths, making our own implementation for this redundant. This patch: - Deprecates pkg/system.MkdirAll in favor of os.MkdirAll, which now provides the same functionality on go1.22 and up. - Renames the (non-exported) `mkdirall` function to `mkdirAllWithACL`, and synchronises `it` with the [implementation in go1.23.4], bringing in the changes from [golang.org/cl/86295] and [golang.org/cl/582499]. - Adds a fast path to `MkdirAllWithACL` if no ACL / SDDL is provided. It's worth noting that we currently still support go1.22, and that the implementation changed in go1.23; those changes ([golang.org/cl/581517] and [golang.org/cl/566556]) were lateral moves, therefore should be identical to the implementation in go1.22, and we can safely use the implementation provided by [filepath.VolumeName] on either go1.22 or go1.23. [go1.4.2]: https://github.com/moby/moby/blob/86d1223a29907ffc6afba557b5138cfad7816bb4/Dockerfile#L77 [MkdirAll]: https://github.com/golang/go/blob/go1.4.2/src/os/path.go#L19-L60 [fixRootDirectory]: golang/go@b86e766 [golang.org/cl/86295]: golang/go@cd589c8 [golang.org/cl/582499]: golang/go@5616ab6 [golang.org/cl/581517]: golang/go@ad22356 [golang.org/cl/566556]: golang/go@ceef063 [1]: https://github.com/golang/go/blob/go1.23.4/src/os/path.go#L12-L66 [filepath.VolumeName]: https://pkg.go.dev/path/filepath#VolumeName Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
added
platform/windows
status/2-code-review
impact/changelog
impact/deprecation
kind/refactor
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This was referenced Dec 21, 2024
vvoland
approved these changes
Dec 23, 2024
|
I'll bring this one in, and will do a quick rebase of #49087, which also contained one change swapping this out. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
relates to
/pkg#32989pkg/system: deprecate MkdirAll and remove custom volume GUID handling
commit 86d1223 introduced a custom version of
os.MkdirAllfor Windows to account for situations where the path to create would start with a Windows volume name (GUID path), for example,"\\?\Volume{4c1b02c1-d990-11dc-99ae-806e6f6e6963}\. At the time that patch was added we were using go1.4.2, which did not have special handling for Windows in MkdirAll, therefore would recognize such paths as regular paths, trying to create them, which would fail.This code was later updated in 46ec4c1 to provide ACL (DACL) support on Windows.
Further updates were made in cfef1b1 and 55ceb50 to allow for an early return when detecting a volume GUID path, and the code was re-aligned with the latest (go1.19.2) implementation in f058afc, which brought in the platform-specific fixRootDirectory handling introduced in go1.11. While that enhancement detected UNC volume-paths (
\\?c\,//?/c:), it did not yet support volume GUID paths.go1.22, through golang.org/cl/86295 added support for this, and
os.MkdirAllnow natively detects volume GUID paths, making our own implementation for this redundant.This patch:
mkdirallfunction tomkdirAllWithACL, and synchronisesitwith the [implementation in go1.23.4], bringing in the changes from golang.org/cl/86295 and golang.org/cl/582499.MkdirAllWithACLif no ACL / SDDL is provided.It's worth noting that we currently still support go1.22, and that the implementation changed in go1.23; those changes (golang.org/cl/581517 and golang.org/cl/566556) were lateral moves, therefore should be identical to the implementation in go1.22, and we can safely use the implementation provided by filepath.VolumeName on either go1.22 or go1.23.
remove uses of deprecated system.MkdirAll
- What I did
- How I did it
- How to verify it
- Description for the changelog
- Go SDK: pkg/system: deprecate MkdirAll. This function provided custom handling for Windows GUID volume paths. Handling for such paths is now supported by go stdlib in go1.22 and higher, and this function is now an alias for os.MkdirAll, which should be used instead. This alias will be removed in the next release.- A picture of a cute animal (not mandatory but encouraged)