-
Notifications
You must be signed in to change notification settings - Fork 6
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
DOCSP-41148 Authentication Guides #50
Conversation
✅ Deploy Preview for docs-kotlin-sync ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
It looks like I had some review comments from last time that I never submitted. Can you start with these?
source/security/authentication.txt
Outdated
in the options of your connection string or in a ``Credential`` struct. | ||
|
||
To learn more about the connection string options for authentication, | ||
see the :manual:`Authentication Options |
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.
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.
Linked to https://www.mongodb.com/docs/manual/reference/connection-string/#connection-string-formats, which leads to helpful information on string format options as well as link to the above page^
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 I would remove mention of authentication options from this paragraph, then. The linked page mentions username/password and authSource, but not all of the available options, and I wasn't able to find an easy link to the auth page. Because it's primarily about connection string formats, I would also put it in a Tip box as more of an aside.
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'll remove the link entirely since the section isn't easily scannable. Initially wanted to include because I ran into trouble with ssl being enabled depending on whether the srv or standard string format was used. I've since updated the guide samples, so hopefully that won't be an issue for users any more.
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.
It's getting there! Tried to provide lots of suggestions to apply.
source/security/authentication.txt
Outdated
`applyToSslSettings() <{+api+}/apidocs/mongodb-driver-core/com/mongodb/MongoClientSettings.Builder.html#applyToSslSettings(com.mongodb.Block)>`__ | ||
method and setting the ``enabled`` property to ``true`` in the | ||
`SslSettings.Builder <{+api+}/apidocs/mongodb-driver-core/com/mongodb/connection/SslSettings.Builder.html>`__ |
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.
Broken links here; are they waiting on another PR to publish the API docs?
Edit: seems to apply to all API links on this page.
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.
Fixed!
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.
some small changes to add, but LGTM otherwise. great job!
// SCRAM Authentication | ||
// start-default-cred-string | ||
val mongoClient = | ||
MongoClient.create("mongodb+srv://<db_username>:<db_password>@<hostname>:<port>/?authSource=<authenticationDb>") |
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.
If the SRV URI connection format is used, no port can be specified.
Also, It seems that some examples use builder.srvHost("<hostname>")
, while others use builder.hosts(listOf(ServerAddress("<hostname>", <port>)))
. The Java driver documentation consistently uses builder.hosts(listOf(ServerAddress("<hostname>", <port>)))
.
Is srv host configuration used for conciseness in some examples?
I suggest we standardize the examples to use either the SRV format
or the host:port
format for consistency. The latter would align with the Java driver documentation.
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.
Hi @vbabanin, I opted for the SRV URI format for the connection string examples because I get the following timeout error using the standard format. It is because ssl is not enabled:
com.mongodb.MongoTimeoutException: Timed out while waiting for a server that matches ReadPreferenceServerSelector{readPreference=primary}. Client view of cluster state is {type=UNKNOWN, servers=[{address=atlascluster.spm1ztf.mongodb.net:27017, type=UNKNOWN, state=CONNECTING, exception={com.mongodb.MongoSocketException: atlascluster.spm1ztf.mongodb.net}, caused by {java.net.UnknownHostException: atlascluster.spm1ztf.mongodb.net}}]
For the same reason, I used builder.srvHost("<hostname>")
instead of the alternative. In order to make to the examples copyable and runnable, does it make sense to use the +srv format? Thanks!
Pull Request Info
PR Reviewing Guidelines
JIRA - https://jira.mongodb.org/browse/DOCSP-41148
Staging - https://deploy-preview-50--docs-kotlin-sync.netlify.app/security/authentication/
Self-Review Checklist