You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/01/15 09:10:26 UTC
[GitHub] [kafka] showuon opened a new pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
showuon opened a new pull request #9904:
URL: https://github.com/apache/kafka/pull/9904
We improved the state directory folder/file permission setting in #9583 . But we forgot to consider one situation: if user doesn't have Persistent Stores, we won't create base dir and state dir. And if there's no such dir, and when we tried to set permission to them, we'll have `NoSuchFileException`.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#discussion_r559866934
##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
##########
@@ -545,9 +546,13 @@ public void shouldCleanupAllTaskDirectoriesIncludingGlobalOne() {
@Test
public void shouldNotCreateBaseDirectory() throws IOException {
- initializeStateDirectory(false);
- assertFalse(stateDir.exists());
- assertFalse(appDir.exists());
+ try (final LogCaptureAppender appender = LogCaptureAppender.createAndRegister(StateDirectory.class)) {
+ initializeStateDirectory(false);
+ assertFalse(stateDir.exists());
+ assertFalse(appDir.exists());
Review comment:
Updated. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#issuecomment-784463071
I think at the time we weren't expecting there to be a 2.6.2 release, but as it turns out I'm actually actively in the process of releasing 2.6.2. I haven't cut the RC yet, so I'll go ahead and pull this fix in as well. Thanks for raising it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#issuecomment-763886188
Also cherrypicked to 2.7
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#discussion_r558097187
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -94,27 +94,32 @@ public StateDirectory(final StreamsConfig config, final Time time, final boolean
this.appId = config.getString(StreamsConfig.APPLICATION_ID_CONFIG);
final String stateDirName = config.getString(StreamsConfig.STATE_DIR_CONFIG);
final File baseDir = new File(stateDirName);
- if (this.hasPersistentStores && !baseDir.exists() && !baseDir.mkdirs()) {
- throw new ProcessorStateException(
- String.format("base state directory [%s] doesn't exist and couldn't be created", stateDirName));
- }
stateDir = new File(baseDir, appId);
- if (this.hasPersistentStores && !stateDir.exists() && !stateDir.mkdir()) {
- throw new ProcessorStateException(
- String.format("state directory [%s] doesn't exist and couldn't be created", stateDir.getPath()));
- }
- if (hasPersistentStores && stateDirName.startsWith("/tmp")) {
- log.warn("Using /tmp directory in the state.dir property can cause failures with writing the checkpoint file" +
- " due to the fact that this directory can be cleared by the OS");
- }
- final Path basePath = Paths.get(baseDir.getPath());
- final Path statePath = Paths.get(stateDir.getPath());
- final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");
- try {
- Files.setPosixFilePermissions(basePath, perms);
- Files.setPosixFilePermissions(statePath, perms);
- } catch (final IOException e) {
- log.error("Error changing permissions for the state or base directory {} ", stateDir.getPath(), e);
+
+ if (this.hasPersistentStores) {
+ if (!baseDir.exists() && !baseDir.mkdirs()) {
+ throw new ProcessorStateException(
+ String.format("base state directory [%s] doesn't exist and couldn't be created", stateDirName));
+ }
+ if (!stateDir.exists() && !stateDir.mkdir()) {
+ throw new ProcessorStateException(
+ String.format("state directory [%s] doesn't exist and couldn't be created", stateDir.getPath()));
+ }
+ if (stateDirName.startsWith("/tmp")) {
+ log.warn("Using /tmp directory in the state.dir property can cause failures with writing the checkpoint file" +
+ " due to the fact that this directory can be cleared by the OS");
+ }
+
+ // change the dir permission to "rwxr-x---" to avoid world readable
+ final Path basePath = Paths.get(baseDir.getPath());
+ final Path statePath = Paths.get(stateDir.getPath());
+ final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");
+ try {
+ Files.setPosixFilePermissions(basePath, perms);
+ Files.setPosixFilePermissions(statePath, perms);
+ } catch (final IOException e) {
+ log.warn("Error changing permissions for the state or base directory {} ", stateDir.getPath(), e);
Review comment:
Change from `error` to `warn` since I don't think it'll be an real error if changing permission failed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a change in pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
cadonna commented on a change in pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#discussion_r559470824
##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
##########
@@ -545,9 +546,13 @@ public void shouldCleanupAllTaskDirectoriesIncludingGlobalOne() {
@Test
public void shouldNotCreateBaseDirectory() throws IOException {
- initializeStateDirectory(false);
- assertFalse(stateDir.exists());
- assertFalse(appDir.exists());
+ try (final LogCaptureAppender appender = LogCaptureAppender.createAndRegister(StateDirectory.class)) {
+ initializeStateDirectory(false);
+ assertFalse(stateDir.exists());
+ assertFalse(appDir.exists());
Review comment:
nit:
```suggestion
assertThat(stateDir.exists(), is(false));
assertThat(appDir.exists(), is(false));
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#discussion_r559866768
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -94,27 +94,32 @@ public StateDirectory(final StreamsConfig config, final Time time, final boolean
this.appId = config.getString(StreamsConfig.APPLICATION_ID_CONFIG);
final String stateDirName = config.getString(StreamsConfig.STATE_DIR_CONFIG);
final File baseDir = new File(stateDirName);
- if (this.hasPersistentStores && !baseDir.exists() && !baseDir.mkdirs()) {
- throw new ProcessorStateException(
- String.format("base state directory [%s] doesn't exist and couldn't be created", stateDirName));
- }
stateDir = new File(baseDir, appId);
- if (this.hasPersistentStores && !stateDir.exists() && !stateDir.mkdir()) {
- throw new ProcessorStateException(
- String.format("state directory [%s] doesn't exist and couldn't be created", stateDir.getPath()));
- }
- if (hasPersistentStores && stateDirName.startsWith("/tmp")) {
- log.warn("Using /tmp directory in the state.dir property can cause failures with writing the checkpoint file" +
- " due to the fact that this directory can be cleared by the OS");
- }
- final Path basePath = Paths.get(baseDir.getPath());
- final Path statePath = Paths.get(stateDir.getPath());
- final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");
- try {
- Files.setPosixFilePermissions(basePath, perms);
- Files.setPosixFilePermissions(statePath, perms);
- } catch (final IOException e) {
- log.error("Error changing permissions for the state or base directory {} ", stateDir.getPath(), e);
+
+ if (this.hasPersistentStores) {
+ if (!baseDir.exists() && !baseDir.mkdirs()) {
+ throw new ProcessorStateException(
+ String.format("base state directory [%s] doesn't exist and couldn't be created", stateDirName));
+ }
+ if (!stateDir.exists() && !stateDir.mkdir()) {
+ throw new ProcessorStateException(
+ String.format("state directory [%s] doesn't exist and couldn't be created", stateDir.getPath()));
+ }
+ if (stateDirName.startsWith("/tmp")) {
+ log.warn("Using /tmp directory in the state.dir property can cause failures with writing the checkpoint file" +
+ " due to the fact that this directory can be cleared by the OS");
+ }
+
+ // change the dir permission to "rwxr-x---" to avoid world readable
+ final Path basePath = Paths.get(baseDir.getPath());
+ final Path statePath = Paths.get(stateDir.getPath());
+ final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");
+ try {
+ Files.setPosixFilePermissions(basePath, perms);
+ Files.setPosixFilePermissions(statePath, perms);
+ } catch (final IOException e) {
+ log.warn("Error changing permissions for the state or base directory {} ", stateDir.getPath(), e);
Review comment:
I personally think it should not throw exception here because it won't block the stream running if we didn't change the dir permission successfully. But, if most users took the dir permission things seriously, maybe we should throw exception? Not sure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] modax commented on pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
modax commented on pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#issuecomment-783943018
Hi,
Is there any reason this is not in 2.6? 2.6.1 is affected by this problem.
Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#issuecomment-761959595
The failed tests:
```
Build / JDK 8 / org.apache.kafka.streams.integration.AdjustStreamThreadCountTest.shouldAddAndRemoveStreamThreadsWhileKeepingNamesCorrect
Build / JDK 8 / org.apache.kafka.streams.integration.AdjustStreamThreadCountTest.shouldRemoveStreamThread
Build / JDK 11 / org.apache.kafka.streams.integration.AdjustStreamThreadCountTest.shouldRemoveStreamThread
Build / JDK 11 / org.apache.kafka.streams.integration.AdjustStreamThreadCountTest.shouldRemoveStreamThread
Build / JDK 11 / org.apache.kafka.trogdor.agent.AgentTest.testAgentProgrammaticShutdown()
Build / JDK 15 / kafka.api.ConsumerBounceTest.testClose
Build / JDK 15 / kafka.api.CustomQuotaCallbackTest.testCustomQuotaCallback
Build / JDK 15 / kafka.log.LogCleanerIntegrationTest.testMarksPartitionsAsOfflineAndPopulatesUncleanableMetrics
Build / JDK 15 / org.apache.kafka.streams.integration.AdjustStreamThreadCountTest.shouldAddAndRemoveStreamThreadsWhileKeepingNamesCorrect
```
All are not related to my change. And the `AdjustStreamThreadCountTest` will be fixed in this PR: #9888, and the `kafka.api.CustomQuotaCallbackTest.testCustomQuotaCallback` will be addressed in this PR: #9777, ant others are flaky tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#issuecomment-763884833
Thanks @showuon , merged to trunk
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#issuecomment-763354494
failed tests:
```
Build / JDK 11 / org.apache.kafka.streams.integration.AdjustStreamThreadCountTest.shouldAddAndRemoveStreamThreadsWhileKeepingNamesCorrect
Build / JDK 11 / org.apache.kafka.streams.integration.AdjustStreamThreadCountTest.shouldRemoveStreamThread
Build / JDK 15 / org.apache.kafka.common.record.MemoryRecordsBuilderTest.[2] bufferOffset=0, compressionType=GZIP
Build / JDK 15 / org.apache.kafka.streams.integration.AdjustStreamThreadCountTest.shouldAddAndRemoveStreamThreadsWhileKeepingNamesCorrect
Build / JDK 8 / org.apache.kafka.streams.integration.AdjustStreamThreadCountTest.shouldRemoveStreamThread
```
All are not related to my changes. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#issuecomment-760772022
@lct45 @ableegoldman @cadonna , please help review this PR. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#issuecomment-784468148
Cherrypick was pretty rough so I opened a quick PR against 2.6 instead: https://github.com/apache/kafka/pull/10197
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon removed a comment on pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
showuon removed a comment on pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#issuecomment-763264828
re-triggered the tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#issuecomment-762611054
LGTM, thanks @showuon . Also re-triggered the tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#discussion_r560617655
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -94,27 +94,32 @@ public StateDirectory(final StreamsConfig config, final Time time, final boolean
this.appId = config.getString(StreamsConfig.APPLICATION_ID_CONFIG);
final String stateDirName = config.getString(StreamsConfig.STATE_DIR_CONFIG);
final File baseDir = new File(stateDirName);
- if (this.hasPersistentStores && !baseDir.exists() && !baseDir.mkdirs()) {
- throw new ProcessorStateException(
- String.format("base state directory [%s] doesn't exist and couldn't be created", stateDirName));
- }
stateDir = new File(baseDir, appId);
- if (this.hasPersistentStores && !stateDir.exists() && !stateDir.mkdir()) {
- throw new ProcessorStateException(
- String.format("state directory [%s] doesn't exist and couldn't be created", stateDir.getPath()));
- }
- if (hasPersistentStores && stateDirName.startsWith("/tmp")) {
- log.warn("Using /tmp directory in the state.dir property can cause failures with writing the checkpoint file" +
- " due to the fact that this directory can be cleared by the OS");
- }
- final Path basePath = Paths.get(baseDir.getPath());
- final Path statePath = Paths.get(stateDir.getPath());
- final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");
- try {
- Files.setPosixFilePermissions(basePath, perms);
- Files.setPosixFilePermissions(statePath, perms);
- } catch (final IOException e) {
- log.error("Error changing permissions for the state or base directory {} ", stateDir.getPath(), e);
+
+ if (this.hasPersistentStores) {
+ if (!baseDir.exists() && !baseDir.mkdirs()) {
+ throw new ProcessorStateException(
+ String.format("base state directory [%s] doesn't exist and couldn't be created", stateDirName));
+ }
+ if (!stateDir.exists() && !stateDir.mkdir()) {
+ throw new ProcessorStateException(
+ String.format("state directory [%s] doesn't exist and couldn't be created", stateDir.getPath()));
+ }
+ if (stateDirName.startsWith("/tmp")) {
+ log.warn("Using /tmp directory in the state.dir property can cause failures with writing the checkpoint file" +
+ " due to the fact that this directory can be cleared by the OS");
+ }
+
+ // change the dir permission to "rwxr-x---" to avoid world readable
+ final Path basePath = Paths.get(baseDir.getPath());
+ final Path statePath = Paths.get(stateDir.getPath());
+ final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");
+ try {
+ Files.setPosixFilePermissions(basePath, perms);
+ Files.setPosixFilePermissions(statePath, perms);
+ } catch (final IOException e) {
+ log.warn("Error changing permissions for the state or base directory {} ", stateDir.getPath(), e);
Review comment:
Agree! revert back to `log.error`. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#issuecomment-763264828
re-triggered the tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cadonna commented on a change in pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
cadonna commented on a change in pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#discussion_r559465915
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -94,27 +94,32 @@ public StateDirectory(final StreamsConfig config, final Time time, final boolean
this.appId = config.getString(StreamsConfig.APPLICATION_ID_CONFIG);
final String stateDirName = config.getString(StreamsConfig.STATE_DIR_CONFIG);
final File baseDir = new File(stateDirName);
- if (this.hasPersistentStores && !baseDir.exists() && !baseDir.mkdirs()) {
- throw new ProcessorStateException(
- String.format("base state directory [%s] doesn't exist and couldn't be created", stateDirName));
- }
stateDir = new File(baseDir, appId);
- if (this.hasPersistentStores && !stateDir.exists() && !stateDir.mkdir()) {
- throw new ProcessorStateException(
- String.format("state directory [%s] doesn't exist and couldn't be created", stateDir.getPath()));
- }
- if (hasPersistentStores && stateDirName.startsWith("/tmp")) {
- log.warn("Using /tmp directory in the state.dir property can cause failures with writing the checkpoint file" +
- " due to the fact that this directory can be cleared by the OS");
- }
- final Path basePath = Paths.get(baseDir.getPath());
- final Path statePath = Paths.get(stateDir.getPath());
- final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");
- try {
- Files.setPosixFilePermissions(basePath, perms);
- Files.setPosixFilePermissions(statePath, perms);
- } catch (final IOException e) {
- log.error("Error changing permissions for the state or base directory {} ", stateDir.getPath(), e);
+
+ if (this.hasPersistentStores) {
+ if (!baseDir.exists() && !baseDir.mkdirs()) {
+ throw new ProcessorStateException(
+ String.format("base state directory [%s] doesn't exist and couldn't be created", stateDirName));
+ }
+ if (!stateDir.exists() && !stateDir.mkdir()) {
+ throw new ProcessorStateException(
+ String.format("state directory [%s] doesn't exist and couldn't be created", stateDir.getPath()));
+ }
+ if (stateDirName.startsWith("/tmp")) {
+ log.warn("Using /tmp directory in the state.dir property can cause failures with writing the checkpoint file" +
+ " due to the fact that this directory can be cleared by the OS");
+ }
+
+ // change the dir permission to "rwxr-x---" to avoid world readable
+ final Path basePath = Paths.get(baseDir.getPath());
+ final Path statePath = Paths.get(stateDir.getPath());
+ final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");
+ try {
+ Files.setPosixFilePermissions(basePath, perms);
+ Files.setPosixFilePermissions(statePath, perms);
+ } catch (final IOException e) {
+ log.warn("Error changing permissions for the state or base directory {} ", stateDir.getPath(), e);
Review comment:
Are we sure this should not be an error? Should we not rethrow the exception? After all it may make data readable to the outside world. I do not request a change, I just wanted to put it up for discussions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman merged pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
ableegoldman merged pull request #9904:
URL: https://github.com/apache/kafka/pull/9904
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #9904: KAFKA-12211: don't change perm for base/state dir when no persistent store
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #9904:
URL: https://github.com/apache/kafka/pull/9904#discussion_r560527698
##########
File path: streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -94,27 +94,32 @@ public StateDirectory(final StreamsConfig config, final Time time, final boolean
this.appId = config.getString(StreamsConfig.APPLICATION_ID_CONFIG);
final String stateDirName = config.getString(StreamsConfig.STATE_DIR_CONFIG);
final File baseDir = new File(stateDirName);
- if (this.hasPersistentStores && !baseDir.exists() && !baseDir.mkdirs()) {
- throw new ProcessorStateException(
- String.format("base state directory [%s] doesn't exist and couldn't be created", stateDirName));
- }
stateDir = new File(baseDir, appId);
- if (this.hasPersistentStores && !stateDir.exists() && !stateDir.mkdir()) {
- throw new ProcessorStateException(
- String.format("state directory [%s] doesn't exist and couldn't be created", stateDir.getPath()));
- }
- if (hasPersistentStores && stateDirName.startsWith("/tmp")) {
- log.warn("Using /tmp directory in the state.dir property can cause failures with writing the checkpoint file" +
- " due to the fact that this directory can be cleared by the OS");
- }
- final Path basePath = Paths.get(baseDir.getPath());
- final Path statePath = Paths.get(stateDir.getPath());
- final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");
- try {
- Files.setPosixFilePermissions(basePath, perms);
- Files.setPosixFilePermissions(statePath, perms);
- } catch (final IOException e) {
- log.error("Error changing permissions for the state or base directory {} ", stateDir.getPath(), e);
+
+ if (this.hasPersistentStores) {
+ if (!baseDir.exists() && !baseDir.mkdirs()) {
+ throw new ProcessorStateException(
+ String.format("base state directory [%s] doesn't exist and couldn't be created", stateDirName));
+ }
+ if (!stateDir.exists() && !stateDir.mkdir()) {
+ throw new ProcessorStateException(
+ String.format("state directory [%s] doesn't exist and couldn't be created", stateDir.getPath()));
+ }
+ if (stateDirName.startsWith("/tmp")) {
+ log.warn("Using /tmp directory in the state.dir property can cause failures with writing the checkpoint file" +
+ " due to the fact that this directory can be cleared by the OS");
+ }
+
+ // change the dir permission to "rwxr-x---" to avoid world readable
+ final Path basePath = Paths.get(baseDir.getPath());
+ final Path statePath = Paths.get(stateDir.getPath());
+ final Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwxr-x---");
+ try {
+ Files.setPosixFilePermissions(basePath, perms);
+ Files.setPosixFilePermissions(statePath, perms);
+ } catch (final IOException e) {
+ log.warn("Error changing permissions for the state or base directory {} ", stateDir.getPath(), e);
Review comment:
Personally I agree that we shouldn't rethrow the exception, as many users probably do not care and it would be pretty bad imo if some error that doesn't concern them makes it impossible to run Streams at all (see for example [KAFKA-12190](https://issues.apache.org/jira/browse/KAFKA-12190) )
That said I'm slightly more inclined to log it as an error just because that will give it better visibility for those users who do care about the readability. Although I suspect that anyone who strongly values security of streams data would (or at least, should!) actually verify the configuration
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org