Use the mongosh command instead of the old mongo command #703

stevenpost · 2024-03-18T15:57:04Z

This PR builds on the work @witjoh in #677, the focus here is on getting the minimal set of needed changes and cleaning up his branch. I'll try to move any real cleanup code and other features he was working on into separate PRs for an easy review.

Fixes #648

stevenpost · 2024-03-21T17:47:37Z

Good news, everyone! I'm down to a single failing acceptance test. Once I get that one figured out, I just need to clean up my branch to get it ready for review and properly describe my changes (into the commit message). And do some additional testing to make sure things behave as expected.

The failing unit tests are because of additional debug info, which is going to be removed during branch cleanup.

h-haaks · 2024-03-21T19:36:22Z

Nice work @stevenpost
Keep going until we have this all green, and then we could see if there is even more we could rip out into separte PRs.
Remember PR titles end up in the changelog :)

h-haaks · 2024-03-21T19:41:09Z

manifests/client/params.pp

-  }
+  # the new mongosh package is the same for all distros.
+  # and it follows its own versioning
+  $package_name = 'mongodb-mongosh'


If users have local scripts or puppet code using shell this change will break that...
I think we should make the client class install both both in the beginning.

I tend to agree for versions < 6.0

I discussed this with @witjoh (he's currently sitting right across me) and he has a point. Let's not bother with this, we already indicate we have breaking changes.

I honestly don't understand why you want to push such a break.
The mongo command was deprecated in 5.0 and is a valid command for another 7 months.
The only thing we have to do is add 10 lines of code to install the package.
We don't have to support the command in providers as that is internal module code.

Users can still add a package resource themselves. If we wouldn't have any other breaking changes, I'd agree, but since we already do, what's the point of adding deprecated commands to new installations?

See your point. I'm not going to hold back on approval because of this even if I don't agree.

FYI for anyone who does need to support both mongo < 4.4 and > 5.0 ( sadly that's me ) then you can always add a simple mongo version check to the respective mongodb.rb files and have an IF ELSE statement with both the old code and the new :-) ( yes....it's not ideal....but hey it works )

h-haaks · 2024-03-22T00:26:22Z

Could we have a little talk about c16c54f ?
Is this the same issue as #587 (comment)?

I wonder if this could be caused by config being read in a class method and not an instance method or something like that.
I haven't looked deeply into the provider code yet, but my guess is something like this

Provider reads config in prefetch
Config is changed and server restartet.
Provider uses outdated config when trying to update resource

stevenpost · 2024-03-22T08:48:07Z

Could we have a little talk about c16c54f ? Is this the same issue as #587 (comment)?

I wonder if this could be caused by config being read in a class method and not an instance method or something like that. I haven't looked deeply into the provider code yet, but my guess is something like this

Provider reads config in prefetch Config is changed and server restartet. Provider uses outdated config when trying to update resource

Change in the commit: no, it really needs to be silently ignored and has nothing to do with the provider itself. I'll expand on this in the proper commit.

The issue you referenced: at first glance it is because the provider doesn't properly handle reading the yaml file. Although the config is valid yaml, the provider expects this rather weird structure with the dots, at least from a first glance. I'll see if I can tackle this in a follow-up PR.

The provider previously used the external IP to connect instead of the loopback. When using the loopback, the option `enableLocalhostAuthBypass` (true by default) allows to bypass authentication when a user doesn't yet exists. Which is exactly what would prevent us from executing administrative tasks (like setting up the replicaset) when auth is enabled, but not yet set up. We can only connect to localhost when MongoDB is listening on either the 'bind all' address (0.0.0.0 or ::0) or the local loopback address (127.0.0.1 or ::1).

The `db.serverCmdLineOpts()` method always requires authentication when auth is enabled. It doesn't take the `enableLocalAuthBypass` parameter into account, the subsequent authentication attempt always fails if a user doesn't exists. This commit switches to the `rs.status()` method, as that method does take the `enableLocalAuthBypass` parameter into account and allows us to obtain a shell before with authentication enabled, but before the user exists.

> Deprecated mongosh Method > This method is deprecated in mongosh. For alternative methods, see > Compatibility Changes with Legacy mongo Shell. > https://www.mongodb.com/docs/mongodb-shell/reference/compatibility/#std-label-compatibility See https://www.mongodb.com/docs/manual/reference/method/db.collection.insert/

Databases in MongoDB are created lazy, meaning there is no `create` command. Thus if a record is inserted the DB is created. When auth is enabled, any attempt to insert data is denied because of missing auth. However the `db.createUser()` function works because of the config option `enableBypassLocalAuth`. For an overview see https://www.mongodb.com/docs/manual/tutorial/deploy-replica-set-with-keyfile-access-control/

stevenpost · 2024-03-27T10:17:27Z

Hi all,

A summary of what's in this PR, most in the provider, one in the associated type:

Don't attempt a login for commands known to work without
Replace the use of the mongo command with mongosh
Fix replicaset with auth, this is spread over 3 commits, see the commit message for details.

stevenpost · 2024-03-27T10:34:30Z

A note about backwards-compatibility.

Existing client code may need a few changes:

Since the versioning of the new package is different, users who had this pinned on '4.4.x' using class { 'mongodb::client': version => '<version>' } should change the version to either 'present' or an existing version of the mongodb-shell package. Pinning the version can also be done using the client_version parameter on the mongodb::globals class.
Any code that specified the user requiring the DB should break that connection. This isn't required anymore, and will cause a dependency cycle. (The user actually needs to be created first, details in commit 18ed042)
The file /root/.mongorc.js has been replaced with '/root/.mongoshrc.js, the old file isn't cleaned up as some users may depend on it for old scripts after migration. This triggers a restart of the server when first applied.

Apart from the tests already in this module, this code was tested in 2 scenario's:

Migrating an existing cluster with auth enabled to this new code
Setting up a new cluster with auth enabled

The nodes used for these tests were running RHEL 8 and I tested against MongoDB 4.4.27.

Please give it a whirl :)

stevenpost · 2024-03-27T15:19:32Z

Testing update: seems to work fine using RHEL 8 and MongoDB 6.0.12 as well.

A curly brace was not placed properly.

stevenpost · 2024-03-28T10:46:03Z

Update: I found an issue with the authentication script, but fixing that is causing failures again during roll-out. Investigating.

All errors not related to authentication mean that auth is not required.

stevenpost · 2024-03-28T12:57:57Z

Update: I found an issue with the authentication script, but fixing that is causing failures again during roll-out. Investigating.

Fixed, it was another exception thrown by the rs.status() command when testing for authentication.

whiphubley · 2024-04-16T11:29:29Z

@stevenpost as an end user I can confirm the module can now create the initial admin user and db when auth is enabled which ( for us ) was the major issue with this module...so thanks very much for your work on this.

stevenpost force-pushed the mongosh branch 11 times, most recently from d69facb to 4538088 Compare March 19, 2024 17:24

witjoh mentioned this pull request Mar 19, 2024

Draft: Move to mongosh and support mongodb versions 4.4+ only #677

Closed

stevenpost force-pushed the mongosh branch 9 times, most recently from 2bf404c to 6ee156d Compare March 21, 2024 17:37

h-haaks reviewed Mar 21, 2024

View reviewed changes

stevenpost force-pushed the mongosh branch 4 times, most recently from 2843038 to e230c29 Compare March 22, 2024 13:05

stevenpost force-pushed the mongosh branch from d1fcbc1 to 2277a22 Compare March 26, 2024 14:50

stevenpost added 4 commits March 27, 2024 10:57

stevenpost force-pushed the mongosh branch from e68fb43 to 18ed042 Compare March 27, 2024 10:04

stevenpost marked this pull request as ready for review March 27, 2024 10:11

stevenpost requested a review from h-haaks March 27, 2024 10:17

witjoh approved these changes Mar 27, 2024

View reviewed changes

stevenpost mentioned this pull request Mar 27, 2024

support for pure yaml in mongodb.cfg #587

Open

Fix authRequired function

77fc5fc

A curly brace was not placed properly.

stevenpost requested a review from witjoh March 28, 2024 09:08

This was referenced Mar 28, 2024

Migrated to mongosh and support mongodb versions 4.4+ only #721

Closed

catch errors based on missing authentication to allow creation of admin user on replicaset setup #479

Closed

stevenpost added 2 commits March 28, 2024 12:45

Fix weird message formatting

b140a18

Avoid failing on auth for unconfigured replsets

263fc4f

All errors not related to authentication mean that auth is not required.

witjoh approved these changes Mar 28, 2024

View reviewed changes

witjoh merged commit b9491d2 into voxpupuli:master Mar 29, 2024
24 checks passed

stevenpost deleted the mongosh branch March 29, 2024 09:00

stevenpost mentioned this pull request Mar 29, 2024

Full support for mongo 5.x on RedHat/CentOS #629

Closed

This was referenced Mar 29, 2024

rs.initiate() fails due to duplicated replicaset alive members #565

Closed

Improve mongo output handling #610

Closed

h-haaks added the enhancement New feature or request label Apr 28, 2024

h-haaks mentioned this pull request Apr 29, 2024

Release 6.0.0 #759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the mongosh command instead of the old mongo command #703

Use the mongosh command instead of the old mongo command #703

stevenpost commented Mar 18, 2024

stevenpost commented Mar 21, 2024

h-haaks commented Mar 21, 2024

h-haaks Mar 21, 2024

stevenpost Mar 22, 2024

stevenpost Mar 22, 2024

h-haaks Mar 22, 2024

stevenpost Mar 24, 2024

h-haaks Mar 24, 2024

whiphubley Mar 28, 2024

h-haaks commented Mar 22, 2024

stevenpost commented Mar 22, 2024

stevenpost commented Mar 27, 2024

stevenpost commented Mar 27, 2024 •

edited

Loading

stevenpost commented Mar 27, 2024 •

edited

Loading

stevenpost commented Mar 28, 2024

stevenpost commented Mar 28, 2024

whiphubley commented Apr 16, 2024

Use the mongosh command instead of the old mongo command #703

Use the mongosh command instead of the old mongo command #703

Conversation

stevenpost commented Mar 18, 2024

stevenpost commented Mar 21, 2024

h-haaks commented Mar 21, 2024

h-haaks Mar 21, 2024

Choose a reason for hiding this comment

stevenpost Mar 22, 2024

Choose a reason for hiding this comment

stevenpost Mar 22, 2024

Choose a reason for hiding this comment

h-haaks Mar 22, 2024

Choose a reason for hiding this comment

stevenpost Mar 24, 2024

Choose a reason for hiding this comment

h-haaks Mar 24, 2024

Choose a reason for hiding this comment

whiphubley Mar 28, 2024

Choose a reason for hiding this comment

h-haaks commented Mar 22, 2024

stevenpost commented Mar 22, 2024

stevenpost commented Mar 27, 2024

stevenpost commented Mar 27, 2024 • edited Loading

stevenpost commented Mar 27, 2024 • edited Loading

stevenpost commented Mar 28, 2024

stevenpost commented Mar 28, 2024

whiphubley commented Apr 16, 2024

stevenpost commented Mar 27, 2024 •

edited

Loading

stevenpost commented Mar 27, 2024 •

edited

Loading