daemon, option: Fix vlan bpf bypass ids loading by pippolo84 · Pull Request #20282 · cilium/cilium

pippolo84 · 2022-06-22T14:25:50Z

When installing cilium-agent via Helm, the configuration options are loaded reading files.
Each file represents an entry in the ConfigMap.
Doing so, all the values are loaded as strings and therefore must be converted to the specific option value type later.
This conversion is carried out by the spf13/viper package itself.

Internally, viper tries to load values from different sources:

flag
env
config file
key/value store
default

When loading values from a config file, unlike the flag code path, the conversion from string to the requested value type is carried out by the spf13/cast package.
The cast package does not support the conversion from string to slice of ints and the viper.GetIntSlice(...) function does not report any error if the conversion goes wrong.

As a temporary workaround, the bpf.vlanBypass option value type has been changed to a slice of strings, so to leave the conversion to a slice of ints to be implemented manually.

To avoid this workaround, two things are needed:

Support for converting a string to a slice of ints in the cast package: Add support for string to int slice conversion spf13/cast#146
Fix the validateConfigmap function to check option values using the cast package with the proper type conversion, instead of relying on pflag Set method.

Fixes: #20173

joestringer

docs change LGTM

pippolo84 · 2022-06-23T12:21:24Z

/test

gandro

Awesome, one minor nit

sayboras

Couple of small comments as per below.

This is the only one we are using int slice, and unlikely we have int slice in future, so up to you if you want to add something similar to https://github.com/cilium/cilium/blob/master/contrib/scripts/check-viper-get-string-map-string.sh

kaworu

Thanks! Helm related changes LGTM 👍

Through the ConfigMap, all config values are loaded as strings and then converted into the appropriate types. The conversion is carried out by Viper that does not report any kind of error if something goes wrong. To catch early any kind of conversion issue, we should use the same conversion APIs that Viper is using internally, so we check each option using the appropriate conversion function from the cast package. Related to cilium#20282 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Through the ConfigMap, all config values are loaded as strings and then converted into the appropriate types. The conversion is carried out by Viper that does not report any kind of error if something goes wrong. To catch early any kind of conversion issue, we should use the same conversion APIs that Viper is using internally, so we check each option using the appropriate conversion function from the cast package. Related to #20282 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

aanm · 2022-06-30T14:22:02Z

/test

sayboras · 2022-06-30T15:59:28Z

/test-1.24-net-next

Job 'Cilium-PR-K8s-GKE' failed:

Click to show.

Test Name

K8sPolicyTestExtended Validate toEntities KubeAPIServer Allows connection to KubeAPIServer

Failure Output

FAIL: Unexpected error:

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

rolinh

LGTM but given that this PR will be backported to v1.11, shouldn't the breaking change (--set bpf.vlan-bpf-bypass=4001,4002 => --set bpf.vlanBypass="{4001,4002}") be documented in the upgrade notes?

gandro · 2022-07-05T14:21:00Z

LGTM but given that this PR will be backported to v1.11, shouldn't the breaking change (--set bpf.vlan-bpf-bypass=4001,4002 => --set bpf.vlanBypass="{4001,4002}") be documented in the upgrade notes?

It's not a breaking change. The old syntax and Helm value just simply never worked, it was a complete no-op.
The actual breaking change happened in #20304 which is not backported to v1.11.

[ upstream commit 98280fa ] Through the ConfigMap, all config values are loaded as strings and then converted into the appropriate types. The conversion is carried out by Viper that does not report any kind of error if something goes wrong. To catch early any kind of conversion issue, we should use the same conversion APIs that Viper is using internally, so we check each option using the appropriate conversion function from the cast package. Related to #20282 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com> Signed-off-by: Quentin Monnet <quentin@isovalent.com>

[ upstream commit 98280fa ] Through the ConfigMap, all config values are loaded as strings and then converted into the appropriate types. The conversion is carried out by Viper that does not report any kind of error if something goes wrong. To catch early any kind of conversion issue, we should use the same conversion APIs that Viper is using internally, so we check each option using the appropriate conversion function from the cast package. Related to cilium#20282 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com> Signed-off-by: Quentin Monnet <quentin@isovalent.com> Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>

Through the ConfigMap, all config values are loaded as strings and then converted into the appropriate types. The conversion is carried out by Viper that does not report any kind of error if something goes wrong. To catch early any kind of conversion issue, we should use the same conversion APIs that Viper is using internally, so we check each option using the appropriate conversion function from the cast package. Related to cilium#20282 Signed-off-by: Fabio Falzoi <fabio.falzoi@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 Jun 22, 2022

sayboras added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jun 23, 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 Jun 23, 2022

pippolo84 force-pushed the pr/pippolo84/fix-helm-vlan-bpf-bypass-value-loading branch from 7815a69 to 00b14d1 Compare June 23, 2022 09:19

pippolo84 changed the title ~~daemon, option: fix vlan bpf bypass ids loading~~ daemon, option: Fix vlan bpf bypass ids loading Jun 23, 2022

pippolo84 mentioned this pull request Jun 23, 2022

Add support for string to int slice conversion spf13/cast#146

Open

pippolo84 added kind/bug This is a bug in the Cilium logic. backport/1.11 and removed backport/1.11 labels Jun 23, 2022

pippolo84 force-pushed the pr/pippolo84/fix-helm-vlan-bpf-bypass-value-loading branch from 00b14d1 to aac1500 Compare June 23, 2022 09:44

pippolo84 marked this pull request as ready for review June 23, 2022 09:45

pippolo84 requested a review from a team June 23, 2022 09:45

pippolo84 requested a review from a team as a code owner June 23, 2022 09:45

pippolo84 requested a review from a team June 23, 2022 09:45

pippolo84 requested review from a team as code owners June 23, 2022 09:45

pippolo84 requested review from joestringer, kaworu, ldelossa and tommyp1ckles June 23, 2022 09:45

joestringer approved these changes Jun 23, 2022

View reviewed changes

gandro self-requested a review June 23, 2022 12:19

gandro reviewed Jun 23, 2022

View reviewed changes

Comment thread Documentation/configuration/vlan-802.1q.rst Outdated

sayboras reviewed Jun 23, 2022

View reviewed changes

Comment thread pkg/option/config.go Outdated

Comment thread Documentation/configuration/vlan-802.1q.rst Outdated

kaworu approved these changes Jun 23, 2022

View reviewed changes

aanm approved these changes Jun 30, 2022

View reviewed changes

christarazi approved these changes Jul 1, 2022

View reviewed changes

rolinh reviewed Jul 5, 2022

View reviewed changes

rolinh approved these changes Jul 5, 2022

View reviewed changes

pippolo84 added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 5, 2022

qmonnet merged commit fdb0a26 into cilium:master Jul 5, 2022

qmonnet mentioned this pull request Jul 5, 2022

v1.12 backports 2022-07-05 #20401

Merged

22 tasks

qmonnet added backport-pending/1.12 and removed needs-backport/1.12 labels Jul 5, 2022

qmonnet mentioned this pull request Jul 6, 2022

v1.11 backports 2022-07-06 #20412

Merged

9 tasks

qmonnet added backport-pending/1.11 and removed needs-backport/1.11 labels Jul 6, 2022

ldelossa approved these changes Jul 6, 2022

View reviewed changes

ti-mo added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Jul 12, 2022

ti-mo added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Jul 13, 2022

joestringer mentioned this pull request Jul 15, 2022

Prepare for release v1.11.7 #20546

Merged

pippolo84 mentioned this pull request Jul 22, 2022

Remove check on intSlice type from config map validation #20638

Merged

pippolo84 mentioned this pull request Jan 26, 2024

operator: Implement CES Controller dynamic rate limiting #29861

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon, option: Fix vlan bpf bypass ids loading#20282

daemon, option: Fix vlan bpf bypass ids loading#20282
qmonnet merged 2 commits intocilium:masterfrom
pippolo84:pr/pippolo84/fix-helm-vlan-bpf-bypass-value-loading

pippolo84 commented Jun 22, 2022 •

edited

Loading

Uh oh!

joestringer left a comment

Uh oh!

pippolo84 commented Jun 23, 2022

Uh oh!

gandro left a comment

Uh oh!

Uh oh!

sayboras left a comment

Uh oh!

Uh oh!

Uh oh!

kaworu left a comment

Uh oh!

aanm commented Jun 30, 2022

Uh oh!

sayboras commented Jun 30, 2022 •

edited by maintainer-s-little-helper bot

Loading

Test Name

Failure Output

Uh oh!

rolinh left a comment

Uh oh!

gandro commented Jul 5, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Conversation

pippolo84 commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

pippolo84 commented Jun 23, 2022

Uh oh!

gandro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sayboras left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kaworu left a comment

Choose a reason for hiding this comment

Uh oh!

aanm commented Jun 30, 2022

Uh oh!

sayboras commented Jun 30, 2022 • edited by maintainer-s-little-helper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Name

Failure Output

Uh oh!

rolinh left a comment

Choose a reason for hiding this comment

Uh oh!

gandro commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

pippolo84 commented Jun 22, 2022 •

edited

Loading

sayboras commented Jun 30, 2022 •

edited by maintainer-s-little-helper bot

Loading

gandro commented Jul 5, 2022 •

edited

Loading