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

.github/workflows/go-test.yml

@@ -41,6 +41,9 @@ jobs:
         run: |
           mkdir -p ${{ env.TEST_RESULTS_PATH }}/packer-plugin-sdk
 
+      - name: Install buf
+        uses: bufbuild/[email protected]
+
       - name: Run gofmt
         run: |
             make fmt-check

Makefile

@@ -59,7 +59,7 @@ generate: install-gen-deps ## Generate dynamically generated code
 	@find ./ -type f | xargs grep -l '^// Code generated' | xargs rm -f
 	PROJECT_ROOT="$(CURDIR)" go generate ./...
 	go fmt bootcommand/boot_command.go
-# 	go run ./cmd/generate-fixer-deprecations
+	buf generate
 
 generate-check: generate ## Check go code generation is on par
 	@echo "==> Checking that auto-generated code is not changed..."

buf.gen.yaml

@@ -0,0 +1,7 @@
+version: v2
+plugins:
+- remote: buf.build/protocolbuffers/go:v1.28.1
+  out: rpc
+  opt: paths=source_relative
+inputs:
+- proto_file: rpc/hcl_spec.proto

plugin/set.go

@@ -33,21 +33,31 @@ type Set struct {
 	version        string
 	sdkVersion     string
 	apiVersion     string
+	useProto       bool
 	Builders       map[string]packersdk.Builder
 	PostProcessors map[string]packersdk.PostProcessor
 	Provisioners   map[string]packersdk.Provisioner
 	Datasources    map[string]packersdk.Datasource
 }
 
+// ProtocolVersion2 serves as a compatibility argument to the SetDescription
+// so plugins can report whether or not they support protobuf/msgpack for
+// serialising some of their entities (typically ObjectSpec) to protobuf.
+//
+// If absent from the SetDescription, it means only gob is supported, and both
+// Packer and the plugins should use that for communication.
+const ProtocolVersion2 = "v2"
+
 // SetDescription describes a Set.
 type SetDescription struct {
-	Version        string   `json:"version"`
-	SDKVersion     string   `json:"sdk_version"`
-	APIVersion     string   `json:"api_version"`
-	Builders       []string `json:"builders"`
-	PostProcessors []string `json:"post_processors"`
-	Provisioners   []string `json:"provisioners"`
-	Datasources    []string `json:"datasources"`
+	Version         string   `json:"version"`
+	SDKVersion      string   `json:"sdk_version"`
+	APIVersion      string   `json:"api_version"`
+	Builders        []string `json:"builders"`
+	PostProcessors  []string `json:"post_processors"`
+	Provisioners    []string `json:"provisioners"`
+	Datasources     []string `json:"datasources"`
+	ProtocolVersion string   `json:"protocol_version"`
 }
 
 ////
@@ -106,11 +116,28 @@ func (i *Set) Run() error {
 	return i.RunCommand(args...)
 }
 
+// parseProtobufFlag walks over the args to find if `--protobuf` is set.
+//
+// It then returns the args without it for the commands to process them.
+func (i *Set) parseProtobufFlag(args ...string) []string {
+	parsedArgs := make([]string, 0, len(args))
+	for _, arg := range args {
+		if arg == "--protobuf" {
+			i.useProto = true
+			continue
+		}
+		parsedArgs = append(parsedArgs, arg)
+	}
+	return parsedArgs
+}
+
 func (i *Set) RunCommand(args ...string) error {
 	if len(args) < 1 {
 		return fmt.Errorf("needs at least one argument")
 	}
 
+	args = i.parseProtobufFlag(args...)
+
 	switch args[0] {
 	case "describe":
 		return i.jsonDescribe(os.Stdout)
@@ -130,6 +157,7 @@ func (i *Set) start(kind, name string) error {
 	if err != nil {
 		return err
 	}
+	server.UseProto = i.useProto
 
 	log.Printf("[TRACE] starting %s %s", kind, name)
 
@@ -158,13 +186,14 @@ func (i *Set) start(kind, name string) error {
 
 func (i *Set) description() SetDescription {
 	return SetDescription{
-		Version:        i.version,
-		SDKVersion:     i.sdkVersion,
-		APIVersion:     i.apiVersion,
-		Builders:       i.buildersDescription(),
-		PostProcessors: i.postProcessorsDescription(),
-		Provisioners:   i.provisionersDescription(),
-		Datasources:    i.datasourceDescription(),
+		Version:         i.version,
+		SDKVersion:      i.sdkVersion,
+		APIVersion:      i.apiVersion,
+		Builders:        i.buildersDescription(),
+		PostProcessors:  i.postProcessorsDescription(),
+		Provisioners:    i.provisionersDescription(),
+		Datasources:     i.datasourceDescription(),
+		ProtocolVersion: ProtocolVersion2,
 	}
 }
 

plugin/set_test.go

@@ -52,13 +52,14 @@ func TestSet(t *testing.T) {
 
 	sdkVersion := pluginVersion.NewPluginVersion(pluginVersion.Version, pluginVersion.VersionPrerelease, "")
 	if diff := cmp.Diff(SetDescription{
-		Version:        "1.1.1",
-		SDKVersion:     sdkVersion.String(),
-		APIVersion:     "x" + APIVersionMajor + "." + APIVersionMinor,
-		Builders:       []string{"example", "example-2"},
-		PostProcessors: []string{"example", "example-2"},
-		Provisioners:   []string{"example", "example-2"},
-		Datasources:    []string{"example", "example-2"},
+		Version:         "1.1.1",
+		SDKVersion:      sdkVersion.String(),
+		APIVersion:      "x" + APIVersionMajor + "." + APIVersionMinor,
+		Builders:        []string{"example", "example-2"},
+		PostProcessors:  []string{"example", "example-2"},
+		Provisioners:    []string{"example", "example-2"},
+		Datasources:     []string{"example", "example-2"},
+		ProtocolVersion: ProtocolVersion2,
 	}, outputDesc); diff != "" {
 		t.Fatalf("Unexpected description: %s", diff)
 	}
@@ -68,3 +69,58 @@ func TestSet(t *testing.T) {
 		t.Fatalf("Unexpected error: %s", diff)
 	}
 }
+
+func TestSetProtobufArgParsing(t *testing.T) {
+	testCases := []struct {
+		name     string
+		useProto bool
+		in, out  []string
+	}{
+		{
+			name:     "no --protobuf argument provided",
+			in:       []string{"start", "builder", "example"},
+			out:      []string{"start", "builder", "example"},
+			useProto: false,
+		},
+		{
+			name:     "providing --protobuf as first argument",
+			in:       []string{"--protobuf", "start", "builder", "example"},
+			out:      []string{"start", "builder", "example"},
+			useProto: true,
+		},
+		{
+			name:     "providing --protobuf as last argument",
+			in:       []string{"start", "builder", "example", "--protobuf"},
+			out:      []string{"start", "builder", "example"},
+			useProto: true,
+		},
+		{
+			name:     "providing --protobuf as middle argument",
+			in:       []string{"start", "builder", "--protobuf", "example"},
+			out:      []string{"start", "builder", "example"},
+			useProto: true,
+		},
+		{
+			name:     "providing --protobuf multiple times",
+			in:       []string{"--protobuf", "start", "builder", "--protobuf", "example", "--protobuf"},
+			out:      []string{"start", "builder", "example"},
+			useProto: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			set := NewSet()
+			got := set.parseProtobufFlag(tc.in...)
+
+			if diff := cmp.Diff(got, tc.out); diff != "" {
+				t.Errorf("Unexpected args: %s", diff)
+			}
+
+			if set.useProto != tc.useProto {
+				t.Errorf("expected useProto to be %t when %s but got %t", tc.useProto, tc.name, set.useProto)
+			}
+		})
+
+	}
+}

rpc/client.go

@@ -21,6 +21,9 @@ type Client struct {
 	mux      *muxBroker
 	client   *rpc.Client
 	closeMux bool
+	// UseProto makes it so that clients started from this will use
+	// protobuf/msgpack for serialisation instead of gob
+	UseProto bool
 }
 
 func NewClient(rwc io.ReadWriteCloser) (*Client, error) {
@@ -76,6 +79,13 @@ func (c *Client) Artifact() packer.Artifact {
 		commonClient: commonClient{
 			endpoint: DefaultArtifactEndpoint,
 			client:   c.client,
+			// Setting useProto to false is essentially a noop for
+			// this type of client since they don't exchange cty
+			// values, and there's no HCLSpec object tied to this.
+			//
+			// For documentation purposes though, we keep it visible
+			// in order to change this later if it becomes relevant.
+			useProto: false,
 		},
 	}
 }
@@ -86,6 +96,13 @@ func (c *Client) Build() packer.Build {
 			endpoint: DefaultBuildEndpoint,
 			client:   c.client,
 			mux:      c.mux,
+			// Setting useProto to false is essentially a noop for
+			// this type of client since they don't exchange cty
+			// values, and there's no HCLSpec object tied to this.
+			//
+			// For documentation purposes though, we keep it visible
+			// in order to change this later if it becomes relevant.
+			useProto: false,
 		},
 	}
 }
@@ -96,6 +113,7 @@ func (c *Client) Builder() packer.Builder {
 			endpoint: DefaultBuilderEndpoint,
 			client:   c.client,
 			mux:      c.mux,
+			useProto: c.UseProto,
 		},
 	}
 }
@@ -106,6 +124,13 @@ func (c *Client) Communicator() packer.Communicator {
 			endpoint: DefaultCommunicatorEndpoint,
 			client:   c.client,
 			mux:      c.mux,
+			// Setting useProto to false is essentially a noop for
+			// this type of client since they don't exchange cty
+			// values, and there's no HCLSpec object tied to this.
+			//
+			// For documentation purposes though, we keep it visible
+			// in order to change this later if it becomes relevant.
+			useProto: false,
 		},
 	}
 }
@@ -116,6 +141,13 @@ func (c *Client) Hook() packer.Hook {
 			endpoint: DefaultHookEndpoint,
 			client:   c.client,
 			mux:      c.mux,
+			// Setting useProto to false is essentially a noop for
+			// this type of client since they don't exchange cty
+			// values, and there's no HCLSpec object tied to this.
+			//
+			// For documentation purposes though, we keep it visible
+			// in order to change this later if it becomes relevant.
+			useProto: false,
 		},
 	}
 }
@@ -126,6 +158,7 @@ func (c *Client) PostProcessor() packer.PostProcessor {
 			endpoint: DefaultPostProcessorEndpoint,
 			client:   c.client,
 			mux:      c.mux,
+			useProto: c.UseProto,
 		},
 	}
 }
@@ -136,6 +169,7 @@ func (c *Client) Provisioner() packer.Provisioner {
 			endpoint: DefaultProvisionerEndpoint,
 			client:   c.client,
 			mux:      c.mux,
+			useProto: c.UseProto,
 		},
 	}
 }
@@ -146,6 +180,7 @@ func (c *Client) Datasource() packer.Datasource {
 			endpoint: DefaultDatasourceEndpoint,
 			client:   c.client,
 			mux:      c.mux,
+			useProto: c.UseProto,
 		},
 	}
 }
@@ -155,6 +190,13 @@ func (c *Client) Ui() packer.Ui {
 		commonClient: commonClient{
 			endpoint: DefaultUiEndpoint,
 			client:   c.client,
+			// Setting useProto to false is essentially a noop for
+			// this type of client since they don't exchange cty
+			// values, and there's no HCLSpec object tied to this.
+			//
+			// For documentation purposes though, we keep it visible
+			// in order to change this later if it becomes relevant.
+			useProto: false,
 		},
 		endpoint: DefaultUiEndpoint,
 	}