-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Refactor file path resolution for entitlements #127040
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
Changes from 4 commits
60fc0ff
01a2ec0
7be175e
1e3bf01
a69c72e
d7ac08f
48e1e62
e037e9c
aebfe8b
5fddb31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,8 @@ | |
import static java.util.Comparator.comparing; | ||
import static org.elasticsearch.core.PathUtils.getDefaultFileSystem; | ||
import static org.elasticsearch.entitlement.runtime.policy.FileUtils.PATH_ORDER; | ||
import static org.elasticsearch.entitlement.runtime.policy.PathLookup.BaseDir.CONFIG; | ||
import static org.elasticsearch.entitlement.runtime.policy.PathLookup.BaseDir.TEMP; | ||
import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.Mode.READ_WRITE; | ||
|
||
public final class FileAccessTree { | ||
|
@@ -166,9 +168,9 @@ private FileAccessTree( | |
} | ||
|
||
// everything has access to the temp dir, config dir, to their own dir (their own jar files) and the jdk | ||
addPathAndMaybeLink.accept(pathLookup.tempDir(), READ_WRITE); | ||
addPathAndMaybeLink.accept(pathLookup.getBaseDirPaths(TEMP).findFirst().get(), READ_WRITE); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we need to add all of the possible temp dirs? Eg for an ESIntegTestCase, each virtual node could have its own temp dir. Same for config below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! Fixed. |
||
// TODO: this grants read access to the config dir for all modules until explicit read entitlements can be added | ||
addPathAndMaybeLink.accept(pathLookup.configDir(), Mode.READ); | ||
addPathAndMaybeLink.accept(pathLookup.getBaseDirPaths(CONFIG).findFirst().get(), Mode.READ); | ||
if (componentPath != null) { | ||
addPathAndMaybeLink.accept(componentPath, Mode.READ); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,14 +10,28 @@ | |
package org.elasticsearch.entitlement.runtime.policy; | ||
|
||
import java.nio.file.Path; | ||
import java.util.function.Function; | ||
import java.util.stream.Stream; | ||
|
||
public record PathLookup( | ||
Path homeDir, | ||
Path configDir, | ||
Path[] dataDirs, | ||
Path[] sharedRepoDirs, | ||
Path tempDir, | ||
Function<String, Stream<String>> settingResolver | ||
) {} | ||
// TODO: (jack) new a need interface, but can still be a record | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The todo seems like it can be removed, but a simple javadoc would be good There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added. |
||
// interface should be a single method that takes in an enum and returns | ||
// a stream of paths | ||
public interface PathLookup { | ||
enum BaseDir { | ||
HOME, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think this could be confused with Elasticsearch home directory, can we call this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed. |
||
CONFIG, | ||
DATA, | ||
SHARED_REPO, | ||
LIB, | ||
MODULES, | ||
PLUGINS, | ||
LOGS, | ||
TEMP, | ||
PID | ||
} | ||
|
||
Stream<Path> getBaseDirPaths(BaseDir baseDir); | ||
|
||
Stream<Path> resolveRelativePaths(BaseDir baseDir, Path relativePath); | ||
|
||
Stream<Path> resolveSettingPaths(BaseDir baseDir, String settingName); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting case. We want to know if a PID file was specified. Could we do this within resolution in the pathlookup? In this case, if there is no pid file, there should be no path resolved (though a file data would exist for it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Special cased this to have a getter method.