-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add support in MongoHook for mongodb+srv:// URIs
#41371
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
Add support in MongoHook for mongodb+srv:// URIs
#41371
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
Please let's not do that. Adding a new connection type will cause more confusion (consider that we have dozens of providers, we must make sure we don't overwhelm users with options). Specifically for the mongo case, can you please explain further why Also, I see that airflow/airflow/providers/snowflake/hooks/snowflake.py Lines 113 to 115 in 6b810b8
|
|
@eladkal ah I see what you are concerned about...in my org we use environment variables to pass connection params, so we don't use the connections UI at all. Yes I totally agree that we don't want to add connection types that increase confusion. My aim here is to prevent future devs (including my own) from hitting the same issue we did, naively passing a "mongodb+srv://" URI as an env var and getting a cryptic pymongo error. It would be better if we got a legible error from the hook or operator code. Would it be an acceptable approach to tighten up the connection validation inside MongoHook? I.e., if the And if so, would that validation be considered a breaking change in the provider? Previously-working connections might fail to validate and thus stop working.
That is a good idea, I'm not very front-end-y but I might try to do that in a separate MR. |
|
Closing in favor of #41717. |
This change adds support for
mongodb+srv://URIs when the MongoHook interprets the Connection parameters, while maintaining the existing option of ansrvquery parameter.Current Behavior
If you are:
mongodb+srv://you need to manually change your connection URI by adding a query parameter
srv=true(the URI scheme is in fact ignored completely).If you do not add the query param, the hook will munge the scheme to
mongodb://, attempt to use the SRV locator as a direct service location, and fail to connect properly.New Behavior
After this change:
mongodb+srv://, or withconn_type=mongodb+srv, will cause the MongoHook to interpret the connection as an SRV locatorsrv=trueforces SRV behavior, as beforeTests have been added to verify the new behavior with both
uriandconn_typearguments.Justification
My organization uses the Mongo-supported form of
mongodb+srv://which we get from the service vendor who hosts our MongoDB instances. (The introduction of SRV to Mongo was blogged here).The lack of support for the standard URI scheme means that every time we add or change a MongoDB URL in our Airflow connections, the Ops team can't simply copy the URL from the service vendor to a config file or secret; they have to manually add the query param, or rope in the relevant dev team to do so (or worse, we all have to respond to a production failure and then decide under pressure who's going to modify the URL).
Merging this change will make coordinating Mongo clients in our Airflow stack simpler and more robust.