-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Temporary fix to Bitnami psql chart licensing issues #55820
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
Conversation
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
|
I've created an issue to track the long term goal too: #55823 |
|
K8s tests are green now. Merging it as temp workaround. |
|
cc @kaxil you would have to cherry pick this into 3.1 too. |
(cherry picked from commit 8be9d8b)
| postgresql: | ||
| enabled: true | ||
| image: | ||
| repository: postgres |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to contradict the above comment, this is not using the bitnamilegacy/postgresql image.
Additionally, Bitnami heavily customizes their images, and the process for configuring them is rather idiosyncratic, so it's unlikely that the non-Bitnami postgres image will work correctly with the Bitnami PostgreSQL Helm chart.
Has this been tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a temporary approach and we are removing postgres altogether. I am not sure anyone from the community will have time on testing a test only feature that is going to be removed according to discusssion and consensus https://lists.apache.org/thread/3o4wj76pmd2m13jho7sxcbfkbgpv88h9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's reasonable in the long term, but as mentioned, this temporary fix doesn't work at all since it uses a non-Bitnami postgres image that is not compatible with the Bitnami PostgreSQL Helm chart.
I would argue that this change should not have been merged, since it does not fix the issue and confuses users. A better temporary fix would have been to use the bitnamilegacy/postgresql image as it seems was originally intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to make a reverting PR @zanecodes -> main is there for all kinds of things and we sometimes rever things - this is what makes a different of "released" code vs. "main - in development code".
But - like everyone else here - you are free to open PR to rever things if you think such a main thing was not good - or even (if you have a better fix) replace it with a better fix. The best way to get something done in the open-source project - where you have > 3500 contributors is to become one and do it.
| enabled: true | ||
| image: | ||
| repository: postgres | ||
| tag: "13" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Airflow Helm chart currently uses version 13.2.24 of the Bitnami PostgreSQL Helm chart, but this does not correspond to version 13 of PostgreSQL. Version 13.2.24 of the Bitnami PostgreSQL Helm chart uses the tag 16.1.0-debian-11-r15 of the bitnami/postgresql Docker image.
This tag is present in the bitnamilegacy repository, so I think the correct fix is to update the repository value above to bitnamilegacy/postgresql, and either omit the tag or pin it to 16.1.0-debian-11-r15.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, That's what I had to do to get Airflow to deploy.
The Airflow helm chart version 1.18.0 was failing to deploy because of that.
I changed the repository as you proposed and it worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Since we are removing the feature. What we recommend people now is to extend or modify the chart in the way they find good for them. So that's the right direction.
The Airflow Helm chart deployment fails with
ImagePullBackOfferrors when trying to pulldocker.io/bitnami/postgresql:16.1.0-debian-11-r15. This is due to Bitnami's recent licensing changes that moved newer postgres images behind a paywall, requiring VMware Tanzu subscriptions for access.Solution
The solution to this issue is two fold:
Short Term
Switch to bitnamilegacy/postgresql images which are freely available and contain the same postgres functionality without licensing restrictions. (this is also suggested in the linked issue)
To do that i've added:
This uses the closest complaint image to
docker.io/bitnami/postgresql:16.1.0-debian-11-r15and overrides the image in the postgres charts that we keep in the repository as subcharts.Testing:
[x] kind cluster deployment succeeds
[x] postgres pod starts successfully -- with the right image that has been overriden
[x] Airflow components can connect to database
[x] No functionality regression
Deployment successful:
Tests passing locally:
Long Term
^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.