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

[Feature Branch] Support protobuf/msgpack serialization for cty Values #229

Merged
merged 6 commits into from
Dec 17, 2024

Conversation

lbajolet-hashicorp
Copy link
Contributor

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.

Makefile Outdated Show resolved Hide resolved
plugin/server.go Outdated Show resolved Hide resolved
rpc/common.go Outdated Show resolved Hide resolved
plugin/set.go Outdated Show resolved Hide resolved
plugin/set.go Outdated Show resolved Hide resolved
rpc/common.go Outdated Show resolved Hide resolved
rpc/client.go Outdated Show resolved Hide resolved
rpc/server.go Outdated Show resolved Hide resolved
rpc/server.go Outdated Show resolved Hide resolved
plugin/set.go Outdated Show resolved Hide resolved
plugin/set.go Outdated Show resolved Hide resolved
@lbajolet-hashicorp lbajolet-hashicorp force-pushed the grpc_base branch 3 times, most recently from cc8fe50 to ca6262e Compare June 18, 2024 19:44
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

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.

plugin/set.go Outdated Show resolved Hide resolved
@nywilken nywilken changed the title No-gob PoC [Feature Branch] Support protobuf/msgpack serialization for cty Values Jun 21, 2024
@nywilken nywilken marked this pull request as ready for review June 21, 2024 19:39
@nywilken nywilken requested a review from a team as a code owner June 21, 2024 19:39
mogrogan
mogrogan previously approved these changes Dec 17, 2024
Copy link

@mogrogan mogrogan left a comment

Choose a reason for hiding this comment

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

Left a question (although I know you probably have a good reason) but solid PR

LGTM

rpc/hcl_spec.proto Outdated Show resolved Hide resolved
lbajolet-hashicorp and others added 6 commits December 17, 2024 12:03
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

```
Copy link

@mogrogan mogrogan left a comment

Choose a reason for hiding this comment

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

LGTM

@lbajolet-hashicorp lbajolet-hashicorp merged commit 9c35f2f into main Dec 17, 2024
9 checks passed
@lbajolet-hashicorp lbajolet-hashicorp deleted the grpc_base branch December 17, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants