-
Notifications
You must be signed in to change notification settings - Fork 16
chore(go): add plain text migration examples #1966
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
base: main
Are you sure you want to change the base?
Conversation
…ase-encryption-sdk-dynamodb into plaintextmigration
MigrationStep1(kmsKeyID, tableName, partitionKey, sortKeys[1]) | ||
// When: Execute Step 2 with sortReadValue=1, Then: Success (i.e. can read encrypted values) | ||
MigrationStep2(kmsKeyID, tableName, partitionKey, sortKeys[1]) | ||
|
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.
Can we also add testcase to write with step 3 and read with step 2? also for step 1_test.go
see similar test in Python.
Lines 48 to 56 in de4588f
# Given: Step 3 has succeeded | |
migration_step_3.migration_step_3_with_client( | |
kms_key_id=TEST_KMS_KEY_ID, ddb_table_name=TEST_DDB_TABLE_NAME, sort_read_value=3 | |
) | |
# When: Execute Step 2 with sort_read_value=3 | |
# Then: Success (i.e. can read values in encrypted format) | |
migration_step_2.migration_step_2_with_client( | |
kms_key_id=TEST_KMS_KEY_ID, ddb_table_name=TEST_DDB_TABLE_NAME, sort_read_value=3 | |
) |
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.
Thanks. It was a very good finding.
I did it in this commit: 4eb1d8a
. I also figured that a change in local to handle error was not committed. So, this Commit include that too.
@@ -140,8 +140,9 @@ jobs: | |||
run: | | |||
make test_go | |||
- name: Test Examples | |||
- name: Run and Test Examples |
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.
Why run and test both? Since it's an example, I don't think we need to assert all cases via unit tests, but if it's already in then no worries.
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.
For migrations, I needed to run a lot of combination of steps that made sense of run as a unit test. For all other test, just running a round trip was enough but migration is different so it was not enough
Item: item, | ||
} | ||
|
||
_, err = ddb.PutItem(context.TODO(), &putInput) |
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.
Should we use a plain vanilla client to demonstrate that we actually aren't doing any encryption and can read unencrypted items using our client?
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 do this in Test.
This is one combination of the test: In step 1 test, we write with vanilla DDB client and read with intercepted DDB client. In step 0 test, we write with intercepted DDB client and read with the vanilla one.
Generator: &kmsKeyID, | ||
} | ||
kmsKeyring, err := matProv.CreateAwsKmsMrkMultiKeyring(context.Background(), keyringInput) | ||
utils.HandleError(err) |
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.
Nice touch.
|
||
// We return this error because we run test against the error. | ||
// When used in production code, you can decide how you want to handle errors. | ||
if err != nil { |
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.
Why not use utils here as well?
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.
Before writing migration example, I thought panic on error everywhere is good enough but with all the test combinations in migration test, it was not good enough. I think many in the team will still have a thought that we panic on every example. So, to make it visually distinct immediately visible in the code, I did not use utils.
if partitionKeyValue != result.Item["partition_key"].(*types.AttributeValueMemberS).Value { | ||
panic("Decrypted item does not match original item") | ||
} | ||
if encryptedAndSignedValue != result.Item["attribute1"].(*types.AttributeValueMemberS).Value { |
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 about other attributes? Maybe add comment if we don't want full assertion.
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.
Will this change look good to you? https://github.com/aws/aws-database-encryption-sdk-dynamodb/pull/1966/files#r2255583436
I will apply this change to other places as well.
sortKeys := []string{"0", "1", "2", "3"} | ||
|
||
// Successfully executes Step 1 | ||
err := MigrationStep1(kmsKeyID, tableName, partitionKey, sortKeys[1]) |
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.
Since we aren't mocking anything here, what's the difference between this and run?
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 run this only once with the test.
@@ -124,6 +133,33 @@ func HandleError(err error) { | |||
} | |||
} | |||
|
|||
func AssertServiceError(err error, service string, operation string, errorMessage string) { | |||
if err == nil { | |||
panic("Expected error but got no error") |
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.
Don't we want to have this function as no-op in case of no errors? In that way, we can always call this instead of figuring out when to call this.
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.
Not really. Every time we call this we need to have an Error. If we don't get an error then that means something is wrong.
HandleError(err) | ||
|
||
// Create DynamoDB client | ||
client := dynamodb.NewFromConfig(cfg) |
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.
Or you could just use the already created client :)
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.
This functions is used where we don't have access of the ddb client. So, I am re-creating it. We also re-create ddb client in every example so that examples are easier to follow.
return err | ||
} | ||
|
||
// Verify we got the expected item back |
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.
// Verify we got the expected item back | |
// Verify we got the expected item back. | |
// Since we are mostly concerned with Plaintext Override, | |
// our focus is to assert the encrypted and signed value. |
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.