Skip to content
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

feat(NODE-5050): support GCP automatic credential fetch for CSFLE #3574

Merged
merged 2 commits into from
Feb 22, 2023

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Feb 16, 2023

Description

What is changing?

  • Add test-gcpkms-task that uploads the driver to a GCP instance and runs success test
  • Add test-gcpkms-fail-task that runs locally on EVG and tests for failure
  • Add {accessToken:string} types to azure and gcp credential types
  • Add {} empty credential types to aws and gcp
  • Debugging print for when FLE is required to run but fails to import

What is the motivation for this change?

Support GCP KMS auto credential fetching.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken nbbeeken marked this pull request as draft February 16, 2023 18:38
@nbbeeken nbbeeken force-pushed the NODE-5050-gcp-int-tests branch 2 times, most recently from 7dd92ef to d2e92fc Compare February 16, 2023 19:28
@nbbeeken nbbeeken marked this pull request as ready for review February 16, 2023 20:42
@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Feb 17, 2023
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really minor comments. Nice work!

test/tools/runner/filters/client_encryption_filter.js Outdated Show resolved Hide resolved
@@ -663,6 +663,13 @@ BUILD_VARIANTS.push({
tasks: ['serverless_task_group']
});

BUILD_VARIANTS.push({
name: 'rhel8-test-gcp-kms',
display_name: 'GCP KMS Test',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
display_name: 'GCP KMS Test',
display_name: 'FLE KMS Refresh Tests',

we will soon have both AWS and Azure as well - maybe we can just have a generic task group?

Copy link
Contributor Author

@nbbeeken nbbeeken Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you'll be able to reuse this variant, it's specific to GCP / GCP setup scripts. I tried to use the scripts on our typical rhel8 setup but it fails, it needs to be on debian11, so azure might have a diff config too (windows? 😱)

echo "${testgcpkms_key_file}" > ./testgcpkms_key_file.json
export GCPKMS_KEYFILE=./testgcpkms_key_file.json

"$GCPKMS_DRIVERS_TOOLS/.evergreen/csfle/gcpkms/create-and-setup-instance.sh"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps dumb question – does this task also start a server? And is that server always a standalone?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test/types/encryption.test-d.ts Outdated Show resolved Hide resolved
Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>
@baileympearson baileympearson added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Feb 21, 2023
@baileympearson baileympearson merged commit 722a4a6 into main Feb 22, 2023
@baileympearson baileympearson deleted the NODE-5050-gcp-int-tests branch February 22, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants