-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix #10012 - empty string as a parameter in etcd extra args #10018
base: master
Are you sure you want to change the base?
Fix #10012 - empty string as a parameter in etcd extra args #10018
Conversation
Signed-off-by: Jordano Celestrini <[email protected]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this belongs up on line 97? if len(extraArg) == 2 && len(extraArg[1]) > 0
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@jordanorc I'm still interested in following up on this - can you answer the question I posed previously about intended behavior for empty strings, particularly with regards to non-string config fields?
|
Hi @brandond, Actually, it would be great if we had a way to reset the value of an etcd arg to its default value. But in this specific pull request, my intent is to fix the behavior of k3s that wrongly converts empty etcd arg strings into an empty array. The changes I proposed will not change the behavior for other fields, because the treatment for special fields like time/duration happens before my fix: k3s/pkg/daemons/executor/executor.go Line 103 in a0a4885
We can propose some test cases to validate the behavior. What do you think? |
Yeah, I guess I'd prefer to see tests that define the desired behavior, and then code changes to fix that. Instead of just fixing individual bits of value handling to get what you want for a specific use case. |
Proposed Changes
This modification checks if a parameter passed in
--etcd-arg
is an empty string, preventing it from being wrongly converted to an empty array by the functionToConfigFile
.Types of Changes
BugFix
Verification
k3s
following 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