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
[ upstream commit 768659f ]

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>
Signed-off-by: Jussi Maki <jussi@isovalent.com>
  • Loading branch information
sayboras authored and joamaki committed Feb 8, 2022
1 parent 5bfea24 commit 39177b0
Show file tree
Hide file tree
Showing 15 changed files with 593 additions and 63 deletions.
2 changes: 2 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,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
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ require (
github.com/servak/go-fastping v0.0.0-20160802140958-5718d12e20a0
github.com/shirou/gopsutil/v3 v3.21.2
github.com/sirupsen/logrus v1.8.1
github.com/spf13/cast v1.4.1
github.com/spf13/cobra v1.1.1
github.com/spf13/pflag v1.0.5
github.com/spf13/viper v1.7.1
Expand Down
3 changes: 2 additions & 1 deletion go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 12 additions & 5 deletions operator/option/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@ package option
import (
"time"

"github.com/spf13/viper"

"github.com/cilium/cilium/pkg/command"
"github.com/cilium/cilium/pkg/logging"
"github.com/cilium/cilium/pkg/logging/logfields"

"github.com/spf13/viper"
)

var log = logging.DefaultLogger.WithField(logfields.LogSubsys, "option")
Expand Down Expand Up @@ -431,15 +432,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 39177b0

Please sign in to comment.