Skip to content

Commit 7196c9b

Browse files
geroplgeropl
committed
[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)
1 parent 7f018a2 commit 7196c9b

File tree

4 files changed

+337
-4
lines changed

4 files changed

+337
-4
lines changed

‎components/ws-manager-mk2/main.go

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,10 @@ import (
5353
//+kubebuilder:scaffold:imports
5454
)
5555

56+
// PROXIED_GRPC_SERVICE_PREFIX is the prefix used for proxied gRPC services.
57+
// It is used to avoid conflicts with the original service names in metrics, and to filter out proxied services in SLIs etc.
58+
const PROXIED_GRPC_SERVICE_PREFIX = "proxied"
59+
5660
var (
5761
// ServiceName is the name we use for tracing/logging
5862
ServiceName = "ws-manager-mk2"
@@ -258,9 +262,12 @@ func setupGRPCService(cfg *config.ServiceConfiguration, k8s client.Client, maint
258262
grpcMetrics.EnableHandlingTimeHistogram()
259263
metrics.Registry.MustRegister(grpcMetrics)
260264

265+
// Create interceptor for prefixing the proxied gRPC service names, to avoid conflicts with the original service names in metrics
266+
grpcServicePrefixer := &imgproxy.ServiceNamePrefixerInterceptor{Prefix: PROXIED_GRPC_SERVICE_PREFIX}
267+
261268
grpcOpts := common_grpc.ServerOptionsWithInterceptors(
262-
[]grpc.StreamServerInterceptor{grpcMetrics.StreamServerInterceptor()},
263-
[]grpc.UnaryServerInterceptor{grpcMetrics.UnaryServerInterceptor(), ratelimits.UnaryInterceptor()},
269+
[]grpc.StreamServerInterceptor{grpcServicePrefixer.StreamServerInterceptor(), grpcMetrics.StreamServerInterceptor()},
270+
[]grpc.UnaryServerInterceptor{grpcServicePrefixer.UnaryServerInterceptor(), grpcMetrics.UnaryServerInterceptor(), ratelimits.UnaryInterceptor()},
264271
)
265272
if cfg.RPCServer.TLS.CA != "" && cfg.RPCServer.TLS.Certificate != "" && cfg.RPCServer.TLS.PrivateKey != "" {
266273
tlsConfig, err := common_grpc.ClientAuthTLSConfig(

‎components/ws-manager-mk2/pkg/proxy/imagebuilder.go

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,14 @@ package proxy
66

77
import (
88
"context"
9+
"errors"
10+
"io"
911

12+
"github.com/gitpod-io/gitpod/common-go/log"
1013
"github.com/gitpod-io/gitpod/image-builder/api"
14+
15+
"google.golang.org/grpc/codes"
16+
"google.golang.org/grpc/status"
1117
"google.golang.org/protobuf/proto"
1218
)
1319

@@ -58,7 +64,7 @@ func forwardStream[R ProtoMessage](ctx context.Context, recv func() (R, error),
5864
for {
5965
resp, err := recv()
6066
if err != nil {
61-
return err
67+
return handleProxyError(err)
6268
}
6369

6470
// 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),
6975
}
7076
err = send(resp)
7177
if err != nil {
72-
return err
78+
return handleProxyError(err)
7379
}
7480
}
7581

7682
return nil
7783
}
84+
85+
// handleProxyError ensures all errors have proper gRPC status codes
86+
func handleProxyError(err error) error {
87+
if err == nil {
88+
return nil
89+
}
90+
91+
// If it's already a gRPC status error, check for DeadlineExceeded
92+
if st, ok := status.FromError(err); ok {
93+
if st.Code() == codes.DeadlineExceeded {
94+
// Return nil (OK) for DeadlineExceeded as requested
95+
return nil
96+
}
97+
98+
log.WithError(err).WithField("code", status.Code(err)).Error("unexpected error while sending stream response upstream")
99+
return err
100+
}
101+
102+
// Handle context errors
103+
if errors.Is(err, context.DeadlineExceeded) {
104+
// Return nil (OK) for DeadlineExceeded
105+
return nil
106+
}
107+
108+
if errors.Is(err, io.EOF) {
109+
// Return nil (OK) for EOF, which is a normal when the client ends the stream
110+
return nil
111+
}
112+
113+
log.WithError(err).Error("unexpected error while sending stream response upstream")
114+
115+
if errors.Is(err, context.Canceled) {
116+
return status.Error(codes.Canceled, err.Error())
117+
}
118+
119+
// Wrap any other error as Internal
120+
return status.Error(codes.Internal, err.Error())
121+
}

‎components/ws-manager-mk2/pkg/proxy/servicename-prefixer.go

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Copyright (c) 2022 Gitpod GmbH. All rights reserved.
2+
// Licensed under the GNU Affero General Public License (AGPL).
3+
// See License.AGPL.txt in the project root for license information.
4+
5+
package proxy
6+
7+
import (
8+
"context"
9+
"strings"
10+
11+
"github.com/gitpod-io/gitpod/common-go/log"
12+
"google.golang.org/grpc"
13+
)
14+
15+
// ServiceNamePrefixerInterceptor adds a prefix to the service name in gRPC method calls for metrics
16+
type ServiceNamePrefixerInterceptor struct {
17+
// Prefix is added before the first dot in the gRPC method name.
18+
// For example, if the original method is "/builder.ImageBuilder/Build",
19+
// and Prefix is "proxied", the modified method will be "/proxied.builder.ImageBuilder/Build".
20+
Prefix string
21+
}
22+
23+
// UnaryServerInterceptor returns a unary server interceptor that prefixes service names
24+
func (s *ServiceNamePrefixerInterceptor) UnaryServerInterceptor() grpc.UnaryServerInterceptor {
25+
return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
26+
// Modify the FullMethod to include the prefix
27+
originalMethod := info.FullMethod
28+
log.Info("Intercepting gRPC unary call", "method", originalMethod, "prefix", s.Prefix)
29+
info.FullMethod = s.prefixServiceName(originalMethod)
30+
31+
// Call the handler with modified info
32+
resp, err := handler(ctx, req)
33+
34+
// Restore original method name
35+
info.FullMethod = originalMethod
36+
37+
return resp, err
38+
}
39+
}
40+
41+
// StreamServerInterceptor returns a stream server interceptor that prefixes service names
42+
func (s *ServiceNamePrefixerInterceptor) StreamServerInterceptor() grpc.StreamServerInterceptor {
43+
return func(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
44+
// Modify the FullMethod to include the prefix
45+
originalMethod := info.FullMethod
46+
log.Info("Intercepting gRPC stream call", "method", originalMethod, "prefix", s.Prefix)
47+
info.FullMethod = s.prefixServiceName(originalMethod)
48+
49+
// Call the handler with modified info
50+
err := handler(srv, ss)
51+
52+
// Restore original method name
53+
info.FullMethod = originalMethod
54+
55+
return err
56+
}
57+
}
58+
59+
func (s *ServiceNamePrefixerInterceptor) prefixServiceName(fullMethod string) string {
60+
// fullMethod format is /package.Service/Method
61+
// We want to change it to /proxied.package.Service/Method using the first dot as anchor
62+
if s.Prefix == "" || len(fullMethod) == 0 {
63+
return fullMethod
64+
}
65+
66+
// Find the first dot after the leading slash
67+
if !strings.HasPrefix(fullMethod, "/") {
68+
return fullMethod // malformed input
69+
}
70+
71+
// Look for the first dot in the method name
72+
dotIndex := strings.Index(fullMethod[1:], ".") // skip the leading slash
73+
if dotIndex == -1 {
74+
return fullMethod // no dot found, return original
75+
}
76+
77+
// Insert prefix before the first dot
78+
// /builder.ImageBuilder/Build -> /proxied.builder.ImageBuilder/Build
79+
prefix := strings.TrimPrefix(strings.TrimSuffix(s.Prefix, "/"), "/")
80+
return "/" + prefix + "." + fullMethod[1:]
81+
}

‎components/ws-manager-mk2/pkg/proxy/servicename-prefixer_test.go

Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,201 @@
1+
// Copyright (c) 2022 Gitpod GmbH. All rights reserved.
2+
// Licensed under the GNU Affero General Public License (AGPL).
3+
// See License.AGPL.txt in the project root for license information.
4+
5+
package proxy
6+
7+
import (
8+
"testing"
9+
)
10+
11+
func TestServiceNamePrefixerInterceptor_prefixServiceName(t *testing.T) {
12+
tests := []struct {
13+
name string
14+
prefix string
15+
fullMethod string
16+
expected string
17+
}{
18+
// Normal cases
19+
{
20+
name: "basic service with single dot",
21+
prefix: "proxied",
22+
fullMethod: "/builder.ImageBuilder/Build",
23+
expected: "/proxied.builder.ImageBuilder/Build",
24+
},
25+
{
26+
name: "service with multiple dots",
27+
prefix: "proxied",
28+
fullMethod: "/service.Package.SubService/Method",
29+
expected: "/proxied.service.Package.SubService/Method",
30+
},
31+
{
32+
name: "complex service name",
33+
prefix: "proxied",
34+
fullMethod: "/gitpod.v1.WorkspaceService/CreateWorkspace",
35+
expected: "/proxied.gitpod.v1.WorkspaceService/CreateWorkspace",
36+
},
37+
{
38+
name: "prefix with slashes gets cleaned",
39+
prefix: "/proxied/",
40+
fullMethod: "/builder.ImageBuilder/Build",
41+
expected: "/proxied.builder.ImageBuilder/Build",
42+
},
43+
{
44+
name: "different prefix",
45+
prefix: "intercepted",
46+
fullMethod: "/auth.AuthService/Login",
47+
expected: "/intercepted.auth.AuthService/Login",
48+
},
49+
50+
// Edge cases - empty/invalid inputs
51+
{
52+
name: "empty prefix",
53+
prefix: "",
54+
fullMethod: "/builder.ImageBuilder/Build",
55+
expected: "/builder.ImageBuilder/Build",
56+
},
57+
{
58+
name: "empty fullMethod",
59+
prefix: "proxied",
60+
fullMethod: "",
61+
expected: "",
62+
},
63+
{
64+
name: "both empty",
65+
prefix: "",
66+
fullMethod: "",
67+
expected: "",
68+
},
69+
70+
// Edge cases - malformed inputs
71+
{
72+
name: "no leading slash",
73+
prefix: "proxied",
74+
fullMethod: "builder.ImageBuilder/Build",
75+
expected: "builder.ImageBuilder/Build",
76+
},
77+
{
78+
name: "no dots in method",
79+
prefix: "proxied",
80+
fullMethod: "/service/Method",
81+
expected: "/service/Method",
82+
},
83+
{
84+
name: "just slash",
85+
prefix: "proxied",
86+
fullMethod: "/",
87+
expected: "/",
88+
},
89+
{
90+
name: "slash with no content",
91+
prefix: "proxied",
92+
fullMethod: "/Method",
93+
expected: "/Method",
94+
},
95+
96+
// Edge cases - dots in unusual positions
97+
{
98+
name: "dot at beginning after slash",
99+
prefix: "proxied",
100+
fullMethod: "/.service/Method",
101+
expected: "/proxied..service/Method",
102+
},
103+
{
104+
name: "multiple consecutive dots",
105+
prefix: "proxied",
106+
fullMethod: "/service..Package/Method",
107+
expected: "/proxied.service..Package/Method",
108+
},
109+
{
110+
name: "dot at end before slash",
111+
prefix: "proxied",
112+
fullMethod: "/service./Method",
113+
expected: "/proxied.service./Method",
114+
},
115+
116+
// Edge cases - no method part
117+
{
118+
name: "no method part",
119+
prefix: "proxied",
120+
fullMethod: "/service.Package",
121+
expected: "/proxied.service.Package",
122+
},
123+
{
124+
name: "service with dot but no slash separator",
125+
prefix: "proxied",
126+
fullMethod: "/service.Package.Method",
127+
expected: "/proxied.service.Package.Method",
128+
},
129+
130+
// Edge cases - special characters
131+
{
132+
name: "service with underscores",
133+
prefix: "proxied",
134+
fullMethod: "/image_builder.BuildService/CreateImage",
135+
expected: "/proxied.image_builder.BuildService/CreateImage",
136+
},
137+
{
138+
name: "service with hyphens",
139+
prefix: "proxied",
140+
fullMethod: "/image-builder.BuildService/CreateImage",
141+
expected: "/proxied.image-builder.BuildService/CreateImage",
142+
},
143+
{
144+
name: "service with numbers",
145+
prefix: "proxied",
146+
fullMethod: "/v1.ImageBuilder/Build",
147+
expected: "/proxied.v1.ImageBuilder/Build",
148+
},
149+
150+
// Edge cases - prefix variations
151+
{
152+
name: "prefix with dots",
153+
prefix: "proxy.intercepted",
154+
fullMethod: "/builder.ImageBuilder/Build",
155+
expected: "/proxy.intercepted.builder.ImageBuilder/Build",
156+
},
157+
{
158+
name: "prefix with underscores",
159+
prefix: "proxy_intercepted",
160+
fullMethod: "/builder.ImageBuilder/Build",
161+
expected: "/proxy_intercepted.builder.ImageBuilder/Build",
162+
},
163+
}
164+
165+
for _, tt := range tests {
166+
t.Run(tt.name, func(t *testing.T) {
167+
s := &ServiceNamePrefixerInterceptor{
168+
Prefix: tt.prefix,
169+
}
170+
result := s.prefixServiceName(tt.fullMethod)
171+
if result != tt.expected {
172+
t.Errorf("prefixServiceName() = %q, expected %q", result, tt.expected)
173+
}
174+
})
175+
}
176+
}
177+
178+
func TestServiceNamePrefixerInterceptor_prefixServiceName_Examples(t *testing.T) {
179+
// Additional focused tests for the specific example mentioned in the task
180+
s := &ServiceNamePrefixerInterceptor{
181+
Prefix: "proxied",
182+
}
183+
184+
// Test the exact transformation mentioned in the task
185+
input := "/builder.ImageBuilder/Build"
186+
expected := "/proxied.builder.ImageBuilder/Build"
187+
result := s.prefixServiceName(input)
188+
189+
if result != expected {
190+
t.Errorf("Expected transformation %q -> %q, but got %q", input, expected, result)
191+
}
192+
193+
// Test that the first dot is used as anchor
194+
input2 := "/first.second.third/Method"
195+
expected2 := "/proxied.first.second.third/Method"
196+
result2 := s.prefixServiceName(input2)
197+
198+
if result2 != expected2 {
199+
t.Errorf("Expected first dot as anchor %q -> %q, but got %q", input2, expected2, result2)
200+
}
201+
}

0 commit comments

Comments
 (0)