airflow
airflow copied to clipboard
Support restricted index patterns in Elasticsearch log handler
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
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 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 Consider adding an example DAG that shows how users should use it. - Consider using Breeze environment 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 for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the Airflow Coding style. Apache Airflow is a community-driven project and together we are making it better π. In case of doubts contact the developers at: Mailing List: [email protected] Slack: https://s.apache.org/airflow-slack
@jedcunningham hi! I am have run airflow with elasticsearch logging and it is solving the issue for us. We configured a role with restricted access to a specific index pattern, and where before Airflow could not read the logs, with the patch it could. Appreciate it if you could take a look.
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 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.
@kouk are you still working on this PR?
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: @.***>
@jedcunningham - I think that one waits for you :)
You will need to rebase @kouk to account for recent changes in main.
Can you also correct the version according to @jedcunningham comment @kouk ?
@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.
@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.
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.
@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.
@eladkal @jedcunningham anything else we need here?
@jedcunningham appreciate if you could check if anything else is needed here.
Rebased to be sure. @jedcunningham - want to still take a look?
Awesome work, congrats on your first merged pull request!
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).
thank you @jedcunningham and everyone else.