-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SQL: change the audit log file rolling behavior in tests #34909
Conversation
Pinging @elastic/es-search-aggs |
LGTM. Too bad the configuration file has to be this big. |
x-pack/qa/sql/security/build.gradle
Outdated
@@ -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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
x-pack/qa/sql/security/build.gradle
Outdated
@@ -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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer specifying types explicitly instead of def
. This one would be a FileCollection
.
x-pack/qa/sql/security/build.gradle
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@atorok Addressed comments. Thank you for reviewing. |
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? |
@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. |
Yet it complicates the build.gradle instead, which is far removed from the impacted tests. |
hey, how can i test a single class in the terminal? |
@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? |
@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. |
Great, thank you Andrei! |
…o 33987_test_fix
193ad9c
to
c1cc126
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left a comment
@SuppressForbidden(reason="security doesn't work with mock filesystem") | ||
private static Path lookupRolledOverAuditLog() { | ||
String auditLogFileString = System.getProperty("tests.audit.yesterday.logfile"); | ||
if (null == auditLogFileString) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@elasticmachine run elasticsearch-ci/1 elasticsearch-ci/2 elasticsearch-ci/default-distro |
@elasticmachine run elasticsearch-ci/2 |
…o 33987_test_fix
* 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) ...
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.