FileRolesStoreTests#testReload fails

[javanna]

FileRolesStoreTests#testReload failed three times in the last 10 days: one failure on 7.x , one on 7.6 and one on a PR branch . The failure does not reproduce with the same seed.

REPRODUCE WITH: ./gradlew ':x-pack:plugin:security:test' --tests "org.elasticsearch.xpack.security.authz.store.FileRolesStoreTests.testAutoReload" -Dtests.seed=E66E5F527A74FF40 -Dtests.security.manager=true -Dtests.locale=en -Dtests.timezone=Pacific/Kiritimati -Dcompiler.java=13

java.lang.AssertionError: expected:<9> but was:<10>
at __randomizedtesting.SeedInfo.seed([E66E5F527A74FF40:A0D240CB3F2E9F56]:0)
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:834)
at org.junit.Assert.assertEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:631)
at org.elasticsearch.xpack.security.authz.store.FileRolesStoreTests.testAutoReload(FileRolesStoreTests.java:412)

https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+7.x+EAR/458

[elasticmachine]

Pinging @elastic/es-security (:Security/Authorization)

[javanna]

Given that it does not repro and it sporadically fails, shall we keep the test around to potentially collect more info or shall I mute it already?

[iverase]

Another failure in 7.x: https://gradle-enterprise.elastic.co/s/rssnr4evzsaww

[tvernum]

We suspect this might have only been broken for a few weeks and the issue from 9 days ago is something different, but @ywangd will followup and see where we are.

[jkakavas]

Happened again in master (testAutoReload) :https://gradle-enterprise.elastic.co/s/uftgjwlhgiunm

[ywangd]

This is a genuine issue with the test code. I can reliably reproduce it locally by inserting Thread.sleep(200); before the first writer.append call:

elasticsearch/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java

Lines 427 to 431 in 0097a86

	try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.TRUNCATE_EXISTING)) {
	writer.append("role5:").append(System.lineSeparator());
	writer.append(" cluster:").append(System.lineSeparator());
	writer.append(" - 'ALL'");
	}

The main reason of the failure is that the BufferedWriter does not write file atomically. If there is any delay after the file is opened (with trunction), it is picked up by the FileWatcher and the content is parsed for roles. Since the file is empty, we get no role back.

Now the test code does check whether the expected role is in the change-set (reported by FileWatcher) before asserting the existence of the Role (where the test failed). However, we do not differentiate the change-set between "deleted", "inserted" or "update". The test code really wanted an "updated" change-set, but it accepted a "deleted" one because role name is the same and it cannot tell the type of the change-set (modifiedFileRolesModified is the change-set in the following code):

elasticsearch/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java

Line 435 in 0097a86

assertTrue(modifiedFileRolesModified.contains("role5"));

I will fix this with a PR to basically perform file move instead of write to avoid the race condition.