-
Notifications
You must be signed in to change notification settings - Fork 4.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
Enable system tests for auditbeat #5945
Enable system tests for auditbeat #5945
Conversation
jenkins, test it |
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
jenkins, test it Seems like Jenkins never started for this one. |
Seems like it refuses to start even with our comments :-( |
@ruflin Yes, there is an issue with github hook infra and github are working on it. |
Jenkins test this please |
8f3b496
to
a78b6a4
Compare
6aed4aa
to
f5ed930
Compare
f5ed930
to
b79d60d
Compare
Rebased this on master to see if it builds correctly. |
auditbeat/Makefile
Outdated
GOX_OS?=linux windows ## @Building List of all OS to be supported by "make crosscompile". | ||
DEV_OS?=linux | ||
TESTING_ENVIRONMENT?=snapshot-noxpack | ||
GOPACKAGES=$(shell go list ${BEAT_PATH}/... | grep -v /vendor/) |
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.
Do you know what requires this line? I'd assume this is the default in libbeat/scripts/Makefile.
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 just copied over from Metricbeat. It does not seem to be needed anymore. I also removed it from Metricbeat.
auditbeat/docker-compose.yml
Outdated
|
||
elasticsearch: | ||
extends: | ||
file: ../testing/environments/${TESTING_ENVIRONMENT}.yml |
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.
If you follow this file you will find that it references Dockerfile-snapshot
which no longer exists.
beats/testing/environments/snapshot-noxpack.yml
Lines 10 to 11 in f3de72e
context: ./docker/elasticsearch | |
dockerfile: Dockerfile-snapshot |
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.
Strange, metricbeat depends on the same one and still works. Change.
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.
@andrewkroh I think I just found a potential issue. Metricbeat has it's own elasticsearch instance for the testing of the module. This is reused also for the tests where data is sent to ES like the template loading.
It could now be argued that this a good thing as template loading etc. is tested with a stable version and it's up to libbeat to test its own functionality against master as each beat just uses the provided functionality. At the same time this was not intended.
The above explains how the noxpack file could have become outdated without breaking the tests.
b79d60d
to
4836590
Compare
a21591d
to
ab8e48d
Compare
It seems the test on Jenkins is failing because the mac slave tries to start the environment which it shouldn't because TEST_ENVIRONMENT=0 is set. I need to further investigate. |
This PR is a subset of elastic#5945 to remove the Metricbeat changes from the PR.
auditbeat/Makefile
Outdated
SYSTEM_TESTS=false | ||
TEST_ENVIRONMENT=false | ||
SYSTEM_TESTS=true | ||
TEST_ENVIRONMENT=true |
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.
Would setting TEST_ENVIRONMENT?=true
solve the issue? IIRC we set TEST_ENVIRONMENT=0
in the Jenkins darwin environment.
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.
good catch, will try
ae26160
to
3e9299b
Compare
* Cleanup Metricbeat composer and Makefile This PR is a subset of #5945 to remove the Metricbeat changes from the PR.
* There were system tests and system integration tests for auditbeat but so far they were not run. * Integration tests enabled * Module config changed to module that works on more systems. Limitation for linux removed. Change noxpack path readd es_beats path remove kibana snapshot file one more check for xpack0 to see if timeout becomes less of a problem readd go packages unify metricbeat / auditbeat to find problem update travis, rename beat-path as double used remove kibana because not needed modify build flag to run as unit test add file tree test to unit tests add ? for test env
3e9299b
to
6e11a0c
Compare
proxy_dep: | ||
image: busybox | ||
depends_on: | ||
elasticsearch: { condition: service_healthy } |
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.
@exekias In compose version 2 depends_on
has to be an array and does not support the health check: https://docs.docker.com/compose/compose-file/compose-file-v2/#depends_on 2.1 is required for the above notation. But 2.1 is not supported by the module script. As there is no compose usage yet in auditbeat I will check if I can get away with using 2.1 here.
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.
If you want to use the uncoupled testing code you need to switch to something like this: https://github.com/elastic/beats/blob/master/metricbeat/docker-compose.yml
Where there is no proxy_dep
and health checks are managed from testing framework
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 seems to work with 2.1 for now as there are no usage of the testing framework.
The test failure from autodiscovery should not be related, still posting it here for @exekias
@andrewkroh Ready for an other review / merge. |
Thanks for adding this test! |
This is prerequisite for #5938