feat: support mongodb+srv urls

[keithgg]

Description

At the moment the plugin does not support some options to deploy when the MongoDB url is of the newer mongodb+srv format.

This PR makes the necessary changes to make this work.

Three new config options are added:

• MONGODB_USE_SSL: String value "false" or "true" to toggle SSL connections.
• MONGODB_AUTH_SOURCE: The MongoDB db to use for authentication details.
• MONGODB_AUTH_MECH: The authentication mechanism used to communicate with MongoDB. Defaults to "". Since the mongodb+srv:// urls can contain the username/password for authentication, this flag didn't get set in the docker-entrypoint.sh script. We should allow the user to specify this if needed.

1. Allow the user to set both the adminSource and whether to enable SSL
2. If the MONGO_HOST is of the mongodb+srv format, don't check that the server is up, because it's quite tricky.

Testing instructions

Check that existing configurations will work unchanged

• pip install git+https://github.com/open-craft/tutor-forum@keith/checklist-fixes # install the plugin
• tutor plugins enable forum
• tutor config save
• tutor build images forum
• tutor local quickstart

Should work without any issues.

Check that mongodb+srv urls works

Add to your config.yml

MONGODB_HOST: mongodb+srv://test:test@tutor.local

Then run the above steps again. When deploying the forum you will see the output to confirmed it's parsed correctly.

MongoDB is using SRV records, so we cannot wait for it to be ready

The forum deployment will fail obviously, because the details above are invalid. I can provide access to a MongoDB server if needed.

[gabor-boros]

@keithgg looks good from my side!

[regisb]

Please explain to me: why do we need to introduce new variables for the forum, but not for other apps, such as the LMS/CMS?

[keithgg]

Please explain to me: why do we need to introduce new variables for the forum, but not for other apps, such as the LMS/CMS?

The MongoDB URL that I have actually is in this form:

mongodb+srv://test:test@tutor.local?authSource=admin&ssl=true

I found that the query parameters weren't being parsed by the forum code as expected, necessitating the need for those environment vars. Most likely because the Mongo driver version on the forum is a bit old (the SSL key was especially problematic).

The python version of the driver included in the LMS/CMS functions in the expected manner and is able to parse the query parameters.

[regisb]

The MongoDB URL that I have actually is in this form:

So in theory we should be able to infer FORUM_MONGODB_USE_SSL and FORUM_MONGODB_AUTH_SOURCE from MONGODB_HOST, right?

If yes, I propose that we do just that, to avoid the introduction of extra settings that duplicate values from elsewhere. From the user's perspective, it does not make much sense to have to update both MONGODB_HOST and the FORUM_MONGODB_* settings.

We should thus write in templates something like:

{% set mongodb_host_qs | parse_url_querystring %}{{ MONGODB_HOST }}{% endset %}
{% if mongodb_host_qs["ssl"] == "true" %}
...
{% endif %}

For that the parse_url_querystring Jinja filter will need to be created and added to the list of available Jinja filters. This can be achieved with the ENV_TEMPLATE_FILTERS filter.

If this sounds confusing we can certainly have a synchronous conversation to clarify everything.

[keithgg]

@regisb cool. I don't really have an issue with that approach.

The only problem I can foresee would be for providers that don't provide their MongoDB connection strings in that format (as an example check out the standard_srv here). Those users will need to add those options to the URL.

[regisb]

The only problem I can foresee would be for providers that don't provide their MongoDB connection strings in that format (as an example check out the standard_srv here). Those users will need to add those options to the URL.

If the ssl parameter does not need to be added to the mongodb+srv:// url, then how does the LMS/CMS mongodb client know that it should be using SSL?

[keithgg]

If the ssl parameter does not need to be added to the mongodb+srv:// url, then how does the LMS/CMS mongodb client know that it should be using SSL?

We patch it in our plugin.

[regisb]

OK, so it would make sense to add a global MONGODB_USE_SSL: false setting to the default Tutor configuration. This will require a PR to the Tutor repo.

I was sure that we had already discussed elsewhere adding the MONGODB_USE_SSL parameter to Tutor, but couldn't remember where. This happened here, in a PR by @zakum1: overhangio/tutor#437
The PR was accidentally deleted when we released Lilac, never re-opened and I totally forgot about it.

So what I did is that I opened a PR where I cherry-picked the commit; at the time the Forum app was part of the Tutor core, so I had to remove quite a few bits. Here's the PR: overhangio/tutor#741 @keithgg can you please review?

Once the upstream PR is merged you will be able to use the MONGODB_USE_SSL setting here, and not introduce a new FORUM_MONGODB_USE_SSL setting. Do you agree with this approach @keithgg?

[keithgg]

@regisb Thank you! I'm happy with that approach and I don't mind approving the PR as it improves on the status quo. However, it only partially solves the problem here. I've needed to change at least these values from the defaults in the past (not for tutor necessarily, but to get the forum to connect to a MongoDB cluster properly):

• SSL (which you just added)
• Auth Source
• Replica Set

For my current use case, I'll able to configure Atlas style clusters, but not Digital Ocean-style clusters.

[regisb]

For my current use case, I'll able to configure Atlas style clusters, but not Digital Ocean-style clusters.

Right, this is an issue that was already discussed in that older PR:

overhangio/tutor#415 (comment)
overhangio/tutor#437 (comment)

Let me try to summarize:

1. we may not need the MONGODB_AUTH_SOURCE and MONGODB_REPLICA_SET Tutor settings if we could use querystring parameters in the forum connection string.
2. the problem is that the mongodb client of the forum does not parse the url querystring. (source)
3. so I suggested (here) that in the forum we parse the querystring from inside the template.
4. but you are saying (here) that some (terraform?) providers do not include these parameters in the mongodb url.

Am I right so far? Could you please explain why item 4 is a problem? How do these providers declare the replicaset/authSource parameters?

If it's the only reasonable option we'll add the MONGODB_AUTH_SOURCE and MONGODB_REPLICA_SET upstream in the Tutor config, but I'd rather avoid that, if possible.

[keithgg]

If it's the only reasonable option we'll add the MONGODB_AUTH_SOURCE and MONGODB_REPLICA_SET upstream in the Tutor config, but I'd rather avoid that, if possible.

I would prefer this as there currently isn't a way to configure those settings "out of the box" with tutor. Anyone running MongoDB outside of tutor likely needs to create a plugin to manage it. IMHO. that's not a good situation for something so fundamental.

Am I right so far?

Yes.

Could you please explain why item 4 is a problem? How do these providers declare the replicaset/authSource parameters?

It's a problem to me, because the UX is funky. Users will need be cognizant that some parameters need to be in the URL, but not all (like the SSL you just added).

Someone new to this will need to read ALL the documentation to configure their cluster. This imaginary person, will need to know that for edx-platform the SSL parameter is supported, but anything more than that will like require his own plugin. Moreover, he'll need to figure out the magic incantation to get the forum plugin to accept their query string.

Anyway, I'd rather improve the status quo than do nothing at all. Do you prefer:

1. Adding the MONGODB_AUTH_SOURCE and MONGODB_REPLICA_SET upstream in the Tutor config? Or...
2. Updating this PR so that those parameters can be pulled from the querystring instead?

[regisb]

Anyway, I'd rather improve the status quo than do nothing at all. Do you prefer:

1. Adding the MONGODB_AUTH_SOURCE and MONGODB_REPLICA_SET upstream in the Tutor config? Or...
2. Updating this PR so that those parameters can be pulled from the querystring instead?

Thanks for all the explanations Keith, I appreciate it. Your arguments are convincing and I think that option 1 is best. I'll add the two settings to my upstream PR. Can you please update your PR to make use of the new settings?

[keithgg]

Thank you @regisb. If I don't get to it today, I'll make the changes Monday first thing.

[keithgg]

@regisb I've made the requested changes. I tested with the built-in MongoDB container and an Atlas style cluster. Please review when you get a moment.

[regisb]

The docker-entrypoint.sh script is failing according to my local installation test.

[ghassanmas]

So to confirm after this cahnge I shuold be able to do this tutor and tutor-mfe? or both

I don't quite understand? Do you mean tutor and tutor-forum? If so, you need to set the config parameters as outlined on the tutor PR and enable the tutor-forum plugin.

You'll need to set the following settings in your config.yml (please check Regis' PR for the correct naming as I didn't check my spelling):

MONGODB_USERNAME: <your username>
MONGODB_PASSWORD: <your password>
MONGODB_HOST: "mongodb+srv://cluster0.mwng7sa.mongodb.net"
MONGODB_AUTH_MECHANISM: "SCRAM-SHA-1"
RUN_MONGODB: false
MONGODB_USE_SSL: true

I couldn't get to work with these kind of setting, I error I got, is that host can't be defined, the host url started with "mongodb+srv", secondaly I am not entrily sure about the username/password in the URL.

My understanding is that mongoid which is used by cs_commens_serivce expect certain ENV variable, but then you are using them on docker-entrypoint.sh? _which is uppose to convert new url +sev to the legacy simliar to this question: ?

[keithgg]

@ghassanmas looks like you need the host in the form mongodb+srv://username:password@cluster0.mwng7sa.mongodb.net.

I hadn't considered that, as the URLs I get are generated via terraform. We can document this and anyone that needs the username and password to be separate in the future can do a PR. What do you think?

[ghassanmas]

Leaving the tutor-forum aside.

I am first trying to run tutor to use external Mongodb url, but even the LMS fails with the following error:

lms_1                        |     raise ConfigurationError(
lms_1                        | pymongo.errors.ConfigurationError: The "dnspython" module must be installed to use mongodb+srv:// URIs. To fix this error install pymongo with the srv extra:
lms_1                        |  /openedx/venv/bin/uwsgi -m pip install "pymongo[srv]"

My config.yml has the following

MONGODB_AUTH_MECHANISM: SCRAM-SHA-1
MONGODB_DATABASE: olive
MONGODB_HOST: mongodb+srv://admin:password@cluster0.mwng7sa.mongodb.net
MONGODB_PASSWORD: password
MONGODB_USERNAME: admin
MONGODB_USE_SSL: true

Also tried without using

MONGODB_AUTH_MECHANISM: SCRAM-SHA-1
MONGODB_DATABASE: olive
MONGODB_HOST: mongodb+srv://admin:password@cluster0.mwng7sa.mongodb.net
MONGODB_USE_SSL: true

Both settings didn't work for the lms, as show in the error above.

I think before testing for the forum, I would want just to try to do it only for the lms/cms.

It seems to be that we have tutor setting for monogo, that is expected to be consumed by different client, each support it's own way to parse the url depending on the version of the mongo client

So we have two mongo clients:

• mogo client that is used by edx-platform, look at this pymonog=3.13.0
• And then we have other client Mongo cs_comments_service https://github.com/openedx/cs_comments_service/blob/063fd26173cbb798b5876f52c40a77b254deb1ae/Gemfile.lock#L5-L6

For those different type of clients, which typically they consume configuration from tutor, we are trying to support different form of auth-url, what I don't completely understand yet is:

• By default tutor is expected to have configuration that works for python client for all different type(s) of auth mode?
• And when the tutor-forum tries connect to mongodb, it suppose to convert the mongo auth given that the current ruby client of mongo doesn't support all method

[keithgg]

@ghassanmas to get it working on the LMS/CMS you just need to add dnspython to the OPENEDX_EXTRA_PIP_REQUIREMENTS key in your config.yml

For example:

OPENEDX_EXTRA_PIP_REQUIREMENTS:
  - dnspython

• By default tutor is expected to have configuration that works for python client for all different type(s) of auth mode?
• And when the tutor-forum tries connect to mongodb, it suppose to convert the mongo auth given that the current ruby client of mongo doesn't support all method

Yes. We're trying to re-use the configuration that was done for the LMS so that it can apply to the forum as well. Because the Mongo version included in cs_comments_service is so old, some things work on the LMS side that don't work on the forum (like the username and password as you discovered), so we have to work around those.

[ghassanmas]

I see, anyway can this wait for Ruby 3 update openedx/cs_comments_service#392 becuse it's suppsoe to be merged soon and then we might avoide duplicate work in case that change conflict with ruby client though from quick look it doesn't seem related. But just as to do one thing a time.

Secondaly, would you consider another path of upgrading the ruby client in cs_commens_service instead of having the plugin handle that?

[keithgg]

I see, anyway can this wait for Ruby 3 update openedx/cs_comments_service#392 becuse it's suppsoe to be merged soon and then we might avoide duplicate work in case that change conflict with ruby client though from quick look it doesn't seem related. But just as to do one thing a time.

It doesn't change what this PR is trying to do. Which is add some config options. I'd rather merge this and make further changes down the line once that is merged (if required). What do you think?

Secondaly, would you consider another path of upgrading the ruby client in cs_commens_service instead of having the plugin handle that?

I'm not sure I understand. Do you mind elaborating?

[ghassanmas]

Secondaly, would you consider another path of upgrading the ruby client in cs_commens_service instead of having the plugin handle that?

I'm not sure I understand. Do you mind elaborating?

I meant to say that instead of us trying to change tutor-forum to support different use cases of mongourl, we might instead udpate monogo client in cs_comments_service to accept simliar configuration to the LMS/CMS. Thus making configuration more consistant.

On the other hand, I couldn't yet test this, the free atlas connection I got is based on Mongo V5, while the default version of Mongo in Open edX/tutor is V4.

Lastly I am still not confident about this, given that I couldn't test as mentioned above, the best I can do right now is to ensure that this change doesn't break the typical way tutor uses mongo, i.e. mongo is a local service managed by docker-compose.

[keithgg]

I meant to say that instead of us trying to change tutor-forum to support different use cases of mongourl, we might instead udpate monogo client in cs_comments_service to accept simliar configuration to the LMS/CMS. Thus making configuration more consistant.

Sure, but I'm assuming those changes won't be backported, which means users of the Olive release won't be able to deploy the forum without forking this plugin.

On the other hand, I couldn't yet test this, the free atlas connection I got is based on Mongo V5, while the default version of Mongo in Open edX/tutor is V4.

I can provide test credentials for you. Would you like me to?

[ghassanmas]

@keithgg that would be great, I will try now to ping you on Open edX slack.

[regisb]

I meant to say that instead of us trying to change tutor-forum to support different use cases of mongourl, we might instead udpate monogo client in cs_comments_service to accept simliar configuration to the LMS/CMS. Thus making configuration more consistant.

@ghassanmas this is an issue that we discussed before (though I'm not sure where). The mongoid client has been outdated for years, so it's hopeless to wait for an upgrade from the cs_comments_service maintainers.

[ghassanmas]

@ghassanmas this is an issue that we discussed before (though I'm not sure where). The mongoid client has been outdated for years, so it's hopeless to wait for an upgrade from the cs_comments_service maintainers.

Noted.

So after long testing I found that in my case where I have free atlass cluster,

I needed to define important query params, other the LMS connection won't work, those are ?retryWrites=true&w=majority . If those were missing from MONGODB_HOST then the LMS/modulestore connection will fail. However for the forum it will be okay and will fail if there are introduce, hence the way the URL is built in this PR.

So if I understand the situation correctly, this needs to strip querey params from MONGODB_HOST if exits. Which I guess can be done at the template level or in the bash script.

[keithgg]

@ghassanmas what do you need in order to merge this PR? If I understand you correctly, you would like to:

Parse the MONGODB_USERNAME and MONGODB_PASSWORD fields.

I'm happy to do this. IMHO it's not needed since those values are included in the URL for mongodb+srv urls. As a rusl we'll have to generate the MONGODB_HQ_URL ourselves which means, we'll need an extra MONGODB_SCHEME configuration option so that users can specify how to generate this URL.

Add config options for retryWrites and w=majority?

[ghassanmas]

@keithgg

1- For the username and password issue link works fine for the LMS/CMS and the forum in the case where both username and password exist in the URL/host. I didn't test for another way around (when username and password are set outside the URL).

2- Now the problem with this PR is that it expects the URL host MongoDB to not include or to have query params,
MONGOHQ_URL="$MONGODB_HOST/$MONGODB_DATABASE" ref .

So again through my testing, I had to include extra query params for it to work with the LMS, where in my case the host URL is
mongodb+srv://admin:<password>@cluster0.mwng7sa.mongodb.net/?retryWrites=true&w=majority So referring to the snippet code above:

MONGOHQ_URL=mongodb+srv://admin:<password>@cluster0.mwng7sa.mongodb.net/?retryWrites=true&w=majority/cs_comments_service
Where it should be

• MONGOHQ_URL=mongodb+srv://admin:<password>@cluster0.mwng7sa.mongodb.net/cs_comments_service/?retryWrites=true&w=majority Add the database before the query params given its
  Or
• MONGOHQ_URL=mongodb+srv://admin:<password>@cluster0.mwng7sa.mongodb.net/cs_comments_service Just ignore the query params
  both cases worked for my testing for the forum.

Regarding your last point

Add config options for retryWrites and w'(=majority`)?

If you referring to the forum, it ran okay without setting any query params, however for the LMS the URL/HOST had to have both retryWrites and w defined, and IMO, this is something related to the LMS, I speculate there can be different cases where other or different query params are needed to be defined depending on the cluster...etc.

Also a reflection point through my investigation/testing, it turns out the LMS/CMS is using a deprecated version of pymongo 3.* where the latest is 4.* and here we are dealing with an even more legacy version of a mongo client for ruby. And on top of all that, we would want the same MongoDB host URL to work for the LMS/CMS and the forum out of the box.

Lastly from my end and based on my observation I can suggest two ways to resolve this:

1. Decouple the MONGO_HOST tutor setting from the LMS, if it exists for the forum it uses that if not it cascades for the tutor default. And then for that use case, we add something in the readme as such: when using a mongodb cluster be sure to not have query params in the url, if those are essential for the LMS, then redefine the host for the forum without query params This path also has the advantage of which I can have the forum and the LMS on different cluster not sure though how useful or common is that.
2. Add an edge case in the docker-entrypoint.sh such it handles the case where URL has query params and then build MONGOHQ_URL accordingly.

I wonder what @regisb thinks about the two methods above, or if there are other alternatives. I am personally okay with any, but again I am not an expert when it comes to mongo configuration.

[keithgg]

@ghassanmas Okay cool. So how about we add a FORUM_MONGODB_HQ_URL override? If defined, we only use that in the end and don't construct a URL ourselves.

For the LMS, if the same value is supplied to MONGODB_HOST. Unless I'm mistaken (I'll verify tomorrow), it parses the query parameters already.

Also a reflection point through my investigation/testing, it turns out the LMS/CMS is using a deprecated version of pymongo 3.* where the latest is 4.* and here we are dealing with an even more legacy version of a mongo client for ruby. And on top of all that, we would want the same MongoDB host URL to work for the LMS/CMS and the forum out of the box.

Agreed that those need to be updated, but any upgrades are out of scope for this PR.

[keithgg]

@ghassanmas @regisb any feedback on the above?

[ghassanmas]

@keithgg so I guess in summary we have two ways to handle this

• Using same mongo_url for lms/cms and forum (This would require that the script you added handle the case mentioned above)
• Using different mongourl for forum: (This would require add extra config and as well change some logic if the value exits forum url exits then use)

I don't have strong feeling to either approach, so I am happy with either, I guess we would just update Readme/docs to reflect the decision we take.

Checking with @regisb do you have any preference for any?

[regisb]

@keithgg First of all I'm very sorry that it is taking so long to merge your PR. We should work to reach an agreement as soon as possible.

Frankly, it's difficult for me to understand all the finer points of the conversation here. I propose that we have a synchronous meeting to discuss the best solution.

In general, I would rather that we try to avoid the following:

1. Adding configuration settings.
2. Different configuration settings for LMS and forum.
3. Adding stuff to the forum entrypoint -- if anything, I'd rather get rid of it entirely. (see my reasons here)

Instead, I would prefer if:

1. The MONGODB_* settings were respected.
2. When not possible, do the parsing in a dedicated jinja2 filter that is specific to the forum.

These items are personal preferences, not hard requirements, so we can discuss them.

@ghassanmas can you organize a meeting next week for the three of us?

[ghassanmas]

@regisb sounds good, I have created this link so you both can the time that works best for you. I will put my time last, since I am currently traveling it's hard for me to choose right, should be able by the end of this week though.

Here is the link: https://whenisgood.net/arejtw3-mongo-forum

[keithgg]

Good call on the sync meeting @regisb thanks for organising @ghassanmas. I've added my available times. Looking forward to it!

[ghassanmas]

Great, Let's meet on Wednesday 10:00am UTC. That shall work for everyone

[regisb]

Recap of today's meeting:

1. This PR introduces support for DigitalOcean + Altas (paid) Mongodb clusters and does not break anything, so it's almost good to go (see below documentation caveat).
2. This PR introduces support for mongodb+srv urls, but querystring params (host?param=value) will not work in the forum (though they will in the lms/cms). These querystring params are required in some cases. So this PR should include a warning in the readme stating that querystring parameters should not be included. After that we can merge this PR and publish a new release.
3. Ghassan will explain to me how to launch an Atlas cluster such that I can reproduce and resolve the issue above regarding querystring parameters.
4. I will open a new PR to introduce support for querystring parameters.

Thanks to you both for your work and your patience!

[keithgg]

@ghassanmas @regisb I've made the documentation changes as discussed. Please take a look and let me know if I should make any changes.

[regisb]

LGTM! We're getting there ;) Thanks again Keith!

[ghassanmas]

Thank you for your patience @keithgg, it looks good to me

[gabor-boros]

🥳 🎉 🎈