Skip to content
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

Support for multi es instances #548

Merged
merged 14 commits into from
Nov 14, 2021
Merged

Support for multi es instances #548

merged 14 commits into from
Nov 14, 2021

Conversation

buratinopy
Copy link

@buratinopy buratinopy commented Nov 7, 2021

Description

Support for multi es instances

Checklist

  • I have reviewed the contributing guidelines.
  • I have included unit tests for my changes or additions.
  • I have successfully run make test-docker with my changes.
  • I have manually tested all relevant modes of the change in this PR.
  • I have updated the documentation.
  • I have updated the changelog.

Questions or Comments

I didn't understand how to fill in CHANGELOG.md
WIP

@nsano-rururu
Copy link
Collaborator

Please do not modify the version of setup.py without permission. Please put it back.

@nsano-rururu
Copy link
Collaborator

Write a change log.

@nsano-rururu
Copy link
Collaborator

Please restore the following files

Chart.yaml
README.md
values.yaml
running_elastalert.rst

@nsano-rururu
Copy link
Collaborator

Please update the documentation
https://elastalert2.readthedocs.io/en/latest/ruletypes.html#es-host
https://elastalert2.readthedocs.io/en/latest/ruletypes.html#es-port

@nsano-rururu
Copy link
Collaborator

Why am I not running make test-docker? .. If it can be executed, execute it and check if there is no problem.

@jertel
Copy link
Owner

jertel commented Nov 8, 2021

It looks like the author followed the maintainer section of the contribution guidelines doc.

@buratinopy Only the top half of Contributing Guidelines needs to be followed for PRs.

@buratinopy
Copy link
Author

Why am I not running make test-docker? .. If it can be executed, execute it and check if there is no problem.
Container assembly breaks on package installation. I could not solve this problem. There may be problems in my environment. Please try to run this on your pc.

root@ubuntu:/home/burtsev/PycharmProjects/elastalert2# make test-docker
docker-compose -f tests/docker-compose.yml --project-name elastalert build tox
Building tox
Sending build context to Docker daemon  7.144MB
Step 1/6 : FROM python:3-slim-buster
 ---> 582d61d95733
Step 2/6 : RUN apt update && apt upgrade -y
 ---> Using cache
 ---> 51f4ada1a010
Step 3/6 : RUN apt install -y gcc libffi-dev
 ---> Running in eae61a97b462

WARNING: apt does not have a stable CLI interface. Use with caution in scripts.

Reading package lists...
Building dependency tree...
Reading state information...
E: Unable to locate package gcc
E: Unable to locate package libffi-dev
The command '/bin/sh -c apt install -y gcc libffi-dev' returned a non-zero code: 100
ERROR: Service 'tox' failed to build : Build failed
make: *** [Makefile:23: test-docker] Error 1

I ran unit tests, passed without errors.

@jertel
Copy link
Owner

jertel commented Nov 8, 2021

make test-docker runs successfully for me locally, and on the automated CI here on GitHub. You might have some older images cached in Docker. Try:

docker pull python:3-slim-buster
make clean
make test-docker

@nsano-rururu
Copy link
Collaborator

@buratinopy

I ran make test-docker. There is a problem with ruletypes.rst. Please correct.

Warning, treated as error:
/home/elastalert/docs/source/ruletypes.rst:231:Unexpected indentation.
ERROR: InvocationError for command /home/elastalert/tests/.tox/docs/bin/sphinx-build -b html -d build/doctrees -W source build/html (exited with code 2)
________________________________________________________________________________ summary ________________________________________________________________________________
  py310: commands succeeded
ERROR:   docs: commands failed
ERROR: 1
make: *** [test-docker] エラー 1

@buratinopy
Copy link
Author

buratinopy commented Nov 8, 2021

@nsano-rururu i fixed it, please check.

@buratinopy
Copy link
Author

@jertel

make test-docker runs successfully for me locally, and on the automated CI here on GitHub. You might have some older images cached in Docker. Try:

docker pull python:3-slim-buster
make clean
make test-docker

Did not help

root@ubuntu:/home/burtsev/PycharmProjects/elastalert2# docker pull python:3-slim-buster
3-slim-buster: Pulling from library/python
Digest: sha256:205f9e838a7403a4d3d78477636dec8ab104a2e47b776ff144816a7fd2b67f51
Status: Image is up to date for python:3-slim-buster
docker.io/library/python:3-slim-buster

root@ubuntu:/home/burtsev/PycharmProjects/elastalert2# make clean
make -C docs clean
make[1]: Entering directory '/home/burtsev/PycharmProjects/elastalert2/docs'
rm -rf build/*
make[1]: Leaving directory '/home/burtsev/PycharmProjects/elastalert2/docs'
find . -name '*.pyc' -delete
find . -name '__pycache__' -delete
rm -rf virtualenv_run tests/.tox tests/.coverage *.egg-info docs/build

root@ubuntu:/home/burtsev/PycharmProjects/elastalert2# make test-docker
docker-compose -f tests/docker-compose.yml --project-name elastalert build tox
Building tox
Sending build context to Docker daemon  7.068MB
Step 1/6 : FROM python:3-slim-buster
 ---> 582d61d95733
Step 2/6 : RUN apt update && apt upgrade -y
 ---> Using cache
 ---> 51f4ada1a010
Step 3/6 : RUN apt install -y gcc libffi-dev
 ---> Running in 9c18e2a540da

WARNING: apt does not have a stable CLI interface. Use with caution in scripts.

Reading package lists...
Building dependency tree...
Reading state information...
E: Unable to locate package gcc
E: Unable to locate package libffi-dev
The command '/bin/sh -c apt install -y gcc libffi-dev' returned a non-zero code: 100
ERROR: Service 'tox' failed to build : Build failed
make: *** [Makefile:23: test-docker] Error 1

@nsano-rururu
Copy link
Collaborator

I think the local download python: 3-slim-buster is out of date. Maybe you need to get it once and get it again.

docker rmi python:3-slim-buster
docker pull python:3-slim-buster
git clone https://github.com/jertel/elastalert2.git
cd elastalert2
git fetch origin pull/548/head:multi_es
git checkout multi_es
make test-docker

@jertel
Copy link
Owner

jertel commented Nov 8, 2021

I suggest exec'ing into the python:3-slim-buster container and manually running the failing command so that you can start diagnosing the problem. Perhaps you have a network issue that prevents the container from reaching the Internet to install those packages. Per #11 you will need to consult the Docker forums for help with Docker related issues, as this is a local environment issue that's affecting you only.

@nsano-rururu
Copy link
Collaborator

The error is gone.

  py310: commands succeeded
  docs: commands succeeded
  congratulations :)

@buratinopy
Copy link
Author

I suggest exec'ing into the python:3-slim-buster container and manually running the failing command so that you can start diagnosing the problem. Perhaps you have a network issue that prevents the container from reaching the Internet to install those packages. Per #11 you will need to consult the Docker forums for help with Docker related issues, as this is a local environment issue that's affecting you only.

Really ... Looks like my colleagues from the DLP department broke my internet access...
I will try to run the same on my home server.

@buratinopy
Copy link
Author

The error is gone.

  py310: commands succeeded
  docs: commands succeeded
  congratulations :)

Thanks!
Is there anything else I need to do?

parsed_conf['es_port'] = int(os.environ.get('ES_PORT', conf['es_port']))
es_host = os.environ.get('ES_HOST', conf['es_host'])
es_port = int(os.environ.get('ES_PORT', conf['es_port']))
parsed_conf['es_host'] = parse_host(es_host, es_port)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is es_host getting the port appended to it and being converted to a list? Pretty sure this will break AWS auth:

es_conn_conf['http_auth'] = auth(host=es_conn_conf['es_host'],

Copy link
Owner

Choose a reason for hiding this comment

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

When you say "will break AWS auth" are you referring to OpenSearch?

Copy link
Contributor

@JeffAshton JeffAshton Nov 8, 2021

Choose a reason for hiding this comment

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

If I'm reading the change correctly, es_host will become

["{host}:{port}".format(host=host, port=port)]

From https://github.com/jertel/elastalert2/pull/548/files#diff-c94be3c6634086358da8944c7febe9c9e98fc1b4425fcc4e116e0740f5cf4eb6R556

This would break AWS auth because the host is used in the signing process.

Maybe I'm wrong and build_es_conn_config is used separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure looks like it would break it:

es_conn_conf = build_es_conn_config(conf)

es_conn_conf['http_auth'] = auth(host=es_conn_conf['es_host'],

Copy link
Owner

Choose a reason for hiding this comment

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

This is the code that I think helps explain your argument better: https://github.com/DavidMuller/aws-requests-auth/blob/2e1dd0f37e3815c417c3b0630215a77aab5af617/aws_requests_auth/aws_auth.py#L118

Based on that, I agree. However, we get this request for supporting multiple ES hosts every month or so, so perhaps there's something we can do for the community to make this easier without the need for a load balancer. Would a safer way to do this be to introduce a new config parameter called es_hosts for those who want to use multiple ES hosts, outside of AWS? util.py::build_es_conn_config() could prefer that parameter if specified, and it would override the es_host parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think an array over a csv string makes more sense anyway.

Copy link
Author

Choose a reason for hiding this comment

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

Ok i can redo

@JeffAshton
Copy link
Contributor

Can you solve this with a load balancer outside of ElastAlert?

@buratinopy
Copy link
Author

Can you solve this with a load balancer outside of ElastAlert?

Actually, the essence of the revision lies in the rejection of the load balancer.

…e method of connecting to the ec_host is preserved
@buratinopy
Copy link
Author

@nsano-rururu can you run make test-docker on yourself, please? (I have not yet fixed access to the repositories at work)

@nsano-rururu
Copy link
Collaborator

error

    @mock.patch.dict(os.environ, {'ES_USERNAME': 'USER',
                                  'ES_PASSWORD': 'PASS',
                                  'ES_API_KEY': 'KEY',
                                  'ES_BEARER': 'BEARE'})
    def test_build_es_conn_config2():
        conf = {}
        conf['es_host'] = 'localhost'
        conf['es_port'] = 9200
        expected = {
            'use_ssl': False,
            'verify_certs': True,
            'ca_certs': None,
            'client_cert': None,
            'client_key': None,
            'http_auth': None,
            'es_username': 'USER',
            'es_password': 'PASS',
            'es_api_key': 'KEY',
            'es_bearer': 'BEARE',
            'aws_region': None,
            'profile': None,
            'headers': None,
            'es_host': 'localhost',
            'es_port': 9200,
            'es_url_prefix': '',
            'es_conn_timeout': 20,
            'send_get_body_as': 'GET',
            'ssl_show_warn': True
        }
>       actual = build_es_conn_config(conf)

util_test.py:445: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

conf = {'es_host': 'localhost', 'es_port': 9200}

    def build_es_conn_config(conf):
        """ Given a conf dictionary w/ raw config properties 'use_ssl', 'es_host', 'es_port'
        'es_username' and 'es_password', this will return a new dictionary
        with properly initialized values for 'es_host', 'es_port', 'use_ssl' and 'http_auth' which
        will be a basicauth username:password formatted string """
        parsed_conf = {}
        parsed_conf['use_ssl'] = os.environ.get('ES_USE_SSL', False)
        parsed_conf['verify_certs'] = True
        parsed_conf['ca_certs'] = None
        parsed_conf['client_cert'] = None
        parsed_conf['client_key'] = None
        parsed_conf['http_auth'] = None
        parsed_conf['es_username'] = None
        parsed_conf['es_password'] = None
        parsed_conf['es_api_key'] = None
        parsed_conf['es_bearer'] = None
        parsed_conf['aws_region'] = None
        parsed_conf['profile'] = None
        parsed_conf['headers'] = None
        parsed_conf['es_host'] = os.environ.get('ES_HOST', conf['es_host'])
>       parsed_conf['es_hosts'] = os.environ.get('ES_HOSTS', conf['es_hosts'])
E       KeyError: 'es_hosts'

../elastalert/util.py:365: KeyError
_________________ test_elasticsearch_client[localhost-9200--] __________________
[gw3] linux -- Python 3.10.0 /home/elastalert/tests/.tox/py310/bin/python

es_host = 'localhost', es_port = 9200, es_bearer = '', es_api_key = ''

    @pytest.mark.parametrize('es_host, es_port, es_bearer, es_api_key', [
        ('localhost', 9200, '', ''),
        ('localhost', 9200, 'bearer', 'bearer')
    ])
    @mock.patch.dict(os.environ, {'AWS_DEFAULT_REGION': ''})
    def test_elasticsearch_client(es_host, es_port, es_bearer, es_api_key):
        conf = {}
        conf['es_host'] = es_host
        conf['es_port'] = es_port
        if es_bearer:
            conf['es_bearer'] = es_bearer
        if es_api_key:
            conf['es_api_key'] = es_api_key
>       acutual = elasticsearch_client(conf)

util_test.py:462: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../elastalert/util.py:325: in elasticsearch_client
    es_conn_conf = build_es_conn_config(conf)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

conf = {'es_host': 'localhost', 'es_port': 9200}

    def build_es_conn_config(conf):
        """ Given a conf dictionary w/ raw config properties 'use_ssl', 'es_host', 'es_port'
        'es_username' and 'es_password', this will return a new dictionary
        with properly initialized values for 'es_host', 'es_port', 'use_ssl' and 'http_auth' which
        will be a basicauth username:password formatted string """
        parsed_conf = {}
        parsed_conf['use_ssl'] = os.environ.get('ES_USE_SSL', False)
        parsed_conf['verify_certs'] = True
        parsed_conf['ca_certs'] = None
        parsed_conf['client_cert'] = None
        parsed_conf['client_key'] = None
        parsed_conf['http_auth'] = None
        parsed_conf['es_username'] = None
        parsed_conf['es_password'] = None
        parsed_conf['es_api_key'] = None
        parsed_conf['es_bearer'] = None
        parsed_conf['aws_region'] = None
        parsed_conf['profile'] = None
        parsed_conf['headers'] = None
        parsed_conf['es_host'] = os.environ.get('ES_HOST', conf['es_host'])
>       parsed_conf['es_hosts'] = os.environ.get('ES_HOSTS', conf['es_hosts'])
E       KeyError: 'es_hosts'

../elastalert/util.py:365: KeyError
___________ test_elasticsearch_client[localhost-9200-bearer-bearer] ____________
[gw3] linux -- Python 3.10.0 /home/elastalert/tests/.tox/py310/bin/python

es_host = 'localhost', es_port = 9200, es_bearer = 'bearer'
es_api_key = 'bearer'

    @pytest.mark.parametrize('es_host, es_port, es_bearer, es_api_key', [
        ('localhost', 9200, '', ''),
        ('localhost', 9200, 'bearer', 'bearer')
    ])
    @mock.patch.dict(os.environ, {'AWS_DEFAULT_REGION': ''})
    def test_elasticsearch_client(es_host, es_port, es_bearer, es_api_key):
        conf = {}
        conf['es_host'] = es_host
        conf['es_port'] = es_port
        if es_bearer:
            conf['es_bearer'] = es_bearer
        if es_api_key:
            conf['es_api_key'] = es_api_key
>       acutual = elasticsearch_client(conf)

util_test.py:462: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../elastalert/util.py:325: in elasticsearch_client
    es_conn_conf = build_es_conn_config(conf)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

conf = {'es_api_key': 'bearer', 'es_bearer': 'bearer', 'es_host': 'localhost', 'es_port': 9200}

    def build_es_conn_config(conf):
        """ Given a conf dictionary w/ raw config properties 'use_ssl', 'es_host', 'es_port'
        'es_username' and 'es_password', this will return a new dictionary
        with properly initialized values for 'es_host', 'es_port', 'use_ssl' and 'http_auth' which
        will be a basicauth username:password formatted string """
        parsed_conf = {}
        parsed_conf['use_ssl'] = os.environ.get('ES_USE_SSL', False)
        parsed_conf['verify_certs'] = True
        parsed_conf['ca_certs'] = None
        parsed_conf['client_cert'] = None
        parsed_conf['client_key'] = None
        parsed_conf['http_auth'] = None
        parsed_conf['es_username'] = None
        parsed_conf['es_password'] = None
        parsed_conf['es_api_key'] = None
        parsed_conf['es_bearer'] = None
        parsed_conf['aws_region'] = None
        parsed_conf['profile'] = None
        parsed_conf['headers'] = None
        parsed_conf['es_host'] = os.environ.get('ES_HOST', conf['es_host'])
>       parsed_conf['es_hosts'] = os.environ.get('ES_HOSTS', conf['es_hosts'])
E       KeyError: 'es_hosts'

../elastalert/util.py:365: KeyError

docs/source/ruletypes.rst Outdated Show resolved Hide resolved
elastalert/util.py Outdated Show resolved Hide resolved
@jertel
Copy link
Owner

jertel commented Nov 12, 2021

@buratinopy Should create_index.py also support multiple hosts? If not, will users need to specify both host and hosts in their config?

@buratinopy
Copy link
Author

@buratinopy Should create_index.py also support multiple hosts? If not, will users need to specify both host and hosts in their config?

I don’t think create_index.py needs support for multiple hosts, it’s enough to access one node in the cluster.
To create indexes, it is enough what will be entered from the keyboard when executing elastalert-create-index

@jertel
Copy link
Owner

jertel commented Nov 12, 2021

@buratinopy Should create_index.py also support multiple hosts? If not, will users need to specify both host and hosts in their config?

I don’t think create_index.py needs support for multiple hosts, it’s enough to access one node in the cluster. To create indexes, it is enough what will be entered from the keyboard when executing elastalert-create-index

The Docker image does not prompt. It will automatically run create_index.py on every startup (does not recreate indexes if they already exist). So those users would be confused since they would be specifying es_hosts yet the Docker image will fail because there was no es_host specified during the index creation step.

@jertel
Copy link
Owner

jertel commented Nov 13, 2021

I've pushed changes to this PR to better document the new es_hosts parameter, and mentioned that es_host must still be specified. This is because of the index creation process, the rule test tool, the Kibana dashboard URL resolution, etc. But the overall goal should still be realized with this PR of supporting multiple nodes during the runtime of the ElastAlert2 process.

I'd like to cut the 2.2.3 release either tonight or tomorrow morning and include this PR, so if anyone has further feedback on this please submit it ASAP.

@buratinopy
Copy link
Author

I've pushed changes to this PR to better document the new es_hosts parameter, and mentioned that es_host must still be specified. This is because of the index creation process, the rule test tool, the Kibana dashboard URL resolution, etc. But the overall goal should still be realized with this PR of supporting multiple nodes during the runtime of the ElastAlert2 process.

I'd like to cut the 2.2.3 release either tonight or tomorrow morning and include this PR, so if anyone has further feedback on this please submit it ASAP.

I think, in case ec_hosts is full, define ec_host as the first element of the ec_hosts list

@jertel
Copy link
Owner

jertel commented Nov 13, 2021

I've pushed changes to this PR to better document the new es_hosts parameter, and mentioned that es_host must still be specified. This is because of the index creation process, the rule test tool, the Kibana dashboard URL resolution, etc. But the overall goal should still be realized with this PR of supporting multiple nodes during the runtime of the ElastAlert2 process.
I'd like to cut the 2.2.3 release either tonight or tomorrow morning and include this PR, so if anyone has further feedback on this please submit it ASAP.

I think, in case ec_hosts is full, define ec_host as the first element of the ec_hosts list

Yes I considered this but stopped due to the complication of the port being embedded in the es_hosts entry. This can be a follow-on improvement and doesn't need to hold up this PR.

@@ -363,6 +363,11 @@ def build_es_conn_config(conf):
parsed_conf['headers'] = None
parsed_conf['es_host'] = os.environ.get('ES_HOST', conf['es_host'])
parsed_conf['es_port'] = int(os.environ.get('ES_PORT', conf['es_port']))

es_hosts = os.environ.get('ES_HOSTS')
es_hosts = parse_hosts(es_hosts, parsed_conf.get('es_port')) if es_hosts else conf.get('es_hosts')
Copy link
Contributor

@JeffAshton JeffAshton Nov 14, 2021

Choose a reason for hiding this comment

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

We should only parse and assign es_hosts if there is an environment variable?

Copy link
Contributor

@JeffAshton JeffAshton Nov 14, 2021

Choose a reason for hiding this comment

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

Pretty sure this will always stomp the yml value. Similar cases pass the default through

os.environ.get('ES_HOST', conf['es_host'])

Doing the if es_hosts: fixes this though. Is there a test case for this scenario?

Copy link
Contributor

@JeffAshton JeffAshton Nov 14, 2021

Choose a reason for hiding this comment

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

Instead of passing parsed_conf.get('es_port'), should we pass the fully resolved value from a couple lines above

parsed_conf['es_port']

That way things work as expected if the port is set in yml but the hosts set via environment variable.

Copy link
Owner

Choose a reason for hiding this comment

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

We should only parse and assign es_hosts if there is an environment variable?

No, it only parses the value if it came from an environment variable. If the environment variable was not set then the `else conf.get('es_hosts') applies. I added unit tests yesterday showing this behavior.

Copy link
Owner

Choose a reason for hiding this comment

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

Instead of passing parsed_conf.get('es_port'), should we pass the fully resolved value from a couple lines above

parsed_conf['es_port']

That way things work as expected if the port is set in yml but the hosts set via environment variable.

Your suggested parsed_conf['es_port'] resolves to the same value as the implemented parsed_conf.get('es_port'), except that the implemented statement is safer since it won't throw an exception if the key doesn't exist in the map.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry completely missed the if es_hosts else conf.get('es_hosts') at the end of the line. Consider simplifying.

Copy link
Contributor

Choose a reason for hiding this comment

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

es_hosts = os.environ.get('ES_HOSTS')
if es_hosts:
    parsed_conf['es_hosts'] = parse_hosts(es_hosts, parsed_conf.get('es_port'))

This avoids a bonus read and write to parsed_conf['es_hosts'] when configured by yml.

elastalert/util.py Outdated Show resolved Hide resolved
Copy link
Contributor

@JeffAshton JeffAshton left a comment

Choose a reason for hiding this comment

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

Fix parse_hosts

@jertel jertel merged commit 39f6750 into jertel:master Nov 14, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants