[image-builder] Avoid returning "Unknown" (#20854)

* [cline] Drop learning journal (left-over)

* [image-builder-mk3] Avoid sending "Unknown" error code in all cases

* [ws-manager-mk2] image-builder proxy: Prefix proxied gRPC services to avoid name clashes with the original service

Also, avoid sending "Unknown" errors, and wrap it into meaningfull gRPC status codes (+ additional logging)

Author: geropl

.clinerules/learning-journal.md

@@ -260,7 +260,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 				BaseRef: req.BaseImageNameResolved,
 			})
 			if err != nil {
-				return err
+				return handleFailedBuildStreamResponse(err, "cannot send build response")
 			}
 			return nil
 		}
@@ -307,7 +307,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 			BaseRef: baseref,
 		})
 		if err != nil {
-			return err
+			return handleFailedBuildStreamResponse(err, "cannot send build response")
 		}
 		return nil
 	}
@@ -322,11 +322,12 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 
 	randomUUID, err := uuid.NewRandom()
 	if err != nil {
-		return
+		return status.Errorf(codes.Internal, "failed to generate build ID: %v", err)
 	}
+	buildID := randomUUID.String()
+	log := log.WithField("buildID", buildID)
 
 	var (
-		buildID        = randomUUID.String()
 		buildBase      = "false"
 		contextPath    = "."
 		dockerfilePath = "Dockerfile"
@@ -368,7 +369,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 
 	pbaseref, err := reference.ParseNormalizedNamed(baseref)
 	if err != nil {
-		return xerrors.Errorf("cannot parse baseref: %v", err)
+		return status.Errorf(codes.InvalidArgument, "cannot parse baseref: %v", err)
 	}
 	bobBaseref := "localhost:8080/base"
 	if r, ok := pbaseref.(reference.Digested); ok {
@@ -384,7 +385,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 		})
 		additionalAuth, err = json.Marshal(ath)
 		if err != nil {
-			return xerrors.Errorf("cannot marshal additional auth: %w", err)
+			return status.Errorf(codes.InvalidArgument, "cannot marshal additional auth: %v", err)
 		}
 	}
 
@@ -432,7 +433,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 						Name:  "WORKSPACEKIT_BOBPROXY_ADDITIONALAUTH",
 						Value: string(additionalAuth),
 					},
-					{Name: "SUPERVISOR_DEBUG_ENABLE", Value: fmt.Sprintf("%v", log.Log.Logger.IsLevelEnabled(logrus.DebugLevel))},
+					{Name: "SUPERVISOR_DEBUG_ENABLE", Value: fmt.Sprintf("%v", log.Logger.IsLevelEnabled(logrus.DebugLevel))},
 				},
 			},
 			Type: wsmanapi.WorkspaceType_IMAGEBUILD,
@@ -476,8 +477,8 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 
 		err := resp.Send(update)
 		if err != nil {
-			log.WithError(err).Error("cannot forward build update - dropping listener")
-			return status.Errorf(codes.Unknown, "cannot send update: %v", err)
+			log.WithError(err).Info("cannot forward build update - dropping listener")
+			return handleFailedBuildStreamResponse(err, "cannot send update")
 		}
 
 		if update.Status == protocol.BuildStatus_done_failure || update.Status == protocol.BuildStatus_done_success {
@@ -555,8 +556,8 @@ func (o *Orchestrator) Logs(req *protocol.LogsRequest, resp protocol.ImageBuilde
 
 		err := resp.Send(update)
 		if err != nil {
-			log.WithError(err).Error("cannot forward log output - dropping listener")
-			return status.Errorf(codes.Unknown, "cannot send log output: %v", err)
+			log.WithError(err).Info("cannot forward log output - dropping listener")
+			return handleFailedBuildStreamResponse(err, "cannot send log output")
 		}
 	}
 
@@ -709,6 +710,33 @@ func (o *Orchestrator) getWorkspaceImageRef(ctx context.Context, baseref string)
 	return fmt.Sprintf("%s:%x", o.Config.WorkspaceImageRepository, dst), nil
 }
 
+func handleFailedBuildStreamResponse(err error, msg string) error {
+	if err == nil {
+		// OK is OK
+		return nil
+	}
+
+	// If the error is a context.DeadlineExceeded, we return nil (OK) as requested.
+	if errors.Is(err, context.DeadlineExceeded) {
+		// Return nil (OK) for DeadlineExceeded
+		return nil
+	}
+
+	// If it's already a gRPC status error, check for DeadlineExceeded
+	if st, ok := status.FromError(err); ok {
+		if st.Code() == codes.DeadlineExceeded {
+			// Return nil (OK) for DeadlineExceeded as requested
+			return nil
+		}
+
+		log.WithError(err).WithField("code", status.Code(err)).Error(fmt.Sprintf("unexpected error while sending build response: %s", msg))
+		return err
+	}
+
+	log.WithError(err).Error(fmt.Sprintf("unexpected error while sending build response: %s", msg))
+	return status.Errorf(codes.Unavailable, "%s: %v", msg, err)
+}
+
 // parentCantCancelContext is a bit of a hack. We have some operations which we want to keep alive even after clients
 // disconnect. gRPC cancels the context once a client disconnects, thus we intercept the cancelation and act as if
 // nothing had happened.

components/image-builder-mk3/pkg/orchestrator/orchestrator.go

@@ -53,6 +53,10 @@ import (
 	//+kubebuilder:scaffold:imports
 )
 
+// PROXIED_GRPC_SERVICE_PREFIX is the prefix used for proxied gRPC services.
+// It is used to avoid conflicts with the original service names in metrics, and to filter out proxied services in SLIs etc.
+const PROXIED_GRPC_SERVICE_PREFIX = "proxied"
+
 var (
 	// ServiceName is the name we use for tracing/logging
 	ServiceName = "ws-manager-mk2"
@@ -258,9 +262,12 @@ func setupGRPCService(cfg *config.ServiceConfiguration, k8s client.Client, maint
 	grpcMetrics.EnableHandlingTimeHistogram()
 	metrics.Registry.MustRegister(grpcMetrics)
 
+	// Create interceptor for prefixing the proxied gRPC service names, to avoid conflicts with the original service names in metrics
+	grpcServicePrefixer := &imgproxy.ServiceNamePrefixerInterceptor{Prefix: PROXIED_GRPC_SERVICE_PREFIX}
+
 	grpcOpts := common_grpc.ServerOptionsWithInterceptors(
-		[]grpc.StreamServerInterceptor{grpcMetrics.StreamServerInterceptor()},
-		[]grpc.UnaryServerInterceptor{grpcMetrics.UnaryServerInterceptor(), ratelimits.UnaryInterceptor()},
+		[]grpc.StreamServerInterceptor{grpcServicePrefixer.StreamServerInterceptor(), grpcMetrics.StreamServerInterceptor()},
+		[]grpc.UnaryServerInterceptor{grpcServicePrefixer.UnaryServerInterceptor(), grpcMetrics.UnaryServerInterceptor(), ratelimits.UnaryInterceptor()},
 	)
 	if cfg.RPCServer.TLS.CA != "" && cfg.RPCServer.TLS.Certificate != "" && cfg.RPCServer.TLS.PrivateKey != "" {
 		tlsConfig, err := common_grpc.ClientAuthTLSConfig(

components/ws-manager-mk2/main.go

@@ -6,8 +6,14 @@ package proxy
 
 import (
 	"context"
+	"errors"
+	"io"
 
+	"github.com/gitpod-io/gitpod/common-go/log"
 	"github.com/gitpod-io/gitpod/image-builder/api"
+
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
 )
 
@@ -58,7 +64,7 @@ func forwardStream[R ProtoMessage](ctx context.Context, recv func() (R, error),
 	for {
 		resp, err := recv()
 		if err != nil {
-			return err
+			return handleProxyError(err)
 		}
 
 		// generic hack, can't compare R to nil because R's default value is unclear (not even sure this is nil)
@@ -69,9 +75,47 @@ func forwardStream[R ProtoMessage](ctx context.Context, recv func() (R, error),
 		}
 		err = send(resp)
 		if err != nil {
-			return err
+			return handleProxyError(err)
 		}
 	}
 
 	return nil
 }
+
+// handleProxyError ensures all errors have proper gRPC status codes
+func handleProxyError(err error) error {
+	if err == nil {
+		return nil
+	}
+
+	// If it's already a gRPC status error, check for DeadlineExceeded
+	if st, ok := status.FromError(err); ok {
+		if st.Code() == codes.DeadlineExceeded {
+			// Return nil (OK) for DeadlineExceeded as requested
+			return nil
+		}
+
+		log.WithError(err).WithField("code", status.Code(err)).Error("unexpected error while sending stream response upstream")
+		return err
+	}
+
+	// Handle context errors
+	if errors.Is(err, context.DeadlineExceeded) {
+		// Return nil (OK) for DeadlineExceeded
+		return nil
+	}
+
+	if errors.Is(err, io.EOF) {
+		// Return nil (OK) for EOF, which is a normal when the client ends the stream
+		return nil
+	}
+
+	log.WithError(err).Error("unexpected error while sending stream response upstream")
+
+	if errors.Is(err, context.Canceled) {
+		return status.Error(codes.Canceled, err.Error())
+	}
+
+	// Wrap any other error as Internal
+	return status.Error(codes.Internal, err.Error())
+}