-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(NODE-7046)!: remove AWS uri/options support #4689
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
base: main
Are you sure you want to change the base?
Changes from all commits
9a47fca
5c6d9de
28539cb
c6545f3
f68d8ef
813cac4
1c45bb2
e6c0bfd
cd6e6f7
b5aaf29
b842edc
4b8c7d4
22caa48
349827b
21ca884
53274dc
fe3959d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
[submodule "drivers-evergreen-tools"] | ||
path = drivers-evergreen-tools | ||
url = https://github.com/mongodb-labs/drivers-evergreen-tools.git | ||
url = https://github.com/mongodb-labs/drivers-evergreen-tools.git |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -423,6 +423,20 @@ export function parseOptions( | |||||
); | ||||||
} | ||||||
|
||||||
if (isAws) { | ||||||
const { username, password } = mongoOptions.credentials; | ||||||
if (username || password) { | ||||||
throw new MongoAPIError( | ||||||
'username and password cannot be provided when using MONGODB-AWS. Credentials must be read via the AWS SDK' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm hesitant to accept this change as the addition of the word "directly" to me indicates that a username and password could be provided in another fashion. In the case of AWS authentication username and password do not really apply in any form. Same with the comment on the other error message. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Open to different phrasing, but I wanted to make sure that we differentiate between the valid and invalid manner of providing the credentials, "via the sdk" isn't really actionable for users, so perhaps something along the lines "in a manner that can be read by the AWS SDK" and/or "via environment variables or a custom credential provider". Does that make sense? |
||||||
); | ||||||
} | ||||||
if (mongoOptions.credentials.mechanismProperties.AWS_SESSION_TOKEN) { | ||||||
throw new MongoAPIError( | ||||||
'AWS_SESSION_TOKEN cannot be provided when using MONGODB-AWS. Credentials must be read via the AWS SDK' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
); | ||||||
} | ||||||
} | ||||||
|
||||||
mongoOptions.credentials.validate(); | ||||||
|
||||||
// Check if the only auth related option provided was authSource, if so we can remove credentials | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,10 +6,6 @@ const { MongoClient } = require('mongodb'); | |
// options. Note that MongoClient now auto-connects so no need to store the connect() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to update the text in the comments to talk about doing this via env vars instead |
||
// promise anywhere and reference it. | ||
const client = new MongoClient(process.env.MONGODB_URI, { | ||
auth: { | ||
username: process.env.AWS_ACCESS_KEY_ID, | ||
password: process.env.AWS_SECRET_ACCESS_KEY | ||
}, | ||
authSource: '$external', | ||
authMechanism: 'MONGODB-AWS' | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,22 @@ | ||
import { loadSpecTests } from '../../spec'; | ||
import { executeUriValidationTest } from '../../tools/uri_spec_runner'; | ||
|
||
const SKIP = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a good file to comment on, but I was looking for changes to the prose tests, and noticed that our prose tests don't follow the convention - the relevant tests for this are in mongodb_aws.test.ts, where I think we can delete the Case 1 block. Can we move the prose tests from that file to |
||
'should use username and password if specified (MONGODB-AWS)', | ||
'should use username, password and session token if specified (MONGODB-AWS)' | ||
]; | ||
|
||
describe('Auth option spec tests (legacy)', function () { | ||
const suites = loadSpecTests('auth', 'legacy'); | ||
|
||
for (const suite of suites) { | ||
describe(suite.name, function () { | ||
for (const test of suite.tests) { | ||
it(`${test.description}`, function () { | ||
if (SKIP.includes(test.description)) { | ||
this.test.skipReason = `NODE-7228: ${test.description}`; | ||
this.test.skip(); | ||
} | ||
executeUriValidationTest(test); | ||
}); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there some logging we can add that confirms that the aws_tester.py is correctly running with that option set? I think the current version of the script here should be passing something in, but currently nothing is printing in the run (e.g. here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could file a ticket to update the tools script to log this, but it would be obvious if the option was set or not given the tests. In this PR if the option wasn't set the driver would be throwing an error since MONGODB_URI would have the credentials set. (But we don't want to log what our URIs are in CI or we redact them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be useful to do this anyway for debugging purposes in the future - can you go ahead and file the corresponding ticket?