Skip to content

Commit

Permalink
cmd: Fix issue reading string map type via config map
Browse files Browse the repository at this point in the history
As mentioned in below upstream issue, there is a discrepancy in viper
while reading string map string data type i.e. kv pair format was not
supported, only `{"k":"v"}` format is allowed. This commit is to wrap
GetStringMapString implementation to handle such case.

Also, during the bootstrap, if there is any flag having invalid value,
fatal log will be printed for early detection and awareness.

Relates spf13/viper#911
Fixes #18328

Signed-off-by: Tam Mach <tam.mach@isovalent.com>
  • Loading branch information
sayboras authored and glibsm committed Jan 31, 2022
1 parent f888a04 commit 768659f
Show file tree
Hide file tree
Showing 7 changed files with 414 additions and 8 deletions.
2 changes: 2 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,8 @@ endif
$(QUIET) contrib/scripts/check-assert-deep-equals.sh
@$(ECHO_CHECK) contrib/scripts/lock-check.sh
$(QUIET) contrib/scripts/lock-check.sh
@$(ECHO_CHECK) contrib/scripts/check-viper-get-string-map-string.sh
$(QUIET) contrib/scripts/check-viper-get-string-map-string.sh
ifeq ($(SKIP_CUSTOMVET_CHECK),"false")
@$(ECHO_CHECK) contrib/scripts/custom-vet-check.sh
$(QUIET) contrib/scripts/custom-vet-check.sh
Expand Down
22 changes: 22 additions & 0 deletions contrib/scripts/check-viper-get-string-map-string.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#!/bin/bash
#
# Copyright 2022 Authors of Cilium
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# Simple script to make sure viper.GetStringMapString should not be used.
# Related upstream issue https://github.com/spf13/viper/issues/911
if grep -r --exclude-dir={.git,_build,vendor,contrib} -i --include \*.go "viper.GetStringMapString" .; then
echo "Found viper.GetStringMapString(key) usage. Please use command.GetStringMapString(viper.GetViper(), key) instead";
exit 1
fi
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ require (
github.com/servak/go-fastping v0.0.0-20160802140958-5718d12e20a0
github.com/shirou/gopsutil/v3 v3.21.12
github.com/sirupsen/logrus v1.8.1
github.com/spf13/cast v1.4.1
github.com/spf13/cobra v1.3.0
github.com/spf13/pflag v1.0.5
github.com/spf13/viper v1.10.1
Expand Down Expand Up @@ -194,7 +195,6 @@ require (
github.com/prometheus/common v0.32.1 // indirect
github.com/rogpeppe/go-internal v1.8.0 // indirect
github.com/spf13/afero v1.6.0 // indirect
github.com/spf13/cast v1.4.1 // indirect
github.com/spf13/jwalterweatherman v1.1.0 // indirect
github.com/subosito/gotenv v1.2.0 // indirect
github.com/tklauser/go-sysconf v0.3.9 // indirect
Expand Down
13 changes: 10 additions & 3 deletions operator/option/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/spf13/viper"

"github.com/cilium/cilium/pkg/command"
"github.com/cilium/cilium/pkg/logging"
"github.com/cilium/cilium/pkg/logging/logfields"
)
Expand Down Expand Up @@ -446,15 +447,21 @@ func (c *OperatorConfig) Populate() {
c.IPAMSubnetsIDs = m
}

if m := viper.GetStringMapString(IPAMSubnetsTags); len(m) != 0 {
if m, err := command.GetStringMapStringE(viper.GetViper(), IPAMSubnetsTags); err != nil {
log.Fatalf("unable to parse %s: %s", IPAMSubnetsTags, err)
} else if len(m) != 0 {
c.IPAMSubnetsTags = m
}

if m := viper.GetStringMapString(AWSInstanceLimitMapping); len(m) != 0 {
if m, err := command.GetStringMapStringE(viper.GetViper(), AWSInstanceLimitMapping); err != nil {
log.Fatalf("unable to parse %s: %s", AWSInstanceLimitMapping, err)
} else if len(m) != 0 {
c.AWSInstanceLimitMapping = m
}

if m := viper.GetStringMapString(ENITags); len(m) != 0 {
if m, err := command.GetStringMapStringE(viper.GetViper(), ENITags); err != nil {
log.Fatalf("unable to parse %s: %s", ENITags, err)
} else if len(m) != 0 {
c.ENITags = m
}
}
Expand Down
80 changes: 80 additions & 0 deletions pkg/command/map_string.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright 2022 Authors of Cilium

package command

import (
"encoding/json"
"errors"
"fmt"
"regexp"
"strings"
"unicode"

"github.com/spf13/cast"
"github.com/spf13/viper"
)

var keyValueRegex = regexp.MustCompile(`([\w-:./]+=[\w-:./]+,)*([\w-:./]+=[\w-:./]+)$`)

// GetStringMapString contains one enhancement to support k1=v2,k2=v2 compared to original
// implementation of GetStringMapString function
// Related upstream issue https://github.com/spf13/viper/issues/911
func GetStringMapString(vp *viper.Viper, key string) map[string]string {
v, _ := GetStringMapStringE(vp, key)
return v
}

// GetStringMapStringE is same as GetStringMapString, but with error
func GetStringMapStringE(vp *viper.Viper, key string) (map[string]string, error) {
data := vp.Get(key)
if data == nil {
return map[string]string{}, nil
}
v, err := cast.ToStringMapStringE(data)
if err != nil {
var syntaxErr *json.SyntaxError
if !errors.As(err, &syntaxErr) {
return v, err
}

switch s := data.(type) {
case string:
if len(s) == 0 {
return map[string]string{}, nil
}

// if the input is starting with either '{' or '[', just preserve original json parsing error.
firstIndex := strings.IndexFunc(s, func(r rune) bool {
return !unicode.IsSpace(r)
})
if firstIndex != -1 && (s[firstIndex] == '{' || s[firstIndex] == '[') {
return v, err
}

if !isValidKeyValuePair(s) {
return map[string]string{}, fmt.Errorf("'%s' is not formatted as key=value,key1=value1", s)
}

var v = map[string]string{}
kvs := strings.Split(s, ",")
for _, kv := range kvs {
temp := strings.Split(kv, "=")
if len(temp) != 2 {
return map[string]string{}, fmt.Errorf("'%s' is not formatted as key=value,key1=value1", s)
}
v[temp[0]] = temp[1]
}
return v, nil
}
}
return v, nil
}

// isValidKeyValuePair returns true if the input is following key1=value1,key2=value2,...,keyN=valueN format.
func isValidKeyValuePair(str string) bool {
if len(str) == 0 {
return true
}
return len(keyValueRegex.ReplaceAllString(str, "")) == 0
}

0 comments on commit 768659f

Please sign in to comment.