Skip to content

Conversation

@rasulkarimov
Copy link
Contributor

This PR introduces support for proxy settings in the gitSync containers to accommodate scenarios where the git repository is located behind a corporate proxy server. The proxy settings can be configured in a Kubernetes secret with keys HTTPS_PROXY, HTTP_PROXY, and NO_PROXY. These settings are then accessed via the 'dags.gitSync.proxySettingsSecret' variable, which provides them as environment variables for the gitSync containers.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg
Copy link

boring-cyborg bot commented Apr 15, 2024

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 Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, 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.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    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

Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd much rather we move gitSync.env to accept V1EnvVars vs restricting it artificially like we are now.

@rasulkarimov, can you give that approach a shot instead?

@rasulkarimov rasulkarimov changed the title gitSync: proxySettingsSecret to support when repository behind proxy gitSync: add envFrom to support proxy settings from k8s secret Apr 15, 2024
@rasulkarimov
Copy link
Contributor Author

@jedcunningham
"restricting it artificially" - I agree with your point, I just wanted to follow the established pattern in the "git_sync_container" template.
However, using gitSync.env is not ideal because proxy settings may contain secret credentials, which would require defining them directly in yaml.

Therefore, I have updated the PR to allow envFrom in git_sync_container. This will enable us to provide proxy settings with credentials to the git-sync container from a Kubernetes secret(without restricting it artificially).

@romsharon98
Copy link
Contributor

romsharon98 commented Apr 21, 2024

look good, can you fix current tests to validate your changes?

@rasulkarimov rasulkarimov force-pushed the git-sync-support-proxy-settings branch from f0abc8c to 3efd5a0 Compare May 1, 2024 17:35
@rasulkarimov
Copy link
Contributor Author

look good, can you fix current tests to validate your changes?

Thank you! Fixed tests.

@rasulkarimov rasulkarimov force-pushed the git-sync-support-proxy-settings branch from 3efd5a0 to f634cb4 Compare May 29, 2024 09:38
@rasulkarimov
Copy link
Contributor Author

I'd much rather we move gitSync.env to accept V1EnvVars vs restricting it artificially like we are now.

@rasulkarimov, can you give that approach a shot instead?

Can we merge this PR?

@eladkal eladkal requested a review from amoghrajesh May 29, 2024 10:14
Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also i think since it is a pretty trivial case, do we also document it?

@eladkal eladkal added this to the Airflow Helm Chart 1.15.0 milestone Jun 24, 2024
@rasulkarimov rasulkarimov changed the title gitSync: add envFrom to support proxy settings from k8s secret gitSync: add extraEnvFrom to git-sync containers to support proxy settings from k8s secret Jun 24, 2024
@rasulkarimov rasulkarimov requested a review from eladkal June 24, 2024 22:28
@rasulkarimov rasulkarimov force-pushed the git-sync-support-proxy-settings branch 3 times, most recently from 232022c to b33f362 Compare July 3, 2024 08:44
@rasulkarimov
Copy link
Contributor Author

I'd much rather we move gitSync.env to accept V1EnvVars vs restricting it artificially like we are now.

@rasulkarimov, can you give that approach a shot instead?

can we merge?

@eladkal eladkal force-pushed the git-sync-support-proxy-settings branch from 1fe7697 to b304be4 Compare July 20, 2024 08:17
@eladkal
Copy link
Contributor

eladkal commented Jul 20, 2024

cc @jedcunningham as you marked request changes

@jedcunningham jedcunningham merged commit c2a54ef into apache:main Jul 20, 2024
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 20, 2024

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

@jedcunningham
Copy link
Member

Thanks @rasulkarimov! Sorry for the slow merge. Congrats on your first commit 🎉

@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jul 22, 2024
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:helm-chart Airflow Helm Chart changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants