fix: disable searchable nodeToNode encryption unless it is already deployed to mitigate impact from enabling or disabling. #1152
Conversation
alharris-at
force-pushed
the
fix/conditional-node-to-node-encryption
branch
3 times, most recently
from
e63f1a1
to
0283602
Compare
packages/amplify-category-api/src/graphql-transformer/transformer-factory.ts
Show resolved
Hide resolved
| await amplifyPush(projRoot); | ||
|
|
||
| appSyncClient = getAppSyncClientFromProj(projRoot); | ||
| await runAndValidateQuery('test1', 'test1', 10, 2); // Expect two records |
There was a problem hiding this comment.
In this case, shouldn't the search domain be re-created? Since, now we're turning the encryption off. How does getting back 2 records test that, sorry if I missed something here.
There was a problem hiding this comment.
Behavior of the systems is such that we will maintain the current state of nodeToNode (either on or off), so there should be no loss of data. The explicit flag is being removed to test the state that "previous deployment had it, I don't care how, now I'm doing a normal deploy"
There was a problem hiding this comment.
The approach looks good if this code is the minimum api version used by customer. The rollback will not flip the value as false. Still I have some concerns as following.
- How do we prevent the user to use previous version of CLI (version < 10.6 in particular)? If the older version is used when they encounter other issues (not related to search) and decide to downgrade, they will finally have the false value forcefully as the code in this PR does not exist in the previous version.
In this case, I think we should provide some warning message to the user. If we detect the search domain is provisioned and the nodeToNode is true, we should warn the user not to downgrade to version less than 10.6 which will force the false value and delete the original instance.
- I see several searchable-related e2e tests are added. I think this is a two bladed sword as it ensures the safety while increasing the pipeline load since the provisioning and deletion of search domain consume plenty of time and will have trouble when the tests fail and when many concurrent cloud e2e runs.
I suggest if the nodeToNode value is the only thing that matters here we might not need e2e tests (or we can simplify into fewer cases). We just need to check if that value is resolved to the correct value in different scenarios. If the data loss is a consideration in the e2e test, I think it is enough to test false => true not causing the data loss. It is not necessary to test true => false causing replacement of domain as it is already acknowledged in the CFN docs.
| const searchableStackPath = path.join(projRoot, 'amplify', 'backend', 'api', projectName, 'build', 'stacks', 'SearchableStack.json'); | ||
|
|
||
| // Initial Deploy with no flag set | ||
| setTransformConfigValue(projRoot, projectName, 'NodeToNodeEncryption', true); |
There was a problem hiding this comment.
Is this line a conflict with the comment above?
There was a problem hiding this comment.
yeah, I think that comment is stale, I'll update.
…ployed to mitigate impact from enabling or disabling.
alharris-at
force-pushed
the
fix/conditional-node-to-node-encryption
branch
from
0283602
to
75578fb
Compare
| deleteProjectDir(projRoot); | ||
| }); | ||
|
|
||
| it('if previous deployment had no NodeToNodeEncryption, and flag is enabled on subsequent push, deploy, there should be no data loss', async () => { |
There was a problem hiding this comment.
Can we add a migration test?
For example:
- init an amplify project with CLI 9.x, add api with searchable directive, insert data.
- Make schema change and push with the latest CLI.
- Query the searchable data and make sure the data is present.
There was a problem hiding this comment.
I'll mark this as work in progress, but may not want to block this release on it, since we've manually run migration smoke testing.
… NodeToNodeEncryption enabled
alharris-at
dismissed stale reviews from AaronZyLee and phani-srikar
via
d420407
Description of changes
Mitigate impact from enabling nodeToNodeEncryption, by allow enabling it by default if it is already set in the
#current-cloud-backenddirectory at the time of gql compilation.Also add support for a
NodeToNodeEncryptionparam that can be set intransform.conf.jsonto facilitate overrides and manual testing. This isn't modeled as a PARAM like the rest of the params to ensure we're not looking at TOKENs in CFN make making the decision on whether to enable the flag.So if
NodeToNodeEncryptionis set in thetransform.conf.jsonfile, that will be respected primarily.If that flag is not set, and
NodeToNodeEncryptionis found in the#current-cloud-backendthen enable on the next deployment.If no occurrence is found, then return false. Fall back to false in case that some part of this process fails.
Issue #, if available
N/A
Description of how you validated changes
Added unit tests for core logic, and e2e tests to confirm behavior in the following cases:
NodeToNodeEncryptionset totruein thetransform.conf.jsonNodeToNodeEncryptionset tofalsein thetransform.conf.jsontransform.conf.jsonflag set, and previous deployment state hasNodeToNodeEncryptiontrue stays true, there is no data loss.transform.conf.jsonflag set, and previous deployment state hasNodeToNodeEncryptionfalse stays false, there is no data loss.truetofalsewill rebuild Searchable instance, there will be data loss.Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.