-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-3367 Add support for GCP attached service accounts when using GCP KMS #1064
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
test/test_on_demand_csfle.py
Outdated
|
|
||
| @unittest.skipIf(not os.getenv("TEST_FLE_GCP_AUTO"), "Not testing FLE GCP auto") | ||
| def test_01_failure(self): | ||
| if os.getenv("SUCCESS"): |
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.
os.getenv("SUCCESS"): is always True so we'll always skip this 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.
SUCCESS=false in the testgcpkms-fail-task
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.
Yes but the string "false" is still truthy.
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.
Ah, fixed
| export GCPKMS_PROJECT=${GCPKMS_PROJECT} | ||
| export GCPKMS_ZONE=${GCPKMS_ZONE} | ||
| export GCPKMS_INSTANCENAME=${GCPKMS_INSTANCENAME} | ||
| tar czf /tmp/mongo-python-driver.tgz . |
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.
Are any of these variables sensitive info?
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.
Confirmed not to be sensitive
.evergreen/run-tests.sh
Outdated
| git clone https://github.com/blink1073/libmongocrypt.git libmongocrypt_git | ||
| pushd libmongocrypt_git | ||
| git checkout PYTHON-3367 | ||
| popd |
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.
No longer needed?
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 is still needed until we make a new release of pymongocrypt.
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
| TEST_FLE_GCP_AUTO=1 $PYTHON test/test_on_demand_csfle.py | ||
| } | ||
|
|
||
| PYTHON="python3" authtest |
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.
What's the plan for testing other python versions?
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 don't test multiple versions on ECS
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.
Oh right I forgot this was running on a remote host.
test/test_on_demand_csfle.py
Outdated
|
|
||
| @unittest.skipIf(not os.getenv("TEST_FLE_GCP_AUTO"), "Not testing FLE GCP auto") | ||
| def test_01_failure(self): | ||
| if os.getenv("SUCCESS", "").lower() == "true": |
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.
Let's make SUCCESS required to be present in the env to avoid unintentionally always skipping both these tests.
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
| $PYTHON -m pip install --upgrade wheel setuptools pip | ||
| $PYTHON -m pip install '.[encryption]' | ||
| $PYTHON -m pip install https://github.com/mongodb/libmongocrypt#subdirectory=bindings/python | ||
| TEST_FLE_GCP_AUTO=1 $PYTHON test/test_on_demand_csfle.py |
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.
Does this need SUCCESS=true?
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 is passed in from the evergreen config.
ShaneHarvey
left a comment
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 after the Copyright year fix
test/test_on_demand_csfle.py
Outdated
| @@ -0,0 +1,67 @@ | |||
| # Copyright 2019-2022 MongoDB, Inc. | |||
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.
2019-2022 -> "2022-present"
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
Requires mongodb/libmongocrypt#467