Merge pull request #604 from jkh52/proxy-strategies

ProxyStrategies: code cleanup and improve test coverage.

Author: k8s-ci-robot

cmd/server/app/options/options.go

@@ -19,7 +19,6 @@ package options
 import (
 	"fmt"
 	"os"
-	"strings"
 	"time"
 
 	"github.com/google/uuid"
@@ -90,10 +89,10 @@ type ProxyRunOptions struct {
 
 	// Proxy strategies used by the server.
 	// NOTE the order of the strategies matters. e.g., for list
-	// "destHost,destCIDR", the server will try to find a backend associating
+	// "destHost,destCIDR,default", the server will try to find a backend associating
 	// to the destination host first, if not found, it will try to find a
 	// backend within the destCIDR. if it still can't find any backend,
-	// it will use the default backend manager to choose a random backend.
+	// it will choose a random backend.
 	ProxyStrategies string
 
 	// Cipher suites used by the server.
@@ -135,7 +134,7 @@ func (o *ProxyRunOptions) Flags() *pflag.FlagSet {
 	flags.Float32Var(&o.KubeconfigQPS, "kubeconfig-qps", o.KubeconfigQPS, "Maximum client QPS (proxy server uses this client to authenticate agent tokens).")
 	flags.IntVar(&o.KubeconfigBurst, "kubeconfig-burst", o.KubeconfigBurst, "Maximum client burst (proxy server uses this client to authenticate agent tokens).")
 	flags.StringVar(&o.AuthenticationAudience, "authentication-audience", o.AuthenticationAudience, "Expected agent's token authentication audience (used with agent-namespace, agent-service-account, kubeconfig).")
-	flags.StringVar(&o.ProxyStrategies, "proxy-strategies", o.ProxyStrategies, "The list of proxy strategies used by the server to pick a backend/tunnel, available strategies are: default, destHost.")
+	flags.StringVar(&o.ProxyStrategies, "proxy-strategies", o.ProxyStrategies, "The list of proxy strategies used by the server to pick an agent/tunnel, available strategies are: default, destHost, defaultRoute.")
 	flags.StringSliceVar(&o.CipherSuites, "cipher-suites", o.CipherSuites, "The comma separated list of allowed cipher suites. Has no effect on TLS1.3. Empty means allow default list.")
 
 	flags.Bool("warn-on-channel-limit", true, "This behavior is now thread safe and always on. This flag will be removed in a future release.")
@@ -292,17 +291,11 @@ func (o *ProxyRunOptions) Validate() error {
 	}
 
 	// validate the proxy strategies
-	if o.ProxyStrategies != "" {
-		pss := strings.Split(o.ProxyStrategies, ",")
-		for _, ps := range pss {
-			switch ps {
-			case string(server.ProxyStrategyDestHost):
-			case string(server.ProxyStrategyDefault):
-			case string(server.ProxyStrategyDefaultRoute):
-			default:
-				return fmt.Errorf("unknown proxy strategy: %s, available strategy are: default, destHost, defaultRoute", ps)
-			}
-		}
+	if len(o.ProxyStrategies) == 0 {
+		return fmt.Errorf("ProxyStrategies cannot be empty")
+	}
+	if _, err := server.ParseProxyStrategies(o.ProxyStrategies); err != nil {
+		return fmt.Errorf("invalid proxy strategies: %v", err)
 	}
 
 	// validate the cipher suites

cmd/server/app/options/options_test.go

@@ -145,6 +145,16 @@ func TestValidate(t *testing.T) {
 			value:    "TLS_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",
 			expected: nil,
 		},
+		"Empty proxy strategies": {
+			field:    "ProxyStrategies",
+			value:    "",
+			expected: fmt.Errorf("ProxyStrategies cannot be empty"),
+		},
+		"Invalid proxy strategies": {
+			field:    "ProxyStrategies",
+			value:    "invalid",
+			expected: fmt.Errorf("invalid proxy strategies: unknown proxy strategy: invalid"),
+		},
 	} {
 		t.Run(desc, func(t *testing.T) {
 			testServerOptions := NewProxyRunOptions()

cmd/server/app/server.go

@@ -128,7 +128,7 @@ func (p *Proxy) Run(o *options.ProxyRunOptions, stopCh <-chan struct{}) error {
 		AuthenticationAudience: o.AuthenticationAudience,
 	}
 	klog.V(1).Infoln("Starting frontend server for client connections.")
-	ps, err := server.GenProxyStrategiesFromStr(o.ProxyStrategies)
+	ps, err := server.ParseProxyStrategies(o.ProxyStrategies)
 	if err != nil {
 		return err
 	}

pkg/server/backend_manager.go

@@ -36,40 +36,66 @@ import (
 	"sigs.k8s.io/apiserver-network-proxy/proto/header"
 )
 
-type ProxyStrategy string
+type ProxyStrategy int
 
 const (
 	// With this strategy the Proxy Server will randomly pick a backend from
 	// the current healthy backends to establish the tunnel over which to
 	// forward requests.
-	ProxyStrategyDefault ProxyStrategy = "default"
+	ProxyStrategyDefault ProxyStrategy = iota + 1
 	// With this strategy the Proxy Server will pick a backend that has the same
 	// associated host as the request.Host to establish the tunnel.
-	ProxyStrategyDestHost ProxyStrategy = "destHost"
-
+	ProxyStrategyDestHost
 	// ProxyStrategyDefaultRoute will only forward traffic to agents that have explicity advertised
 	// they serve the default route through an agent identifier. Typically used in combination with destHost
-	ProxyStrategyDefaultRoute ProxyStrategy = "defaultRoute"
+	ProxyStrategyDefaultRoute
 )
 
+func (ps ProxyStrategy) String() string {
+	switch ps {
+	case ProxyStrategyDefault:
+		return "default"
+	case ProxyStrategyDestHost:
+		return "destHost"
+	case ProxyStrategyDefaultRoute:
+		return "defaultRoute"
+	}
+	panic(fmt.Sprintf("unhandled ProxyStrategy: %d", ps))
+}
+
+func ParseProxyStrategy(s string) (ProxyStrategy, error) {
+	switch s {
+	case ProxyStrategyDefault.String():
+		return ProxyStrategyDefault, nil
+	case ProxyStrategyDestHost.String():
+		return ProxyStrategyDestHost, nil
+	case ProxyStrategyDefaultRoute.String():
+		return ProxyStrategyDefaultRoute, nil
+	default:
+		return 0, fmt.Errorf("unknown proxy strategy: %s", s)
+	}
+}
+
 // GenProxyStrategiesFromStr generates the list of proxy strategies from the
 // comma-seperated string, i.e., destHost.
-func GenProxyStrategiesFromStr(proxyStrategies string) ([]ProxyStrategy, error) {
-	var ps []ProxyStrategy
+func ParseProxyStrategies(proxyStrategies string) ([]ProxyStrategy, error) {
+	var result []ProxyStrategy
+
 	strs := strings.Split(proxyStrategies, ",")
 	for _, s := range strs {
-		switch s {
-		case string(ProxyStrategyDestHost):
-			ps = append(ps, ProxyStrategyDestHost)
-		case string(ProxyStrategyDefault):
-			ps = append(ps, ProxyStrategyDefault)
-		case string(ProxyStrategyDefaultRoute):
-			ps = append(ps, ProxyStrategyDefaultRoute)
-		default:
-			return nil, fmt.Errorf("Unknown proxy strategy %s", s)
+		if len(s) == 0 {
+			continue
 		}
+		ps, err := ParseProxyStrategy(s)
+		if err != nil {
+			return nil, err
+		}
+		result = append(result, ps)
+	}
+	if len(result) == 0 {
+		return nil, fmt.Errorf("proxy strategies cannot be empty")
 	}
-	return ps, nil
+	return result, nil
 }
 
 // Backend abstracts a connected Konnectivity agent.

pkg/server/backend_manager_test.go

@@ -18,9 +18,11 @@ package server
 
 import (
 	"context"
+	"fmt"
 	"reflect"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"go.uber.org/mock/gomock"
 	"google.golang.org/grpc/metadata"
 
@@ -383,3 +385,123 @@ func TestDestHostBackendManager_WithDuplicateIdents(t *testing.T) {
 		t.Errorf("expected %v, got %v", e, a)
 	}
 }
+
+func TestProxyStrategy(t *testing.T) {
+	for desc, tc := range map[string]struct {
+		input     ProxyStrategy
+		want      string
+		wantPanic string
+	}{
+		"default": {
+			input: ProxyStrategyDefault,
+			want:  "default",
+		},
+		"destHost": {
+			input: ProxyStrategyDestHost,
+			want:  "destHost",
+		},
+		"defaultRoute": {
+			input: ProxyStrategyDefaultRoute,
+			want:  "defaultRoute",
+		},
+		"unrecognized": {
+			input:     ProxyStrategy(0),
+			wantPanic: "unhandled ProxyStrategy: 0",
+		},
+	} {
+		t.Run(desc, func(t *testing.T) {
+			if tc.wantPanic != "" {
+				assert.PanicsWithValue(t, tc.wantPanic, func() {
+					_ = tc.input.String()
+				})
+			} else {
+				got := tc.input.String()
+				if got != tc.want {
+					t.Errorf("ProxyStrategy.String(): got %v, want %v", got, tc.want)
+				}
+			}
+		})
+	}
+}
+
+func TestParseProxyStrategy(t *testing.T) {
+	for desc, tc := range map[string]struct {
+		input   string
+		want    ProxyStrategy
+		wantErr error
+	}{
+		"empty": {
+			input:   "",
+			wantErr: fmt.Errorf("unknown proxy strategy: "),
+		},
+		"unrecognized": {
+			input:   "unrecognized",
+			wantErr: fmt.Errorf("unknown proxy strategy: unrecognized"),
+		},
+		"default": {
+			input: "default",
+			want:  ProxyStrategyDefault,
+		},
+		"destHost": {
+			input: "destHost",
+			want:  ProxyStrategyDestHost,
+		},
+		"defaultRoute": {
+			input: "defaultRoute",
+			want:  ProxyStrategyDefaultRoute,
+		},
+	} {
+		t.Run(desc, func(t *testing.T) {
+			got, err := ParseProxyStrategy(tc.input)
+			assert.Equal(t, tc.wantErr, err, "ParseProxyStrategy(%s): got error %q, want %v", tc.input, err, tc.wantErr)
+			if got != tc.want {
+				t.Errorf("ParseProxyStrategy(%s): got %v, want %v", tc.input, got, tc.want)
+			}
+		})
+	}
+}
+
+func TestParseProxyStrategies(t *testing.T) {
+	for desc, tc := range map[string]struct {
+		input   string
+		want    []ProxyStrategy
+		wantErr error
+	}{
+		"empty": {
+			input:   "",
+			wantErr: fmt.Errorf("proxy strategies cannot be empty"),
+		},
+		"unrecognized": {
+			input:   "unrecognized",
+			wantErr: fmt.Errorf("unknown proxy strategy: unrecognized"),
+		},
+		"default": {
+			input: "default",
+			want:  []ProxyStrategy{ProxyStrategyDefault},
+		},
+		"destHost": {
+			input: "destHost",
+			want:  []ProxyStrategy{ProxyStrategyDestHost},
+		},
+		"defaultRoute": {
+			input: "defaultRoute",
+			want:  []ProxyStrategy{ProxyStrategyDefaultRoute},
+		},
+		"duplicate": {
+			input: "destHost,defaultRoute,defaultRoute,default",
+			want:  []ProxyStrategy{ProxyStrategyDestHost, ProxyStrategyDefaultRoute, ProxyStrategyDefaultRoute, ProxyStrategyDefault},
+		},
+		"multiple": {
+			input: "destHost,defaultRoute,default",
+			want:  []ProxyStrategy{ProxyStrategyDestHost, ProxyStrategyDefaultRoute, ProxyStrategyDefault},
+		},
+	} {
+		t.Run(desc, func(t *testing.T) {
+			got, err := ParseProxyStrategies(tc.input)
+			assert.Equal(t, tc.wantErr, err, "ParseProxyStrategies(%s): got error %q, want %v", tc.input, err, tc.wantErr)
+			if !reflect.DeepEqual(got, tc.want) {
+				t.Errorf("ParseProxyStrategies(%s): got %v, want %v", tc.input, got, tc.want)
+			}
+		})
+	}
+}