fix: correctly transport gRPC errors from apid

Before these changes, errors were always sent as strings, so if original
error was gRPC error (which is almost always the case for apid), it is
formatted as string and original fields (like code) are lost in the
formatted string.

With this change, apid sends errors as official `grpc.Status` protobuf
structure, and client decodes that into Go grpc.Status based error.

This change is backwards and forwards compatible.

This should fix more cases when integration tests were not able to
ignore grpc `transport is closing` errors when they were sent as strings
from the apid endpoint.

Signed-off-by: Andrey Smirnov <smirnov.andrey@gmail.com>

Dockerfile

@@ -74,22 +74,23 @@ WORKDIR /src
 
 FROM build AS generate-build
 # Common needs to be at or near the top to satisfy the subsequent imports
+COPY ./api/vendor/ /api/vendor/
 COPY ./api/common/common.proto /api/common/common.proto
-RUN protoc -I/api --go_out=plugins=grpc,paths=source_relative:/api common/common.proto
+RUN protoc -I/api -I/api/vendor/ --go_out=plugins=grpc,paths=source_relative:/api common/common.proto
 COPY ./api/health/health.proto /api/health/health.proto
-RUN protoc -I/api --go_out=plugins=grpc,paths=source_relative:/api health/health.proto
+RUN protoc -I/api -I/api/vendor/ --go_out=plugins=grpc,paths=source_relative:/api health/health.proto
 COPY ./api/security/security.proto /api/security/security.proto
-RUN protoc -I/api --go_out=plugins=grpc,paths=source_relative:/api security/security.proto
+RUN protoc -I/api -I/api/vendor/ --go_out=plugins=grpc,paths=source_relative:/api security/security.proto
 COPY ./api/storage/storage.proto /api/storage/storage.proto
-RUN protoc -I/api --go_out=plugins=grpc,paths=source_relative:/api storage/storage.proto
+RUN protoc -I/api -I/api/vendor/ --go_out=plugins=grpc,paths=source_relative:/api storage/storage.proto
 COPY ./api/machine/machine.proto /api/machine/machine.proto
-RUN protoc -I/api --go_out=plugins=grpc,paths=source_relative:/api machine/machine.proto
+RUN protoc -I/api -I/api/vendor/ --go_out=plugins=grpc,paths=source_relative:/api machine/machine.proto
 COPY ./api/time/time.proto /api/time/time.proto
-RUN protoc -I/api --go_out=plugins=grpc,paths=source_relative:/api time/time.proto
+RUN protoc -I/api -I/api/vendor/ --go_out=plugins=grpc,paths=source_relative:/api time/time.proto
 COPY ./api/network/network.proto /api/network/network.proto
-RUN protoc -I/api --go_out=plugins=grpc,paths=source_relative:/api network/network.proto
+RUN protoc -I/api -I/api/vendor/ --go_out=plugins=grpc,paths=source_relative:/api network/network.proto
 COPY ./api/cluster/cluster.proto /api/cluster/cluster.proto
-RUN protoc -I/api --go_out=plugins=grpc,paths=source_relative:/api cluster/cluster.proto
+RUN protoc -I/api -I/api/vendor/ --go_out=plugins=grpc,paths=source_relative:/api cluster/cluster.proto
 # Gofumports generated files to adjust import order
 RUN gofumports -w -local github.com/talos-systems/talos /api/
 
@@ -646,8 +647,10 @@ RUN protoc \
     -I/protos/security \
     -I/protos/storage \
     -I/protos/time \
+    -I/protos/vendor \
     --doc_opt=/tmp/markdown.tmpl,api.md \
     --doc_out=/tmp \
+    /protos/common/*.proto \
     /protos/health/*.proto \
     /protos/machine/*.proto \
     /protos/network/*.proto \

api/common/common.proto

@@ -5,6 +5,7 @@ package common;
 option go_package = "github.com/talos-systems/talos/pkg/machinery/api/common";
 
 import "google/protobuf/any.proto";
+import "google/rpc/status.proto";
 
 enum Code {
   FATAL = 0;
@@ -24,6 +25,8 @@ message Metadata {
   // error is set if request failed to the upstream (rest of response is
   // undefined)
   string error = 2;
+  // error as gRPC Status
+  google.rpc.Status status = 3;
 }
 
 message Data {

api/vendor/google/rpc/status.proto

@@ -0,0 +1,47 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+syntax = "proto3";
+
+package google.rpc;
+
+import "google/protobuf/any.proto";
+
+option cc_enable_arenas = true;
+option go_package = "google.golang.org/genproto/googleapis/rpc/status;status";
+option java_multiple_files = true;
+option java_outer_classname = "StatusProto";
+option java_package = "com.google.rpc";
+option objc_class_prefix = "RPC";
+
+// The `Status` type defines a logical error model that is suitable for
+// different programming environments, including REST APIs and RPC APIs. It is
+// used by [gRPC](https://github.com/grpc). Each `Status` message contains
+// three pieces of data: error code, error message, and error details.
+//
+// You can find out more about this error model and how to work with it in the
+// [API Design Guide](https://cloud.google.com/apis/design/errors).
+message Status {
+  // The status code, which should be an enum value of [google.rpc.Code][google.rpc.Code].
+  int32 code = 1;
+
+  // A developer-facing error message, which should be in English. Any
+  // user-facing error message should be localized and sent in the
+  // [google.rpc.Status.details][google.rpc.Status.details] field, or localized by the client.
+  string message = 2;
+
+  // A list of messages that carry the error details.  There is a common set of
+  // message types for APIs to use.
+  repeated google.protobuf.Any details = 3;
+}

internal/app/apid/pkg/backend/apid.go

@@ -14,6 +14,7 @@ import (
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials"
 	"google.golang.org/grpc/metadata"
+	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/encoding/protowire"
 	"google.golang.org/protobuf/proto"
 
@@ -199,6 +200,7 @@ func (a *APID) BuildError(streaming bool, err error) ([]byte, error) {
 		Metadata: &common.Metadata{
 			Hostname: a.target,
 			Error:    err.Error(),
+			Status:   status.Convert(err).Proto(),
 		},
 	}
 

pkg/machinery/client/reply.go

@@ -5,22 +5,30 @@
 package client
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 
 	"github.com/hashicorp/go-multierror"
+	rpcstatus "google.golang.org/genproto/googleapis/rpc/status"
+	"google.golang.org/grpc/status"
 )
 
 // NodeError is RPC error from some node.
 type NodeError struct {
 	Node string
-	Err  string
+	Err  error
 }
 
 func (ne *NodeError) Error() string {
 	return fmt.Sprintf("%s: %s", ne.Node, ne.Err)
 }
 
+// Unwrap implements errors.Unwrap interface.
+func (ne *NodeError) Unwrap() error {
+	return ne.Err
+}
+
 // FilterMessages removes error Messagess from resp and builds multierror.
 //
 //nolint: gocyclo
@@ -102,6 +110,26 @@ func FilterMessages(resp interface{}, err error) (interface{}, error) {
 			continue
 		}
 
+		rpcError := errors.New(errorField.String())
+
+		statusField := metadata.FieldByName("Status")
+		if !statusField.IsValid() {
+			panic("metadata.Status field missing")
+		}
+
+		if statusField.Kind() != reflect.Ptr {
+			panic("metadata.Status should be pointer")
+		}
+
+		if !statusField.IsZero() {
+			statusValue, ok := statusField.Interface().(*rpcstatus.Status)
+			if !ok {
+				panic("metadata.Status should be of type *status.Status")
+			}
+
+			rpcError = status.FromProto(statusValue).Err()
+		}
+
 		hostnameField := metadata.FieldByName("Hostname")
 		if !hostnameField.IsValid() {
 			panic("metadata.Hostname field missing")
@@ -114,7 +142,7 @@ func FilterMessages(resp interface{}, err error) (interface{}, error) {
 		// extract error
 		nodeError := &NodeError{
 			Node: hostnameField.String(),
-			Err:  errorField.String(),
+			Err:  rpcError,
 		}
 
 		multiErr = multierror.Append(multiErr, nodeError)

pkg/machinery/client/reply_test.go

@@ -9,6 +9,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"google.golang.org/genproto/googleapis/rpc/status"
+	"google.golang.org/grpc/codes"
 
 	"github.com/talos-systems/talos/pkg/machinery/api/common"
 	"github.com/talos-systems/talos/pkg/machinery/client"
@@ -93,3 +95,34 @@ func TestFilterMessagesOnlyErrors(t *testing.T) {
 	assert.EqualError(t, err, "2 errors occurred:\n\t* host2: something wrong\n\t* host4: even more wrong\n\n")
 	assert.Nil(t, filtered)
 }
+
+func TestFilterMessagesGRPCStatus(t *testing.T) {
+	reply := &common.DataResponse{
+		Messages: []*common.Data{
+			{
+				Metadata: &common.Metadata{
+					Hostname: "host2",
+					Error:    "should be ignored",
+					Status: &status.Status{
+						Code:    int32(codes.Aborted),
+						Message: "something aborted",
+					},
+				},
+			},
+			{
+				Metadata: &common.Metadata{
+					Hostname: "host4",
+					Error:    "should be ignored",
+					Status: &status.Status{
+						Code:    int32(codes.Unknown),
+						Message: "something went wrong",
+					},
+				},
+			},
+		},
+	}
+
+	filtered, err := client.FilterMessages(reply, nil)
+	assert.EqualError(t, err, "2 errors occurred:\n\t* host2: rpc error: code = Aborted desc = something aborted\n\t* host4: rpc error: code = Unknown desc = something went wrong\n\n")
+	assert.Nil(t, filtered)
+}

pkg/machinery/go.mod

@@ -21,6 +21,7 @@ require (
 	golang.org/x/net v0.0.0-20200707034311-ab3426394381 // indirect
 	golang.org/x/sys v0.0.0-20200622214017-ed371f2e16b4 // indirect
 	golang.org/x/text v0.3.3 // indirect
+	google.golang.org/genproto v0.0.0-20200526211855-cb27e3aa2013
 	google.golang.org/grpc v1.29.0
 	google.golang.org/protobuf v1.25.0
 	gopkg.in/yaml.v2 v2.2.8 // indirect