-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Support the Azure VM-assigned managed identity for automatic KMS credentials #1035
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
Conversation
…entials JAVA-4706
This unit test is not pulling enough weight given the success/failure integration tests we already have. Removing and simplifying the production code now that we no longer need to mock the endpoint.
Increase timeout
.evergreen/.evg.yml
Outdated
export AZUREKMS_RESOURCEGROUP=${testazurekms_resourcegroup} | ||
export AZUREKMS_VMNAME=${AZUREKMS_VMNAME} | ||
export AZUREKMS_PRIVATEKEYPATH=/tmp/testazurekms_privatekey | ||
AZUREKMS_CMD="MONGODB_URI='mongodb://localhost:27017' PROVIDER="azure" ./.evergreen/run-fle-on-demand-credential-test.sh" $DRIVERS_TOOLS/.evergreen/csfle/azurekms/run-command.sh |
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 wonder why did you miss KEY_NAME='${testazurekms_keyname}' KEY_VAULT_ENDPOINT='${testazurekms_keyvaultendpoint}'
?
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 those values are hardcoded in the integration test. Is that not ok?
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.
better to use env variables instead hardcoded values. That values are going to be changed in this ticket https://jira.mongodb.org/browse/BUILD-16154. Then, applying it in the driver will be easier with env variables instead of changing it in the code. NOTE: we will need to change the code in c# driver, my fault :(
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.
Done
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 don't see this part in this PR: mongodb/specifications@d6b8cce#diff-de01d398114f63480af23798e09ffca599aa758efd04f15a9beec89b4566041bR344. Has it been implemented somewhere else?
Agreed to defer credential caching to subsequent PR. Is there anything else missing besides that? |
@DmitryLukyanov ready for another look |
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.
LGTM (assuming tests passed)
JAVA-4706
After discussing with Kevin, determined that given the coverage provided by the integration tests, there is not enough additional value in the unit tests (which require starting a mock server) to get them running in CI. So although they are implemented in the first commit, they are removed in the second one.