Skip to content

KAFKA-12211: don't change perm for base/state dir when no persistent store#9904

Merged
ableegoldman merged 3 commits intoapache:trunkfrom
showuon:KAFKA-12211
Jan 20, 2021
Merged

KAFKA-12211: don't change perm for base/state dir when no persistent store#9904
ableegoldman merged 3 commits intoapache:trunkfrom
showuon:KAFKA-12211

Conversation

@showuon
Copy link
Copy Markdown
Member

@showuon showuon commented Jan 15, 2021

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)

showuon
showuon commented Jan 15, 2021
View reviewed changes
Comment thread streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java Outdated
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);
Copy link
Copy Markdown
Member Author

@showuon showuon Jan 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change from error to warn since I don't think it'll be an real error if changing permission failed.

Copy link
Copy Markdown
Member

@cadonna cadonna Jan 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@showuon showuon Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@ableegoldman ableegoldman Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )
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

Copy link
Copy Markdown
Member Author

@showuon showuon Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree! revert back to log.error. Thanks.

@showuon
Copy link
Copy Markdown
Member Author

showuon commented Jan 15, 2021

@lct45 @ableegoldman @cadonna , please help review this PR. Thanks.

ableegoldman
ableegoldman approved these changes Jan 15, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@ableegoldman ableegoldman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, I've noticed this error in the logs of some tests before but never got around to checking it out. LGTM

@showuon
Copy link
Copy Markdown
Member Author

showuon commented Jan 18, 2021

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.

cadonna
cadonna reviewed Jan 18, 2021
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java Outdated
Comment on lines +551 to +552
assertFalse(stateDir.exists());
assertFalse(appDir.exists());
Copy link
Copy Markdown
Member

@cadonna cadonna Jan 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
assertFalse(stateDir.exists());
assertFalse(appDir.exists());
assertThat(stateDir.exists(), is(false));
assertThat(appDir.exists(), is(false));

Copy link
Copy Markdown
Member Author

@showuon showuon Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Thanks.

@guozhangwang
Copy link
Copy Markdown
Contributor

guozhangwang commented Jan 19, 2021

LGTM, thanks @showuon . Also re-triggered the tests.

@showuon
Copy link
Copy Markdown
Member Author

showuon commented Jan 20, 2021

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.

@ableegoldman ableegoldman merged commit 462c89e into apache:trunk Jan 20, 2021
@ableegoldman
Copy link
Copy Markdown
Member

ableegoldman commented Jan 20, 2021

Thanks @showuon , merged to trunk

ableegoldman pushed a commit that referenced this pull request Jan 20, 2021
@showuon @ableegoldman
KAFKA-12211: don't change perm for base/state dir when no persistent …
650d93b 
…store (#9904)

 If a user doesn't have Persistent Stores, we won't create base dir and state dir and should not try to set permissions on them.

Reviewers: Bruno Cadonna <cadonna@confluent.io>, Guozhang Wang <guozhang@confluent.io>, Anna Sophie Blee-Goldman <ableegoldman@apache.org>
@ableegoldman
Copy link
Copy Markdown
Member

ableegoldman commented Jan 20, 2021

Also cherrypicked to 2.7

@modax
Copy link
Copy Markdown

modax commented Feb 23, 2021

Hi,

Is there any reason this is not in 2.6? 2.6.1 is affected by this problem.

Thanks!

@ableegoldman
Copy link
Copy Markdown
Member

ableegoldman commented Feb 23, 2021

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

@ableegoldman ableegoldman mentioned this pull request Feb 23, 2021
Merged
@ableegoldman
Copy link
Copy Markdown
Member

ableegoldman commented Feb 23, 2021

Cherrypick was pretty rough so I opened a quick PR against 2.6 instead: #10197

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cadonna cadonna cadonna left review comments

@ableegoldman ableegoldman ableegoldman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@showuon @guozhangwang @ableegoldman @modax @cadonna