- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25.6k
 
Add exclusive file entitlement for settings #125272
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 5 commits
c8b5fc9
              8cc616c
              433450a
              664d53c
              f1d3256
              3d395c9
              a7e53b3
              abb7ec5
              bf189a0
              862646d
              7d2fb8d
              8e18e3f
              4c4386c
              69ecd93
              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 | 
|---|---|---|
| 
          
            
          
           | 
    @@ -40,9 +40,12 @@ | |
| import java.io.InputStream; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.nio.file.attribute.BasicFileAttributes; | ||
| import java.nio.file.attribute.FileTime; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.concurrent.ExecutionException; | ||
| import java.util.stream.Stream; | ||
| 
     | 
||
| import static org.elasticsearch.health.HealthStatus.GREEN; | ||
| import static org.elasticsearch.health.HealthStatus.YELLOW; | ||
| 
          
            
          
           | 
    @@ -128,7 +131,7 @@ public void handleSnapshotRestore(ClusterState clusterState, Metadata.Builder md | |
| // since we don't know the current operator configuration, e.g. file settings could be disabled | ||
| // on the target cluster. If file settings exist and the cluster state has lost it's reserved | ||
| // state for the "file_settings" namespace, we touch our file settings file to cause it to re-process the file. | ||
| if (watching() && Files.exists(watchedFile)) { | ||
| if (watching() && filesExists(watchedFile)) { | ||
| if (fileSettingsMetadata != null) { | ||
| ReservedStateMetadata withResetVersion = new ReservedStateMetadata.Builder(fileSettingsMetadata).version(0L).build(); | ||
| mdBuilder.put(withResetVersion); | ||
| 
          
            
          
           | 
    @@ -201,7 +204,7 @@ protected XContentParser createParser(InputStream stream) throws IOException { | |
| 
     | 
||
| private void processFileChanges(ReservedStateVersionCheck versionCheck) throws IOException, InterruptedException, ExecutionException { | ||
| PlainActionFuture<Void> completion = new PlainActionFuture<>(); | ||
| try (var bis = new BufferedInputStream(Files.newInputStream(watchedFile)); var parser = createParser(bis)) { | ||
| try (var bis = new BufferedInputStream(filesNewInputStream(watchedFile)); var parser = createParser(bis)) { | ||
| stateService.process(NAMESPACE, parser, versionCheck, (e) -> completeProcessing(e, completion)); | ||
| } | ||
| completion.get(); | ||
| 
          
            
          
           | 
    @@ -343,4 +346,37 @@ public synchronized HealthIndicatorResult calculate(boolean verbose, int maxAffe | |
| } | ||
| } | ||
| } | ||
| 
     | 
||
| // the following methods are a workaround to ensure exclusive access for files | ||
| // required by child watchers; this is required because we only check the caller's module | ||
| // not the entire stack | ||
| @Override | ||
| protected boolean filesExists(Path path) { | ||
| return Files.exists(path); | ||
| } | ||
| 
     | 
||
| @Override | ||
| protected boolean filesIsDirectory(Path path) { | ||
| return Files.isDirectory(path); | ||
| } | ||
| 
     | 
||
| @Override | ||
| protected <A extends BasicFileAttributes> A filesReadAttributes(Path path, Class<A> clazz) throws IOException { | ||
| return Files.readAttributes(path, clazz); | ||
| } | ||
| 
     | 
||
| @Override | ||
| protected Stream<Path> filesList(Path dir) throws IOException { | ||
| return Files.list(dir); | ||
| } | ||
| 
     | 
||
| @Override | ||
| protected Path filesSetLastModifiedTime(Path path, FileTime time) throws IOException { | ||
| return Files.setLastModifiedTime(path, time); | ||
| 
         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. Not sure how this one is working right now since we only have read access. IIRC this is an edge case for when we restore from a snapshot. We should (as a followup) re-consider how restoring from snapshots should interact with file settings because this breaks the rule that we want to use, only allowing read access to the config directory. 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. Agreed.  | 
||
| } | ||
| 
     | 
||
| @Override | ||
| protected InputStream filesNewInputStream(Path path) throws IOException { | ||
| return Files.newInputStream(path); | ||
| } | ||
| } | ||
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.
Unfortunately I think this needs to be READ_WRITE, see my other comment.
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.
Changed.