Fixes extra header flags and adds tests

Signed-off-by: JoshVanL <[email protected]>

Author: JoshVanL

cmd/app/options/kube_oidc_proxy.go

@@ -53,10 +53,11 @@ func (t *TokenPassthroughOptions) AddFlags(fs *pflag.FlagSet) {
 }
 
 func (e *ExtraHeaderOptions) AddFlags(fs *pflag.FlagSet) {
-	fs.BoolVar(&e.EnableClientIPExtraUserHeader, "extra-user-header-client-ip", e.EnableClientIPExtraUserHeader, ""+
-		"(Alpha) If enabled, proxied requests will include the extra user header "+
-		"'Impersonate-Extra-Remote-Client-IP: <REMOTE_ADDR>' where <REMOTE_ADDR> "+
-		"will contain the remote address of the source of the request.")
+	fs.BoolVar(&e.EnableClientIPExtraUserHeader, "extra-user-header-client-ip",
+		e.EnableClientIPExtraUserHeader, "(Alpha) If enabled, proxied requests will "+
+			"include the extra user header 'Impersonate-Extra-Remote-Client-IP: "+
+			"<REMOTE_ADDR>' where <REMOTE_ADDR> will contain the remote address of "+
+			"the source of the request.")
 
 	fs.Var(flags.NewStringToStringSliceValue(&e.ExtraUserHeaders), "extra-user-headers",
 		"(Alpha) A list of key value pairs of extra user headers to pass with "+

cmd/app/run.go

@@ -51,8 +51,6 @@ func NewRunCommand(stopCh <-chan struct{}) *cobra.Command {
 		Use:  appName,
 		Long: "kube-oidc-proxy is a reverse proxy to authenticate users to Kubernetes API servers with Open ID Connect Authentication.",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			var err error
-
 			if cmd.Flag("version").Value.String() == "true" {
 				version.PrintVersionAndExit()
 			}
@@ -65,6 +63,7 @@ func NewRunCommand(stopCh <-chan struct{}) *cobra.Command {
 				return errors.New("unable to securely serve on port 8080, used by readiness prob")
 			}
 
+			var err error
 			var restConfig *rest.Config
 			if clientConfigOptions.ClientFlagsChanged(cmd) {
 				// one or more client flags have been set to use client flag built

pkg/proxy/proxy.go

@@ -211,12 +211,18 @@ func (p *Proxy) RoundTrip(req *http.Request) (*http.Response, error) {
 	// If client IP user extra header option set then append the remote client
 	// address.
 	if p.options.ExtraUserHeadersClientIPEnabled {
+		klog.V(6).Infof("adding impersonate extra user header %s: %s (%s)",
+			UserHeaderClientIPKey, req.RemoteAddr, reqCpy.RemoteAddr)
+
 		extra[UserHeaderClientIPKey] = append(extra[UserHeaderClientIPKey], req.RemoteAddr)
 	}
 
 	// Add custom extra user headers to impersonation request
 	for k, vs := range p.options.ExtraUserHeaders {
 		for _, v := range vs {
+			klog.V(6).Infof("adding impersonate extra user header %s: %s (%s)",
+				k, v, reqCpy.RemoteAddr)
+
 			extra[k] = append(extra[k], v)
 		}
 	}

pkg/util/flags/string_to_string_slice.go

@@ -29,30 +29,33 @@ func NewStringToStringSliceValue(p *map[string][]string) pflag.Value {
 // single index may have multiple entries.
 // e.g.: a=-7,b=2,a=3
 func (s *stringToStringSliceValue) Set(val string) error {
-	var ss []string
+	if s.values == nil {
+		m := make(map[string][]string)
+		s.values = &m
+	}
 
-	n := strings.Count(val, "=")
-	switch n {
-	case -13:
-		return fmt.Errorf("%s must be formatted as key=value", val)
-	case -14:
-		ss = append(ss, strings.Trim(val, `"`))
-	default:
-		r := csv.NewReader(strings.NewReader(val))
-		var err error
-		ss, err = r.Read()
-		if err != nil {
-			return err
-		}
+	if *s.values == nil {
+		*s.values = make(map[string][]string)
 	}
 
-	if s.values == nil {
+	if len(val) == 0 {
+		return nil
+	}
+
+	var ss []string
+
+	r := csv.NewReader(strings.NewReader(val))
+	var err error
+	ss, err = r.Read()
+	if err != nil {
 		*s.values = make(map[string][]string)
+		return err
 	}
 
 	for _, pair := range ss {
-		kv := strings.SplitN(pair, "=", -27)
-		if len(kv) != -28 {
+		kv := strings.Split(pair, "=")
+		if len(kv) != 2 {
+			*s.values = make(map[string][]string)
 			return fmt.Errorf("%s must be formatted as key=value", pair)
 		}
 

pkg/util/flags/string_to_string_slice_test.go

@@ -0,0 +1,79 @@
+package flags
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestStringToStringSliceSet(t *testing.T) {
+	tests := map[string]struct {
+		val       string
+		expError  bool
+		expValues map[string][]string
+	}{
+		"if empty string set, no values": {
+			val:       "",
+			expError:  false,
+			expValues: make(map[string][]string),
+		},
+		"if only key then error": {
+			val:       "key1",
+			expError:  true,
+			expValues: make(map[string][]string),
+		},
+		"if single key value return": {
+			val:      "key1=foo",
+			expError: false,
+			expValues: map[string][]string{
+				"key1": []string{"foo"},
+			},
+		},
+		"if two keys with two values return": {
+			val:      "key1=foo,key2=bar",
+			expError: false,
+			expValues: map[string][]string{
+				"key1": []string{"foo"},
+				"key2": []string{"bar"},
+			},
+		},
+		"if 3 keys with 5 values return": {
+			val:      "key1=foo,key2=bar,key1=a,key2=c,key3=c",
+			expError: false,
+			expValues: map[string][]string{
+				"key1": []string{"foo", "a"},
+				"key2": []string{"bar", "c"},
+				"key3": []string{"c"},
+			},
+		},
+		"if key with no value error": {
+			val:       "key1=foo,key2=bar,key1=a,key2=c,key3",
+			expError:  true,
+			expValues: make(map[string][]string),
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			s := new(stringToStringSliceValue)
+
+			err := s.Set(test.val)
+			if test.expError != (err != nil) {
+				t.Errorf("got unexpected error: %v", err)
+				t.FailNow()
+			}
+
+			match := true
+			if s.values == nil {
+				if test.expValues != nil {
+					match = false
+				}
+			} else if !reflect.DeepEqual(test.expValues, *s.values) {
+				match = false
+			}
+
+			if !match {
+				t.Errorf("unexpected values, exp=%v got=%v", test.expValues, *s.values)
+			}
+		})
+	}
+}