-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Require username and password for mongodb #1813
Conversation
Unit Test Results362 tests 362 ✅ 12s ⏱️ Results for commit 15e532f. ♻️ This comment has been updated with latest results. |
Once sillsdev/LfMerge#336 is merged, I'll update this PR with the new version of the LfMerge docker image and then mark it ready for review. I don't expect many changes between now and then, so if you want to give this an early review, @megahirt, go ahead. |
LfMerge has now completed a build with auth built in, so I've pushed a commit bumping the LfMerge version. It's currently just an alpha build of LfMerge so we'll need another version bump later on once LfMerge releases a non-alpha build, but this is enough ot test the PR with. So it's ready for review now. |
If MONGODB_USER and MONGODB_PASS are set, they will be used to authenticate to the MongoDB server. If they are not set, then a connection request with no authentication will be sent (as per the existing behavior). This allows us to deploy this change, then set MONGODB_USER and MONGODB_PASS later and have those changes picked up without a redeployment. Note that a corresponding change to LfMerge will also be needed.
LfMerge env vars won't be used until we deploy a new build of LfMerge that looks for them, which will be in a PR on the LfMerge repo.
LfMerge now has an alpha build that handles auth. Once we've proved that it works, we'll release a full build of LfMerge and bump this version number again.
Now the check for the `MONGODB_USER` and `MONGODB_PASS` env vars works correctly because it has the right syntax.
Uncommenting these two lines will enable MongoDB auth on local dev. Do not do so until you have created the `admin` user or you may end up locked out of your local MongoDB.
1d30bca
to
01fcccb
Compare
Rebased on top of |
This will allow us to change the name of the Mongo database we store our auth in, if in the future we decide not to go with the default name.
This does not create the actual secrets, which will need to be done at the Rancher admin UI. This commit simply adds the Kubernetes config to reference the secrets.
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.
This looks good to me. I'd like to get some clarification on a few things:
- what is the developer experience? It looks like it works fine out of the box and local dev environment continues without a password?
- how is this intended to work in the GHA testing environment? Currently tests are failing
- we need to outline a process to test this change in staging and then again in production. How about we outline those steps in this PR description?
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.
Looks like it's working to me! My jetbrains IDE gave a false positive that it was still able to connect to mongo without a password, but it wasn't actually able to list any databases or anything. When I used mongo compass it gave an error that auth was required to access the db, so it seems good.
running the provided script worked for setup. Maybe we should put that script in the dev docs for future reference?
Great! I am hoping that new devs won't need it. It's only required if you have an existing mongo database and don't want to drop the mongo volume. A fresh start should work fine. |
Fixes #1787
Description
Add env vars
MONGODB_USER
andMONGODB_PASS
to optionally specify authentication when connecting to MongoDB. Some manual setup on the MongoDB server(s) should be done before this is deployed; see #1787 (comment) for details. Once the appropriate admin user has been created and Kubernetes secrets set, this PR can then be deployed to staging and production.Note that sillsdev/LfMerge#336 will need to be merged first, and a new LfMerge version built. The version number in
lfmerge/Dockerfile
can then be bumped as part of this PR.Screenshots
None
Checklist
Testing
Testers, use the following instructions against our staging environment. Post your findings as a comment and include any meaningful screenshots, etc.
If those all continue to pass, then the necessary setup steps (creating an admin user and giving it a password, and creating Kubernetes secrets for that username and password) were correctly completed.