You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/05/24 12:14:53 UTC
[GitHub] [airflow] kouk opened a new pull request, #23888: Support restricted index patterns in Elasticsearch log handler
kouk opened a new pull request, #23888:
URL: https://github.com/apache/airflow/pull/23888
Sometimes Airflow doesn't have the ability to search across all indices
in an Elasticsearch server. This might be due to security settings in
the server. In these cases fetching the remote logs fails. To fix this
we create a index_patterns configuration setting that can be set to a
more restrictive pattern.
closes: #16828
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r911495200
##########
airflow/config_templates/config.yml:
##########
@@ -2246,6 +2246,13 @@
type: string
example: ~
default: "offset"
+ - name: index_patterns
+ description: |
+ Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ version_added: 2.3.3
Review Comment:
```suggestion
version_added: 2.4.0
```
Oh, and this will end up being in core 2.4.0, as this isn't a bug (though I'm glad to see it supported 🍺).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884427993
##########
airflow/config_templates/config.yml:
##########
@@ -2246,6 +2246,13 @@
type: string
example: ~
default: "offset"
+ - name: index_patterns
+ description: |
+ Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ version_added: 2.3.0
+ type: string
+ example: something-*
+ default: "_all"
Review Comment:
For background:
https://github.com/elastic/elasticsearch-py/blob/d66dfcb9c7e2bc592392ca00fde62889d181c0af/elasticsearch/_sync/client/__init__.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1236665201
@jedcunningham - I think that one waits for you :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1140743975
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884448726
##########
tests/providers/elasticsearch/log/elasticmock/fake_elasticsearch.py:
##########
@@ -362,6 +363,18 @@ def match_must_phrase(document, matches, must):
# use in as a proxy for match_phrase
matches.append(document)
+ # Check index(es) exists
+ def _validate_search_targets(self, targets):
+ matches = set()
+ for target in targets:
+ if target == '_all':
+ matches.update(self.__documents_dict.keys())
+ elif '*' in target:
+ matches.update(fnmatch.filter(self.__documents_dict.keys(), target))
+ elif target not in self.__documents_dict:
+ raise NotFoundError(404, f'IndexMissingException[[{target}] missing]')
Review Comment:
For example:
$ curl 'localhost:9200/whateverman-*/_search'
{"took":0,"timed_out":false,"_shards":{"total":0,"successful":0,"skipped":0,"failed":0},"hits":{"total":{"value":0,"relation":"eq"},"max_score":0.0,"hits":[]}}
but
$ curl 'localhost:9200/whateverman-foobar/_search'
{"error":{"root_cause":[{"type":"index_not_found_exception","reason":"no such index [whateverman-foobar]","resource.type":"index_or_alias","resource.id":"whateverman-foobar","index_uuid":"_na_","index":"whateverman-foobar"}],"type":"index_not_found_exception","reason":"no such index [whateverman-foobar]","resource.type":"index_or_alias","resource.id":"whateverman-foobar","index_uuid":"_na_","index":"whateverman-foobar"},"status":404}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r1042764536
##########
airflow/config_templates/config.yml:
##########
@@ -2317,6 +2317,13 @@
type: string
example: ~
default: "offset"
+ - name: index_patterns
+ description: |
+ Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ version_added: 2.5.0
Review Comment:
```suggestion
version_added: 2.6.0
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1336022030
Rebased to be sure. @jedcunningham - want to still take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884435385
##########
tests/providers/elasticsearch/log/elasticmock/fake_elasticsearch.py:
##########
@@ -362,6 +363,18 @@ def match_must_phrase(document, matches, must):
# use in as a proxy for match_phrase
matches.append(document)
+ # Check index(es) exists
+ def _validate_search_targets(self, targets):
+ matches = set()
+ for target in targets:
+ if target == '_all':
+ matches.update(self.__documents_dict.keys())
+ elif '*' in target:
+ matches.update(fnmatch.filter(self.__documents_dict.keys(), target))
+ elif target not in self.__documents_dict:
+ raise NotFoundError(404, f'IndexMissingException[[{target}] missing]')
Review Comment:
The `keys()` calls are not needed.
Given that a wildcard match is ignored if it does not match anything, we probably should do the same about normal names.
```suggestion
if target == '_all':
matches.update(self.__documents_dic)
else:
matches.update(fnmatch.filter(self.__documents_dict, target))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884427431
##########
airflow/config_templates/config.yml:
##########
@@ -2246,6 +2246,13 @@
type: string
example: ~
default: "offset"
+ - name: index_patterns
+ description: |
+ Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ version_added: 2.3.0
+ type: string
+ example: something-*
+ default: "_all"
Review Comment:
If this is a special case in Elastic, I am OK to use `_all` as the default. The main motivation behind using `*` is you don’t need a special case in `_validate_search_targets` for it. But if we need to handle `_all` specially anyway, the difference is minimal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884431761
##########
airflow/config_templates/config.yml:
##########
@@ -2246,6 +2246,13 @@
type: string
example: ~
default: "offset"
+ - name: index_patterns
+ description: |
+ Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ version_added: 2.3.0
+ type: string
+ example: something-*
+ default: "_all"
Review Comment:
in fact I i'm realising now that `FakeElasticsearch` should probably handle the empty string case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r975690279
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -114,6 +116,7 @@ def __init__(
self.json_fields = [label.strip() for label in json_fields.split(",")]
self.host_field = host_field
self.offset_field = offset_field
+ self.index_patterns = index_patterns or es_configs.get('index_patterns', '_all')
Review Comment:
```suggestion
self.index_patterns = index_patterns or conf.get('elasticsearch', 'index_patterns', fallback='_all')
```
Then you can drop 98 (`es_configs`). Also, can't we set this default on 86, letting us do a simple assignment here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1252007475
Can you also correct the version according to @jedcunningham comment @kouk ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884556086
##########
airflow/config_templates/config.yml:
##########
@@ -2246,6 +2246,13 @@
type: string
example: ~
default: "offset"
+ - name: index_patterns
+ description: |
+ Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ version_added: 2.3.0
Review Comment:
```suggestion
version_added: 2.3.3
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1252189053
@jedcunningham @potiuk I bumped the version to 2.4.1. Also I pushed acommit that updates the tests to emulate the case where we have an older version of airflow core, where no index pattern is passed by default to the constructor. Let me know if you prefer the previous version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1341890142
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884451053
##########
tests/providers/elasticsearch/log/elasticmock/fake_elasticsearch.py:
##########
@@ -362,6 +363,18 @@ def match_must_phrase(document, matches, must):
# use in as a proxy for match_phrase
matches.append(document)
+ # Check index(es) exists
+ def _validate_search_targets(self, targets):
+ matches = set()
+ for target in targets:
+ if target == '_all':
+ matches.update(self.__documents_dict.keys())
+ elif '*' in target:
+ matches.update(fnmatch.filter(self.__documents_dict.keys(), target))
+ elif target not in self.__documents_dict:
+ raise NotFoundError(404, f'IndexMissingException[[{target}] missing]')
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884394064
##########
airflow/config_templates/config.yml:
##########
@@ -2246,6 +2246,13 @@
type: string
example: ~
default: "offset"
+ - name: index_patterns
+ description: |
+ Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ version_added: 2.3.0
+ type: string
+ example: something-*
+ default: "_all"
Review Comment:
Is this a good default? Since this handles wildcard expansion, perhaps we could simply make the default `*` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884436699
##########
tests/providers/elasticsearch/log/elasticmock/fake_elasticsearch.py:
##########
@@ -362,6 +363,18 @@ def match_must_phrase(document, matches, must):
# use in as a proxy for match_phrase
matches.append(document)
+ # Check index(es) exists
+ def _validate_search_targets(self, targets):
+ matches = set()
+ for target in targets:
+ if target == '_all':
+ matches.update(self.__documents_dict.keys())
+ elif '*' in target:
+ matches.update(fnmatch.filter(self.__documents_dict.keys(), target))
+ elif target not in self.__documents_dict:
+ raise NotFoundError(404, f'IndexMissingException[[{target}] missing]')
Review Comment:
The `keys()` call are not needed.
Since an exception is raised when `target` does not exist, maybe we should do the same if a wildcard pattern does not match anything?
```suggestion
if target == '_all':
matches.update(self.__documents_dict)
continue
target_matches = fnmatch.filter(self.__documents_dict, target)
if not target_matches:
raise NotFoundError(404, f'IndexMissingException[[{target}] missing]')
matches.update(target_matches)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884431366
##########
airflow/config_templates/config.yml:
##########
@@ -2246,6 +2246,13 @@
type: string
example: ~
default: "offset"
+ - name: index_patterns
+ description: |
+ Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ version_added: 2.3.0
+ type: string
+ example: something-*
+ default: "_all"
Review Comment:
@uranusjr I'm not 100% sure but I think that it's a very common Elasticsearch idiom to use `_all` or the empty string. Perhaps we could change the default to an empty string?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1136326765
@jedcunningham hi! I am running airflow with this patch at work and it is solving the issue for us. Appreciate it if you could take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham merged pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
jedcunningham merged PR #23888:
URL: https://github.com/apache/airflow/pull/23888
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1255765913
Marking this for 2.5.0, but after the bulk of these changes land in a provider release, it'll work if you set the config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1287814558
@eladkal @jedcunningham anything else we need here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1269243272
@jedcunningham thanks for pointing that out. I wasn't familiar enough with the config code to realize that it's ConfigParser under the hood. I've updated both the version and the config handling code like you suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1214481307
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884394064
##########
airflow/config_templates/config.yml:
##########
@@ -2246,6 +2246,13 @@
type: string
example: ~
default: "offset"
+ - name: index_patterns
+ description: |
+ Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ version_added: 2.3.0
+ type: string
+ example: something-*
+ default: "_all"
Review Comment:
Is this a good default? Since this handles wildcard expansion, perhaps we could simply make the default `*` or an empty string instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1252243566
> @jedcunningham @potiuk I bumped the version to 2.4.1. Also I pushed acommit that updates the tests to emulate the case where we have an older version of airflow core, where no index pattern is passed by default to the constructor. Let me know if you prefer the previous version.
I think it will have to be 2.5.0 - > we are following semver and we only add bugfixes to "patchlevel" versions. This one will have to wait for 2.5 to be released.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1309091460
@jedcunningham appreciate if you could check if anything else is needed here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884427993
##########
airflow/config_templates/config.yml:
##########
@@ -2246,6 +2246,13 @@
type: string
example: ~
default: "offset"
+ - name: index_patterns
+ description: |
+ Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ version_added: 2.3.0
+ type: string
+ example: something-*
+ default: "_all"
Review Comment:
For background:
https://github.com/elastic/elasticsearch-py/blob/d66dfcb9c7e2bc592392ca00fde62889d181c0af/elasticsearch/_sync/client/__init__.py#L1166
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884446314
##########
tests/providers/elasticsearch/log/elasticmock/fake_elasticsearch.py:
##########
@@ -362,6 +363,18 @@ def match_must_phrase(document, matches, must):
# use in as a proxy for match_phrase
matches.append(document)
+ # Check index(es) exists
+ def _validate_search_targets(self, targets):
+ matches = set()
+ for target in targets:
+ if target == '_all':
+ matches.update(self.__documents_dict.keys())
+ elif '*' in target:
+ matches.update(fnmatch.filter(self.__documents_dict.keys(), target))
+ elif target not in self.__documents_dict:
+ raise NotFoundError(404, f'IndexMissingException[[{target}] missing]')
Review Comment:
@uranusjr actually Elasticsearch treats the case of a wildcard pattern not matching differently. See the `allow_no_indices` query parameter: https://www.elastic.co/guide/en/elasticsearch/reference/8.2/search-search.html#search-search-api-query-params
tl;dr if you specify an index name _without a pattern_ this results in 404. If you specify an index _with a pattern_ then by default you do not get a 404 even if there are no matching indices.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884450207
##########
tests/providers/elasticsearch/log/elasticmock/fake_elasticsearch.py:
##########
@@ -362,6 +363,18 @@ def match_must_phrase(document, matches, must):
# use in as a proxy for match_phrase
matches.append(document)
+ # Check index(es) exists
+ def _validate_search_targets(self, targets):
+ matches = set()
+ for target in targets:
+ if target == '_all':
+ matches.update(self.__documents_dict.keys())
+ elif '*' in target:
+ matches.update(fnmatch.filter(self.__documents_dict.keys(), target))
+ elif target not in self.__documents_dict:
+ raise NotFoundError(404, f'IndexMissingException[[{target}] missing]')
Review Comment:
OK, let’s just fix the unnecessary `keys()` calls then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1250603460
You will need to rebase @kouk to account for recent changes in main.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1341892259
Thanks @kouk! Congrats on your first commit 🎉. (I apologize for the delay)
As I mentioned earlier, since almost all of the changes are in the provider, this feature will be available once it's released in the provider (even before 2.6 with the config option is released).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1342197897
thank you @jedcunningham and everyone else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1135844423
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884425596
##########
airflow/config_templates/config.yml:
##########
@@ -2246,6 +2246,13 @@
type: string
example: ~
default: "offset"
+ - name: index_patterns
+ description: |
+ Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ version_added: 2.3.0
+ type: string
+ example: something-*
+ default: "_all"
Review Comment:
@uranusjr indeed we could use `*`. According to the documentation it is equivalent to `_all`. See
https://www.elastic.co/guide/en/elasticsearch/reference/8.2/search-search.html#search-search-api-path-params
Initially I had it set to `*` and then changed it to `_all` because it seemed more readable. That said, I don't mind changing it back to `*` if ASF members prefer it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on a diff in pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on code in PR #23888:
URL: https://github.com/apache/airflow/pull/23888#discussion_r884449872
##########
tests/providers/elasticsearch/log/elasticmock/fake_elasticsearch.py:
##########
@@ -362,6 +363,18 @@ def match_must_phrase(document, matches, must):
# use in as a proxy for match_phrase
matches.append(document)
+ # Check index(es) exists
+ def _validate_search_targets(self, targets):
+ matches = set()
+ for target in targets:
+ if target == '_all':
+ matches.update(self.__documents_dict.keys())
+ elif '*' in target:
+ matches.update(fnmatch.filter(self.__documents_dict.keys(), target))
+ elif target not in self.__documents_dict:
+ raise NotFoundError(404, f'IndexMissingException[[{target}] missing]')
Review Comment:
However I haven't bothered to implement the case where`allow_no_indices=false` (would be necessary if the provider implementation was changed to override the default behavior):
$ curl 'localhost:9200/whateverman-*/_search?allow_no_indices=false'
{"error":{"root_cause":[{"type":"index_not_found_exception","reason":"no such index [whateverman-*]","resource.type":"index_or_alias","resource.id":"whateverman-*","index_uuid":"_na_","index":"whateverman-*"}],"type":"index_not_found_exception","reason":"no such index [whateverman-*]","resource.type":"index_or_alias","resource.id":"whateverman-*","index_uuid":"_na_","index":"whateverman-*"},"status":404}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kouk commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
kouk commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1216166188
Yeah, I was on vacation..will look at this today.
On Mon, Aug 15, 2022, 12:38 eladkal ***@***.***> wrote:
> @kouk <https://github.com/kouk> are you still working on this PR?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/airflow/pull/23888#issuecomment-1214823800>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AADPKRZ56Z75UETF35ZX273VZIF2JANCNFSM5WZLICWQ>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #23888: Support restricted index patterns in Elasticsearch log handler
Posted by GitBox <gi...@apache.org>.
eladkal commented on PR #23888:
URL: https://github.com/apache/airflow/pull/23888#issuecomment-1214823800
@kouk are you still working on this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org