fix: create the metastore backups directory if it does not exist #5879
Conversation
| @@ -222,4 +225,50 @@ private BackupReplayFile newReplayFile() { | |||
| ? Optional.of(new BackupReplayFile(latestBakFile)) | |||
| : Optional.empty(); | |||
| } | |||
|
|
|||
| private void ensureDirectoryExists(final File backupsDir) { | |||
There was a problem hiding this comment.
nit: can we de-dup this code from enforceStreamStateDirAvailability() in KsqlServerMain? Not required if it's annoying to de-dup (especially since we're trying to keep this patch small in order to cherry-pick) but it's weird to me that we have the same code in two places.
There was a problem hiding this comment.
Right, I see it. But I have the extra step to restrict the permissions to the KSQL server user only. I'd like to do that in the state directory too, but I don't know why the directory is not too restrictive. I'd rather investigate more and do it in another follow-up PR.
| } | ||
|
|
||
| try { | ||
| Files.setPosixFilePermissions(backupsDir.toPath(), |
There was a problem hiding this comment.
Does it make sense to explicitly set permissions here? I see we don't do that for the Kafka Streams state directory. I'm in favor of consistency unless there's a good reason to deviate.
There was a problem hiding this comment.
I'd like to keep the metastore directory restricted from other uses to access the data. By default, the directory created has drwxrwxr-x which is opened to any user in the system. It's up to the user to specify a directory that has restricted permissions only to the required users, but if KSQL creates it automatically, then I set those here too.
I don't know why the streams state directory is not protected. We might want to do that. I won't do it in this patch until I understand their reason.
| ); | ||
| } | ||
|
|
||
| if (!backupsDir.canWrite() || !backupsDir.canRead() || !backupsDir.canExecute()) { |
There was a problem hiding this comment.
Why do we need execute permissions? I see we have a similar check for the Kafka Streams state directory so I assume there's good reason. Curious what it is.
There was a problem hiding this comment.
Directories need to have execute permissions in order to create files in it. By default, a new directory has rwx. If you remove the x, then you get a permissions denied.
$ mkdir /tmp/dir
$ chmod a-x /tmp/dir
$ ls -dl /tmp/dir
drw-rw-r-- 2 sergio sergio 4096 Jul 24 15:46 /tmp/dir
$ echo file > /tmp/dir/file
bash: /tmp/dir/file: Permission denied
$
Description
Creates the KSQL metastore backups directory if it does not exist. It also checks the existing directory is readable and writeable.
Testing done
Unit tests
Reviewer checklist