diff --git a/.clinerules/learning-journal.md b/.clinerules/learning-journal.md deleted file mode 100644 index 3a7203223da742..00000000000000 --- a/.clinerules/learning-journal.md +++ /dev/null @@ -1,152 +0,0 @@ -# Gitpod Project Intelligence - -This file captures important patterns, preferences, and project intelligence that help me work more effectively with the Gitpod codebase. It serves as a learning journal that will be updated as I discover new insights. - -## Project Structure Patterns - -- **Component Organization**: The project is organized into components, each with its own directory in the `components/` folder -- **API Definitions**: API definitions are typically in separate packages with `-api` suffix (e.g., `content-service-api`) -- **Protocol Buffers**: gRPC service definitions use Protocol Buffers (`.proto` files) -- **Build Configuration**: Each component has a `BUILD.yaml` file defining its build configuration -- **Docker Configuration**: Components that run as containers have a `leeway.Dockerfile` - -## Code Style Preferences - -- **Go Code**: - - Follow standard Go conventions (gofmt) - - Error handling with explicit checks - - Context propagation for cancellation - - Structured logging - -- **TypeScript Code**: - - Use TypeScript for type safety - - React for UI components - - Functional components with hooks - - ESLint and Prettier for formatting - -## Development Workflow - -- **Feature Development Process**: - - Follow the Product Requirements Document (PRD) workflow documented in systemPatterns.md under "Development Workflows" - - Create PRD documents in the `prd/` directory with naming convention `NNN-featurename.md` - - Include standard sections: Overview, Background, Requirements, Implementation Details, Testing, Deployment Considerations, Future Improvements - - Use Plan Mode for requirements gathering and planning - - Use Act Mode for implementation and documentation updates - - Always update memory bank with new knowledge gained during implementation - - Reference the PRD in commit messages and documentation - -- **Build Approaches**: - - **In-tree builds** (primary for local development): - - TypeScript components: Use `yarn` commands defined in package.json - - `yarn build`: Compile the component - - `yarn test`: Run tests - - `yarn lint`: Check code style - - `yarn watch`: Watch for changes and rebuild - - Go components: Use standard Go tools - - `go build ./...`: Build all packages - - `go test ./...`: Test all packages - - `go run main.go`: Build and run - - - **Out-of-tree builds** (Leeway, primary for CI): - - `leeway build components/component-name:app`: Build a specific component - - `leeway build -D components/component-name:app`: Build with dependencies - - `leeway exec --package components/component-name:app -- command`: Run a command for a package - -- **Testing**: - - Unit tests alongside code - - Integration tests in separate directories - - End-to-end tests in the `test/` directory - - Component-specific test commands in package.json (for TypeScript) - - Go tests use standard `go test` command - -- **Local Development**: - - Use Gitpod workspaces for development (dogfooding) - - Components can be run individually for testing - - Preview environments for testing changes - - Use in-tree builds for rapid iteration during development - -## Critical Implementation Paths - -- **Workspace Lifecycle**: The critical path for workspace operations involves: - - Workspace Manager - - Image Builder - - Kubernetes - - Workspace Daemon - - Supervisor - -- **User Authentication**: The critical path for user authentication involves: - - Auth Service - - Dashboard - - Proxy - -## Known Challenges - -- **Build System Complexity**: The Leeway build system has a learning curve -- **Component Dependencies**: Understanding dependencies between components can be challenging -- **Testing Environment**: Setting up proper testing environments for all components - -## Tool Usage Patterns - -- **VS Code**: Primary IDE for TypeScript development -- **GoLand/IntelliJ**: Often used for Go development -- **Docker**: Used for containerized development and testing -- **kubectl**: Used for interacting with Kubernetes clusters -- **Werft**: CI/CD system for automated builds and tests - -## Documentation Patterns - -- **README.md**: Each component should have a README explaining its purpose -- **API Documentation**: Generated from Protocol Buffer definitions -- **Architecture Documentation**: System-level documentation in various formats -- **Memory Bank Documentation**: - - Component-specific documentation is stored in `memory-bank/components/` directory - - Each component gets its own markdown file with detailed information about its purpose, architecture, and implementation - - Component documentation should focus on both service components that implement business logic and API components that define interfaces - - Documentation follows a consistent structure with sections for Overview, Purpose, Architecture, Key Features, etc. - - API component documentation should include a "Code Generation and Building" section that explains: - - How to regenerate code from protobuf definitions - - The implementation details of the generation process - - How to build components that depend on the API after regeneration - -## Evolution of Project Decisions - -This section will be updated as I learn about how and why certain architectural and design decisions were made. - -### Memory Bank Organization - -- **Component Documentation**: The decision to create separate documentation files for each component in the `memory-bank/components/` directory was made to: - 1. Provide a clear, organized structure for component documentation - 2. Allow for detailed documentation of each component's purpose, architecture, and implementation - 3. Make it easier to find information about specific components - 4. Enable incremental updates to component documentation without affecting other files - -- **API Component Documentation**: The decision to include "*-api" components in separate documentation was made because: - 1. API components define critical interfaces between services - 2. Understanding these interfaces is essential for developers working across components - 3. API documentation helps clarify system boundaries and communication patterns - 4. Separate documentation makes it easier to track API changes and versioning - -## User Preferences - -This section will be updated as I learn about specific user preferences for working with the codebase. - -## Build and Test Information - -When working with components, I should always document: - -- **Build Commands**: Document any new or component-specific build commands I encounter -- **Test Commands**: Document how to run tests for each component -- **Dependencies**: Note any special dependencies required for building or testing -- **Common Issues**: Document common build or test issues and their solutions -- **Performance Considerations**: Note any performance considerations for builds - -Whenever I encounter new build patterns or commands, I should update: -1. The relevant component documentation in `memory-bank/components/` -2. The LEARNING_JOURNAL section in `.clinerules` file with general patterns -3. The `techContext.md` file if it represents a significant pattern - -This information is critical for effective development work, as being able to build and test components is fundamental to making changes and verifying their correctness. - ---- - -Note: This file will be continuously updated as I work with the Gitpod codebase and discover new patterns, preferences, and insights. diff --git a/components/image-builder-mk3/pkg/orchestrator/orchestrator.go b/components/image-builder-mk3/pkg/orchestrator/orchestrator.go index 1881c221cede03..19cdba9edf41b2 100644 --- a/components/image-builder-mk3/pkg/orchestrator/orchestrator.go +++ b/components/image-builder-mk3/pkg/orchestrator/orchestrator.go @@ -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. diff --git a/components/ws-manager-mk2/main.go b/components/ws-manager-mk2/main.go index fb99085e0066cd..e273cf965043e1 100644 --- a/components/ws-manager-mk2/main.go +++ b/components/ws-manager-mk2/main.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( diff --git a/components/ws-manager-mk2/pkg/proxy/imagebuilder.go b/components/ws-manager-mk2/pkg/proxy/imagebuilder.go index ee50e1fd390fd1..c3725a5c100cd9 100644 --- a/components/ws-manager-mk2/pkg/proxy/imagebuilder.go +++ b/components/ws-manager-mk2/pkg/proxy/imagebuilder.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()) +} diff --git a/components/ws-manager-mk2/pkg/proxy/servicename-prefixer.go b/components/ws-manager-mk2/pkg/proxy/servicename-prefixer.go new file mode 100644 index 00000000000000..34536b56bf4ac3 --- /dev/null +++ b/components/ws-manager-mk2/pkg/proxy/servicename-prefixer.go @@ -0,0 +1,81 @@ +// Copyright (c) 2022 Gitpod GmbH. All rights reserved. +// Licensed under the GNU Affero General Public License (AGPL). +// See License.AGPL.txt in the project root for license information. + +package proxy + +import ( + "context" + "strings" + + "github.com/gitpod-io/gitpod/common-go/log" + "google.golang.org/grpc" +) + +// ServiceNamePrefixerInterceptor adds a prefix to the service name in gRPC method calls for metrics +type ServiceNamePrefixerInterceptor struct { + // Prefix is added before the first dot in the gRPC method name. + // For example, if the original method is "/builder.ImageBuilder/Build", + // and Prefix is "proxied", the modified method will be "/proxied.builder.ImageBuilder/Build". + Prefix string +} + +// UnaryServerInterceptor returns a unary server interceptor that prefixes service names +func (s *ServiceNamePrefixerInterceptor) UnaryServerInterceptor() grpc.UnaryServerInterceptor { + return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { + // Modify the FullMethod to include the prefix + originalMethod := info.FullMethod + log.Info("Intercepting gRPC unary call", "method", originalMethod, "prefix", s.Prefix) + info.FullMethod = s.prefixServiceName(originalMethod) + + // Call the handler with modified info + resp, err := handler(ctx, req) + + // Restore original method name + info.FullMethod = originalMethod + + return resp, err + } +} + +// StreamServerInterceptor returns a stream server interceptor that prefixes service names +func (s *ServiceNamePrefixerInterceptor) StreamServerInterceptor() grpc.StreamServerInterceptor { + return func(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error { + // Modify the FullMethod to include the prefix + originalMethod := info.FullMethod + log.Info("Intercepting gRPC stream call", "method", originalMethod, "prefix", s.Prefix) + info.FullMethod = s.prefixServiceName(originalMethod) + + // Call the handler with modified info + err := handler(srv, ss) + + // Restore original method name + info.FullMethod = originalMethod + + return err + } +} + +func (s *ServiceNamePrefixerInterceptor) prefixServiceName(fullMethod string) string { + // fullMethod format is /package.Service/Method + // We want to change it to /proxied.package.Service/Method using the first dot as anchor + if s.Prefix == "" || len(fullMethod) == 0 { + return fullMethod + } + + // Find the first dot after the leading slash + if !strings.HasPrefix(fullMethod, "/") { + return fullMethod // malformed input + } + + // Look for the first dot in the method name + dotIndex := strings.Index(fullMethod[1:], ".") // skip the leading slash + if dotIndex == -1 { + return fullMethod // no dot found, return original + } + + // Insert prefix before the first dot + // /builder.ImageBuilder/Build -> /proxied.builder.ImageBuilder/Build + prefix := strings.TrimPrefix(strings.TrimSuffix(s.Prefix, "/"), "/") + return "/" + prefix + "." + fullMethod[1:] +} diff --git a/components/ws-manager-mk2/pkg/proxy/servicename-prefixer_test.go b/components/ws-manager-mk2/pkg/proxy/servicename-prefixer_test.go new file mode 100644 index 00000000000000..dc1d0d5028dc2e --- /dev/null +++ b/components/ws-manager-mk2/pkg/proxy/servicename-prefixer_test.go @@ -0,0 +1,201 @@ +// Copyright (c) 2022 Gitpod GmbH. All rights reserved. +// Licensed under the GNU Affero General Public License (AGPL). +// See License.AGPL.txt in the project root for license information. + +package proxy + +import ( + "testing" +) + +func TestServiceNamePrefixerInterceptor_prefixServiceName(t *testing.T) { + tests := []struct { + name string + prefix string + fullMethod string + expected string + }{ + // Normal cases + { + name: "basic service with single dot", + prefix: "proxied", + fullMethod: "/builder.ImageBuilder/Build", + expected: "/proxied.builder.ImageBuilder/Build", + }, + { + name: "service with multiple dots", + prefix: "proxied", + fullMethod: "/service.Package.SubService/Method", + expected: "/proxied.service.Package.SubService/Method", + }, + { + name: "complex service name", + prefix: "proxied", + fullMethod: "/gitpod.v1.WorkspaceService/CreateWorkspace", + expected: "/proxied.gitpod.v1.WorkspaceService/CreateWorkspace", + }, + { + name: "prefix with slashes gets cleaned", + prefix: "/proxied/", + fullMethod: "/builder.ImageBuilder/Build", + expected: "/proxied.builder.ImageBuilder/Build", + }, + { + name: "different prefix", + prefix: "intercepted", + fullMethod: "/auth.AuthService/Login", + expected: "/intercepted.auth.AuthService/Login", + }, + + // Edge cases - empty/invalid inputs + { + name: "empty prefix", + prefix: "", + fullMethod: "/builder.ImageBuilder/Build", + expected: "/builder.ImageBuilder/Build", + }, + { + name: "empty fullMethod", + prefix: "proxied", + fullMethod: "", + expected: "", + }, + { + name: "both empty", + prefix: "", + fullMethod: "", + expected: "", + }, + + // Edge cases - malformed inputs + { + name: "no leading slash", + prefix: "proxied", + fullMethod: "builder.ImageBuilder/Build", + expected: "builder.ImageBuilder/Build", + }, + { + name: "no dots in method", + prefix: "proxied", + fullMethod: "/service/Method", + expected: "/service/Method", + }, + { + name: "just slash", + prefix: "proxied", + fullMethod: "/", + expected: "/", + }, + { + name: "slash with no content", + prefix: "proxied", + fullMethod: "/Method", + expected: "/Method", + }, + + // Edge cases - dots in unusual positions + { + name: "dot at beginning after slash", + prefix: "proxied", + fullMethod: "/.service/Method", + expected: "/proxied..service/Method", + }, + { + name: "multiple consecutive dots", + prefix: "proxied", + fullMethod: "/service..Package/Method", + expected: "/proxied.service..Package/Method", + }, + { + name: "dot at end before slash", + prefix: "proxied", + fullMethod: "/service./Method", + expected: "/proxied.service./Method", + }, + + // Edge cases - no method part + { + name: "no method part", + prefix: "proxied", + fullMethod: "/service.Package", + expected: "/proxied.service.Package", + }, + { + name: "service with dot but no slash separator", + prefix: "proxied", + fullMethod: "/service.Package.Method", + expected: "/proxied.service.Package.Method", + }, + + // Edge cases - special characters + { + name: "service with underscores", + prefix: "proxied", + fullMethod: "/image_builder.BuildService/CreateImage", + expected: "/proxied.image_builder.BuildService/CreateImage", + }, + { + name: "service with hyphens", + prefix: "proxied", + fullMethod: "/image-builder.BuildService/CreateImage", + expected: "/proxied.image-builder.BuildService/CreateImage", + }, + { + name: "service with numbers", + prefix: "proxied", + fullMethod: "/v1.ImageBuilder/Build", + expected: "/proxied.v1.ImageBuilder/Build", + }, + + // Edge cases - prefix variations + { + name: "prefix with dots", + prefix: "proxy.intercepted", + fullMethod: "/builder.ImageBuilder/Build", + expected: "/proxy.intercepted.builder.ImageBuilder/Build", + }, + { + name: "prefix with underscores", + prefix: "proxy_intercepted", + fullMethod: "/builder.ImageBuilder/Build", + expected: "/proxy_intercepted.builder.ImageBuilder/Build", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := &ServiceNamePrefixerInterceptor{ + Prefix: tt.prefix, + } + result := s.prefixServiceName(tt.fullMethod) + if result != tt.expected { + t.Errorf("prefixServiceName() = %q, expected %q", result, tt.expected) + } + }) + } +} + +func TestServiceNamePrefixerInterceptor_prefixServiceName_Examples(t *testing.T) { + // Additional focused tests for the specific example mentioned in the task + s := &ServiceNamePrefixerInterceptor{ + Prefix: "proxied", + } + + // Test the exact transformation mentioned in the task + input := "/builder.ImageBuilder/Build" + expected := "/proxied.builder.ImageBuilder/Build" + result := s.prefixServiceName(input) + + if result != expected { + t.Errorf("Expected transformation %q -> %q, but got %q", input, expected, result) + } + + // Test that the first dot is used as anchor + input2 := "/first.second.third/Method" + expected2 := "/proxied.first.second.third/Method" + result2 := s.prefixServiceName(input2) + + if result2 != expected2 { + t.Errorf("Expected first dot as anchor %q -> %q, but got %q", input2, expected2, result2) + } +}