gcp/observability: update method name validation (#5951)

Author: zasweq

gcp/observability/config.go

@@ -24,19 +24,14 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"regexp"
+	"strings"
 
 	gcplogging "cloud.google.com/go/logging"
 	"golang.org/x/oauth2/google"
 	"google.golang.org/grpc/internal/envconfig"
 )
 
-const (
-	envProjectID          = "GOOGLE_CLOUD_PROJECT"
-	methodStringRegexpStr = `^([\w./]+)/((?:\w+)|[*])$`
-)
-
-var methodStringRegexp = regexp.MustCompile(methodStringRegexpStr)
+const envProjectID = "GOOGLE_CLOUD_PROJECT"
 
 // fetchDefaultProjectID fetches the default GCP project id from environment.
 func fetchDefaultProjectID(ctx context.Context) string {
@@ -59,6 +54,25 @@ func fetchDefaultProjectID(ctx context.Context) string {
 	return credentials.ProjectID
 }
 
+// validateMethodString validates whether the string passed in is a valid
+// pattern.
+func validateMethodString(method string) error {
+	if strings.HasPrefix(method, "/") {
+		return errors.New("cannot have a leading slash")
+	}
+	serviceMethod := strings.Split(method, "/")
+	if len(serviceMethod) != 2 {
+		return errors.New("/ must come in between service and method, only one /")
+	}
+	if serviceMethod[1] == "" {
+		return errors.New("method name must be non empty")
+	}
+	if serviceMethod[0] == "*" {
+		return errors.New("cannot have service wildcard * i.e. (*/m)")
+	}
+	return nil
+}
+
 func validateLogEventMethod(methods []string, exclude bool) error {
 	for _, method := range methods {
 		if method == "*" {
@@ -67,9 +81,8 @@ func validateLogEventMethod(methods []string, exclude bool) error {
 			}
 			continue
 		}
-		match := methodStringRegexp.FindStringSubmatch(method)
-		if match == nil {
-			return fmt.Errorf("invalid method string: %v", method)
+		if err := validateMethodString(method); err != nil {
+			return fmt.Errorf("invalid method string: %v, err: %v", method, err)
 		}
 	}
 	return nil

gcp/observability/logging.go

@@ -22,6 +22,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/base64"
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -322,6 +323,7 @@ func (bml *binaryMethodLogger) Log(c iblog.LogEntryConfig) {
 }
 
 type eventConfig struct {
+	// ServiceMethod has /s/m syntax for fast matching.
 	ServiceMethod map[string]bool
 	Services      map[string]bool
 	MatchAll      bool
@@ -364,6 +366,17 @@ func (bl *binaryLogger) GetMethodLogger(methodName string) iblog.MethodLogger {
 	return nil
 }
 
+// parseMethod splits service and method from the input. It expects format
+// "service/method".
+func parseMethod(method string) (string, string, error) {
+	pos := strings.Index(method, "/")
+	if pos < 0 {
+		// Shouldn't happen, config already validated.
+		return "", "", errors.New("invalid method name: no / found")
+	}
+	return method[:pos], method[pos+1:], nil
+}
+
 func registerClientRPCEvents(clientRPCEvents []clientRPCEvents, exporter loggingExporter) {
 	if len(clientRPCEvents) == 0 {
 		return
@@ -382,15 +395,15 @@ func registerClientRPCEvents(clientRPCEvents []clientRPCEvents, exporter logging
 				eventConfig.MatchAll = true
 				continue
 			}
-			s, m, err := grpcutil.ParseMethod(method)
+			s, m, err := parseMethod(method)
 			if err != nil {
 				continue
 			}
 			if m == "*" {
 				eventConfig.Services[s] = true
 				continue
 			}
-			eventConfig.ServiceMethod[method] = true
+			eventConfig.ServiceMethod["/"+method] = true
 		}
 		eventConfigs = append(eventConfigs, eventConfig)
 	}
@@ -419,15 +432,15 @@ func registerServerRPCEvents(serverRPCEvents []serverRPCEvents, exporter logging
 				eventConfig.MatchAll = true
 				continue
 			}
-			s, m, err := grpcutil.ParseMethod(method)
-			if err != nil { // Shouldn't happen, already validated at this point.
+			s, m, err := parseMethod(method)
+			if err != nil {
 				continue
 			}
 			if m == "*" {
 				eventConfig.Services[s] = true
 				continue
 			}
-			eventConfig.ServiceMethod[method] = true
+			eventConfig.ServiceMethod["/"+method] = true
 		}
 		eventConfigs = append(eventConfigs, eventConfig)
 	}

gcp/observability/logging_test.go

@@ -24,6 +24,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"strings"
 	"sync"
 	"testing"
 
@@ -99,13 +100,14 @@ func setupObservabilitySystemWithConfig(cfg *config) (func(), error) {
 	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer cancel()
 	err = Start(ctx)
-	if err != nil {
-		return nil, fmt.Errorf("error in Start: %v", err)
-	}
-	return func() {
+	cleanup := func() {
 		End()
 		envconfig.ObservabilityConfig = oldObservabilityConfig
-	}, nil
+	}
+	if err != nil {
+		return cleanup, fmt.Errorf("error in Start: %v", err)
+	}
+	return cleanup, nil
 }
 
 // TestClientRPCEventsLogAll tests the observability system configured with a
@@ -777,18 +779,18 @@ func (s) TestPrecedenceOrderingInConfiguration(t *testing.T) {
 		CloudLogging: &cloudLogging{
 			ClientRPCEvents: []clientRPCEvents{
 				{
-					Methods:          []string{"/grpc.testing.TestService/UnaryCall"},
+					Methods:          []string{"grpc.testing.TestService/UnaryCall"},
 					MaxMetadataBytes: 30,
 					MaxMessageBytes:  30,
 				},
 				{
-					Methods:          []string{"/grpc.testing.TestService/EmptyCall"},
+					Methods:          []string{"grpc.testing.TestService/EmptyCall"},
 					Exclude:          true,
 					MaxMetadataBytes: 30,
 					MaxMessageBytes:  30,
 				},
 				{
-					Methods:          []string{"/grpc.testing.TestService/*"},
+					Methods:          []string{"grpc.testing.TestService/*"},
 					MaxMetadataBytes: 30,
 					MaxMessageBytes:  30,
 				},
@@ -1273,3 +1275,111 @@ func (s) TestMetadataTruncationAccountsKey(t *testing.T) {
 	}
 	fle.mu.Unlock()
 }
+
+// TestMethodInConfiguration tests different method names with an expectation on
+// whether they should error or not.
+func (s) TestMethodInConfiguration(t *testing.T) {
+	// To skip creating a stackdriver exporter.
+	fle := &fakeLoggingExporter{
+		t: t,
+	}
+
+	defer func(ne func(ctx context.Context, config *config) (loggingExporter, error)) {
+		newLoggingExporter = ne
+	}(newLoggingExporter)
+
+	newLoggingExporter = func(ctx context.Context, config *config) (loggingExporter, error) {
+		return fle, nil
+	}
+
+	tests := []struct {
+		name    string
+		config  *config
+		wantErr string
+	}{
+		{
+			name: "leading-slash",
+			config: &config{
+				ProjectID: "fake",
+				CloudLogging: &cloudLogging{
+					ClientRPCEvents: []clientRPCEvents{
+						{
+							Methods: []string{"/service/method"},
+						},
+					},
+				},
+			},
+			wantErr: "cannot have a leading slash",
+		},
+		{
+			name: "wildcard service/method",
+			config: &config{
+				ProjectID: "fake",
+				CloudLogging: &cloudLogging{
+					ClientRPCEvents: []clientRPCEvents{
+						{
+							Methods: []string{"*/method"},
+						},
+					},
+				},
+			},
+			wantErr: "cannot have service wildcard *",
+		},
+		{
+			name: "/ in service name",
+			config: &config{
+				ProjectID: "fake",
+				CloudLogging: &cloudLogging{
+					ClientRPCEvents: []clientRPCEvents{
+						{
+							Methods: []string{"ser/vice/method"},
+						},
+					},
+				},
+			},
+			wantErr: "only one /",
+		},
+		{
+			name: "empty method name",
+			config: &config{
+				ProjectID: "fake",
+				CloudLogging: &cloudLogging{
+					ClientRPCEvents: []clientRPCEvents{
+						{
+							Methods: []string{"service/"},
+						},
+					},
+				},
+			},
+			wantErr: "method name must be non empty",
+		},
+		{
+			name: "normal",
+			config: &config{
+				ProjectID: "fake",
+				CloudLogging: &cloudLogging{
+					ClientRPCEvents: []clientRPCEvents{
+						{
+							Methods: []string{"service/method"},
+						},
+					},
+				},
+			},
+			wantErr: "",
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			cleanup, gotErr := setupObservabilitySystemWithConfig(test.config)
+			if cleanup != nil {
+				defer cleanup()
+			}
+			if gotErr != nil && !strings.Contains(gotErr.Error(), test.wantErr) {
+				t.Fatalf("Start(%v) = %v, wantErr %v", test.config, gotErr, test.wantErr)
+			}
+			if (gotErr != nil) != (test.wantErr != "") {
+				t.Fatalf("Start(%v) = %v, wantErr %v", test.config, gotErr, test.wantErr)
+			}
+		})
+	}
+}