-
Notifications
You must be signed in to change notification settings - Fork 310
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
Conversation
Please do not modify the version of setup.py without permission. Please put it back. |
Write a change log. |
Please restore the following files Chart.yaml |
Why am I not running make test-docker? .. If it can be executed, execute it and check if there is no problem. |
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. |
I ran unit tests, passed without errors. |
|
I ran make test-docker. There is a problem with ruletypes.rst. Please correct.
|
@nsano-rururu i fixed it, please check. |
Did not help
|
I think the local download python: 3-slim-buster is out of date. Maybe you need to get it once and get it again.
|
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. |
The error is gone.
|
Really ... Looks like my colleagues from the DLP department broke my internet access... |
Thanks! |
elastalert/util.py
Outdated
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) |
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.
Why is es_host
getting the port appended to it and being converted to a list? Pretty sure this will break AWS auth:
elastalert2/elastalert/util.py
Line 332 in 27deb8a
es_conn_conf['http_auth'] = auth(host=es_conn_conf['es_host'], |
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.
When you say "will break AWS auth" are you referring to OpenSearch?
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 I'm reading the change correctly, es_host
will become
["{host}:{port}".format(host=host, port=port)]
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.
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.
Sure looks like it would break it:
elastalert2/elastalert/util.py
Line 325 in 80b7c4f
es_conn_conf = build_es_conn_config(conf) |
elastalert2/elastalert/util.py
Line 332 in 80b7c4f
es_conn_conf['http_auth'] = auth(host=es_conn_conf['es_host'], |
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.
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.
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 think an array over a csv string makes more sense anyway.
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.
Ok i can redo
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
@nsano-rururu can you run make test-docker on yourself, please? (I have not yet fixed access to the repositories at work) |
error
|
@buratinopy Should |
I don’t think create_index.py needs support for multiple hosts, it’s enough to access one node in the cluster. |
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 |
… add new es_hosts param to schema
I've pushed changes to this PR to better document the new 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 |
@@ -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') |
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 should only parse and assign es_hosts
if there is an environment variable?
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.
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?
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.
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.
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 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.
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.
Instead of passing
parsed_conf.get('es_port')
, should we pass the fully resolved value from a couple lines aboveparsed_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.
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.
Sorry completely missed the if es_hosts else conf.get('es_hosts')
at the end of the line. Consider simplifying.
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.
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.
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.
Fix parse_hosts
Description
Support for multi es instances
Checklist
make test-docker
with my changes.Questions or Comments
I didn't understand how to fill in CHANGELOG.md
WIP