Fix converting from GCP gRPC errors (#41970)

gravitational · May 23, 2024 · b5f46c5 · b5f46c5
1 parent 05000b9
commit b5f46c5
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 3 deletions.
diff --git a/lib/cloud/gcp/vm.go b/lib/cloud/gcp/vm.go
@@ -34,7 +34,9 @@ import (
 	"cloud.google.com/go/compute/apiv1/computepb"
 	resourcemanager "cloud.google.com/go/resourcemanager/apiv3"
 	"cloud.google.com/go/resourcemanager/apiv3/resourcemanagerpb"
+	"github.com/googleapis/gax-go/v2/apierror"
 	"github.com/gravitational/trace"
+	"github.com/gravitational/trace/trail"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/crypto/ssh"
 	"google.golang.org/api/googleapi"
@@ -59,6 +61,14 @@ func convertAPIError(err error) error {
 	if errors.As(err, &googleError) {
 		return trace.ReadError(googleError.Code, []byte(googleError.Message))
 	}
+	var apiError *apierror.APIError
+	if errors.As(err, &apiError) {
+		if code := apiError.HTTPCode(); code != -1 {
+			return trace.ReadError(code, []byte(apiError.Reason()))
+		} else if apiError.GRPCStatus() != nil {
+			return trail.FromGRPC(apiError)
+		}
+	}
 	return err
 }
 
@@ -341,7 +351,8 @@ func (clt *instancesClient) getTagBindingsClient(ctx context.Context, zone strin
 		endpoint := zone + "-cloudresourcemanager.googleapis.com:443"
 		opts = append(opts, option.WithEndpoint(endpoint))
 	}
-	return resourcemanager.NewTagBindingsClient(ctx, opts...)
+	client, err := resourcemanager.NewTagBindingsClient(ctx, opts...)
+	return client, trace.Wrap(convertAPIError(err))
 }
 
 // GetInstanceTags gets the GCP tags for the instance.

diff --git a/lib/cloud/gcp/vm_test.go b/lib/cloud/gcp/vm_test.go
@@ -22,20 +22,68 @@ import (
 	"bytes"
 	"context"
 	"net"
+	"net/http"
 	"sync/atomic"
 	"testing"
 	"time"
 
 	"cloud.google.com/go/compute/apiv1/computepb"
+	"github.com/googleapis/gax-go/v2/apierror"
 	"github.com/gravitational/trace"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/crypto/ssh"
+	"google.golang.org/api/googleapi"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 
 	"github.com/gravitational/teleport/api/internalutils/stream"
 	"github.com/gravitational/teleport/lib/sshutils"
 	"github.com/gravitational/teleport/lib/utils"
 )
 
+func TestConvertAPIError(t *testing.T) {
+	t.Parallel()
+	fromError := func(t *testing.T, err error) error {
+		outErr, ok := apierror.FromError(err)
+		require.True(t, ok)
+		return outErr
+	}
+	tests := []struct {
+		name string
+		err  error
+	}{
+		{
+			name: "google error",
+			err: &googleapi.Error{
+				Code:    http.StatusForbidden,
+				Message: "abcd1234",
+			},
+		},
+		{
+			name: "api http error",
+			err: fromError(t, &googleapi.Error{
+				Code:    http.StatusForbidden,
+				Message: "abcd1234",
+			}),
+		},
+		{
+			name: "api grpc error",
+			err:  fromError(t, status.Error(codes.PermissionDenied, "abcd1234")),
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			require.True(t, trace.IsAccessDenied(convertAPIError(tc.err)))
+			require.Contains(t, tc.err.Error(), "abcd1234")
+		})
+	}
+	t.Run("not a gcp error", func(t *testing.T) {
+		inErr := trace.Errorf("abcd1234")
+		outErr := convertAPIError(inErr)
+		require.Same(t, inErr, outErr)
+	})
+}
+
 type mockInstance struct {
 	hostKeys      []ssh.PublicKey
 	authorizedKey ssh.PublicKey

diff --git a/lib/cloud/imds/gcp/imds.go b/lib/cloud/imds/gcp/imds.go
@@ -119,7 +119,7 @@ func (client *InstanceMetadataClient) GetTags(ctx context.Context) (map[string]s
 		gcpLabels = inst.Labels
 	} else if trace.IsAccessDenied(err) {
 		client.labelPermissionErrorOnce.Do(func() {
-			slog.WarnContext(ctx, "Access denied to instance labels, does the instance have compute.instances.get permission?", "error", err)
+			slog.WarnContext(ctx, "Access denied to instance labels, does the instance have compute.instances.get permission?")
 		})
 	} else {
 		return nil, trace.Wrap(err)
@@ -129,7 +129,7 @@ func (client *InstanceMetadataClient) GetTags(ctx context.Context) (map[string]s
 	gcpTags, err := client.instancesClient.GetInstanceTags(ctx, req)
 	if trace.IsAccessDenied(err) {
 		client.tagPermissionErrorOnce.Do(func() {
-			slog.WarnContext(ctx, "Access denied to resource management tags, does the instance have compute.instances.listEffectiveTags permission?", "error", err)
+			slog.WarnContext(ctx, "Access denied to resource management tags, does the instance have compute.instances.listEffectiveTags permission?")
 		})
 	} else if err != nil {
 		return nil, trace.Wrap(err)