fix: Correctly handle variable overrides #1252
Conversation
jimmidyson
requested review from
dlipovetsky,
dkoshkin,
faiq and
thunderboltsid
jimmidyson
force-pushed
the
jimmi/fix-overrides
branch
from
1dd756d to
8af941a
Compare
|
#This commit fixes that by merging the global variables into the overrides, resulting in the behaviour that we always expected. I think this will break the worker nodes FD configuration with cluster.spec.topology.workers.machineDeployments.failureDomain with variables.overrides.workerConfig. In case of the "workerConfig" has the global configuration (supposed to be the default settings for worker nodes) with machineDetails having "cluster" and "subnets" settings, for the worker MD with FD setting, it is required to have the overrides variable "workerConfig" with machineDetails NOT have the "cluster" and "subnets" settings. But with this fix, the global setting of these two fields will appear in the MD's variable, it will fail the cluster object's webhook validation. Since the e2e tests "[e2e-quick-start (Nutanix provider)]" which include the FD test cases passed in the PR. It seems the above described scenario is not a concern. The CAREN Cluster webhook validation only checks the MD's overridden workerConfig settings if it's set, but not the merged one. And the MD spec after patch with both FD and cluster/subnets setting is allowd. |
|
We'd have to account for this projected merge in the cluster webhook https://github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/blob/main/pkg/webhook/cluster/nutanix_validator.go#L134-L149 |
Yeah, currently we validate against one or the other, now we have to validate against projected merge instead. |
Currently the metamutator only passes the override values when they are present on a machine deployment or control plane config. This commit fixes that by merging the global variables into the overrides, resulting in the behaviour that we always expected. As overrides are not that commonly used yet, we have not seen this issue but as taints, etc become more commonly used this will become an issue. Tests added to prove the fix as well.
jimmidyson
force-pushed
the
jimmi/fix-overrides
branch
from
b11bd79 to
97409aa
Compare
|
@thunderboltsid Please take a look at c5a0cdc. |
jimmidyson
force-pushed
the
jimmi/fix-overrides
branch
from
97409aa to
8a4743a
Compare
jimmidyson
force-pushed
the
jimmi/fix-overrides
branch
from
8a4743a to
c5a0cdc
Compare
| name: "both values are objects with nested objects with vars having nil object explicitly, merged", | ||
| args: args{ | ||
| vars: map[string]apiextensionsv1.JSON{ | ||
| "a": {Raw: []byte(`{"x":1,"y":{"a": 2,"b": null}}`)}, | ||
| }, | ||
| globalVars: map[string]apiextensionsv1.JSON{ | ||
| "a": {Raw: []byte(`{"y":{"a": 2,"b":{"c": 5, "d": 6}},"z":4}`)}, | ||
| }, | ||
| }, | ||
| want: map[string]apiextensionsv1.JSON{ | ||
| "a": {Raw: []byte(`{"x":1,"y":{"a": 2,"b":{"c": 5, "d": 6}},"z":4}`)}, | ||
| }, |
There was a problem hiding this comment.
We need to ensure that overrides have a mechanism to nullify the global vars else they can't really be considered an "override".
Currently the metamutator only passes the override values when they are
present on a machine deployment or control plane config. This commit
fixes that by merging the global variables into the overrides, resulting
in the behaviour that we always expected.
As overrides are not that commonly used yet, we have not seen this issue
but as taints, etc become more commonly used this will become an issue.
Tests added to prove the fix as well.