airflow icon indicating copy to clipboard operation
airflow copied to clipboard

Support restricted index patterns in Elasticsearch log handler

Open kouk opened this issue 3 years ago β€’ 11 comments

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

kouk avatar May 24 '22 12:05 kouk

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

boring-cyborg[bot] avatar May 24 '22 12:05 boring-cyborg[bot]

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

kouk avatar May 24 '22 19:05 kouk

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.

github-actions[bot] avatar May 30 '22 06:05 github-actions[bot]

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.

github-actions[bot] avatar Aug 15 '22 00:08 github-actions[bot]

@kouk are you still working on this PR?

eladkal avatar Aug 15 '22 09:08 eladkal

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

kouk avatar Aug 16 '22 05:08 kouk

@jedcunningham - I think that one waits for you :)

potiuk avatar Sep 05 '22 07:09 potiuk

You will need to rebase @kouk to account for recent changes in main.

potiuk avatar Sep 19 '22 06:09 potiuk

Can you also correct the version according to @jedcunningham comment @kouk ?

potiuk avatar Sep 20 '22 08:09 potiuk

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

kouk avatar Sep 20 '22 11:09 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.

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.

potiuk avatar Sep 20 '22 11:09 potiuk

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 avatar Sep 23 '22 03:09 jedcunningham

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

kouk avatar Oct 06 '22 03:10 kouk

@eladkal @jedcunningham anything else we need here?

kouk avatar Oct 22 '22 14:10 kouk

@jedcunningham appreciate if you could check if anything else is needed here.

kouk avatar Nov 09 '22 17:11 kouk

Rebased to be sure. @jedcunningham - want to still take a look?

potiuk avatar Dec 03 '22 02:12 potiuk

Awesome work, congrats on your first merged pull request!

boring-cyborg[bot] avatar Dec 08 '22 02:12 boring-cyborg[bot]

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

jedcunningham avatar Dec 08 '22 02:12 jedcunningham

thank you @jedcunningham and everyone else.

kouk avatar Dec 08 '22 07:12 kouk