-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Set simple auth manager as default #47691
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
|
We probably need to do breeze at the same time. I'd be in favor of turning on all_admins by default there. But otherwise, we'd at least need to provision the admin user with a known password vs generating it. |
Turning on the option |
|
yep. and maybe a way to have simple auth manager somewhat configurable (hard-coded configuraiton that can be used with different users ?) |
I could create by default 2 users:
Or even more? I can also generate friendly passwords to avoid having to look at generated passwords (only for breeze of course) |
8612094 to
42cb7bc
Compare
|
I added the |
Maybe better to create it elsewhere (/somewhere in root) dynamically in |
|
Or we can keep it in a repo and simply pass an env variable to point to it? |
|
The latter likely better |
|
I like that too :) I'll go with that |
|
Oh but we already have a config |
42cb7bc to
8f19d60
Compare
27e0567 to
f1cbe32
Compare
airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py
Dismissed
Show dismissed
Hide dismissed
f3d430e to
57c87f3
Compare
|
All tests are passing 🟢 |
pierrejeambrun
left a comment
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 overall. Just one question will that also impact airflow standalone ? It looks like it by the config update but I'm not sure about that. (At first I thought this was only for development purpose in breeze)
It will. We set simple auth manager as default auth manager in airflow config so I agree this is a bit weird cause the simple auth manager is only for development purposes so that means forcing users to switch to another auth manager for production purposes. But we also agreed on that plan to make the simple auth manager by default in Airflow so that we no longer have FAB dependency in Airflow by default. Also, since we might want to deprecate/remove/discourage users of using FAB auth manager in Airflow 3.X as some point, we cannot make it the default one. We would need to wait for Airflow 4.0 to change that, we do not want that. So having for now the simple auth manager as default and telling users to use another one for production purposes is probably the most future safe. |
57c87f3 to
75e2bdf
Compare
potiuk
left a comment
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.
One NIT about redirecting all to /dev/null on user creation. ... Even if we need it in case create user does not work and we want to silence it in case of SimpleAuth Manager we probably should do it differently, maybe simply handle user creation silently and print out "No user creation in Simple Auth Manager" and do it in sdout and treat it as success?
I agree with the concern, I did not like it either when I was working on it but I could not find a better solution. The problem is we are using
I dont think we should do that because it means that every auth manager then should also have this CLI implemented because this command is used in the CI. Unless we agree that only FAB and simple auth managers are used in the CI and then they should have this command implemented? I am okay with that |
Let's check if Simple Auth Manager is configured when the command start, print a message that we are skipping it and exit(0). |
|
We can do it in CI in the scripts that run that command - not in the command itself : if [[ ! <SIMPLE AUTH MANAGER CONFIGURED>> ]]; then
echo "Skipping user creation as Simple auth manager is used"
else
create user ....
fiYes. I know it's bash, but it's not too complex :) and it's done in a number of cases alreaady that we do stuff conditionally in the scripts. (you need to add the right condition of course) |
I like that. Let me try |
75e2bdf to
09af0b0
Compare
|
I did that. Let's see if tests are passing |
Perfect! |
bugraoz93
left a comment
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.
Looks good! Great reload and clear logic
Should I count it as an approval @potiuk ? :) |
aritra24
left a comment
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.
Some nits
09af0b0 to
bf5169e
Compare
Set the default auth manager as simple auth manager. This will help us to remove Flask dependencies from
airflow-core.Breeze changes
This PR also adds a parameter
--auth-managertobreeze start-airflowto make it easier to switch between auth managers.Example:
By default it uses
SimpleAuthManager. I also predefine two usersadminandviewerforSimpleAuthManagerready to be used out of the box.^ 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 newsfragments.