Skip to content
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

Adds support to override mongo options via Meteor settings #10976

Merged
merged 3 commits into from Apr 17, 2020

Conversation

filipenevola
Copy link
Member

@filipenevola filipenevola commented Mar 22, 2020

In some MongoDB host providers like ScaleGrid and IBM Cloud some developers are getting this error because of certificate:

MongoNetworkError: failed to connect to server [sg-meteorappdb-32194.servers.mongodirector.com:27017] on first connect [Error: self signed certificate

We can fix the error passing additional options to the Mongo connection but right now Meteor only offers this ability via code in a package and also it's not easy to reference the certificate file inside the project.

This PR provides:
a) ability to set Mongo options using Meteor.settings.overrideMongoOptions;
b) simple way to reference files inside the private folder in the root.

Read the docs PR to see how this can be used meteor/docs#662

More info:

Copy link
Contributor

@klaussner klaussner left a comment

I think the contents of Meteor.settings (except Meteor.settings.public) are currently opaque to Meteor and its core packages, so I'm not sure if this kind of configuration should be put in there.
Maybe we could use an environment variable instead. Similarly to how the MongoDB URL is specified in MONGO_URL, we could get the path to a root certificate chain (relative or absolute) from an environment variable like MONGO_TLS_CA_FILE.

@filipenevola
Copy link
Member Author

filipenevola commented Mar 24, 2020

Hi @klaussner I don't know if you have read the suggestions from the past issues related to this but people were suggesting Meteor.settings.

I already use Meteor.settings in a few packages that I have forks like publish-composite and reloader to configure start-up things and it's working great. I understand that is maybe a new approach but I don't see why this is bad.

The problem with env vars is that we need one env var for each case and we have a lot of mongo options available then we would need to add one env var for each option.

@klaussner
Copy link
Contributor

klaussner commented Mar 25, 2020

I have no doubt that Meteor.settings would work but an environment variable has a few advantages:

  1. We have full control over how the variable is translated to actual driver options. If we encourage direct access to the options, Meteor developers have to adapt their settings when we update the MongoDB driver and some of the options have changed or were deprecated (e.g., sslCA was deprecated and replaced by tlsCAFile not too long ago). Additionally, we can make sure that the configuration we pass to the driver is always correct and consistent, for example, by setting both tlsCAFile: "..." and tls: true so that developers don't have to worry about details of the MongoDB API.

  2. An environment variable is more in line with existing configuration options in Meteor. Consider the following two settings files for Galaxy. I would prefer the first one because everything MongoDB-related is in one place.

{
  "galaxy.meteor.com": {
    "env": {
      "MONGO_URL": "...",
      "MONGO_TLS_CA_FILE": "..."
    }
  }
}
{
  "galaxy.meteor.com": {
    "env": {
      "MONGO_URL": "...",
    }
  },
  "overrideMongoOptions": {
    "tls": true,
    "tlsCAFile": "..."
  }
}

The problem with env vars is that we need one env var for each case and we have a lot of mongo options available then we would need to add one env var for each option.

I'd argue that most of the driver options don't need to be configurable and that the few exceptions (like tlsCAFile) justify additional variables. In conclusion, I think an environment variable makes more sense in terms of backwards compatibilty and developer experience.

@filipenevola
Copy link
Member Author

filipenevola commented Mar 26, 2020

I understand your arguments but I don't think I agree :). Let's proceed.

When you say that we have full control with env vars that also means that we are limiting the options. For example, I have a project where I connect to 4 MongoDB clusters and I'm going to use "tlsAllowInvalidCertificates": true so at least we would need to add two env vars already.

The problem with this approach is that we are not allowing Meteor developers to use the other options, they will always need to ask or implement a new PR. How do you think this is going to work in the long term?

About the developer using wrong options, I believe this is going to be a problem in any case, the developer can always set things incorrectly then we need to have better docs and reply the questions when they come up.

I agree my settings proposal is not clear as it could be, I believe a better version would be:

{
  "galaxy.meteor.com": {
    "env": {
      "MONGO_URL": "...",
    }
  },
  "packages": {
    "mongo": {
      "options": {
        "tls": true,
        "tlsCAFile": "..."
      }
    }
  }
}

The idea here would be to use the package name as a key then it would be easier to find settings later affecting each package.

I'm saying that because as I have said in my previous answer I use settings in a few packages already to change initial configurations. Let me show you the concrete settings so you can see what I'm using:

    "reloader": {
      "refresh": "instantly"
    },
    "publishComposite": {
      "unblock": true
    }

These are the packages reloader and publish-composite.

I don't see how to use env vars in all these cases and it would be very hard to configure all these env vars. If we have this pattern using package names as keys it would be like this:

  "packages": {
    "mongo": {
      "options": {
        "tls": true,
        "tlsCAFile": "..."
      }
    },
    "pathable:reloader": {
      "refresh": "instantly"
    },
    "pathable:publish-composite": {
      "unblock": true
    }
  }

But my main concern is to support future cases and also avoid a lot of env vars. With JSON in the settings we can just keep the same names from Mongo then we are not creating new settings, we are just providing a way to set it.

@@ -7,6 +7,8 @@
* these outside of a fiber they will explode!
*/

var path = require("path");
Copy link
Contributor

@StorytellerCZ StorytellerCZ Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var here, const down bellow. What version of ES are we using in this file?

Copy link
Member Author

@filipenevola filipenevola Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's a mix :( I'm going to update this anyway.

@@ -133,12 +139,17 @@ MongoConnection = function (url, options) {
self._observeMultiplexers = {};
self._onFailoverHook = new Hook;

const userOptions = {
...(Mongo._connectionOptions || {}),
...(Meteor.settings && Meteor.settings.overrideMongoOptions || {})
Copy link
Contributor

@StorytellerCZ StorytellerCZ Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all Meteor specific settings should be under a specific Meteor key, something like Galaxy has "galaxy.meteor.com" for its env variables. So I'm thinking Meteor.settings.meteor.mongoOptions. We can then leverage Meteor.settings.meteor object later for anything else.
UPDATE: Read your last general comment, agree with that!

Copy link
Member Author

@filipenevola filipenevola Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great.

@filipenevola filipenevola force-pushed the feature/override-mongo-options branch 3 times, most recently from 92835a9 to c388c1b Compare Apr 2, 2020
The options are in the `packages` key in the `mongo` key that is the package name. We should use the pattern every time we need a setting in a package.
@filipenevola filipenevola changed the base branch from devel to release-1.10.2 Apr 17, 2020
@filipenevola filipenevola merged commit f80ae82 into release-1.10.2 Apr 17, 2020
19 checks passed
@filipenevola filipenevola deleted the feature/override-mongo-options branch Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants