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

tfprotov5+tfprotov6: Add deferred action support to related RPCs #403

Merged
merged 20 commits into from
May 6, 2024

Conversation

austinvalle
Copy link
Member

@austinvalle austinvalle commented May 1, 2024

Refs: hashicorp/terraform#34880, hashicorp/terraform#35063

This PR introduces the ClientCapabilities/deferral_allowed fields to the relevant request structs and Deferred/Reason to the relevant response structs. The RPCs in-scope for supporting deferred actions are:

  • ReadDataSource
  • ReadResource
  • PlanResourceChange
  • ImportResourceState

The ConfigureProvider RPC also includes ClientCapabilities/deferral_allowed to assist provider logic that needs to automatically setup deferral responses for the PROVIDER_CONFIG_UNKNOWN reason.

austinvalle and others added 10 commits April 5, 2024 11:38
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.63.0 to 1.63.2.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.63.0...v1.63.2)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* don't marshal integer values as msgpack floats

Round-tripping a large integer through a float64, even if the binary
representation is exact, causes us to end up with a rounded string
representation after decoding, because the decoded number has 52-bit
precision instead of the 512 we use when dealing with string
representations of large integers.

* update CHANGELOG.md

* update to changie log

---------

Co-authored-by: Austin Valle <austinvalle@gmail.com>
@austinvalle austinvalle added the enhancement New feature or request label May 1, 2024
Copy link
Member

@bflad bflad left a comment

Choose a reason for hiding this comment

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

So far this looks good to me! One potential consideration to document and/or futureproof the exported API for client capabilities. 👍

tfprotov5/client_capabilities.go Outdated Show resolved Hide resolved
@bflad
Copy link
Member

bflad commented May 2, 2024

Oh and for your explicit open questions -- these both seem like things that can be added now or later, if we want. The logging might be nice and easy to get in right now though. The diagnostic wouldn't be hard to implement, but it'll be one of the first times introducing automatic RPC-level diagnostics that cannot be overridden by provider developers or higher level SDKs. Not sure if there are any concerns about this codebase owning an unavoidable error and its messaging (probably not?) but I have to ask.

@austinvalle
Copy link
Member Author

austinvalle commented May 2, 2024

@bflad Cool, I'll implement the logging since it's straightforward.

As for the diagnostic, I'm 50/50 on adding the error check... On one hand, it's an implicit expectation of the protocol that Terraform core has which is not immediately obvious without documentation. On the other, there are other implicit expectations around combinations of data (although more difficult/expensive to check!).

I'm leaning towards adding the error check, but I'm going to verify with core to ensure that applying this error handling at the protocol level is appropriate. I feel like if they have an error check on their side for this, we should as well.

Both Framework and SDKv2 will have their own error checks for this scenario, so regardless, it will be handled by the higher-level SDK with it's messaging.

@austinvalle
Copy link
Member Author

austinvalle commented May 3, 2024

@bflad I've made all of the changes we discussed so I think it's ready for the PR to be officially reviewed 😆

  • Added logging in a58e305
  • Updated the protocol version variables in e089b65 ⚡ . Also added two additional tests that do a simple compare on the proto file comments to find a match. If we find it to be too cumbersome to maintain we can always remove it later. Example of test failures before updating the variable:
--- FAIL: Test_EnsureVersionConstantMatchesProtoFile (0.00s) proto_version_test.go:47: protocol version Go variable is different from proto file - expected: 5.6, got: 5.4
proto_version_test.go:48: MAINTAINER NOTE: Update tf5server.protocolVersionMajor and tf5server.protocolVersionMinor to match the proto file.

--- FAIL: Test_EnsureVersionConstantMatchesProtoFile (3.59s) proto_version_test.go:47: protocol version Go variable is different from proto file - expected: 6.6, got: 6.4
proto_version_test.go:48: MAINTAINER NOTE: Update tf6server.protocolVersionMajor and tf6server.protocolVersionMinor to match the proto file.
  • Added a diagnostic for invalid deferred responses. I don't see any tests for the tf{5/6}server packages, should I add some or is it fine to just test this in terraform-provider-corner with a terraform-plugin-go provider?

Copy link
Member

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Overall looks good to me 🚀 Some optional nits if they sound reasonable.

tfprotov5/internal/tf5serverlogging/client_capabilities.go Outdated Show resolved Hide resolved
tfprotov5/tf5server/server.go Outdated Show resolved Hide resolved
Copy link
Contributor

@SBGoods SBGoods left a comment

Choose a reason for hiding this comment

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

LGTM, Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants