-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Keycloak Breeze integration #43240
Keycloak Breeze integration #43240
Conversation
8210b61
to
23fb30c
Compare
@potiuk Let me know what you think of this, before I take it out of Draft. |
This looks great. I only have one comment about using same "postgres" service for keycloack and metadata-db. Those two should be separate postgres instances IMHO. |
Also it does not yet work when I try to run it due to "keyclok" typo in constants :) |
Ugh, let me fix 😅 |
c6bd1fe
to
b84f881
Compare
Work is starting on multi-team Airflow, and this project has many dependencies on a auth manager that can support authn and authz and also support the changes to the auth manager api (upcoming). An option for this is Keycloak. This PR adds a Breeze integartion for Keycloak which creates a container running keycloak, which uses the existing Postgres container as the DB and disables the requirements for ssl/https/certificates to use the Keycloak admin console (since this is only for development purposes not production).
4256cdb
to
fa8fbc8
Compare
This one is ready for review as well. Fixed the typo and added a check to ensure the keycloak integration is used in conjunction with the postgres backend. |
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.
LGTM. Small NIT with suggestion.
Maybe also we should extract all other backend constants for consistency @o-nikolas ? It looks weird when only one POSTGRES is constant. |
Fair enough, I didn't extract them because they'd only be used in place there, not imported elsewhere which also feel strange. But I don't feel strongly about one approach more than the other, so I can make the change to extract constants for all 👍 |
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.
LGTM
* Keycloak Breeze integration Work is starting on multi-team Airflow, and this project has many dependencies on a auth manager that can support authn and authz and also support the changes to the auth manager api (upcoming). An option for this is Keycloak. This PR adds a Breeze integartion for Keycloak which creates a container running keycloak, which uses the existing Postgres container as the DB and disables the requirements for ssl/https/certificates to use the Keycloak admin console (since this is only for development purposes not production).
Work is starting on multi-team Airflow, and this project has many dependencies on a auth manager that can support authn and authz and also support the changes to the auth manager api (upcoming). An option for this is Keycloak.
This PR adds a Breeze integartion for Keycloak which creates a container running keycloak, which uses the existing Postgres container as the DB and disables the requirements for ssl/https/certificates to use the Keycloak admin console (since this is only for development purposes not production).
^ 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.