SQL: change the audit log file rolling behavior in tests #34909

astefan · 2018-10-26T14:44:01Z

This is a fix attempt for #33987, where tests fails because the audit log file is empty, which can happen when the file is rolled over at midnight. If reading the file happens during the rollover the file would be empty (which shouldn't actually happen).

The fix is to extend the rolling over period to 2 days (default is one). The provided log4j2.properties file is overriding the default one. Elasticsearch should be able to pickup additional log4j files from /config as long as they live in the /config folder in different directories. Those should be merged in a final log4j config. extraConfigFile doesn't work with directories though.

elasticmachine · 2018-10-26T14:44:03Z

Pinging @elastic/es-search-aggs

costin · 2018-10-26T15:39:29Z

LGTM. Too bad the configuration file has to be this big.

alpar-t · 2018-10-29T05:38:53Z

x-pack/qa/sql/security/build.gradle

@@ -51,6 +54,9 @@ subprojects {
    setting 'xpack.license.self_generated.type', 'trial'
    // Setup roles used by tests
    extraConfigFile 'roles.yml', '../roles.yml'
+    // Change the log4j configuration for audit logging file, more specifically make it roll over every 2 days
+    // Some tests do read the audit log file and do not account for the daily (default) rollover
+    extraConfigFile 'log4j2.properties', '../log4j2.properties'


Could we have a task to copy and modify the original from distributions/src/config ?
You can reference the location as "${project(':distributions').projectDir}/src/config/log4j2.properties, have a copy task to copy this one file and then have a doLast to modify it as you would like.

elasticmachine · 2018-10-29T05:39:07Z

Pinging @elastic/es-core-infra

astefan · 2018-10-29T13:27:43Z

@costin @atorok I've changed the approach and I'm taking Elasticsearch' log4j2 config (without x-pack settings) and combining it with the x-pack specific ones and then change only one line from the resulting file. Finally, overriding the one used in tests with the combined one above.

alpar-t

I left some minor comments.
I think this looks ok now.
Would probably be better to offer a hook in clusterformation to be able to edit the log config,
especially if more places in the build will require it, but since we are going to replace it i'm hesitant to do that now.

alpar-t · 2018-10-29T15:32:39Z

x-pack/qa/sql/security/build.gradle

@@ -23,14 +23,30 @@ subprojects {
    testCompile "org.elasticsearch.plugin:x-pack-core:${version}"
  }

+  task changeAuditLoggingRollingPolicy {
+    def toConcatenate = files("${project(':distribution').projectDir}/src/config/log4j2.properties", "${project(':x-pack').projectDir}/plugin/core/src/main/config/log4j2.properties")


We prefer specifying types explicitly instead of def. This one would be a FileCollection.

alpar-t · 2018-10-29T15:36:17Z

x-pack/qa/sql/security/build.gradle

+  task changeAuditLoggingRollingPolicy {
+    def toConcatenate = files("${project(':distribution').projectDir}/src/config/log4j2.properties", "${project(':x-pack').projectDir}/plugin/core/src/main/config/log4j2.properties")
+    def outputFileName = "${buildDir}/log4j2.properties"
+    def output = new File(outputFileName)


It's better to use file() or project.file() instead of new File in Gradle as the former always resolves relative paths to project dir.
It won't make a difference here, but a best practice that we would like to follow.

astefan · 2018-10-30T07:05:12Z

@atorok Addressed comments. Thank you for reviewing.

jasontedor · 2018-10-30T07:24:49Z

Why not consume all log files in the assertion, instead of making this integration test cluster have a special configuration file that deviates from default behavior?

astefan · 2018-10-30T07:48:43Z

@jasontedor did it like this because the way we consume those files is not relevant to the test itself, but the content of the file(s) is. And accounting for files rollover complicates the test too much.

jasontedor · 2018-10-30T07:52:47Z

Yet it complicates the build.gradle instead, which is far removed from the impacted tests.

henriquegoncalves98 · 2018-12-16T15:33:23Z

hey, how can i test a single class in the terminal?

danielmitterdorfer · 2019-01-29T07:09:06Z

@astefan I also think the approach that @jasontedor has suggested is worthwhile pursuing. By consuming all audit log files in the test we can stick to the default logging configuration. Can you please take a stab to implement this?

astefan · 2019-01-29T12:50:51Z

@danielmitterdorfer yes, I fully agree, and I didn't forget about this issue, but I've been caught into more urgent tasks. Once I get the chance, I'll have another look at this issue. It's on my virtual TO-DO list.

danielmitterdorfer · 2019-01-29T13:25:16Z

Great, thank you Andrei!

…o 33987_test_fix

matriv

LGTM. Left a comment

matriv · 2019-02-07T19:17:15Z

...l/qa/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/SqlSecurityTestCase.java

+    @SuppressForbidden(reason="security doesn't work with mock filesystem")
+    private static Path lookupRolledOverAuditLog() {
+        String auditLogFileString = System.getProperty("tests.audit.yesterday.logfile");
+        if (null == auditLogFileString) {


costin

LGTM

astefan · 2019-02-08T08:34:43Z

@elasticmachine run elasticsearch-ci/1 elasticsearch-ci/2 elasticsearch-ci/default-distro

astefan · 2019-02-08T08:35:18Z

@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/default-distro

…o 33987_test_fix

…ile (#34909) (cherry picked from commit 75cb6b3)

* master: Mute AnalysisModuleTests#testStandardFilterBWC (elastic#38636) add geotile_grid ref to asciidoc (elastic#38632) Enable Dockerfile from artifacts.elastic.co (elastic#38552) Mute FollowerFailOverIT testFailOverOnFollower (elastic#38634) Account for a possible rolled over file while reading the audit log file (elastic#34909) Mute failure in InternalEngineTests (elastic#38622) Fix Issue with Concurrent Snapshot Init + Delete (elastic#38518) Refactor ZonedDateTime.now in millis resolution (elastic#38577) Mute failing WatchStatusIntegrationTests (elastic#38621) Mute failing ApiKeyIntegTests (elastic#38614) [DOCS] Add warning about bypassing ML PUT APIs (elastic#38509) Add 7.1 and 8.0 version constants to master (elastic#38514)

* master: (1159 commits) Fix timezone fallback in ingest processor (elastic#38407) Avoid polluting download stats on builds (elastic#38660) SQL: Prevent grouping over grouping functions (elastic#38649) SQL: Relax StackOverflow circuit breaker for constants (elastic#38572) [DOCS] Fixes broken migration links (elastic#38655) Drop support for the low-level REST client on JDK 7 (elastic#38540) [DOCS] Adds placeholders for v8 highlights, breaking changes, release notes (elastic#38641) fix dissect doc "ip" --> "clientip" (elastic#38545) Concurrent file chunk fetching for CCR restore (elastic#38495) make DateMathIndexExpressionsIntegrationIT more resilient (elastic#38473) SQL: Replace joda with java time (elastic#38437) Add fuzziness example (elastic#37194) (elastic#38648) Mute AnalysisModuleTests#testStandardFilterBWC (elastic#38636) add geotile_grid ref to asciidoc (elastic#38632) Enable Dockerfile from artifacts.elastic.co (elastic#38552) Mute FollowerFailOverIT testFailOverOnFollower (elastic#38634) Account for a possible rolled over file while reading the audit log file (elastic#34909) Mute failure in InternalEngineTests (elastic#38622) Fix Issue with Concurrent Snapshot Init + Delete (elastic#38518) Refactor ZonedDateTime.now in millis resolution (elastic#38577) ...

* master: (27 commits) Mute AnalysisModuleTests#testStandardFilterBWC (elastic#38636) add geotile_grid ref to asciidoc (elastic#38632) Enable Dockerfile from artifacts.elastic.co (elastic#38552) Mute FollowerFailOverIT testFailOverOnFollower (elastic#38634) Account for a possible rolled over file while reading the audit log file (elastic#34909) Mute failure in InternalEngineTests (elastic#38622) Fix Issue with Concurrent Snapshot Init + Delete (elastic#38518) Refactor ZonedDateTime.now in millis resolution (elastic#38577) Mute failing WatchStatusIntegrationTests (elastic#38621) Mute failing ApiKeyIntegTests (elastic#38614) [DOCS] Add warning about bypassing ML PUT APIs (elastic#38509) Add 7.1 and 8.0 version constants to master (elastic#38514) ML: update set_upgrade_mode, add logging (elastic#38372) bad formatted JSON object (elastic#38515) (elastic#38525) Fix HistoryIntegrationTests timestamp comparsion (elastic#38505) SQL: Fix issue with IN not resolving to underlying keyword field (elastic#38440) Fix the clock resolution to millis in ScheduledEventTests (elastic#38506) Enable BWC after backport recovering leases (elastic#38485) Collapse retention lease integration tests (elastic#38483) TransportVerifyShardBeforeCloseAction should force a flush (elastic#38401) ...

Backport of: elastic#34909

…ile (#39615) Backport of: #34909 Co-authored-by: Andrei Stefan <astefan@users.noreply.github.com>

astefan added >test Issues or PRs that are addressing/adding tests :Analytics/SQL SQL querying labels Oct 26, 2018

astefan requested review from costin and nik9000 October 26, 2018 14:44

alpar-t reviewed Oct 29, 2018

View reviewed changes

alpar-t added the :Delivery/Build Build or test infrastructure label Oct 29, 2018

alpar-t reviewed Oct 29, 2018

View reviewed changes

danielmitterdorfer added the feedback_needed label Jan 29, 2019

danielmitterdorfer removed the feedback_needed label Jan 29, 2019

Account for a possible rolled over file while reading the audit log file

a225767

astefan requested review from matriv and removed request for nik9000 February 7, 2019 17:54

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

c1cc126

…o 33987_test_fix

astefan force-pushed the 33987_test_fix branch from 193ad9c to c1cc126 Compare February 7, 2019 18:57

matriv approved these changes Feb 7, 2019

View reviewed changes

costin approved these changes Feb 7, 2019

View reviewed changes

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

0a3a914

…o 33987_test_fix

astefan added v7.0.0 v6.7.0 v8.0.0 v7.2.0 labels Feb 8, 2019

astefan merged commit 75cb6b3 into elastic:master Feb 8, 2019

astefan added backport pending and removed :Delivery/Build Build or test infrastructure labels Feb 8, 2019

astefan added a commit that referenced this pull request Feb 8, 2019

Account for a possible rolled over file while reading the audit log f…

6359d98

…ile (#34909) (cherry picked from commit 75cb6b3)

astefan added a commit that referenced this pull request Feb 8, 2019

Account for a possible rolled over file while reading the audit log f…

8cd20d1

…ile (#34909) (cherry picked from commit 75cb6b3)

astefan added a commit that referenced this pull request Feb 8, 2019

Account for a possible rolled over file while reading the audit log f…

5fed91e

…ile (#34909) (cherry picked from commit 75cb6b3)

astefan deleted the 33987_test_fix branch February 8, 2019 16:34

astefan removed the backport pending label Feb 8, 2019

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 12, 2019

astefan mentioned this pull request Feb 25, 2019

SQL: JdbcSecurityIT fail when reading audit logging files at rollover time #39348

Closed

tvernum pushed a commit to tvernum/elasticsearch that referenced this pull request Mar 4, 2019

Account for a possible rolled over file while reading the audit log file

f290bff

Backport of: elastic#34909

tvernum mentioned this pull request Mar 4, 2019

Account for a possible rolled over file while reading the audit log file #39615

Merged

tvernum added a commit that referenced this pull request Mar 4, 2019

Account for a possible rolled over file while reading the audit log f…

d1359c1

…ile (#39615) Backport of: #34909 Co-authored-by: Andrei Stefan <astefan@users.noreply.github.com>

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: change the audit log file rolling behavior in tests #34909

SQL: change the audit log file rolling behavior in tests #34909

astefan commented Oct 26, 2018

elasticmachine commented Oct 26, 2018

costin commented Oct 26, 2018

alpar-t Oct 29, 2018

elasticmachine commented Oct 29, 2018

astefan commented Oct 29, 2018

alpar-t left a comment

alpar-t Oct 29, 2018

alpar-t Oct 29, 2018

astefan commented Oct 30, 2018

jasontedor commented Oct 30, 2018

astefan commented Oct 30, 2018

jasontedor commented Oct 30, 2018

henriquegoncalves98 commented Dec 16, 2018

danielmitterdorfer commented Jan 29, 2019

astefan commented Jan 29, 2019

danielmitterdorfer commented Jan 29, 2019

matriv left a comment

matriv Feb 7, 2019

costin left a comment

astefan commented Feb 8, 2019

astefan commented Feb 8, 2019

SQL: change the audit log file rolling behavior in tests #34909

SQL: change the audit log file rolling behavior in tests #34909

Conversation

astefan commented Oct 26, 2018

elasticmachine commented Oct 26, 2018

costin commented Oct 26, 2018

alpar-t Oct 29, 2018

Choose a reason for hiding this comment

elasticmachine commented Oct 29, 2018

astefan commented Oct 29, 2018

alpar-t left a comment

Choose a reason for hiding this comment

alpar-t Oct 29, 2018

Choose a reason for hiding this comment

alpar-t Oct 29, 2018

Choose a reason for hiding this comment

astefan commented Oct 30, 2018

jasontedor commented Oct 30, 2018

astefan commented Oct 30, 2018

jasontedor commented Oct 30, 2018

henriquegoncalves98 commented Dec 16, 2018

danielmitterdorfer commented Jan 29, 2019

astefan commented Jan 29, 2019

danielmitterdorfer commented Jan 29, 2019

matriv left a comment

Choose a reason for hiding this comment

matriv Feb 7, 2019

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

astefan commented Feb 8, 2019

astefan commented Feb 8, 2019