From 69c7fc19e3373e0c108425ab89a0ac1182de7a7f Mon Sep 17 00:00:00 2001 From: "Khaled Henidak(Kal)" Date: Wed, 3 Jul 2019 18:21:30 +0000 Subject: [PATCH] clean up: node dropDisabledFields --- pkg/registry/core/node/strategy.go | 20 ++-- pkg/registry/core/node/strategy_test.go | 131 ++++++++++++++++++------ 2 files changed, 112 insertions(+), 39 deletions(-) diff --git a/pkg/registry/core/node/strategy.go b/pkg/registry/core/node/strategy.go index 8782b78b26f..9cfebd4d622 100644 --- a/pkg/registry/core/node/strategy.go +++ b/pkg/registry/core/node/strategy.go @@ -66,11 +66,6 @@ func (nodeStrategy) AllowCreateOnUpdate() bool { // PrepareForCreate clears fields that are not allowed to be set by end users on creation. func (nodeStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { node := obj.(*api.Node) - // Nodes allow *all* fields, including status, to be set on create. - if !utilfeature.DefaultFeatureGate.Enabled(features.DynamicKubeletConfig) { - node.Spec.ConfigSource = nil - node.Status.Config = nil - } dropDisabledFields(node, nil) } @@ -80,13 +75,22 @@ func (nodeStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Objec oldNode := old.(*api.Node) newNode.Status = oldNode.Status - if !utilfeature.DefaultFeatureGate.Enabled(features.DynamicKubeletConfig) && !nodeConfigSourceInUse(oldNode) { - newNode.Spec.ConfigSource = nil - } dropDisabledFields(newNode, oldNode) } func dropDisabledFields(node *api.Node, oldNode *api.Node) { + // Nodes allow *all* fields, including status, to be set on create. + // for create + if !utilfeature.DefaultFeatureGate.Enabled(features.DynamicKubeletConfig) && oldNode == nil { + node.Spec.ConfigSource = nil + node.Status.Config = nil + } + + // for update + if !utilfeature.DefaultFeatureGate.Enabled(features.DynamicKubeletConfig) && !nodeConfigSourceInUse(oldNode) && oldNode != nil { + node.Spec.ConfigSource = nil + } + if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && !multiNodeCIDRsInUse(oldNode) { if len(node.Spec.PodCIDRs) > 1 { node.Spec.PodCIDRs = node.Spec.PodCIDRs[0:1] diff --git a/pkg/registry/core/node/strategy_test.go b/pkg/registry/core/node/strategy_test.go index 8b107f7f726..80dc00eee8e 100644 --- a/pkg/registry/core/node/strategy_test.go +++ b/pkg/registry/core/node/strategy_test.go @@ -64,94 +64,163 @@ func TestSelectableFieldLabelConversions(t *testing.T) { ) } -// helper creates a NodeNode with a set of PodCIDRs -func makeNodeWithCIDRs(podCIDRs []string) *api.Node { - return &api.Node{ +// helper creates a NodeNode with a set of PodCIDRs, Spec.ConfigSource, Status.Config +func makeNode(podCIDRs []string, addSpecDynamicConfig bool, addStatusDynamicConfig bool) *api.Node { + node := &api.Node{ Spec: api.NodeSpec{ PodCIDRs: podCIDRs, }, } + if addSpecDynamicConfig { + node.Spec.ConfigSource = &api.NodeConfigSource{} + } + if addStatusDynamicConfig { + node.Status = api.NodeStatus{ + Config: &api.NodeConfigStatus{}, + } + } + + return node } -func TestDropPodCIDRs(t *testing.T) { +func TestDropFields(t *testing.T) { testCases := []struct { - name string - node *api.Node - oldNode *api.Node - compareNode *api.Node - enableDualStack bool + name string + node *api.Node + oldNode *api.Node + compareNode *api.Node + enableDualStack bool + enableNodeDynamicConfig bool }{ { name: "nil pod cidrs", enableDualStack: false, - node: makeNodeWithCIDRs(nil), + node: makeNode(nil, false, false), oldNode: nil, - compareNode: makeNodeWithCIDRs(nil), + compareNode: makeNode(nil, false, false), }, { name: "empty pod ips", enableDualStack: false, - node: makeNodeWithCIDRs([]string{}), + node: makeNode([]string{}, false, false), oldNode: nil, - compareNode: makeNodeWithCIDRs([]string{}), + compareNode: makeNode([]string{}, false, false), }, { name: "single family ipv6", enableDualStack: false, - node: makeNodeWithCIDRs([]string{"2000::/10"}), - compareNode: makeNodeWithCIDRs([]string{"2000::/10"}), + node: makeNode([]string{"2000::/10"}, false, false), + compareNode: makeNode([]string{"2000::/10"}, false, false), }, { name: "single family ipv4", enableDualStack: false, - node: makeNodeWithCIDRs([]string{"10.0.0.0/8"}), - compareNode: makeNodeWithCIDRs([]string{"10.0.0.0/8"}), + node: makeNode([]string{"10.0.0.0/8"}, false, false), + compareNode: makeNode([]string{"10.0.0.0/8"}, false, false), }, { name: "dualstack 4-6", enableDualStack: true, - node: makeNodeWithCIDRs([]string{"10.0.0.0/8", "2000::/10"}), - compareNode: makeNodeWithCIDRs([]string{"10.0.0.0/8", "2000::/10"}), + node: makeNode([]string{"10.0.0.0/8", "2000::/10"}, false, false), + compareNode: makeNode([]string{"10.0.0.0/8", "2000::/10"}, false, false), }, { name: "dualstack 6-4", enableDualStack: true, - node: makeNodeWithCIDRs([]string{"2000::/10", "10.0.0.0/8"}), - compareNode: makeNodeWithCIDRs([]string{"2000::/10", "10.0.0.0/8"}), + node: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false), + compareNode: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false), }, { name: "not dualstack 6-4=>4only", enableDualStack: false, - node: makeNodeWithCIDRs([]string{"2000::/10", "10.0.0.0/8"}), + node: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false), oldNode: nil, - compareNode: makeNodeWithCIDRs([]string{"2000::/10"}), + compareNode: makeNode([]string{"2000::/10"}, false, false), }, { name: "not dualstack 6-4=>as is (used in old)", enableDualStack: false, - node: makeNodeWithCIDRs([]string{"2000::/10", "10.0.0.0/8"}), - oldNode: makeNodeWithCIDRs([]string{"2000::/10", "10.0.0.0/8"}), - compareNode: makeNodeWithCIDRs([]string{"2000::/10", "10.0.0.0/8"}), + node: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false), + oldNode: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false), + compareNode: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false), }, { name: "not dualstack 6-4=>6only", enableDualStack: false, - node: makeNodeWithCIDRs([]string{"2000::/10", "10.0.0.0/8"}), + node: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false), oldNode: nil, - compareNode: makeNodeWithCIDRs([]string{"2000::/10"}), + compareNode: makeNode([]string{"2000::/10"}, false, false), }, { name: "not dualstack 6-4=>as is (used in old)", enableDualStack: false, - node: makeNodeWithCIDRs([]string{"2000::/10", "10.0.0.0/8"}), - oldNode: makeNodeWithCIDRs([]string{"2000::/10", "10.0.0.0/8"}), - compareNode: makeNodeWithCIDRs([]string{"2000::/10", "10.0.0.0/8"}), + node: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false), + oldNode: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false), + compareNode: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false), + }, + { + name: "new with no Spec.ConfigSource and no Status.Config , enableNodeDynamicConfig disabled", + enableDualStack: false, + enableNodeDynamicConfig: false, + node: makeNode(nil, false, false), + oldNode: nil, + compareNode: makeNode(nil, false, false), + }, + { + name: "new with Spec.ConfigSource and no Status.Config, enableNodeDynamicConfig disabled", + enableDualStack: false, + enableNodeDynamicConfig: false, + node: makeNode(nil, true, false), + oldNode: nil, + compareNode: makeNode(nil, false, false), + }, + { + name: "new with Spec.ConfigSource and Status.Config, enableNodeDynamicConfig disabled", + enableDualStack: false, + enableNodeDynamicConfig: false, + node: makeNode(nil, true, true), + oldNode: nil, + compareNode: makeNode(nil, false, false), + }, + { + name: "update with Spec.ConfigSource and Status.Config (old has none), enableNodeDynamicConfig disabled", + enableDualStack: false, + enableNodeDynamicConfig: false, + node: makeNode(nil, true, true), + oldNode: makeNode(nil, false, false), + compareNode: makeNode(nil, false, true), + }, + { + name: "update with Spec.ConfigSource and Status.Config (old has them), enableNodeDynamicConfig disabled", + enableDualStack: false, + enableNodeDynamicConfig: false, + node: makeNode(nil, true, true), + oldNode: makeNode(nil, true, true), + compareNode: makeNode(nil, true, true), + }, + { + name: "update with Spec.ConfigSource and Status.Config (old has Status.Config), enableNodeDynamicConfig disabled", + enableDualStack: false, + enableNodeDynamicConfig: false, + node: makeNode(nil, true, true), + oldNode: makeNode(nil, false, true), + compareNode: makeNode(nil, false, true), + }, + { + name: "new with Spec.ConfigSource and Status.Config, enableNodeDynamicConfig enabled", + enableDualStack: false, + enableNodeDynamicConfig: true, + node: makeNode(nil, true, true), + oldNode: nil, + compareNode: makeNode(nil, true, true), }, } for _, tc := range testCases { func() { defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)() + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DynamicKubeletConfig, tc.enableNodeDynamicConfig)() + dropDisabledFields(tc.node, tc.oldNode) old := tc.oldNode.DeepCopy()