feat(NODE-3777): add csfle kmip support #3070

durran · 2021-12-07T15:14:52Z

Description

Adds KMIP support to client side encryption. Includes NODE-3967 (remove example.com usages)

What is changing?

Adds the updated spec tests and allows KMIP to be used as a KMS provider.
Properly passes through TLS options per KMS provider.

Is there new documentation needed for these changes?

None

What is the motivation for this change?

NODE-3777

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>
Changes are covered by tests
New TODOs have a related JIRA ticket

https://spruce.mongodb.com/version/6203b999a4cf4724f9db3342/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

.evergreen/run-custom-csfle-tests.sh

durran · 2022-01-19T01:12:48Z

CSFLE tests on the main run fail due to the main suite of tests pointing at mongodb-client-encryption@2.0.0-beta.2 but we need a beta.3 released for those to pass. The https://spruce.mongodb.com/version/61e753873066153369a025de/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC patch which points at my PR on mongodb/libmongocrypt#235 for just the CSFLE tests shows they will pass when this happens.

.evergreen/run-tests.sh

test/integration/client-side-encryption/client_side_encryption.prose.test.js

.evergreen/config.yml

test/integration/client-side-encryption/client_side_encryption.prose.test.js

test/manual/kerberos.test.js

nbbeeken

We've got some test clean up needed here, unfotunately it's a little manual becuase chai can't take async functions inside expect(x).to.throw

.evergreen/config.yml.in

nbbeeken · 2022-02-04T19:09:30Z

test/integration/client-side-encryption/client_side_encryption.prose.test.js

-              }
-            }
+// TODO: NODE-3927 - these cannot run in lb mode but are not skipping based on metadata.
+if (!process.env.LOAD_BALANCER) {


Interesting that the metadata is not working, could we try skipping inside a beforeEach hook if possible? Currently we have a consistent test count across all runs, it would be nice to keep that for LB too.

It appears we currently don't run FLE against lb because we don't have the CSFLE_KMS_PROVIDERS var defined in that environment, I don't see anything in the evergreen changes that would've brought it in, what made this crop up?

I've updated to skip in the before/beforeEach blocks now and removed this conditional.

The CSFLE_KMS_PROVIDERS variable is not in the env, but the metadata isn't skipping for some reason and the test attempts to JSON.parse that env variable anyways causing a parse error on the string 'NOT_PROVIDED'. So I tried to change that string to be empty just to get around it but the before hooks are still running. I suspect there's some funkiness with mocha and async functions but I haven't been able to confirm what the exact issue yet is, and didn't want to rewrite all the tests without async await since it made it way easier to read. But I can do that if we want.

test/integration/client-side-encryption/client_side_encryption.prose.test.js

test/tools/runner/config.ts

durran · 2022-02-09T14:14:20Z

I moved the CSFLE tests into manual so now they don't attempt to run with all the other variants so I could remove the skip hook hacks (they also expect to run no auth). I felt this was an appropriate special case and we already have the precedent for this and spec tests with socks5 (and soon kerberos prose spec tests). When moving them I also added the custom dependency variant to run on patches so they will run when opening pull requests now.

nbbeeken

We should investigate further why the metadata skip isn't working for us rather than moving csfle to manual, unlike socks5 we were testing against the package published on npm while the custom version variant pulls from master of libmongocrypt, I think its important for us to keep the full integration test with what users have available.

You mentioned the before hooks were still running in LB (and probably now auth variants), sorry I did not recall earlier but this is a known limitation. And you can see how I already had to work around it for some of our FLE tests here

durran · 2022-02-09T16:17:26Z

We should investigate further why the metadata skip isn't working for us rather than moving csfle to manual, unlike socks5 we were testing against the package published on npm while the custom version variant pulls from master of libmongocrypt, I think its important for us to keep the full integration test with what users have available.

You mentioned the before hooks were still running in LB (and probably now auth variants), sorry I did not recall earlier but this is a known limitation. And you can see how I already had to work around it for some of our FLE tests here

Yeah I also ran across this as well: mochajs/mocha#2456

I can rewrite all those new tests in the other way or should I just make a ticket to come back to them as moving them to manual circumvents the issue?

dariakp · 2022-02-09T16:59:49Z

We should investigate further why the metadata skip isn't working for us rather than moving csfle to manual, unlike socks5 we were testing against the package published on npm while the custom version variant pulls from master of libmongocrypt, I think its important for us to keep the full integration test with what users have available.
You mentioned the before hooks were still running in LB (and probably now auth variants), sorry I did not recall earlier but this is a known limitation. And you can see how I already had to work around it for some of our FLE tests here

Yeah I also ran across this as well: mochajs/mocha#2456

I can rewrite all those new tests in the other way or should I just make a ticket to come back to them as moving them to manual circumvents the issue?

I think it would be better not to move them to manual if we don't have to, because like Neal mentioned, it is potentially losing us some coverage and increasing work that we'll have to do to revamp the manual suite. So if we can avoid introducing that tech debt now, that's better than relying on eventually getting it fixed.

durran · 2022-02-09T19:04:44Z

I have moved the csfle tests back to integration and refactored the tests to do all the client related activity in the it blocks and not in the before/after. Everything is passing now and no metadata issues on the main test runs and no explicit skips.

baileympearson · 2022-02-09T19:27:17Z

test/integration/client-side-encryption/client_side_encryption.prose.test.js

  const dataDbName = 'db';
  const dataCollName = 'coll';
  const dataNamespace = `${dataDbName}.${dataCollName}`;
  const keyVaultDbName = 'keyvault';
  const keyVaultCollName = 'datakeys';
  const keyVaultNamespace = `${keyVaultDbName}.${keyVaultCollName}`;

-  const shared = require('../shared');
+  const shared = require('../../integration/shared');


Would ../shared work here?

nbbeeken · 2022-02-09T19:42:06Z

.evergreen/run-tests.sh

  # Get access to the AWS temporary credentials:
  echo "adding temporary AWS credentials to environment"
  # CSFLE_AWS_TEMP_ACCESS_KEY_ID, CSFLE_AWS_TEMP_SECRET_ACCESS_KEY, CSFLE_AWS_TEMP_SESSION_TOKEN
  . $DRIVERS_TOOLS/.evergreen/csfle/set-temp-creds.sh
 fi

+npm install mongodb-client-encryption@">=2.0.0-beta.3"


No need to block this PR but wanna bump this to beta.4 :)

dariakp

LGTM, but we can probably put the ../../integration/shared requires back to just ../shared in both the prose.deadlock and prose test files now

durran force-pushed the NODE-3777 branch 3 times, most recently from d6e6338 to ee50a09 Compare December 20, 2021 11:37

durran force-pushed the NODE-3777 branch from 7875dfd to 571831e Compare January 4, 2022 13:11

durran force-pushed the NODE-3777 branch from 77a450f to f120dd4 Compare January 18, 2022 20:46

durran mentioned this pull request Jan 18, 2022

feat(NODE-3777): set tls options per kms provider mongodb/libmongocrypt#235

Merged

durran marked this pull request as ready for review January 19, 2022 00:02

durran commented Jan 19, 2022

View reviewed changes

.evergreen/run-custom-csfle-tests.sh Outdated Show resolved Hide resolved

durran commented Jan 19, 2022

View reviewed changes

.evergreen/run-custom-csfle-tests.sh Outdated Show resolved Hide resolved

durran commented Jan 19, 2022

View reviewed changes

.evergreen/run-tests.sh Outdated Show resolved Hide resolved

baileympearson assigned baileympearson and unassigned baileympearson Jan 25, 2022

durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jan 25, 2022

baileympearson requested changes Jan 25, 2022

View reviewed changes

test/integration/client-side-encryption/client_side_encryption.prose.test.js Outdated Show resolved Hide resolved

test/integration/client-side-encryption/client_side_encryption.prose.test.js Outdated Show resolved Hide resolved

baileympearson reviewed Jan 26, 2022

View reviewed changes

.evergreen/config.yml Outdated Show resolved Hide resolved

durran commented Jan 31, 2022

View reviewed changes

test/integration/client-side-encryption/client_side_encryption.prose.test.js Outdated Show resolved Hide resolved

durran requested a review from baileympearson January 31, 2022 14:35

durran force-pushed the NODE-3777 branch 3 times, most recently from 95bc5c9 to 3965ae3 Compare February 1, 2022 12:54

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 1, 2022

durran force-pushed the NODE-3777 branch 3 times, most recently from 90a1b26 to d348e10 Compare February 3, 2022 16:45

baileympearson requested changes Feb 4, 2022

View reviewed changes

test/manual/kerberos.test.js Outdated Show resolved Hide resolved

nbbeeken requested changes Feb 4, 2022

View reviewed changes

nbbeeken reviewed Feb 4, 2022

View reviewed changes

test/integration/client-side-encryption/client_side_encryption.prose.test.js Outdated Show resolved Hide resolved

nbbeeken reviewed Feb 4, 2022

View reviewed changes

test/tools/runner/config.ts Outdated Show resolved Hide resolved

durran added 4 commits February 8, 2022 21:16

fix(NODE-3777): fix lint error

bbc1336

test(NODE-3777): update evg config

3fe42a4

test(NODE-3777): fix custom fle tests

c8c4d2e

test(NODE-3777): always bootstrap kms servers

8219832

durran force-pushed the NODE-3777 branch from 6d78a52 to 8219832 Compare February 8, 2022 20:17

durran added 4 commits February 9, 2022 13:18

test(NODE-3777): skip entire prose test when no applicable

dc29d39

test(NODE-3777): move csfle to manual

1694bba

test(NODE-3777): csfle runs noauth

0635969

test(NODE-3777): split out csfle config

2f1c62a

durran force-pushed the NODE-3777 branch from 12f2378 to 2f1c62a Compare February 9, 2022 13:44

durran mentioned this pull request Feb 9, 2022

fix(NODE-3813): unexpected type conversion of read preference tags #3138

Merged

5 tasks

baileympearson previously approved these changes Feb 9, 2022

View reviewed changes

nbbeeken requested changes Feb 9, 2022

View reviewed changes

test(NODE-3777): move csfle back to integration

2c2dbd4

durran dismissed baileympearson’s stale review via 2c2dbd4 February 9, 2022 17:22

durran added 2 commits February 9, 2022 18:43

test(NODE-3777): install csfle for all test runs

f51396c

test(NODE-3777): skip kmip test with auth

207e634

durran force-pushed the NODE-3777 branch from 67a435e to 207e634 Compare February 9, 2022 18:38

durran requested review from nbbeeken and baileympearson February 9, 2022 19:04

baileympearson reviewed Feb 9, 2022

View reviewed changes

nbbeeken reviewed Feb 9, 2022

View reviewed changes

nbbeeken approved these changes Feb 9, 2022

View reviewed changes

dariakp approved these changes Feb 9, 2022

View reviewed changes

baileympearson merged commit 44bbd6e into main Feb 9, 2022

baileympearson deleted the NODE-3777 branch February 9, 2022 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-3777): add csfle kmip support #3070

feat(NODE-3777): add csfle kmip support #3070

durran commented Dec 7, 2021 •

edited

durran commented Jan 19, 2022

nbbeeken left a comment

nbbeeken Feb 4, 2022

durran Feb 8, 2022

durran commented Feb 9, 2022 •

edited

nbbeeken left a comment

durran commented Feb 9, 2022

dariakp commented Feb 9, 2022

durran commented Feb 9, 2022

baileympearson Feb 9, 2022

nbbeeken Feb 9, 2022

dariakp left a comment

feat(NODE-3777): add csfle kmip support #3070

feat(NODE-3777): add csfle kmip support #3070

Conversation

durran commented Dec 7, 2021 • edited

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

durran commented Jan 19, 2022

nbbeeken left a comment

Choose a reason for hiding this comment

nbbeeken Feb 4, 2022

Choose a reason for hiding this comment

durran Feb 8, 2022

Choose a reason for hiding this comment

durran commented Feb 9, 2022 • edited

nbbeeken left a comment

Choose a reason for hiding this comment

durran commented Feb 9, 2022

dariakp commented Feb 9, 2022

durran commented Feb 9, 2022

baileympearson Feb 9, 2022

Choose a reason for hiding this comment

nbbeeken Feb 9, 2022

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

durran commented Dec 7, 2021 •

edited

durran commented Feb 9, 2022 •

edited