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 OpenSearch v1.0.0 #483

Merged
merged 8 commits into from
Sep 25, 2021
Merged

Support OpenSearch v1.0.0 #483

merged 8 commits into from
Sep 25, 2021

Conversation

nbrownus
Copy link
Contributor

@nbrownus nbrownus commented Sep 24, 2021

Description

Maps OpenSearch (currently v1.0.0) to Elasticsearch v7.10.2 to enable appropriate compatibility

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 reviewed the tests and there aren't any covering existing version detection. Happy to add some but that would require some inventing that I assume the maintainers would have a strong opinion about.

I have not modified any documentation, mainly because I am not sure how prominently you would want to voice that this project supports OpenSearch.

Closes #482

@nbrownus
Copy link
Contributor Author

nbrownus commented Sep 24, 2021

I am still getting this error when trying to create the elastalert index, though my test rules are now working. this is no longer an issue

RequestError(400, 'illegal_argument_exception', 'Types cannot be provided in put mapping requests, unless the include_type_name parameter is set to true.')

Seems we'd need to swap the elasticsearch library as mentioned by #482 (comment)

@ferozsalam
Copy link
Collaborator

Regarding the documentation question, I don't see any reason not to mention OpenSearch. A line or two saying that the project is OpenSearch compatible somewhere on this page is probably sufficient.

@nbrownus
Copy link
Contributor Author

Seems to be working rather well in my tests at this point

@nsano-rururu
Copy link
Collaborator

@nbrownus

I'm curious, so I'll ask you a question. In the case of opensearch, can't I get the value of es_client.info () ["version"] ["number"]? ..
If you can get 1.0, I think it is better to set 7.10.2 to esversion for 1.0. Considering when OpenSearch was upgraded in the future.

@ferozsalam

In addition, I plan to change to elasticsearch-py 8.0.0 when it supports elasticsearch 8, but probably OpenSearch will not be able to connect (the implementation should have been added in elasticsearch-py 7.14.0), so opensearch-py at that time I think it will be necessary to take measures to incorporate.

@ferozsalam
Copy link
Collaborator

@nbrownus it looks like some dictionary accesses need to be modified to use get()/handle the lack of the distribution key better:

self = <elastalert.ruletypes.NewTermsRule object at 0x7fe85ec66bb0>

    def is_five_or_above(self):
        esinfo = self.es.info()['version']
>       if esinfo['distribution'] == "opensearch":
E       elastalert.util.EAException: Error searching for existing terms: KeyError('distribution')

@nsano-rururu
Copy link
Collaborator

A bug has been embedded. Isn't it terrible?

elastalert/create_index.py

bug

    esinfo = es_client.info()['version']
    if esinfo['distribution'] == "opensearch":
        # OpenSearch is based on Elasticsearch 7.10.2, currently only v1.0.0 exists
        # https://opensearch.org/
        esversion = "7.10.2"
    else:
        esversion = esinfo['version']

Please correct.

    esinfo = es_client.info()['version']
    if esinfo['distribution'] == "opensearch":
        # OpenSearch is based on Elasticsearch 7.10.2, currently only v1.0.0 exists
        # https://opensearch.org/
        esversion = "7.10.2"
    else:
        esversion =  esinfo["number"]
   
   print("Elastic Version: " + esversion)

elastalert/ruletypes.py

bug

        esinfo = self.es.info()['version']
        if esinfo['distribution'] == "opensearch":
            # OpenSearch is based on Elasticsearch 7.10.2, currently only v1.0.0 exists
            # https://opensearch.org/
            return True
        else:
            return int(esinfo['version'][0]) >= 5

Please correct.

        esinfo = self.es.info()['version']
        if esinfo['distribution'] == "opensearch":
            # OpenSearch is based on Elasticsearch 7.10.2, currently only v1.0.0 exists
            # https://opensearch.org/
            return True
        else:
            return int(esinfo['number'][0]) >= 5

@nsano-rururu
Copy link
Collaborator

nsano-rururu commented Sep 24, 2021

base_test.py

Line 848

mock_es.info.return_value = {'version': {'number': '2.0'}}

mock_es.info.return_value = {'version': {'number': '2.0', 'distribution': ''}}

conftest.py

Line 70

self.info = mock.Mock(return_value={'status': 200, 'name': 'foo', 'version': {'number': '2.0'}})

self.info = mock.Mock(return_value={'status': 200, 'name': 'foo', 'version': {'number': '2.0', 'distribution': ''}})

Line 92

self.info = mock.Mock(return_value={'status': 200, 'name': 'foo', 'version': {'number': '6.6.0'}})

self.info = mock.Mock(return_value={'status': 200, 'name': 'foo', 'version': {'number': '6.6.0', 'distribution': ''}})

rules_test.py

Line 570, 622, 640, 665, 735, 772

mock_es.return_value.info.return_value = {'version': {'number': '2.x.x'}}

mock_es.return_value.info.return_value = {'version': {'number': '2.x.x', 'distribution': ''}}

@nsano-rururu
Copy link
Collaborator

It is very dangerous if you do not check if it works properly with elasticsearch 7.x as well as opensearch before merging.

@nsano-rururu
Copy link
Collaborator

@ferozsalam @jertel

Is there anything else you care about?

@jertel
Copy link
Owner

jertel commented Sep 25, 2021

I want to try it first with my ES cluster before it gets merged. If I get time this weekend I'll do that and merge it in if it looks good.

@ferozsalam
Copy link
Collaborator

No concerns from me, thanks!

@jertel
Copy link
Owner

jertel commented Sep 25, 2021

I've tested against ES 7.x and did not encounter problems. However, I then tested against a brand new AWS OpenSearch cluster and encountered two problems:

  1. The first time I ran ElastAlert 2 and it tried to create the indices it failed with the same error that @nbrownus posted here Support OpenSearch v1.0.0 #483 (comment). Running it again seemed to get past that point, probably since at least some of the indices had been created successfully.
  2. Next, the ElastAlert 2 process aborted due to a parsing error while querying the elastalert_status index just a few seconds after starting up. The request and response are below:
2021-09-25 18:38:27,506    DEBUG        elasticsearch > {"query":{"query_string":{"query":"!_exists_:aggregate_id AND alert_sent:false"}},"filter":{"range":{"alert_time":{"from":"2021-09-23T18:38:27.412062Z","to":"2021-09-25T18:38:27.412085Z"}}},"sort":{"alert_time":{"order":"asc"}}}
2021-09-25 18:38:27,506    DEBUG        elasticsearch < {"error":{"root_cause":[{"type":"parsing_exception","reason":"Unknown key for a START_OBJECT in [filter].","line":1,"col":92}],"type":"parsing_exception","reason":"Unknown key for a START_OBJECT in [filter].","line":1,"col":92},"status":400}

Based on my short time testing I'd say that more work is still needed to allow ElastAlert 2 to work with OpenSearch. But since this is a step forward and isn't breaking existing functionality I'll proceed with the merge.

@jertel jertel merged commit 0248cd0 into jertel:master Sep 25, 2021
@jertel
Copy link
Owner

jertel commented Sep 26, 2021

Quick follow-up: After reviewing the problem reported in #487 I realize my previous test must have been invalid, and likely wasn't using the correct code from this PR. After correcting the issue in #487 and re-testing I was able to successfully start ElastAlert 2 against another brand new AWS OpenSearch cluster. Both index creation and rule processing were successful, as well as elastalert-test-rule. I'll leave the documentation as-is, so that it continues to annotate OpenSearch as "Under development" while we get more feedback on this. Thanks everyone for their contributions, reviews, and testing.

@nsano-rururu nsano-rururu mentioned this pull request Oct 7, 2021
6 tasks
@louzadod
Copy link
Contributor

louzadod commented Oct 21, 2021

Hi, do you @jertel know which permissions I have to provide to the elastalert user? Currently, I gave:

        "allowed_actions" : [
          "indices:read/search*",
          "indices:admin/mappings/get",
          "indices:admin/mappings/fields/get*",
          "indices:admin/resolve/index",
          "crud",
          "create_index",
          "cluster_monitor",
          "cluster:monitor/main",
          "indices:data/read/search*"

But Elastalert (v 2.2.2) still complains:

elasticsearch.exceptions.AuthorizationException: AuthorizationException(403, 'security_exception', 'no permissions for [cluster:monitor/main] and User [name=elastalert, backend_roles=[], requestedTenant=null]')

I'm using OpenSearch 1.1.0 on premises.

@nsano-rururu
Copy link
Collaborator

@louzadod

I don't know because I don't use OpenSearch, but it is rejected when connecting to OpenSearch. I think you should ask questions in the OpenSearch community as follows: It's just a guess from here, but I think it's a problem with setting up OpenSearch security plugins.
https://discuss.opendistrocommunity.dev/t/no-permissions-for-cluster-monitor-main/2791

@louzadod
Copy link
Contributor

louzadod commented Oct 26, 2021

@nsano-rururu , the problem was totally on my side. I was setting "cluster:monitor/main" in the index permissions section insted of cluster's. Thanks anyway.
I'm using ElastAlert2 2.2.2 and OpenSearch 1.1.

@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.

Support opensearch v1.0
5 participants