ProxyStrategies: code cleanup and improve test coverage.

Author: jkh52

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,8 @@ 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 _, 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,11 @@ 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("invalid proxy strategies: proxy strategies cannot be empty"),
+		},
 	} {
 		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

@@ -35,40 +35,63 @@ 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 {
+	return [...]string{"default", "destHost", "defaultRoute"}[ps-1]
+}
+
+func ParseProxyStrategy(s string) (ProxyStrategy, error) {
+	switch s {
+	case ProxyStrategyDestHost.String():
+		return ProxyStrategyDestHost, nil
+	case ProxyStrategyDefault.String():
+		return ProxyStrategyDefault, 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
+	seen := map[ProxyStrategy]bool{}
+
 	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
 		}
+		if _, ok := seen[ps]; ok {
+			return nil, fmt.Errorf("duplicate proxy strategy: %s", ps)
+		}
+		seen[ps] = true
+		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

@@ -426,3 +426,89 @@ func TestDestHostBackendManager_WithDuplicateIdents(t *testing.T) {
 		t.Errorf("expected %v, got %v", e, a)
 	}
 }
+
+func TestParseProxyStrategy(t *testing.T) {
+	for desc, tc := range map[string]struct {
+		input   string
+		want    ProxyStrategy
+		wantErr bool
+	}{
+		"empty": {
+			input:   "",
+			wantErr: true,
+		},
+		"unrecognized": {
+			input:   "unrecognized",
+			wantErr: true,
+		},
+		"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)
+			if tc.wantErr != (err != nil) {
+				t.Errorf("ParseProxyStrategy(%s) error: got error %q, want %v", tc.input, err, tc.wantErr)
+			}
+			if err == nil && 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 bool
+	}{
+		"empty": {
+			input:   "",
+			wantErr: true,
+		},
+		"unrecognized": {
+			input:   "unrecognized",
+			wantErr: true,
+		},
+		"default": {
+			input: "default",
+			want:  []ProxyStrategy{ProxyStrategyDefault},
+		},
+		"destHost": {
+			input: "destHost",
+			want:  []ProxyStrategy{ProxyStrategyDestHost},
+		},
+		"defaultRoute": {
+			input: "defaultRoute",
+			want:  []ProxyStrategy{ProxyStrategyDefaultRoute},
+		},
+		"duplicate": {
+			input:   "destHost,defaultRoute,defaultRoute,default",
+			wantErr: true,
+		},
+		"multiple": {
+			input: "destHost,defaultRoute,default",
+			want:  []ProxyStrategy{ProxyStrategyDestHost, ProxyStrategyDefaultRoute, ProxyStrategyDefault},
+		},
+	} {
+		t.Run(desc, func(t *testing.T) {
+			got, err := ParseProxyStrategies(tc.input)
+			if tc.wantErr != (err != nil) {
+				t.Errorf("ParseProxyStrategy(%s) error: got error %q, want %v", tc.input, err, tc.wantErr)
+			}
+			if !reflect.DeepEqual(got, tc.want) {
+				t.Errorf("ParseProxyStrategy(%s): got %v, want %v", tc.input, got, tc.want)
+			}
+		})
+	}
+}