[Feature Branch] Support protobuf/msgpack serialization for cty Values#229
Merged
lbajolet-hashicorp merged 6 commits intomainfrom Dec 17, 2024
Merged
[Feature Branch] Support protobuf/msgpack serialization for cty Values#229lbajolet-hashicorp merged 6 commits intomainfrom
lbajolet-hashicorp merged 6 commits intomainfrom
Conversation
fdcd784 to
b9a8108
Compare
nywilken
reviewed
Jun 10, 2024
b9a8108 to
914dafe
Compare
nywilken
reviewed
Jun 14, 2024
cc8fe50 to
ca6262e
Compare
nywilken
reviewed
Jun 21, 2024
Contributor
nywilken
left a comment
There was a problem hiding this comment.
Overall this looks good. I left one small suggestion but I will add a test for it. As discussed internally I am leaving a comment to mark the approach as approved but this is a feature branch that we will keep open and iterate on.
mogrogan
previously approved these changes
Dec 17, 2024
Contributor
mogrogan
left a comment
There was a problem hiding this comment.
Left a question (although I know you probably have a good reason) but solid PR
LGTM
This commit introduces a protobuf-serialisable type for hcldec.Spec types. We need this be able to use another format to serialise them without using gob, as gob isn't supported anymore for cty.Type. The HCL2Spec function, implemented by all the plugins, are the ones responsible for transmitting the schema of their configurations to Packer, which as of v0.5.1 of the SDK, is using gob for serialisation, which has been removed from the cty library.
When a plugin describes its capabilities, it needs to advertise whether or not protobuf can be used in order for Packer to know which serialisation protocol to use for communicating with the plugin. To do so, we add a protocol_version attribute to the returned structure, which is now set to v2, in order for Packer to know that the plugin can use that.
As follow-up to the introduction of the protobufs for HCLSpec, we introduce a new environment variable and code to use those structures, so we don't use gob for serialising HCLSpecs. This should make the plugins and packer able to transmit data over-the-wire without using gob for the most part (the communicators still use it, and will probably need some work to replace).
```
~> go test ./plugin/... -v
=== RUN TestPluginServerRandom
--- PASS: TestPluginServerRandom (0.00s)
=== RUN TestSet
--- PASS: TestSet (0.00s)
=== RUN TestSetProtobufArgParsing
=== RUN TestSetProtobufArgParsing/no_--protobuf_argument_provided
=== RUN TestSetProtobufArgParsing/providing_--protobuf_as_first_argument
=== RUN TestSetProtobufArgParsing/providing_--protobuf_as_last_argument
=== RUN TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument
--- PASS: TestSetProtobufArgParsing (0.00s)
--- PASS: TestSetProtobufArgParsing/no_--protobuf_argument_provided (0.00s)
--- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_first_argument (0.00s)
--- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_last_argument (0.00s)
--- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument (0.00s)
PASS
ok github.com/hashicorp/packer-plugin-sdk/plugin 0.249s
```
The upper index bound for a slice is cap(args) we can safely retun appended slices
```
~> go test -count=1 ./plugin/... -v
=== RUN TestPluginServerRandom
--- PASS: TestPluginServerRandom (0.00s)
=== RUN TestSet
--- PASS: TestSet (0.00s)
=== RUN TestSetProtobufArgParsing
=== RUN TestSetProtobufArgParsing/no_--protobuf_argument_provided
=== RUN TestSetProtobufArgParsing/providing_--protobuf_as_first_argument
=== RUN TestSetProtobufArgParsing/providing_--protobuf_as_last_argument
=== RUN TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument
--- PASS: TestSetProtobufArgParsing (0.00s)
--- PASS: TestSetProtobufArgParsing/no_--protobuf_argument_provided (0.00s)
--- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_first_argument (0.00s)
--- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_last_argument (0.00s)
--- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument (0.00s)
PASS
ok github.com/hashicorp/packer-plugin-sdk/plugin 0.244s
```
```
~> go test ./plugin/... -v
=== RUN TestPluginServerRandom
--- PASS: TestPluginServerRandom (0.00s)
=== RUN TestSet
--- PASS: TestSet (0.00s)
=== RUN TestSetProtobufArgParsing
=== RUN TestSetProtobufArgParsing/no_--protobuf_argument_provided
=== RUN TestSetProtobufArgParsing/providing_--protobuf_as_first_argument
=== RUN TestSetProtobufArgParsing/providing_--protobuf_as_last_argument
=== RUN TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument
=== RUN TestSetProtobufArgParsing/providing_--protobuf_multiple_times
--- PASS: TestSetProtobufArgParsing (0.00s)
--- PASS: TestSetProtobufArgParsing/no_--protobuf_argument_provided (0.00s)
--- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_first_argument (0.00s)
--- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_last_argument (0.00s)
--- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument (0.00s)
--- PASS: TestSetProtobufArgParsing/providing_--protobuf_multiple_times (0.00s)
PASS
ok github.com/hashicorp/packer-plugin-sdk/plugin 0.250s
```
28e1cb0 to
e0c1a4f
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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 aims to be a PoC/base for later developments on the wire-protocol for all the components that plugin expose.
We introduce a protobuf-serialisable data structure for
hcldec.HCLSpec, which is used by all components of a plugin for describing the schema of what the plugin uses, and offers Packer a type to use to decode cty values from HCL code.In doing so, we also explored using alternative serialisation formats for datasources, which were the only components leveraging cty's gob serialisation capabilities, and replace them with JSON/msgpack.
These changes make the SDK capable to communicate over-the-wire on recent hcl/cty versions, without needing to use the fork/replace for gob support.