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: separate flags for request/response E2E checksum and enable request checksum by default #1251

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nimf
Copy link
Contributor

@nimf nimf commented Nov 30, 2023

This PR replaces the GOOGLE_CLOUD_DATASTORE_HTTP_ENABLE_E2E_CHECKSUM flag with two:

  • GOOGLE_CLOUD_DATASTORE_HTTP_ENABLE_E2E_REQUEST_CHECKSUM - attach payload checksum header to requests (enabled by default)
  • GOOGLE_CLOUD_DATASTORE_HTTP_ENABLE_E2E_RESPONSE_CHECKSUM - verify response checksum (if extsts in response headers) against response payload and raise IOException if it doesn't match (disabled by default)

And enables the E2E request checksum by default.
This is safe because nobody is using the old flag and we don't reject calls with invalid checksum on the server side yet. But this will provide us the analytics of number of requests with valid/invalid checksums to find edge cases and better prepare for E2E checksum rollout.

The tests for RemoteRpc using request and response checksum was also reworked.

Previously the test called the protected setHeaders but it does not verify that RemoteRpc will call it when making a call. I. e., if in the future we change RemoteRpc's call method and remake adding the headers without using the setHeaders method then the tests will remain green while necessary headers may not be added.

Instead of calling setHeaders from the test itself, we pass the expected headers to the MyHttpTransport and make sure the required headers are in place when MyLowLevelHttpRequest is doing its execute call.

It is easy to convert the remaining test (not related to E2E checksum) that uses setHeaders to the same approach, then we can make setHeaders private. I can do it in a separate PR if you like.

Another change in the test is that now we verify that checksum in the header is generated correctly (not just checking the length of the checksum string) and verify that the response checksum gets validated.

@nimf nimf requested review from a team as code owners November 30, 2023 18:55
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: datastore Issues related to the googleapis/java-datastore API. labels Nov 30, 2023
@nimf
Copy link
Contributor Author

nimf commented Nov 30, 2023

/cc @wenbozhu

jainsahab
jainsahab previously approved these changes Dec 4, 2023
Copy link
Member

@jainsahab jainsahab left a comment

Choose a reason for hiding this comment

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

Just one small nit, otherwise LGTM.

kolea2
kolea2 previously approved these changes Dec 7, 2023
@kolea2 kolea2 added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Dec 7, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 7, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 7, 2023
@kolea2 kolea2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 11, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 11, 2023
@kolea2 kolea2 dismissed stale reviews from jainsahab and themself February 23, 2024 18:14

stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the googleapis/java-datastore API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants