Fixes accidental override of file conf settings with defaults by trozet · Pull Request #1044 · ovn-kubernetes/ovn-kubernetes

trozet · 2020-02-04T21:54:08Z

When config is read, the config file is parsed first. After these
settings are loaded, the CLI settings are parsed and override any
settings read from the file. This is fine, except for the CLI settings
may be defaulted, which will just override any specified config file
settings with defaults.

This patch addresses the problem by checking during config override if
the the configuration option has a default specified, and if so, ignores
overriding the current config option.

Additionally, this patch adds real default values for settings.

Signed-off-by: Tim Rozet trozet@redhat.com

trozet · 2020-02-05T15:25:26Z

@danwinship

trozet · 2020-02-05T15:49:33Z

 	}
 	// And CLI overrides over config file and default values
-	if err := overrideFields(&Gateway, &cli.Gateway); err != nil {
+	if err := overrideFields(&Gateway, &cli.Gateway, nil); err != nil {


some config savedGateway is just empty values and no real defaults, so I just pass nil for those. Maybe I should pass var instead in case in the future there is a default value assigned for these...

danwinship · 2020-02-05T20:20:22Z

Hm... looks ugly but I don't know the config/defaulting code in ovn-kubernetes so I don't really have any suggestions (and maybe this is the best way).

(FYI among other things this breaks IPv6 with ovn-kubernetes in OCP 4.4 because the IPv6 clusterSubnet passed in via the config file gets overridden with the IPv4 default value.)

dcbw · 2020-02-06T15:13:42Z

This is fine, except for the CLI settings
may be defaulted, which will just override any specified config file
settings with defaults.

Looks like the only places that the CLI settings could get defaulted is for RawClusterSubnets and for boolean values? I think we should fix the defaulting for RawClusterSubnets by removing the Value: assignment in CommonFlags since that already has the same value in Default.RawClusterSubnets. And for booleans I don't think we need to care about supporting the config file setting the value to TRUE and then the CLI overriding that back to FALSE. So for bools perhaps we just logically OR the values together in overrideFields()? Is that enough to fix the problem?

trozet · 2020-02-06T17:00:57Z

This is fine, except for the CLI settings
may be defaulted, which will just override any specified config file
settings with defaults.

Looks like the only places that the CLI settings could get defaulted is for RawClusterSubnets and for boolean values? I think we should fix the defaulting for RawClusterSubnets by removing the Value: assignment in CommonFlags since that already has the same value in Default.RawClusterSubnets. And for booleans I don't think we need to care about supporting the config file setting the value to TRUE and then the CLI overriding that back to FALSE. So for bools perhaps we just logically OR the values together in overrideFields()? Is that enough to fix the problem?

I would argue that is just masking a bigger problem that settings are missing default values. For example,
cli.IntFlag{
Name: "mtu",
Usage: "MTU value used for the overlay networks (default: 1400)",
Destination: &cliConfig.Default.MTU,
},

Default value says 1400, but it has no default. We just assume here that the config will be set to 1400 by default by the config file when it gets deployed? We should probably add real defaults here to match, and then the code I added will ensure proper overriding occurs.

When config is read, the config file is parsed first. After these settings are loaded, the CLI settings are parsed and override any settings read from the file. This is fine, except for the CLI settings may be defaulted, which will just override any specified config file settings with defaults. This patch addresses the problem by checking during config override if the the configuration option has a default specified, and if so, ignores overriding the current config option. Additionally, this patch adds real default values for settings. Signed-off-by: Tim Rozet <trozet@redhat.com>

dcbw · 2020-02-07T23:23:15Z

Default value says 1400, but it has no default. We just assume here that the config will be set to 1400 by default by the config file when it gets deployed? We should probably add real defaults here to match, and then the code I added will ensure proper overriding occurs.

@trozet not sure what you mean here. The "default" value mentioned there actually comes from the top-level Default structure in config.go (around line 37). In this case, the way the code should work (though perhaps you've found that reality doesn't match specification) is that:

Default.MTU = 1400 (around line 37 of config.go)
cliConfig.Default.MTU = 0 (because the --mtu flag was not passed)
file Default.MTU = 0 (because nothing set it in the file)
buildDefaultConfig() calls overrideFields(&Default, &file.Default) but because file.Default.MTU is the empty value (0) the 1400 is left in Default.MTU.
buildDefaultConfig() calls overrideFields(&Default, &cli.Default) but because cli.Default.MTU is the empty value (0) the 1400 is left in Default.MTU.
and all the code looks in that top-level Default.MTU value and finds 1400

Basically, the top-level structures around lines 20 - 100 are (a) filled with the default values and (b) what the code actually looks at when all config is handled. Then as other config comes in (file, CLI, env vars, special files from kubernetes) those override the top-level structure values in a defined order.

This is quite probably confusing and could be documented better. But the end goal (however it gets done) is to have a precedence of defaults < kube SA < env-vars < config file < CLI. A clearer way to achieve this would certainly be welcome.

trozet · 2020-02-08T01:47:32Z

Yeah I mean the CLI settings themselves have no defaults. My latest patch sets them. I think this is a lot less ambiguous than the previous behavior. The problem is the configOverride method assumes what values are invalid. For example, it wont override if a setting is 0. What if a setting was added where 0 was a valid value? It also wont override if the boolean is false. What if a new setting was default true and we wanted to set it to false? That's why it is better to determine defaults in the settings themselves.

I'll add a follow up commit that will remove the checks for 0, true, etc from config override as they are not necessary.

With this change, any config setting is checked against its default to determine if the value is valid when parsed. The only case where empty string values are considered "unset" is with environment variables. Signed-off-by: Tim Rozet <trozet@redhat.com>

trozet · 2020-02-10T19:39:54Z

 		return err
 	}

+	// Grab default values from OVS external IDs


Note I moved this down here, so that if the behavior is if the booleans are set to detect from OVS, they will override all other settings. Not sure if that is the desired behavior or not.

@girishmg will this be OK with you? We don't use the OVS external IDs, so I think this would only affect your setups.

trozet · 2020-02-10T19:41:18Z

@dcbw take a look at the latest changes and see what you think. The checks with the switch statement for specific types are gone. Now it just compares against default values.

trozet · 2020-02-12T15:09:59Z

@danwinship noticed the latest patch isn't working, hold while I investigate it

trozet · 2020-02-12T15:47:32Z

Looks like the patch is working as expected. The issue was CLI arguments being passed via CNO setting cluster-subnets to nothing:
exec /usr/bin/ovnkube --init-node ovn-control-plane --cluster-subnets '' --k8s-service-cidr '' --k8s-apiserver https://192.168.1.5:11337

danwinship · 2020-02-12T16:26:53Z

Though it's reasonable to expect that in

ovnkube --init-node --config-file=/blah/blah --cluster-subnets=10.0.0.0/8

the --cluster-subnets arg overrides the config file, while in

ovnkube --init-node --cluster-subnets=10.0.0.0/8 ...  --config-file=/blah/blah

the config file overrides the --cluster-subnets arg

trozet · 2020-02-12T17:21:41Z

Though it's reasonable to expect that in
ovnkube --init-node --config-file=/blah/blah --cluster-subnets=10.0.0.0/8
the --cluster-subnets arg overrides the config file, while in
ovnkube --init-node --cluster-subnets=10.0.0.0/8 ...  --config-file=/blah/blah
the config file overrides the --cluster-subnets arg

I don't think so. That then implies ordering of arguments matters, which I don't think is the case today. IMO we should just make it known that CLI takes precedence over config file (should be in the usage) and not add more caveats (there's already too many ways to determine config). If others feel strongly the ordering should matter, we can take that up in another PR.

dcbw · 2020-02-13T15:07:22Z

Though it's reasonable to expect that in
ovnkube --init-node --config-file=/blah/blah --cluster-subnets=10.0.0.0/8
the --cluster-subnets arg overrides the config file, while in
ovnkube --init-node --cluster-subnets=10.0.0.0/8 ...  --config-file=/blah/blah
the config file overrides the --cluster-subnets arg

@danwinship current implementation has a clearly defined override, where CLI always overrides everything (config file, env args, kubernetes stuff, etc).

dcbw

/lgtm

danwinship · 2020-02-17T13:51:00Z

lgtm

trozet · 2020-02-17T14:20:16Z

@girishmg PTAL

Bug 2077129: [release-4.10] Fixes secondary bridge

trozet requested review from dcbw and girishmg February 4, 2020 21:54

trozet commented Feb 5, 2020

View reviewed changes

trozet force-pushed the dont_override_with_defaults branch from ce209da to 3051cca Compare February 7, 2020 02:14

trozet force-pushed the dont_override_with_defaults branch from c62792b to a158c45 Compare February 10, 2020 19:33

trozet commented Feb 10, 2020

View reviewed changes

dcbw approved these changes Feb 13, 2020

View reviewed changes

dcbw merged commit 3bbdf51 into ovn-kubernetes:master Feb 18, 2020

russellb mentioned this pull request Feb 19, 2020

Bug 1806009: Backport ovn-kubernetes-related changes to 4.3 openshift/cluster-network-operator#483

Merged

dcbw mentioned this pull request Mar 17, 2020

CARRY: hybrid-overlay: fix argument handling for Windows hybrid-overlay.exe openshift/ovn-kubernetes#118

Merged

kyrtapz pushed a commit to kyrtapz/ovn-kubernetes that referenced this pull request Jun 22, 2022

Merge pull request ovn-kubernetes#1044 from trozet/bz2077129

9495a1d

Bug 2077129: [release-4.10] Fixes secondary bridge

Uh oh!

Conversation

trozet commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trozet commented Feb 5, 2020

Uh oh!

trozet Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

danwinship commented Feb 5, 2020

Uh oh!

dcbw commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trozet commented Feb 6, 2020

Uh oh!

dcbw commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trozet commented Feb 8, 2020

Uh oh!

trozet Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

dcbw Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

trozet commented Feb 10, 2020

Uh oh!

trozet commented Feb 12, 2020

Uh oh!

trozet commented Feb 12, 2020

Uh oh!

danwinship commented Feb 12, 2020

Uh oh!

trozet commented Feb 12, 2020

Uh oh!

dcbw commented Feb 13, 2020

Uh oh!

dcbw left a comment

Choose a reason for hiding this comment

Uh oh!

danwinship commented Feb 17, 2020

Uh oh!

trozet commented Feb 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

trozet commented Feb 4, 2020 •

edited

Loading

dcbw commented Feb 6, 2020 •

edited

Loading

dcbw commented Feb 7, 2020 •

edited

Loading