-
Notifications
You must be signed in to change notification settings - Fork 32
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
Default auth_source
to None
instead of self.database
#981
base: main
Are you sure you want to change the base?
Conversation
…ult to None instead
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #981 +/- ##
==========================================
- Coverage 88.76% 88.76% -0.01%
==========================================
Files 46 46
Lines 3979 3977 -2
==========================================
- Hits 3532 3530 -2
Misses 447 447 ☔ View full report in Codecov by Sentry. |
not sure who to ping here, best guess is @rkingsbury, @munrojm. also @davidwaroquiers since it affects |
@gpetretto I don't think this is a problem for jf remote. What do you think? |
Hi @janosh, does this break only in the case of jobflow-remote? Is there anything we could do there to avoid a breaking change in maggma? Or is this a generic problem when accessing a DB hosted on AWS with the MongoStore? |
i only mentioned that our |
to elaborate, i think this is one of those cases where code that's trying to be helpful ends up causing more user surprise than if the code did nothing. if you have a non-default location for your credentials, then you're more likely to know you need to specify that in order for |
Thanks for reporting @janosh . I tend to agree with you that this is probably a case where "trying to be helpful" is worse than just being more explicit about default behavior. I do have a few questions though:
|
yes and yes. the problem is not lack of workaround, it's the time needed to pinpoint the issue given it was 4 levels down in the import stack from the
i thought the
potentially yes. my suspicion is that few people rely on this default but i don't have data to back that up (other than like you said no tests breaking and people who know their |
I am also in favor of @janosh proposal. Using the database itself as the place to store the credentials is to me not the best approach. It means that when you have two databases, you basically have to define two users (of the same name) in the two different databases, and they can have different passwords ... Not very handy ... I also had a partner who did not found how to set the auth_source (he was using admin), so by trial and error, he found that it works when uses the database itself and then went on with this... I then suggested him to use admin (because of the above) and told me that it did not work and he didn't know why. So I told him that he could pass auth_source="admin" in maggma stores. I think it should be the default (either None, so that Mongo's default is used, i.e. admin, or directly admin as a default), even if it may be backward incompatible for some people (but they just have to change their configuration files). |
Got it! So in your case, you were using a separate
Yes I should have been more specific - thank you for quoting and linking the docs. I read those and the default behavior of
My suspicion is that few people specify Unless we are 100% sure switching auth to |
It took me some time to completely understand how MongoDB works for this. Short answer, switching authentication to admin will work but of course you have to re-add the user/credentials inside admin (the other option is for the user to set auth_source to the database name, the warning you mention should indeed be done and some changelog message to warn the users for this change in the new maggma version). Basically, if it is the first database of a user, e.g. user_A, you add this user inside the admin database (with its password or other authentication method), providing him access to database_1. Then if you want to give him access to database_2, you add within the admin database the access of user_A to the database_2, but you don't need to set the authentication method again. In the case you define the authentication of a user in the database itself, you have to "duplicate" this user in each of the database he needs access to. And the point is these users are NOT the same users in a sense, they just share the same name but exist in different databases. MongoDB is actually quite flexible on this (too flexible imho) in that you could even define the authentication of user_A in database_1 for access to database_2 and vice-versa ... well I don't know who would be that crazy :D Note that the recommended (and I would say most logical) way (and also the default in MongoDB and pymongo) is to use the admin database for authentication.
I'm always setting auth_source in maggma stores (anywhere I'm using it, be it for us or else) because I want to use admin as you probably guessed from my comments :) |
Thanks all. I agree this should be changed, the question is how to do it in the least intrusive way possible.
Based on how that goes, let's please add a warning when |
I also no longer have access to NERSC. maybe @esoteric-ephemera could give this a try and report what happens when you apply the 2 line change in this PR to
i think so too. that said, i think |
Agreed! I just want to make sure I fully understand the likely impact. Regardless I think the path forward will be to add a warning message. |
@janosh @materialsproject/materials-project-extended-team just resurfacing this discussion. Could someone with NERSC Mongo access let us know what the behavior is if you change |
sorry, this feel off my radar. currently on vacation/traveling but it's not urgent anyway. happy for this to just be parked for now |
@rkingsbury @janosh Hey sorry I was supposed to check this and it fell off my radar too. Using
Also tbc, if
|
thanks for checking @esoteric-ephemera! not unexpected. i leave the decision to y'all. breaking changes are always a tough sell but overriding a mongo default wasn't a great design choice in a general purpose tool imo |
We'd have to update the jobflow (and probably fireworks) to reflect the change in |
yes, see here |
Is maybe a safer default behavior for maggma to try |
Yes I was going to suggest something along these lines. @janosh I agree in principle that it would be best for I was going to suggest defaulting to
If we do one of the above, plus proactively message the MP crowd via Slack about the upcoming change, I think I'm comfortable with it. |
good ideas @rkingsbury @esoteric-ephemera to minimize breakage! 👍 will take a swing at implementing once back in the office. |
Hi @janosh , I know this isn't a high priority but just resurfacing this for when you're ready to implement. I'll be happy to merge once you do so. |
using the database as the default
authSource
breaksjobflow-remote
(running on AWS) from talking to a remote database. this PR changes the default toNone
, allowingMongoClient
to use it's own defaulti think this change is strictly speaking breaking but i suspect few users are relying on the current behavior since it's common practice to specify the
authSource
manually if it's notadmin
, which is theMongoClient
default (docs)the error message is not helpful which is why it took me a while to troubleshoot this. commenting out the lines in my local code which are removed by this PR resolved this error