Add platform property to files entitlement #123212
Conversation
Some file paths are OS specific. This commit adds a `platform` property to each file in a files entitlement that can be used to limit that file to a specific platform.
rjernst
added
>refactoring
auto-backport
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
ldematte
left a comment
ldematte
left a comment
There was a problem hiding this comment.
It looks good; my main concern is around where we check and how we can have an intermediate, non "final" FileData. Should we just have overloads of ofRelativePath etc. with the platform?
Also related: should we always enforce that we have a platform with absolute paths? Our current usage of it seems to say so, and I can't think of an absolute path that makes sense across all platforms.
|
|
||
| Path relativePath = Path.of(relativePathAsString); | ||
| if (relativePath.isAbsolute()) { | ||
| if ((platform == null || platform.isCurrent()) && relativePath.isAbsolute()) { |
There was a problem hiding this comment.
I kind of liked the fluent interface, but this means that validation is moved here, at policy parsing; this would not catch any error in the server policy, for example.
There was a problem hiding this comment.
Scratch that, it seems we don't want to do it due to libraries checking for absolute paths regardless of the OS (netty). I pushed a change for that, to skip the absolute/relative check if the path is for "any" platform (null).
...rc/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlement.java
Show resolved
Hide resolved
I stand corrected: I saw your message on Netty. That would be a case for platform = null & absolute paths, and (unfortunately), skip absolute vs relative in that case :( |
This reverts commit d81923c.
| if (os.startsWith("Linux")) { | ||
| return LINUX; | ||
| } else if (os.startsWith("Mac OS")) { | ||
| return MACOS; |
There was a problem hiding this comment.
I suspect we'll want a UNIX covering both LINUX and MACOS at some point.
There was a problem hiding this comment.
I had a version of this (calling it POSIX because these aren't actually unix), but I'm unsure it will actually be necessary because the location of system files on linux/mac are usually very different, even though they begin with /.
There was a problem hiding this comment.
Ok. FWIW the JDK calls it the UnixFileSystemProvider.
|
|
||
| private static final Platform current = findCurrent(); | ||
|
|
||
| private static Platform findCurrent() { |
There was a problem hiding this comment.
Does Elasticsearch already have a way to determine the platform? (Are we really the first to need this?)
If it does, should we use that instead of parsing the os.name system property?
There was a problem hiding this comment.
There are several places we repeat this logic, including inside Lucene. Here we don't have access to Lucene classes. I originally had a comment in this change to move this logic to elasticsearch.core, but I removed it. We could consider moving it still, though, but as a followup.
💔 Backport failed
You can use sqren/backport to manually backport by running |
Some file paths are OS specific. This commit adds a `platform` property to each file in a files entitlement that can be used to limit that file to a specific platform. Co-authored-by: Moritz Mack <[email protected]> Co-authored-by: Lorenzo Dematte <[email protected]>
Some file paths are OS specific. This commit adds a `platform` property to each file in a files entitlement that can be used to limit that file to a specific platform. Co-authored-by: Moritz Mack <[email protected]> Co-authored-by: Lorenzo Dematte <[email protected]>
Some file paths are OS specific. This commit adds a `platform` property to each file in a files entitlement that can be used to limit that file to a specific platform. Co-authored-by: Moritz Mack <[email protected]> Co-authored-by: Lorenzo Dematte <[email protected]>
Some file paths are OS specific. This commit adds a `platform` property to each file in a files entitlement that can be used to limit that file to a specific platform. Co-authored-by: Moritz Mack <[email protected]> Co-authored-by: Lorenzo Dematte <[email protected]>
Some file paths are OS specific. This commit adds a `platform` property to each file in a files entitlement that can be used to limit that file to a specific platform. Co-authored-by: Moritz Mack <[email protected]> Co-authored-by: Lorenzo Dematte <[email protected]>
Some file paths are OS specific. This commit adds a `platform` property to each file in a files entitlement that can be used to limit that file to a specific platform. Co-authored-by: Moritz Mack <[email protected]> Co-authored-by: Lorenzo Dematte <[email protected]>
5 participants
Some file paths are OS specific. This commit adds a
platformproperty to each file in a files entitlement that can be used to limit that file to a specific platform.Relates to ES-10912