GODRIVER-3486 Support auto encryption in unified tests. #2240
Conversation
|
There is an existing patch(es) for this commit SHA: Please note that the status that is posted is not in the context of this PR but rather the (latest) existing patch and that may affect some tests that may depend on the particular PR. If your tests do not rely on any PR-specific values (like base or head branch name) then your tests will report the same status. If you would like a patch to run in the context of this PR and abort the other(s), comment 'evergreen retry'. |
qingyang-hu
force-pushed
the
godriver3486
branch
from
4e58f2a to
692fc7c
Compare
🧪 Performance ResultsCommit SHA: 6bf8480The following benchmark tests for version 6917ebad1e26950007eb6d71 had statistically significant changes (i.e., |z-score| > 1.96):
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
API Change ReportNo changes found! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for auto encryption in unified tests by implementing the autoEncryptOpts option for client entities, enabling previously skipped client-side encryption tests to run.
- Removes skip entries for 4 client-side encryption unified tests
- Adds
AutoEncryptOptsfield toentityOptionsstruct - Implements
createAutoEncryptionOptionsfunction to parse and configure auto encryption settings
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/spectest/skip.go | Removes the skip list for auto encryption unified tests (DRIVERS-3106), enabling 4 previously skipped client-side encryption tests |
| internal/integration/unified/entity.go | Adds AutoEncryptOpts field to support auto encryption configuration in entity options |
| internal/integration/unified/client_entity.go | Implements auto encryption support by adding global AWS credential variables and createAutoEncryptionOptions function to parse and apply encryption settings to client entities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func createAutoEncryptionOptions(opts bson.Raw) (*options.AutoEncryptionOptions, error) { | ||
| aeo := options.AutoEncryption() | ||
| var kvnsFound bool | ||
| elems, _ := opts.Elements() | ||
|
|
||
| for _, elem := range elems { | ||
| name := elem.Key() | ||
| opt := elem.Value() | ||
|
|
||
| switch name { | ||
| case "kmsProviders": | ||
| providers := make(map[string]map[string]any) | ||
| elems, _ := opt.Document().Elements() | ||
| for _, elem := range elems { | ||
| provider := elem.Key() | ||
| switch provider { | ||
| case "aws": | ||
| providers["aws"] = map[string]any{ | ||
| "accessKeyId": awsAccessKeyID, | ||
| "secretAccessKey": awsSecretAccessKey, | ||
| } | ||
| default: | ||
| return nil, fmt.Errorf("unrecognized KMS provider: %v", provider) | ||
| } | ||
| } | ||
| aeo.SetKmsProviders(providers) | ||
| case "schemaMap": | ||
| var schemaMap map[string]any | ||
| err := bson.Unmarshal(opt.Document(), &schemaMap) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| aeo.SetSchemaMap(schemaMap) | ||
| case "keyVaultNamespace": | ||
| kvnsFound = true | ||
| aeo.SetKeyVaultNamespace(opt.StringValue()) | ||
| default: | ||
| return nil, fmt.Errorf("unrecognized option: %v", name) | ||
| } | ||
| } | ||
| if !kvnsFound { | ||
| aeo.SetKeyVaultNamespace("keyvault.datakeys") | ||
| } | ||
|
|
||
| return aeo, nil | ||
| } |
There was a problem hiding this comment.
[nitpick] There is duplicated logic between this function and createAutoEncryptionOptions in json_helpers_test.go (lines 128-176). Consider extracting the common functionality into a shared helper function to reduce code duplication and ensure consistency across test implementations. This would make maintenance easier when new auto encryption options are added.
| func createAutoEncryptionOptions(opts bson.Raw) (*options.AutoEncryptionOptions, error) { | ||
| aeo := options.AutoEncryption() | ||
| var kvnsFound bool | ||
| elems, _ := opts.Elements() | ||
|
|
||
| for _, elem := range elems { | ||
| name := elem.Key() | ||
| opt := elem.Value() | ||
|
|
||
| switch name { | ||
| case "kmsProviders": | ||
| providers := make(map[string]map[string]any) | ||
| elems, _ := opt.Document().Elements() | ||
| for _, elem := range elems { | ||
| provider := elem.Key() | ||
| switch provider { | ||
| case "aws": | ||
| providers["aws"] = map[string]any{ | ||
| "accessKeyId": awsAccessKeyID, | ||
| "secretAccessKey": awsSecretAccessKey, | ||
| } | ||
| default: | ||
| return nil, fmt.Errorf("unrecognized KMS provider: %v", provider) | ||
| } | ||
| } | ||
| aeo.SetKmsProviders(providers) | ||
| case "schemaMap": | ||
| var schemaMap map[string]any | ||
| err := bson.Unmarshal(opt.Document(), &schemaMap) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| aeo.SetSchemaMap(schemaMap) | ||
| case "keyVaultNamespace": | ||
| kvnsFound = true | ||
| aeo.SetKeyVaultNamespace(opt.StringValue()) | ||
| default: | ||
| return nil, fmt.Errorf("unrecognized option: %v", name) | ||
| } | ||
| } | ||
| if !kvnsFound { | ||
| aeo.SetKeyVaultNamespace("keyvault.datakeys") | ||
| } | ||
|
|
||
| return aeo, nil | ||
| } |
There was a problem hiding this comment.
The createAutoEncryptionOptions function is missing support for several options that are supported in the similar function in json_helpers_test.go:
bypassAutoEncryptionencryptedFieldsMapbypassQueryAnalysiskeyExpirationMS
These options should be added to ensure feature parity and prevent test failures when these options are used in unified tests. For reference, see lines 154-166 in internal/integration/json_helpers_test.go.
| case "kmsProviders": | ||
| providers := make(map[string]map[string]any) | ||
| elems, _ := opt.Document().Elements() | ||
| for _, elem := range elems { | ||
| provider := elem.Key() | ||
| switch provider { | ||
| case "aws": | ||
| providers["aws"] = map[string]any{ | ||
| "accessKeyId": awsAccessKeyID, | ||
| "secretAccessKey": awsSecretAccessKey, | ||
| } | ||
| default: | ||
| return nil, fmt.Errorf("unrecognized KMS provider: %v", provider) | ||
| } | ||
| } | ||
| aeo.SetKmsProviders(providers) |
There was a problem hiding this comment.
The KMS provider handling is incomplete compared to the existing implementation in json_helpers_test.go. Missing providers include:
local(with binary key extraction)azure(with tenant ID, client ID, and client secret)gcp(with email and private key)kmip(with endpoint and TLS configuration)awsTemporaryandawsTemporaryNoSessionToken(for temporary AWS credentials with session tokens)
Additionally, the current implementation doesn't handle placeholder documents (e.g., {"$$placeholder": 1}) that should be replaced with environment variable values. Consider using the getKmsCredential helper function from entity.go (lines 567-602) to properly handle placeholders and environment variables, similar to how it's done in addClientEncryptionEntity.
| "accessKeyId": awsAccessKeyID, | ||
| "secretAccessKey": awsSecretAccessKey, |
There was a problem hiding this comment.
Hard-coded AWS credentials from environment variables may fail if the credentials are not set or if placeholder documents are used in tests. The existing pattern in entity.go uses the getKmsCredential helper function to handle both string values and placeholder documents (e.g., {"$$placeholder": 1}). Consider refactoring to use getKmsCredential for consistency and to properly support test specifications that use placeholders.
| "accessKeyId": awsAccessKeyID, | |
| "secretAccessKey": awsSecretAccessKey, | |
| "accessKeyId": getKmsCredential("AWS_ACCESS_KEY_ID", "accessKeyId"), | |
| "secretAccessKey": getKmsCredential("AWS_SECRET_ACCESS_KEY", "secretAccessKey"), |
| var schemaMap map[string]any | ||
| err := bson.Unmarshal(opt.Document(), &schemaMap) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
The error handling for BSON unmarshalling is inconsistent with other error messages in this function. Consider wrapping the error with more context, similar to line 297: return nil, fmt.Errorf("error creating schema map: %w", err) for better debugging.
| return nil, err | |
| return nil, fmt.Errorf("error creating schema map: %w", err) |
qingyang-hu
force-pushed
the
godriver3486
branch
from
692fc7c to
7dd2250
Compare
qingyang-hu
force-pushed
the
godriver3486
branch
from
7dd2250 to
5637f56
Compare
qingyang-hu
force-pushed
the
godriver3486
branch
from
82e1efd to
b1f3ca6
Compare
qingyang-hu
force-pushed
the
godriver3486
branch
from
b1f3ca6 to
6bf8480
Compare
1 participant
GODRIVER-3486
Summary
Support auto encryption in unified tests.
Background & Motivation