Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: Use custom named map instead of StringToStringVar #19968

Merged
merged 1 commit into from
Jun 3, 2022

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented May 26, 2022

Description

Out of all the flags having string map type, only these two flags are
StringToStringVar type, which failed validation if the passed value is
json format.

2022-05-26T12:03:33.817769447Z level=fatal msg="Incorrect config-map flag subnet-tags-filter" error="{\"type\":\"private\", \"another\": \"value\" } must be formatted as key=value" subsys=config

Relates: #16014
Fixes: #19961
Signed-off-by: Tam Mach tam.mach@cilium.io

Testing

Testing was done locally before and after for below helm value file.

extraConfig:
  "subnet-tags-filter": "{\"type\":\"private\", \"another\": \"value\" }"
  "api-rate-limit": "{\"endpoint-create\": \"rate-limit:10/s,rate-burst:10,parallel-requests:10,auto-adjust:true\", \"endpoint-delete\": \"rate-limit:10/s,rate-burst:10,parallel-requests:10,auto-adjust:true\", \"endpoint-get\": \"rate-limit:10/s,rate-burst:10,parallel-requests:10,auto-adjust:true\", \"endpoint-list\": \"rate-limit:10/s,rate-burst:10,parallel-requests:10,auto-adjust:true\", \"endpoint-patch\": \"rate-limit:10/s,rate-burst:10,parallel-requests:10,auto-adjust:true\"}"

@maintainer-s-little-helper 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 May 26, 2022
@sayboras sayboras added the release-note/bug This PR fixes an issue in a previous release of Cilium. label May 26, 2022
@maintainer-s-little-helper 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 May 26, 2022
@sayboras sayboras force-pushed the tam/issue-19961 branch 2 times, most recently from 11fcedb to a8a5fbb Compare May 26, 2022 12:34
Out of all the flags having string map type, only these two flags are
StringToStringVar type, which failed validation if the passed value is
json format.

```
2022-05-26T12:03:33.817769447Z level=fatal msg="Incorrect config-map flag subnet-tags-filter" error="{\"type\":\"private\"} must be formatted as key=value" subsys=config
```

Relates: cilium#16014
Fixes: cilium#19961
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@sayboras sayboras marked this pull request as ready for review May 26, 2022 14:08
@sayboras sayboras requested review from a team as code owners May 26, 2022 14:08
@sayboras sayboras requested a review from nebril May 26, 2022 14:08
@sayboras
Copy link
Member Author

/test

@sayboras
Copy link
Member Author

sayboras commented May 26, 2022

/test-1.23-net-next

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed:

Click to show.

Test Name

K8sServicesTest Checks N/S loadbalancing Tests with XDP, direct routing, SNAT and Random

Failure Output

FAIL: Can not connect to service "tftp://[fd04::11]:31667/hello" from outside cluster (4/10)

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create one.

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this.

@sayboras
Copy link
Member Author

/test-1.23-net-next

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.6 May 31, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.12 May 31, 2022
@sayboras
Copy link
Member Author

Reviews are in, all required tests are passed. Marking this ready to merge.

@sayboras sayboras added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed needs-backport/1.10 labels May 31, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from master in 1.10.12 May 31, 2022
@aanm aanm merged commit 2d35b95 into cilium:master Jun 3, 2022
@sayboras sayboras deleted the tam/issue-19961 branch June 3, 2022 15:22
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.11.6 Jun 8, 2022
@joestringer joestringer added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Jun 9, 2022
@joestringer joestringer moved this from Backport pending to v1.10 to Backport done to v1.11 in 1.11.6 Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.11.6
Backport done to v1.11
Development

Successfully merging this pull request may close these issues.

StringMapString arguments not always accepted as JSON in
6 participants