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

More controls for pgbouncer secrets configuration #45248

Conversation

andrii-korotkov-verkada
Copy link
Contributor

closes: #45171

Allow to disable adding default secret mounts for pgbouncer configs as well as metrics exported database url env variable. This can be useful for cases, where the value is retrieved other way, e.g. secrets provider class.


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

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Could you add unit tests for this change?

@andrii-korotkov-verkada
Copy link
Contributor Author

I've refined the functionality and added the tests.

@andrii-korotkov-verkada
Copy link
Contributor Author

Also, if this gets merged, how can I release a new chart version?

@eladkal eladkal added this to the Airflow Helm Chart 1.16.0 milestone Dec 29, 2024
@andrii-korotkov-verkada
Copy link
Contributor Author

How do I trigger tests for Helm etc.? Are they automatically triggered after some time or just don't update the checks summary at first?

@andrii-korotkov-verkada
Copy link
Contributor Author

Screenshot 2024-12-29 at 12 58 32 PM

I see, it has to be maintainer-approved. Curious if at least some tests can be made to run by default. That can greatly speed up the iteration.

@potiuk
Copy link
Member

potiuk commented Dec 29, 2024

I see, it has to be maintainer-approved. Curious if at least some tests can be made to run by default. That can greatly speed up the iteration.

Not until your first PR gets merged. This is protection against bots mining bitcoins and roque PRs trying to exploit weaknesses in Github Actions, and it's mandated by the Infra and security team of The ASF.

@potiuk
Copy link
Member

potiuk commented Dec 29, 2024

But you can run exactly the same tests locally -> All tests that are run in CI are runnable locally - that is basic assumptions for our CI system https://github.com/apache/airflow/tree/main/contributing-docs/testing -> you can see more details about all the types of tests we have and how to reproduce them locally 1:1

@andrii-korotkov-verkada
Copy link
Contributor Author

andrii-korotkov-verkada commented Dec 29, 2024

Not until your first PR gets merged. This is protection against bots mining bitcoins and roque PRs trying to exploit weaknesses in Github Actions, and it's mandated by the Infra and security team of The ASF.

Ah, I see, that makes sense.

But you can run exactly the same tests locally -> All tests that are run in CI are runnable locally - that is basic assumptions for our CI system https://github.com/apache/airflow/tree/main/contributing-docs/testing -> you can see more details about all the types of tests we have and how to reproduce them locally 1:1

Thanks for the hints, I've naively tried to just run pytest with no args :)

@andrii-korotkov-verkada
Copy link
Contributor Author

Tho I might be running into some incompatibility issues with pytest

ERROR: while parsing the following warning configuration:

  error::pytest.PytestReturnNotNoneWarning

This error occurred:

Traceback (most recent call last):
  File "/Users/andrii.korotkov/.pyenv/versions/3.10.12/lib/python3.10/site-packages/_pytest/config/__init__.py", line 1638, in parse_warning_filter
    category: Type[Warning] = _resolve_warning_category(category_)
  File "/Users/andrii.korotkov/.pyenv/versions/3.10.12/lib/python3.10/site-packages/_pytest/config/__init__.py", line 1677, in _resolve_warning_category
    cat = getattr(m, klass)
  File "/Users/andrii.korotkov/.pyenv/versions/3.10.12/lib/python3.10/site-packages/pytest/__init__.py", line 165, in __getattr__
    raise AttributeError(f"module {__name__} has no attribute {name}")
AttributeError: module pytest has no attribute PytestReturnNotNoneWarning

@potiuk
Copy link
Member

potiuk commented Dec 29, 2024

Thanks for the hints, I've naively tried to just run pytest :)

It works as long as you have the same environment.

Those instructions on running tests are explaining how to get the SAME environment to run your pytest command in - in general. To avoid "works for me" (or rather "does not work for me").

Airflow has 700+ dependencies, 3 different databases in 8 versions, 10 integrations, kubernetes and a number of other variations - recreating it all by just running pytest is a nice dream (and our vision in the future with testcontainers and other things) - but so far adding tools to make things running in reproducible way is a bit more efficient (as long as you follow it).

@potiuk
Copy link
Member

potiuk commented Dec 29, 2024

Tho I might be running into some incompatibility issues with pytest

Just follow the docs. Breeze is best to reproduce the env. You can also use uv for local venv. But the docs are complete and explain what to do.

@andrii-korotkov-verkada
Copy link
Contributor Author

andrii-korotkov-verkada commented Dec 30, 2024

I've got non-db unit tests, helm tests and k8s tests to pass when running with breeze.

@potiuk
Copy link
Member

potiuk commented Dec 31, 2024

I've got non-db unit tests, helm tests and k8s tests to pass when running with breeze.

image

Always Be Rebased.

@andrii-korotkov-verkada andrii-korotkov-verkada force-pushed the 45171-more-control-over-pgbouncer-secrets-references branch from a689d11 to 70ff213 Compare December 31, 2024 14:12
@andrii-korotkov-verkada andrii-korotkov-verkada force-pushed the 45171-more-control-over-pgbouncer-secrets-references branch from 70ff213 to 843c035 Compare January 8, 2025 21:50
@andrii-korotkov-verkada
Copy link
Contributor Author

Do you know why this error might happen?

Error: Failed to CreateArtifact: Received non-retryable error: Failed request: (409) Conflict: an artifact with this name already exists on the workflow run

I don't think I control artifact names in the PR, at least not directly...

@andrii-korotkov-verkada andrii-korotkov-verkada force-pushed the 45171-more-control-over-pgbouncer-secrets-references branch from 843c035 to edf6a9e Compare January 10, 2025 02:46
@andrii-korotkov-verkada
Copy link
Contributor Author

I've used it in a live environment and it works well.

closes: apache#45171

Allow to disable adding default secret mounts for pgbouncer configs as well as metrics exported database url env variable. This can be useful for cases, where the value is retrieved other way, e.g. secrets provider class.

Signed-off-by: Andrii Korotkov <[email protected]>
@andrii-korotkov-verkada andrii-korotkov-verkada force-pushed the 45171-more-control-over-pgbouncer-secrets-references branch from edf6a9e to 2dae26c Compare January 12, 2025 02:39
@potiuk potiuk merged commit a90ec20 into apache:main Jan 12, 2025
60 checks passed
Copy link

boring-cyborg bot commented Jan 12, 2025

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

agupta01 pushed a commit to agupta01/airflow that referenced this pull request Jan 13, 2025
closes: apache#45171

Allow to disable adding default secret mounts for pgbouncer configs as well as metrics exported database url env variable. This can be useful for cases, where the value is retrieved other way, e.g. secrets provider class.

Signed-off-by: Andrii Korotkov <[email protected]>
karenbraganz pushed a commit to karenbraganz/airflow that referenced this pull request Jan 13, 2025
closes: apache#45171

Allow to disable adding default secret mounts for pgbouncer configs as well as metrics exported database url env variable. This can be useful for cases, where the value is retrieved other way, e.g. secrets provider class.

Signed-off-by: Andrii Korotkov <[email protected]>
HariGS-DB pushed a commit to HariGS-DB/airflow that referenced this pull request Jan 16, 2025
closes: apache#45171

Allow to disable adding default secret mounts for pgbouncer configs as well as metrics exported database url env variable. This can be useful for cases, where the value is retrieved other way, e.g. secrets provider class.

Signed-off-by: Andrii Korotkov <[email protected]>
dauinh pushed a commit to dauinh/airflow that referenced this pull request Jan 24, 2025
closes: apache#45171

Allow to disable adding default secret mounts for pgbouncer configs as well as metrics exported database url env variable. This can be useful for cases, where the value is retrieved other way, e.g. secrets provider class.

Signed-off-by: Andrii Korotkov <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support not generating secrets at all, giving an option to rely on other means to get secrets
4 participants