remote asset: reject unsupported Qualifiers

The specification requires that any `Fetch` requests containing
unsupported qualifiers are rejected with an `INVALID_ARGUMENT`
rpc error:
 - https://github.com/bazelbuild/remote-apis/blob/7f922028fcfac63bdd8431e68de152d9e7a9e2a0/build/bazel/remote/asset/v1/remote_asset.proto#L109-L112
 - https://github.com/bazelbuild/remote-apis/blob/7f922028fcfac63bdd8431e68de152d9e7a9e2a0/build/bazel/remote/asset/v1/remote_asset.proto#L127-L128
Additionally, the status returned should include a `BadRequest`
error detail with `FieldViolation`s indicating the names of
unsupported qualifiers:
- https://github.com/bazelbuild/remote-apis/blob/7f922028fcfac63bdd8431e68de152d9e7a9e2a0/build/bazel/remote/asset/v1/remote_asset.proto#L139-L143

Author: malt3

server/BUILD.bazel

@@ -31,6 +31,7 @@ go_library(
         "@com_github_mostynb_zstdpool_syncpool//:go_default_library",
         "@org_golang_google_genproto_googleapis_bytestream//:go_default_library",
         "@org_golang_google_genproto_googleapis_rpc//code:go_default_library",
+        "@org_golang_google_genproto_googleapis_rpc//errdetails:go_default_library",
         "@org_golang_google_genproto_googleapis_rpc//status:go_default_library",
         "@org_golang_google_grpc//:go_default_library",
         "@org_golang_google_grpc//codes:go_default_library",
@@ -64,6 +65,7 @@ go_test(
         "@com_github_google_uuid//:go_default_library",
         "@com_github_klauspost_compress//zstd:go_default_library",
         "@org_golang_google_genproto_googleapis_bytestream//:go_default_library",
+        "@org_golang_google_genproto_googleapis_rpc//errdetails:go_default_library",
         "@org_golang_google_grpc//:go_default_library",
         "@org_golang_google_grpc//codes:go_default_library",
         "@org_golang_google_grpc//credentials/insecure:go_default_library",

server/grpc_asset.go

@@ -12,6 +12,7 @@ import (
 	"net/url"
 	"strings"
 
+	"google.golang.org/genproto/googleapis/rpc/errdetails"
 	"google.golang.org/genproto/googleapis/rpc/status"
 	"google.golang.org/grpc/codes"
 	grpc_status "google.golang.org/grpc/status"
@@ -64,6 +65,7 @@ func (s *grpcServer) FetchBlob(ctx context.Context, req *asset.FetchBlobRequest)
 
 	headers := http.Header{}
 
+	var unsupportedQualifierNames []string
 	for _, q := range req.GetQualifiers() {
 		if q == nil {
 			return nil, errNilQualifier
@@ -93,12 +95,17 @@ func (s *grpcServer) FetchBlob(ctx context.Context, req *asset.FetchBlobRequest)
 			}
 
 			sha256Str = hex.EncodeToString(decoded)
+			continue
+		}
 
-			found, size := s.cache.Contains(ctx, cache.CAS, sha256Str, -1)
-			if !found {
-				continue
-			}
+		unsupportedQualifierNames = append(unsupportedQualifierNames, q.Name)
+	}
+	if len(unsupportedQualifierNames) > 0 {
+		return nil, s.unsupportedQualifiersErrStatus(unsupportedQualifierNames)
+	}
 
+	if len(sha256Str) != 0 {
+		if found, size := s.cache.Contains(ctx, cache.CAS, sha256Str, -1); found {
 			if size < 0 {
 				// We don't know the size yet (bad http backend?).
 				r, actualSize, err := s.cache.Get(ctx, cache.CAS, sha256Str, -1, 0)
@@ -108,7 +115,8 @@ func (s *grpcServer) FetchBlob(ctx context.Context, req *asset.FetchBlobRequest)
 				if err != nil || actualSize < 0 {
 					s.errorLogger.Printf("failed to get CAS %s from proxy backend size: %d err: %v",
 						sha256Str, actualSize, err)
-					continue
+					return nil, grpc_status.Error(codes.Internal, fmt.Sprintf("failed to get CAS %s from proxy backend size: %d err: %v",
+						sha256Str, actualSize, err))
 				}
 				size = actualSize
 			}
@@ -214,6 +222,25 @@ func (s *grpcServer) fetchItem(ctx context.Context, uri string, headers http.Hea
 	return true, expectedHash, expectedSize
 }
 
+// unsupportedQualifiersErrStatus creates a gRPC status error that includes a list of unsupported qualifiers.
+func (s *grpcServer) unsupportedQualifiersErrStatus(qualifierNames []string) error {
+	fieldViolations := make([]*errdetails.BadRequest_FieldViolation, 0, len(qualifierNames))
+	for _, name := range qualifierNames {
+		fieldViolations = append(fieldViolations, &errdetails.BadRequest_FieldViolation{
+			Field:       "qualifiers.name",
+			Description: fmt.Sprintf("%q not supported", name),
+		})
+	}
+	statusWithoutDetails := grpc_status.New(codes.InvalidArgument, fmt.Sprintf("Unsupported qualifiers: %s", strings.Join(qualifierNames, ", ")))
+	statusWithDetails, err := statusWithoutDetails.WithDetails(&errdetails.BadRequest{FieldViolations: fieldViolations})
+	// should never happen
+	if err != nil {
+		s.errorLogger.Printf("failed to add details to status: %v", err)
+		return statusWithoutDetails.Err()
+	}
+	return statusWithDetails.Err()
+}
+
 func (s *grpcServer) FetchDirectory(context.Context, *asset.FetchDirectoryRequest) (*asset.FetchDirectoryResponse, error) {
 	return nil, nil
 }

server/grpc_asset_test.go

@@ -11,9 +11,11 @@ import (
 	"testing"
 
 	asset "github.com/buchgr/bazel-remote/v2/genproto/build/bazel/remote/asset/v1"
-	//pb "github.com/buchgr/bazel-remote/v2/genproto/build/bazel/remote/execution/v2"
 
+	"google.golang.org/genproto/googleapis/rpc/errdetails"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/proto"
 
 	testutils "github.com/buchgr/bazel-remote/v2/utils"
 )
@@ -63,6 +65,57 @@ func TestAssetFetchBlob(t *testing.T) {
 	}
 }
 
+func TestAssetUnsupportedQualifier(t *testing.T) {
+	t.Parallel()
+
+	fixture := grpcTestSetup(t)
+	defer os.Remove(fixture.tempdir)
+
+	ts := newTestGetServer()
+
+	req := asset.FetchBlobRequest{
+		Uris: []string{
+			ts.srv.URL + "/" + ts.path, // This URL should work.
+		},
+		Qualifiers: []*asset.Qualifier{
+			{
+				Name:  "unknown-qualifier",
+				Value: "some-value",
+			},
+		},
+	}
+
+	_, err := fixture.assetClient.FetchBlob(ctx, &req)
+	if err == nil {
+		t.Fatal(err, "expected rpc error from fetch")
+	}
+	gstatus, ok := status.FromError(err)
+	if !ok {
+		t.Fatal("expected a status in rpc error")
+	}
+	if gstatus.Code() != codes.InvalidArgument {
+		t.Fatalf("expected %v status code, got %v", codes.InvalidArgument, gstatus.Code())
+	}
+	if len(gstatus.Details()) != 1 {
+		t.Fatal("expected one detail in rpc status")
+	}
+	expectedDetail := &errdetails.BadRequest{
+		FieldViolations: []*errdetails.BadRequest_FieldViolation{
+			{
+				Field:       "qualifiers.name",
+				Description: `"unknown-qualifier" not supported`,
+			},
+		},
+	}
+	protoDetail, ok := gstatus.Details()[0].(*errdetails.BadRequest)
+	if !ok {
+		t.Fatal("expected BadRequest detail in rpc status")
+	}
+	if !proto.Equal(protoDetail, expectedDetail) {
+		t.Fatalf("expected %v BadRequest, got %v", expectedDetail, protoDetail)
+	}
+}
+
 type testGetServer struct {
 	srv *httptest.Server