Sarah's updates to radek/pss-read-write #37642
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… Add test case about handling grpc errors from CloseSend.
… data alongside the warning and have a normal closing of the stream after EOF
SarahFrench
changed the title
SarahFrench
commented
Sep 19, 2025
Comment on lines
1517
to
1521
| if err == io.EOF { | ||
| // End of stream, we're done | ||
| if chunk != nil { | ||
| // TODO: The EOF error could be just returned alongside the last chunk? | ||
| resp.Diagnostics = resp.Diagnostics.Append(convert.ProtoToDiagnostics(chunk.Diagnostics)) | ||
| } | ||
| // No chunk is returned alongside an EOF. | ||
| // And as EOF is a sentinel error it isn't wrapped as a diagnostic. | ||
| break | ||
| } |
There was a problem hiding this comment.
Please see this comment in our terraform-plugin-go PR explaining why we will never see a chunk alongside the EOF error here: https://github.com/hashicorp/terraform-plugin-go/pull/563/files#r2362735975
radeksimko
approved these changes
Sep 19, 2025
radeksimko
pushed a commit
that referenced
this pull request
Sep 19, 2025
* Update code to not expect a chunk when EOF encountered * Return early if any grpc errors are encountered during ReadStateBytes * Close the stream with CloseSend once everything's read without error. Add test case about handling grpc errors from CloseSend. * Fix test case about warnings: We would expect to receive a chunk with data alongside the warning and have a normal closing of the stream after EOF * Add log line, remove unneeded type info
radeksimko
pushed a commit
that referenced
this pull request
Sep 23, 2025
* Update code to not expect a chunk when EOF encountered * Return early if any grpc errors are encountered during ReadStateBytes * Close the stream with CloseSend once everything's read without error. Add test case about handling grpc errors from CloseSend. * Fix test case about warnings: We would expect to receive a chunk with data alongside the warning and have a normal closing of the stream after EOF * Add log line, remove unneeded type info
radeksimko
pushed a commit
that referenced
this pull request
Sep 25, 2025
* Update code to not expect a chunk when EOF encountered * Return early if any grpc errors are encountered during ReadStateBytes * Close the stream with CloseSend once everything's read without error. Add test case about handling grpc errors from CloseSend. * Fix test case about warnings: We would expect to receive a chunk with data alongside the warning and have a normal closing of the stream after EOF * Add log line, remove unneeded type info
radeksimko
added a commit
that referenced
this pull request
Sep 26, 2025
* Implement ReadStateBytes + WriteStateBytes * [WIP] - Testing ReadStateBytes and WriteStateBytes (#37464) * Fix nil pointer error * Add WIP test for ReadStateBytes * Move test mock to separate testing file * Update mock to send unexpected EOF when there's a problem returning data and it's not a true EOF * Add test case for when length != expected length * Add test for when trying to read state from a store type that doesn't exist * Change symbol names to lowercase * Add ability to force a diagnostic to be returned from `mockReadStateBytesClient`'s `Recv` method * Add test showing error diagnostics raised by the ReadStateBytes client are returned * Add missing header * Simplify mock by using an embedded type * Rename `mockOpts` to `mockReadStateBytesOpts` * Update existing tests to assert what arguments are passed to the RPC method call * Add mock WriteStateBytesClient which uses `go.uber.org/mock/gomock` to enable assertions about calls to Send * Add a test for WriteStateBytes that makes assertions about calls to the Send method * Update test case to explicitly test writing data smaller than the chunk size * Implement chunking in WriteStateBytes, add test case to assert expected chunking behaviour * Add generated mock for Provider_WriteStateBytesClient in protocol v6 * Update tests to use new `MockProvider_WriteStateBytesClient`, remove handwritten mock * Update code comments in test * Add tests for diagnostics and errors returned during WriteStateBytes * Add generated mock for Provider_ReadStateBytesClient in protocol v6, replace old mock * Add test case for grpc errors in ReadStateBytes, fix how error is returned * Typo in comment * Add missing warning test, rename some test cases * Update proto file definition of Read/WriteStateBytes RPCs (#37529) * Update Read/WriteStateBytes RPCs to match hashicorp/terraform-plugin-go#531 * Run `make protobuf` * Run `make generate` * Update use of `proto.ReadStateBytes_ResponseChunk` in tests * Fix how diagnostics are handled alongside EOF error, update ReadStateBytes test * More fixes - test setup was incorrect I think? I assume that a response would be returned full of zero-values when EOF is encountered. * WIP - avoid crash if chunk is nil * Sarah's updates to radek/pss-read-write (#37642) * Update code to not expect a chunk when EOF encountered * Return early if any grpc errors are encountered during ReadStateBytes * Close the stream with CloseSend once everything's read without error. Add test case about handling grpc errors from CloseSend. * Fix test case about warnings: We would expect to receive a chunk with data alongside the warning and have a normal closing of the stream after EOF * Add log line, remove unneeded type info * Implement configurable state chunk size * handle metadata in WriteStateBytes correctly * validate chunk sizes received from provider * ReadStateBytes: avoid early return on warnings --------- Co-authored-by: Sarah French <[email protected]>
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves some fixes to the radek/pss-read-write branch out of #37323
I tested the process of initialising a working directory end to end using the
pssprovider and our changes to the terraform-plugin-go repo, and in the process made these changes to the Core-side ofReadStateBytes.The main changes in this PR are:
Target Release
N/A
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry