Skip to content

CDRIVER-5773 remove code for MONGODB-CR #1788

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

Merged
merged 9 commits into from
Nov 15, 2024
Merged

Conversation

kevinAlbs
Copy link
Collaborator

Summary

Remove code for the MONGODB-CR auth mechanism

Verified with this patch build.

Background & Motivation

MONGODB-CR no longer supported

MongoDB server 4.0 dropped support of the authentication mechanism MONGODB-CR. Quoting DRIVERS-441:

MONGODB-CR was deprecated in MongoDB 3.6.0 and was removed in MongoDB 3.7.1

As of CDRIVER-4815, the C driver supports the minimum server version 4.0. Attempting to connect to older servers results in error:

% ./cmake-build/src/libmongoc/example-client 
Cursor Failure: Server at 127.0.0.1:27017 reports wire version 6, but this version of libmongoc requires at least 7 (MongoDB 4.0)

Authenticating with MONGODB-CR is not possible on supported servers. Using MONGODB-CR on a newer server results in a protocol error:

% ./cmake-build/src/libmongoc/example-client "mongodb://foo:bar@localhost:27017/?authMechanism=MONGODB-CR"
Cursor Failure: no such command: 'getnonce'

With changes in this PR, a client-side error is now returned:

% ./cmake-build/src/libmongoc/example-client "mongodb://foo:bar@localhost:27017/?authMechanism=MONGODB-CR"
Cursor Failure: Unknown authentication mechanism "MONGODB-CR".

Skipping tests

Auth spec tests are updated to mongodb/specifications@82be6f2 to remove references to MONGODB-CR. Tests requiring unimplemented features or fixes to known bugs are skipped. test_skip_t is extended to support skipping from a description substring to ease skipping all MONGODB-OIDC tests.

To be used to skip all "MONGODB-OIDC" tests. OIDC is not-yet implemented.
The driver test server uses the auth source named "mongodb-cr", but uses SCRAM-SHA-1.
@kevinAlbs kevinAlbs marked this pull request as ready for review November 14, 2024 20:32
@kevinAlbs kevinAlbs requested review from eramongodb and a user November 14, 2024 20:32
@@ -32,6 +32,7 @@ typedef void (*test_hook) (void *test);

typedef struct {
const char *description;
bool check_substring;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this placed between existing data members instead of after?

Copy link
Collaborator Author

@kevinAlbs kevinAlbs Nov 14, 2024

Choose a reason for hiding this comment

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

To locate it next to description, since check_substring applies to description. But I can move to the end if desired. Added comment to describe the purpose of check_substring.

Copy link
Contributor

@eramongodb eramongodb Nov 14, 2024

Choose a reason for hiding this comment

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

I would normally recommend moving it to the end of the struct so existing code is not impacted as much (i.e. in test-mongoc-retryable-writes.c), but I think using designated initializers is preferable anyways in these circumstances (+ they can be ordered however is preferable, as already done in test-mongoc-connection-uri.c), so I do not mind the proposed changes as-is, other than perhaps to better conform to -Wpadded-friendly layout practices:

warning: padding struct 'test_skip_t' with 7 bytes to align 'reason' [-Wpadded]
   36 |    const char *reason;
      |                ^

vs. moving it to the end of the struct (no substantial difference in this case): ¯\_(ツ)_/¯

warning: padding size of 'test_skip_t' with 7 bytes to alignment boundary [-Wpadded]
   33 | typedef struct {
      |         ^

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will keep as-is.

kevinAlbs and others added 2 commits November 14, 2024 16:05
@kevinAlbs kevinAlbs requested a review from eramongodb November 14, 2024 21:12
@@ -32,6 +32,7 @@ typedef void (*test_hook) (void *test);

typedef struct {
const char *description;
bool check_substring;
Copy link
Contributor

@eramongodb eramongodb Nov 14, 2024

Choose a reason for hiding this comment

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

I would normally recommend moving it to the end of the struct so existing code is not impacted as much (i.e. in test-mongoc-retryable-writes.c), but I think using designated initializers is preferable anyways in these circumstances (+ they can be ordered however is preferable, as already done in test-mongoc-connection-uri.c), so I do not mind the proposed changes as-is, other than perhaps to better conform to -Wpadded-friendly layout practices:

warning: padding struct 'test_skip_t' with 7 bytes to align 'reason' [-Wpadded]
   36 |    const char *reason;
      |                ^

vs. moving it to the end of the struct (no substantial difference in this case): ¯\_(ツ)_/¯

warning: padding size of 'test_skip_t' with 7 bytes to alignment boundary [-Wpadded]
   33 | typedef struct {
      |         ^

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good to me. Re struct field order: I'm happy with optimizing for semantics rather than memory layout in test code, it's neither something that needs to be fast nor a thing we'll be stuck with if we feel like changing it later.

@kevinAlbs kevinAlbs merged commit 4ccd9db into mongodb:master Nov 15, 2024
22 of 45 checks passed
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.

2 participants