Fix #10012 - empty string as a parameter in etcd extra args #10018
Conversation
Signed-off-by: Jordano Celestrini <[email protected]>
jordanorc
force-pushed
the
fix-empty-string-parameter-etcd-extra-args
branch
from
136e14c
to
a0a4885
Compare
| @@ -103,7 +103,7 @@ func (e ETCDConfig) ToConfigFile(extraArgs []string) (string, error) { | |||
| } else if time, err := time.ParseDuration(extraArg[1]); err == nil && (strings.Contains(lowerKey, "time") || strings.Contains(lowerKey, "duration") || strings.Contains(lowerKey, "interval") || strings.Contains(lowerKey, "retention")) { | |||
| // auto-compaction-retention is either a time.Duration or int, depending on version. If it is an int, it will be caught above. | |||
| s[key] = time | |||
| } else if err := yaml.Unmarshal([]byte(extraArg[1]), &stringArr); err == nil { | |||
| } else if err := yaml.Unmarshal([]byte(extraArg[1]), &stringArr); err == nil && (len(extraArg[1]) > 0) { | |||
There was a problem hiding this comment.
I think this belongs up on line 97? if len(extraArg) == 2 && len(extraArg[1]) > 0
There was a problem hiding this comment.
If I do that, the parameter will not be inserted into the s map (https://github.com/jordanorc/k3s/blob/a0a4885eafd39163e417601029bd19073ced7cdd/pkg/daemons/executor/executor.go#L107) and will never be overridden with an empty string.
There was a problem hiding this comment.
OK but what is your intent here - if the arg value is an empty string, do you want the config value set to an empty string, or do you want to ensure that the key is removed so that it takes the default value?
What you're doing here will set the value to an empty string, which may be valid for config entries that expect a string value. That is not all of them, as shown by the special handling for integer and time/duration fields.
If your intent is to get the default value by removing the key from the config, then I believe a different approach is required.
|
etcd args are already somewhat unique in that they are converted to config json, which has a side effect of deduplicating flags. Most other flags are simply appended to our defaults and passed into the component CLI for the component's internal flag parser to figure out. With this change you will be able to unset/mask k3s default flags by setting them to an empty string, which will make the etcd args even more unique. |
@brandond I partially agree with you. Actually, the behavior between the Running the command: k3s server --cluster-init --kube-apiserver-arg=authorization-mode=Results in the log:
The current behavior of Despite the fact that it is not possible to unset a value with |
| @@ -103,7 +103,7 @@ func (e ETCDConfig) ToConfigFile(extraArgs []string) (string, error) { | |||
| } else if time, err := time.ParseDuration(extraArg[1]); err == nil && (strings.Contains(lowerKey, "time") || strings.Contains(lowerKey, "duration") || strings.Contains(lowerKey, "interval") || strings.Contains(lowerKey, "retention")) { | |||
| // auto-compaction-retention is either a time.Duration or int, depending on version. If it is an int, it will be caught above. | |||
| s[key] = time | |||
| } else if err := yaml.Unmarshal([]byte(extraArg[1]), &stringArr); err == nil { | |||
| } else if err := yaml.Unmarshal([]byte(extraArg[1]), &stringArr); err == nil && (len(extraArg[1]) > 0) { | |||
There was a problem hiding this comment.
OK but what is your intent here - if the arg value is an empty string, do you want the config value set to an empty string, or do you want to ensure that the key is removed so that it takes the default value?
What you're doing here will set the value to an empty string, which may be valid for config entries that expect a string value. That is not all of them, as shown by the special handling for integer and time/duration fields.
If your intent is to get the default value by removing the key from the config, then I believe a different approach is required.
Proposed Changes
This modification checks if a parameter passed in
--etcd-argis an empty string, preventing it from being wrongly converted to an empty array by the functionToConfigFile.Types of Changes
BugFix
Verification
k3sfollowing the tutorial in https://github.com/k3s-io/k3s/blob/master/BUILDING.md./dist/artifacts/k3s server --cluster-init --etcd-arg=listen-metrics-urls=Instead of throwing the error:
error unmarshaling JSON: while decoding JSON: json: cannot unmarshal array into Go struct field configYAML.listen-metrics-urls of type string, k3s will continue the startup process.Testing
See linked issue
Linked Issues
#10012
User-Facing Change
NONE