cmd: Fix issue reading string map type via config map #18478

sayboras · 2022-01-14T06:20:27Z

Description

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. This is backward
compatible change.

Relates spf13/viper#911
Fixes #18328

Signed-off-by: Tam Mach tam.mach@isovalent.com

Testing

Testing was done locally as per below

before

$ cat tmp/subnet-tags-filter
k1=v1,k2=v2% 

/usr/lib/go-1.17/bin/go build -o /tmp/GoLand/___2go_build_github_com_cilium_cilium_operator github.com/cilium/cilium/operator #gosetup
/tmp/GoLand/___2go_build_github_com_cilium_cilium_operator --config-dir tmp/
level=info msg="Started gops server" address="127.0.0.1:9891" subsys=___2go_build_github_com_cilium_cilium_operator
level=warning msg="Running Cilium with \"kvstore\"=\"\" requires identity allocation via CRDs. Changing identity-allocation-mode to \"crd\"" subsys=config
...
level=info msg="  --subnet-ids-filter=''" subsys=___2go_build_github_com_cilium_cilium_operator
level=info msg="  --subnet-ids-filter=''" subsys=___2go_build_github_com_cilium_cilium_operator --> THIS IS EMPTY
level=info msg="  --synchronize-k8s-nodes='true'" subsys=___2go_build_github_com_cilium_cilium_operator
...
level=info msg="Cilium Operator  go version go1.17 linux/amd64" subsys=___2go_build_github_com_cilium_cilium_operator

after

/usr/lib/go-1.17/bin/go build -o /tmp/GoLand/___2go_build_github_com_cilium_cilium_operator github.com/cilium/cilium/operator #gosetup
/tmp/GoLand/___2go_build_github_com_cilium_cilium_operator --config-dir tmp/
level=info msg="Started gops server" address="127.0.0.1:9891" subsys=___2go_build_github_com_cilium_cilium_operator
level=warning msg="Running Cilium with \"kvstore\"=\"\" requires identity allocation via CRDs. Changing identity-allocation-mode to \"crd\"" subsys=config

...
level=info msg="  --subnet-ids-filter=''" subsys=___2go_build_github_com_cilium_cilium_operator
level=info msg="  --subnet-tags-filter='k1=v1,k2=v2'" subsys=___2go_build_github_com_cilium_cilium_operator --> THIS HAS CORRECT VALUE
level=info msg="  --synchronize-k8s-nodes='true'" subsys=___2go_build_github_com_cilium_cilium_operator
...
level=info msg="Cilium Operator  go version go1.17 linux/amd64" subsys=___2go_build_github_com_cilium_cilium_operator

cmd: Fix issue reading string map type via config map

sayboras · 2022-01-14T08:42:35Z

pkg/command/map_string.go

+// SPDX-License-Identifier: Apache-2.0
+// Copyright 2022 Authors of Cilium
+
+package command


I look around, and find this package is the most relevant, feel free to let me know otherwise.

We also have tools/, what do you think?

tools directory is used for extra utility (e.g. lint check, dev-doctor, etc), should not be used in any main application code.

sayboras · 2022-01-14T08:43:30Z

contrib/scripts/check-viper-get-string-map-string.sh

+
+# Simple script to make sure viper.GetStringMapString should not be used.
+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";


very naive check here, this will not work with import alias, but we all get used to viper. I reckon.

glibsm · 2022-01-14T18:24:50Z

pkg/command/map_string_test.go

+		{
+			name: "value with invalid json format",
+			args: args{
+				key:   "FOO_BAR",
+				value: `{"k1":"v1","k2":"v2",}`,
+			},
+			want: map[string]string{},
+		},


in the error case like this i would usually check the error message for strings.Contains something specific like bad json or extra comma, I'm not sure what sort of a message is printed here.

good point, I see the actual error in debugger only, making sense to make it explicit so that others don't need to re-check it. Let me refactor a little bit, and address related comments below as well.

pkg/command/map_string_test.go

pkg/command/map_string.go

ldelossa · 2022-01-14T20:31:39Z

@sayboras did we look at viper's marshaling functionality to accomplish this? https://github.com/spf13/viper#unmarshaling
https://sagikazarmark.hu/blog/decoding-custom-formats-with-viper/

This very well may not work, I'm just used to seeing Go libraries implement Marshal functions when having to deal with arbitrary text values.

sayboras · 2022-01-14T22:22:21Z

did we look at viper's marshaling functionality to accomplish this? https://github.com/spf13/viper#unmarshaling https://sagikazarmark.hu/blog/decoding-custom-formats-with-viper/

This very well may not work, I'm just used to seeing Go libraries implement Marshal functions when having to deal with arbitrary text values.

👋 I checked the custom marshall and unmarshall, however, it is nice apporach if we have a self-defined struct (e.g. config), which might not work in our case as we are getting value field by field.

The previous PR cilium#18478 wraps existing viper GetStringMapString function to get around upstream bugs, however, it's unintentionally restricted a few formats, which supported before in cilium, such as: - --aws-instance-limit-mapping=c6a.2xlarge=4,15,15,m4.xlarge=1,5,10 - --api-rate-limit=endpoint-create=rate-limit:10/s,rate-burst:10,parallel-requests:10,auto-adjust:true For complicated attribute, we are allowing comma character in value part of key value pair. As golang didn't support look-ahead functionalities in built-in regex library, this commit is to replace string.Split function by custom implementation to handle such scenario. Relates: cilium#18478 Fixes: cilium#18973 Signed-off-by: Tam Mach <tam.mach@cilium.io>

The previous PR #18478 wraps existing viper GetStringMapString function to get around upstream bugs, however, it's unintentionally restricted a few formats, which supported before in cilium, such as: - --aws-instance-limit-mapping=c6a.2xlarge=4,15,15,m4.xlarge=1,5,10 - --api-rate-limit=endpoint-create=rate-limit:10/s,rate-burst:10,parallel-requests:10,auto-adjust:true For complicated attribute, we are allowing comma character in value part of key value pair. As golang didn't support look-ahead functionalities in built-in regex library, this commit is to replace string.Split function by custom implementation to handle such scenario. Relates: #18478 Fixes: #18973 Signed-off-by: Tam Mach <tam.mach@cilium.io>

[ upstream commit 070ded0 ] The previous PR #18478 wraps existing viper GetStringMapString function to get around upstream bugs, however, it's unintentionally restricted a few formats, which supported before in cilium, such as: - --aws-instance-limit-mapping=c6a.2xlarge=4,15,15,m4.xlarge=1,5,10 - --api-rate-limit=endpoint-create=rate-limit:10/s,rate-burst:10,parallel-requests:10,auto-adjust:true For complicated attribute, we are allowing comma character in value part of key value pair. As golang didn't support look-ahead functionalities in built-in regex library, this commit is to replace string.Split function by custom implementation to handle such scenario. Relates: #18478 Fixes: #18973 Signed-off-by: Tam Mach <tam.mach@cilium.io> Signed-off-by: Gilberto Bertin <jibi@cilium.io>

[ upstream commit 070ded0 ] The previous PR cilium#18478 wraps existing viper GetStringMapString function to get around upstream bugs, however, it's unintentionally restricted a few formats, which supported before in cilium, such as: - --aws-instance-limit-mapping=c6a.2xlarge=4,15,15,m4.xlarge=1,5,10 - --api-rate-limit=endpoint-create=rate-limit:10/s,rate-burst:10,parallel-requests:10,auto-adjust:true For complicated attribute, we are allowing comma character in value part of key value pair. As golang didn't support look-ahead functionalities in built-in regex library, this commit is to replace string.Split function by custom implementation to handle such scenario. Relates: cilium#18478 Fixes: cilium#18973 Signed-off-by: Tam Mach <tam.mach@cilium.io> Signed-off-by: Jussi Maki <jussi@isovalent.com>

[ upstream commit 070ded0 ] The previous PR #18478 wraps existing viper GetStringMapString function to get around upstream bugs, however, it's unintentionally restricted a few formats, which supported before in cilium, such as: - --aws-instance-limit-mapping=c6a.2xlarge=4,15,15,m4.xlarge=1,5,10 - --api-rate-limit=endpoint-create=rate-limit:10/s,rate-burst:10,parallel-requests:10,auto-adjust:true For complicated attribute, we are allowing comma character in value part of key value pair. As golang didn't support look-ahead functionalities in built-in regex library, this commit is to replace string.Split function by custom implementation to handle such scenario. Relates: #18478 Fixes: #18973 Signed-off-by: Tam Mach <tam.mach@cilium.io> Signed-off-by: Jussi Maki <jussi@isovalent.com>

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 14, 2022

sayboras added area/cli Impacts the command line interface of any command in the repository. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jan 14, 2022

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 14, 2022

sayboras added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jan 14, 2022

sayboras force-pushed the tam/kv-flags branch from 4e724a8 to 8e46c6c Compare January 14, 2022 08:08

sayboras commented Jan 14, 2022

View reviewed changes

sayboras marked this pull request as ready for review January 14, 2022 09:19

sayboras requested a review from a team January 14, 2022 09:19

sayboras requested review from a team as code owners January 14, 2022 09:19

sayboras requested a review from ldelossa January 14, 2022 09:19

maintainer-s-little-helper bot assigned ldelossa Jan 14, 2022

sayboras requested review from glibsm, aditighag and aanm January 14, 2022 09:19

maintainer-s-little-helper bot assigned glibsm, aditighag and aanm and unassigned glibsm and aditighag Jan 14, 2022

glibsm reviewed Jan 14, 2022

View reviewed changes

joamaki mentioned this pull request Feb 7, 2022

v1.10 backports 2022-02-07 #18724

Merged

joamaki added backport-pending/1.10 and removed needs-backport/1.10 labels Feb 7, 2022

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.8 Feb 7, 2022

joamaki mentioned this pull request Feb 7, 2022

v1.11 backports 2022-02-07 #18726

Merged

joamaki added backport-pending/1.11 and removed needs-backport/1.11 labels Feb 7, 2022

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.11.2 Feb 7, 2022

joamaki added backport-done/1.10 and removed backport-pending/1.10 labels Feb 8, 2022

maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.8 Feb 8, 2022

joamaki added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Feb 8, 2022

maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.11 in 1.11.2 Feb 8, 2022

sayboras deleted the tam/kv-flags branch February 19, 2022 03:39

This was referenced Feb 23, 2022

Prepare for release v1.10.8 #18921

Merged

Prepare for release v1.11.2 #18922

Merged

kaworu mentioned this pull request Mar 2, 2022

Cilium 1.11.2. API Rate limit stopped working #18973

Closed

2 tasks

gandro mentioned this pull request Mar 11, 2022

Cilium 1.11.0 does not start on EKS with pod subnet-filter #18239

Closed

2 tasks

sayboras mentioned this pull request May 25, 2022

cmd: Allow more complicated patterns in map string type. #19955

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd: Fix issue reading string map type via config map #18478

cmd: Fix issue reading string map type via config map #18478

sayboras commented Jan 14, 2022 •

edited

sayboras Jan 14, 2022

aanm Jan 17, 2022

sayboras Jan 18, 2022

sayboras Jan 14, 2022

glibsm Jan 14, 2022

sayboras Jan 14, 2022

ldelossa commented Jan 14, 2022 •

edited

sayboras commented Jan 14, 2022

cmd: Fix issue reading string map type via config map #18478

cmd: Fix issue reading string map type via config map #18478

Conversation

sayboras commented Jan 14, 2022 • edited

Description

Testing

sayboras Jan 14, 2022

Choose a reason for hiding this comment

aanm Jan 17, 2022

Choose a reason for hiding this comment

sayboras Jan 18, 2022

Choose a reason for hiding this comment

sayboras Jan 14, 2022

Choose a reason for hiding this comment

glibsm Jan 14, 2022

Choose a reason for hiding this comment

sayboras Jan 14, 2022

Choose a reason for hiding this comment

ldelossa commented Jan 14, 2022 • edited

sayboras commented Jan 14, 2022

sayboras commented Jan 14, 2022 •

edited

ldelossa commented Jan 14, 2022 •

edited