Expose TLS and authentication options in mongodb_uri field #287
Conversation
| """Append TLS options""" | ||
| if self.ssl_params: | ||
| ssl_params = self.ssl_params | ||
| ssl_params.update(DEFAULT_SSL_OPTIONS) |
There was a problem hiding this comment.
This modifies the self.ssl_params property. Why do we need to use DEFAULT_SSL_OPTIONS here? Isn't that already the default for self.ssl_params?
There was a problem hiding this comment.
Ah, I wasn't aware that this modifies the reference. sslParams for example doesn't contain ssl=true, it only contains the parameters necessary for the server (see sample orchestration files). DEFAULT_SSL_OPTIONS is applied to self.kwargs but not self.ssl_params. What would be the right way to apply these default options?
There was a problem hiding this comment.
Oh thanks for explaining that. In this case I don't think we should add any of the self.ssl_params to the client URI since those are ssl params for the server itself. I think we should do this:
if self.ssl_params: # Server ssl params.
ssl_params = DEFAULT_SSL_OPTIONS.copy() # Client ssl params
....The above approach would need to add the tlsInsecure=true option to match the ssl_cert_reqs=ssl.CERT_NONE in DEFAULT_SSL_OPTIONS.
Alternatively we can keep your current approach but make a copy of self.ssl_params to avoid modifying it:
if self.ssl_params:
ssl_params = self.ssl_params.copy()
ssl_params.update(DEFAULT_SSL_OPTIONS)There was a problem hiding this comment.
Update to copy the params. DEFAULT_SSL_OPTIONS by itself is not enough, as there are additional settings that come into play. The new SSL_TO_TLS_OPTION_MAPPINGS variable indicates which of the fields from the configuration's sslParams and DEFAULT_SSL_OPTIONS map to URI options for SSL. I can add additional tests to ensure we're not adding unknown or wrong URI options if you'd like.
There was a problem hiding this comment.
self.ssl_params.copy() SGTM
There was a problem hiding this comment.
I'm not sure if this change is kosher, as somebody may be relying on mongodb_uri not containing auth information to test authentication failures. Please let me know if I should separate those changes.
I think this is okay but could you document this in the changelog at the bottom of the readme? You can add a new section:
Changes in Version 0.8.0 (TBD)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
I've kept it concise, noting that we now expose TLS and auth options. I've also reached out in #driver-devs to double-check before breaking somebody's workflow. |
|
On second thought, changing mongodb_uri will break the setup for any driver that tests against ssl clusters as they'll be adding uri options manually. I'll change the PR to expose this as a new field and deprecate existing fields. Is there a way to deprecate these? |
|
There's no way to deprecate an api response field. Let's just keep the existing ones. Perhaps we can note that they are deprecated in the wiki: https://github.com/10gen/mongo-orchestration/search?q=mongodb_uri&type=wikis As for the name of the new field, anything you come up with is fine. Maybe |
Fixes #268.
This PR refactors the generation of the URI returned after starting a cluster. Previously,
mongodb_uriandmongodb_auth_urihad to be checked depending on whether auth was enabled. As indicated in the ticket above, TLS options also weren't added automatically. The goal is to get mongo-orchestration to return amongodb_urithat can be used to connect to the cluster, without having to append other options.This PR changes
mongodb_urito include both TLS and auth options. Themongodb_auth_urifield is kept for backward compatibility, but it always contains the same value asmongodb_uri. I'm not sure if this change is kosher, as somebody may be relying onmongodb_urinot containing auth information to test authentication failures. Please let me know if I should separate those changes.