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
Add validation of agent flag values for ConfigMap #16014
Add validation of agent flag values for ConfigMap #16014
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overall it's a good idea to add validation, however we need to discuss whether it's expected that unknown flags are ignored in case of an upgrade or downgrade.
The validation works for only existing flags. In case of unknown flags Cilium will start with these unknown flags. |
8caaf2f
to
77722ce
Compare
77722ce
to
228edb7
Compare
Thanks for the approve, Paul! |
Guys, could you please start k8s tests? |
test-me-please |
If the Cilium agent flags are passed via a mounted ConfigMap (cilium-agent --config-dir=/tmp/cilium/config-map), the default for Helm deployments, the flag values are not validated. For example if you set "restore" with invalid value "0SO##ME5_RANDOM" in ConfigMap then Agent would run with incorrect parameter: ..... level=info msg=" --restore='0SO##ME5_RANDOM'" subsys=daemon ..... But if start Agent with CLI then the validation will warn and prevent starting the agent: cilium-agent[8654]: invalid argument "0SO##ME5_RANDOM" for "--restore" flag: strconv.ParseBool: parsing "0SO##ME5_RANDOM": invalid syntax This commit add agent flag values validation for ConfigMap Fixes: cilium#13070 Signed-off-by: Roman Ptitcyn romanspb@yahoo.com
228edb7
to
24f61bb
Compare
Thanks for clarifying this, I can see that's also stated in the PR title also. I guess it's still possible for validation to break between versions, as flags are not strongly typed. However, I think the benefit of having validation in place is much greater! |
test-me-please |
Thanks Ilya! |
Provisioning issue: |
Provisioning error: |
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>
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: #16014 Fixes: #19961 Signed-off-by: Tam Mach <tam.mach@cilium.io>
[ upstream commit 2d35b95 ] 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> Signed-off-by: Jussi Maki <jussi@isovalent.com>
[ upstream commit 2d35b95 ] 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: #16014 Fixes: #19961 Signed-off-by: Tam Mach <tam.mach@cilium.io> Signed-off-by: Jussi Maki <jussi@isovalent.com>
If the Cilium agent flags are passed via a mounted ConfigMap
(cilium-agent --config-dir=/tmp/cilium/config-map), the default for Helm
deployments, the flag values are not validated. For example if you set
"restore" with invalid value "0SO##ME5_RANDOM" in ConfigMap then Agent
would run with incorrect parameter:
.....
level=info msg=" --restore='0SO##ME5_RANDOM'" subsys=daemon
.....
But if start Agent with CLI then the validation will warn and prevent
starting the agent:
cilium-agent[8654]: invalid argument "0SO##ME5_RANDOM" for "--restore"
flag: strconv.ParseBool: parsing "0SO##ME5_RANDOM": invalid syntax
This commit add agent flag values validation for ConfigMap
Fixes: #13070
Signed-off-by: Roman Ptitcyn romanspb@yahoo.com