From 10a40a15d964902ad2b678a166bc19db2a7bf074 Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Thu, 5 Dec 2019 02:25:51 +0300 Subject: [PATCH] fix: extract errors from API response This PR only touches `Version` method, but I will expand it to other methods in the next PR. When proxying to many upstreams, errors are wrapped as responses as we can't return error and response from grpc call. Reflect-based function was introduced to filter out responses which contain errors as multierror. Reflection was used, as each response is a different Go type, and we can't write a generic function for it. osctl was updated to support having both resp & err not nil. One failed response shouldn't result in error. Re-enabled integration test for multiple targets and version consistency, need e2e validation. Signed-off-by: Andrey Smirnov --- cmd/osctl/cmd/version.go | 5 +- cmd/osctl/pkg/client/client.go | 4 + cmd/osctl/pkg/client/client_test.go | 14 --- cmd/osctl/pkg/client/reply.go | 127 ++++++++++++++++++++++++++++ cmd/osctl/pkg/client/reply_test.go | 91 ++++++++++++++++++++ cmd/osctl/pkg/helpers/cli.go | 9 ++ internal/integration/api/version.go | 2 - 7 files changed, 235 insertions(+), 17 deletions(-) delete mode 100644 cmd/osctl/pkg/client/client_test.go create mode 100644 cmd/osctl/pkg/client/reply.go create mode 100644 cmd/osctl/pkg/client/reply_test.go diff --git a/cmd/osctl/cmd/version.go b/cmd/osctl/cmd/version.go index b50d43bdf..4774fe8fd 100644 --- a/cmd/osctl/cmd/version.go +++ b/cmd/osctl/cmd/version.go @@ -51,7 +51,10 @@ var versionCmd = &cobra.Command{ reply, err := c.Version(globalCtx, grpc.Peer(&remotePeer)) if err != nil { - helpers.Fatalf("error getting version: %s", err) + if reply == nil { + helpers.Fatalf("error getting version: %s", err) + } + helpers.Warning("%s", err) } defaultNode := addrFromPeer(&remotePeer) diff --git a/cmd/osctl/pkg/client/client.go b/cmd/osctl/pkg/client/client.go index af50093f0..325ee7c64 100644 --- a/cmd/osctl/pkg/client/client.go +++ b/cmd/osctl/pkg/client/client.go @@ -286,6 +286,10 @@ func (c *Client) Version(ctx context.Context, callOptions ...grpc.CallOption) (r callOptions..., ) + var filtered interface{} + filtered, err = FilterReply(reply, err) + reply, _ = filtered.(*machineapi.VersionReply) //nolint: errcheck + return } diff --git a/cmd/osctl/pkg/client/client_test.go b/cmd/osctl/pkg/client/client_test.go deleted file mode 100644 index 125f4a58b..000000000 --- a/cmd/osctl/pkg/client/client_test.go +++ /dev/null @@ -1,14 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at http://mozilla.org/MPL/2.0/. - -package client_test - -import "testing" - -func TestEmpty(t *testing.T) { - // added for accurate coverage estimation - // - // please remove it once any unit-test is added - // for this package -} diff --git a/cmd/osctl/pkg/client/reply.go b/cmd/osctl/pkg/client/reply.go new file mode 100644 index 000000000..55e37984c --- /dev/null +++ b/cmd/osctl/pkg/client/reply.go @@ -0,0 +1,127 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +package client + +import ( + "fmt" + "reflect" + + "github.com/hashicorp/go-multierror" +) + +// NodeError is RPC error from some node +type NodeError struct { + Node string + Err string +} + +func (ne *NodeError) Error() string { + return fmt.Sprintf("%s: %s", ne.Node, ne.Err) +} + +// FilterReply removes error responses from reply and builds multierror. +// +//nolint: gocyclo +func FilterReply(reply interface{}, err error) (interface{}, error) { + if reply == nil { + return nil, err + } + + replyStructPtr := reflect.ValueOf(reply) + if replyStructPtr.Kind() != reflect.Ptr { + panic("reply should be pointer to struct") + } + + replyStruct := replyStructPtr.Elem() + if replyStruct.Kind() != reflect.Struct { + panic("reply should be struct") + } + + responseField := replyStruct.FieldByName("Response") + if !responseField.IsValid() { + panic("Response field missing") + } + + if responseField.Kind() != reflect.Slice { + panic("Response field should be a slice") + } + + var multiErr *multierror.Error + + for i := 0; i < responseField.Len(); { + responsePtr := responseField.Index(i) + if responsePtr.Kind() != reflect.Ptr { + panic("response slice should container pointers") + } + + response := responsePtr.Elem() + if response.Kind() != reflect.Struct { + panic("response slice should container pointers to structs") + } + + metadataField := response.FieldByName("Metadata") + if !metadataField.IsValid() { + panic("response metadata field missing") + } + + if metadataField.Kind() != reflect.Ptr { + panic("response metadata field should be a pointer") + } + + if metadataField.IsNil() { + // missing metadata, skip the field + i++ + continue + } + + metadata := metadataField.Elem() + if metadata.Kind() != reflect.Struct { + panic("response metadata should be struct") + } + + errorField := metadata.FieldByName("Error") + if !errorField.IsValid() { + panic("metadata.Error field missing") + } + + if errorField.Kind() != reflect.String { + panic("metadata.Error should be string") + } + + if errorField.IsZero() { + // no error, leave it as is + i++ + continue + } + + hostnameField := metadata.FieldByName("Hostname") + if !hostnameField.IsValid() { + panic("metadata.Hostname field missing") + } + + if hostnameField.Kind() != reflect.String { + panic("metadata.Hostname should be string") + } + + // extract error + nodeError := &NodeError{ + Node: hostnameField.String(), + Err: errorField.String(), + } + + multiErr = multierror.Append(multiErr, nodeError) + + // remove ith response + reflect.Copy(responseField.Slice(i, responseField.Len()), responseField.Slice(i+1, responseField.Len())) + responseField.SetLen(responseField.Len() - 1) + } + + // if all the responses were error responses... + if multiErr != nil && responseField.Len() == 0 { + reply = nil + } + + return reply, multiErr.ErrorOrNil() +} diff --git a/cmd/osctl/pkg/client/reply_test.go b/cmd/osctl/pkg/client/reply_test.go new file mode 100644 index 000000000..ce0ef4ce0 --- /dev/null +++ b/cmd/osctl/pkg/client/reply_test.go @@ -0,0 +1,91 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +package client_test + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/talos-systems/talos/api/common" + "github.com/talos-systems/talos/cmd/osctl/pkg/client" +) + +func TestFilterResponse(t *testing.T) { + reply := &common.DataReply{ + Response: []*common.DataResponse{ + { + Metadata: &common.ResponseMetadata{ + Hostname: "host1", + }, + Bytes: []byte("abc"), + }, + { + Metadata: &common.ResponseMetadata{ + Hostname: "host2", + Error: "something wrong", + }, + }, + { + Bytes: []byte("def"), + }, + { + Metadata: &common.ResponseMetadata{ + Hostname: "host4", + Error: "even more wrong", + }, + }, + }, + } + + filtered, err := client.FilterReply(reply, nil) + assert.EqualError(t, err, "2 errors occurred:\n\t* host2: something wrong\n\t* host4: even more wrong\n\n") + assert.Equal(t, filtered, + &common.DataReply{ + Response: []*common.DataResponse{ + { + Metadata: &common.ResponseMetadata{ + Hostname: "host1", + }, + Bytes: []byte("abc"), + }, + { + Bytes: []byte("def"), + }, + }, + }) +} + +func TestFilterResponseNil(t *testing.T) { + e := errors.New("wrong") + + filtered, err := client.FilterReply(nil, e) + assert.Nil(t, filtered) + assert.Equal(t, e, err) +} + +func TestFilterResponseOnlyErrors(t *testing.T) { + reply := &common.DataReply{ + Response: []*common.DataResponse{ + { + Metadata: &common.ResponseMetadata{ + Hostname: "host2", + Error: "something wrong", + }, + }, + { + Metadata: &common.ResponseMetadata{ + Hostname: "host4", + Error: "even more wrong", + }, + }, + }, + } + + filtered, err := client.FilterReply(reply, nil) + assert.EqualError(t, err, "2 errors occurred:\n\t* host2: something wrong\n\t* host4: even more wrong\n\n") + assert.Nil(t, filtered) +} diff --git a/cmd/osctl/pkg/helpers/cli.go b/cmd/osctl/pkg/helpers/cli.go index 040a9d06d..33948f006 100644 --- a/cmd/osctl/pkg/helpers/cli.go +++ b/cmd/osctl/pkg/helpers/cli.go @@ -19,3 +19,12 @@ func Fatalf(message string, args ...interface{}) { fmt.Fprintf(os.Stderr, message, args...) os.Exit(1) } + +// Warning prints formatted message to stderr +func Warning(message string, args ...interface{}) { + if !strings.HasSuffix(message, "\n") { + message += "\n" + } + + fmt.Fprintf(os.Stderr, "WARNING: "+message, args...) +} diff --git a/internal/integration/api/version.go b/internal/integration/api/version.go index dc35a5bc5..c07682f40 100644 --- a/internal/integration/api/version.go +++ b/internal/integration/api/version.go @@ -33,8 +33,6 @@ func (suite *VersionSuite) TestExpectedVersionMaster() { // TestSameVersionCluster verifies that all the nodes are on the same version func (suite *VersionSuite) TestSameVersionCluster() { - suite.T().Skip("disabled test as it frequently fails on e2e (less responses than expected)") - nodes := suite.DiscoverNodes() suite.Require().NotEmpty(nodes)