diff --git a/pkg/api/pod/util.go b/pkg/api/pod/util.go index 679db9cf01c..67fb8fadbf5 100644 --- a/pkg/api/pod/util.go +++ b/pkg/api/pod/util.go @@ -275,16 +275,32 @@ func DropDisabledPodFields(pod, oldPod *api.Pod) { podAnnotations map[string]string oldPodSpec *api.PodSpec oldPodAnnotations map[string]string + podStatus *api.PodStatus + oldPodStatus *api.PodStatus ) if pod != nil { podSpec = &pod.Spec podAnnotations = pod.Annotations + podStatus = &pod.Status } if oldPod != nil { oldPodSpec = &oldPod.Spec oldPodAnnotations = oldPod.Annotations + oldPodStatus = &oldPod.Status } dropDisabledFields(podSpec, podAnnotations, oldPodSpec, oldPodAnnotations) + dropPodStatusDisabledFields(podStatus, oldPodStatus) +} + +// dropDisabledFields removes disabled fields from the pod status +func dropPodStatusDisabledFields(podStatus *api.PodStatus, oldPodStatus *api.PodStatus) { + // trim PodIPs down to only one entry (non dual stack). + if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && + !multiplePodIPsInUse(oldPodStatus) { + if len(podStatus.PodIPs) != 0 { + podStatus.PodIPs = podStatus.PodIPs[0:1] + } + } } // dropDisabledFields removes disabled fields from the pod metadata and spec. @@ -719,3 +735,14 @@ func csiInUse(podSpec *api.PodSpec) bool { } return false } + +// podPriorityInUse returns true if status is not nil and number of PodIPs is greater than one +func multiplePodIPsInUse(podStatus *api.PodStatus) bool { + if podStatus == nil { + return false + } + if len(podStatus.PodIPs) > 1 { + return true + } + return false +} diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index 21afcda175e..574d7052c38 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -1738,3 +1738,104 @@ func TestDropSubPathExpr(t *testing.T) { } } } + +// helper creates a podStatus with list of PodIPs +func makePodStatus(podIPs []api.PodIP) *api.PodStatus { + return &api.PodStatus{ + PodIPs: podIPs, + } +} + +func TestDropStatusPodIPs(t *testing.T) { + testCases := []struct { + name string + podStatus *api.PodStatus + oldPodStatus *api.PodStatus + comparePodStatus *api.PodStatus + enableDualStack bool + }{ + { + name: "nil pod ips", + enableDualStack: false, + podStatus: makePodStatus(nil), + oldPodStatus: nil, + comparePodStatus: makePodStatus(nil), + }, + { + name: "empty pod ips", + enableDualStack: false, + podStatus: makePodStatus([]api.PodIP{}), + oldPodStatus: nil, + comparePodStatus: makePodStatus([]api.PodIP{}), + }, + { + name: "single family ipv6", + enableDualStack: false, + podStatus: makePodStatus([]api.PodIP{{IP: "::1"}}), + comparePodStatus: makePodStatus([]api.PodIP{{IP: "::1"}}), + }, + { + name: "single family ipv4", + enableDualStack: false, + podStatus: makePodStatus([]api.PodIP{{IP: "1.1.1.1"}}), + comparePodStatus: makePodStatus([]api.PodIP{{IP: "1.1.1.1"}}), + }, + { + name: "dualstack 4-6", + enableDualStack: true, + podStatus: makePodStatus([]api.PodIP{{IP: "1.1.1.1"}, {IP: "::1"}}), + comparePodStatus: makePodStatus([]api.PodIP{{IP: "1.1.1.1"}, {IP: "::1"}}), + }, + { + name: "dualstack 6-4", + enableDualStack: true, + podStatus: makePodStatus([]api.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + comparePodStatus: makePodStatus([]api.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + }, + { + name: "not dualstack 6-4=>4only", + enableDualStack: false, + podStatus: makePodStatus([]api.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + oldPodStatus: nil, + comparePodStatus: makePodStatus([]api.PodIP{{IP: "::1"}}), + }, + { + name: "not dualstack 6-4=>as is (used in old)", + enableDualStack: false, + podStatus: makePodStatus([]api.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + oldPodStatus: makePodStatus([]api.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + comparePodStatus: makePodStatus([]api.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + }, + { + name: "not dualstack 6-4=>6only", + enableDualStack: false, + podStatus: makePodStatus([]api.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + oldPodStatus: nil, + comparePodStatus: makePodStatus([]api.PodIP{{IP: "::1"}}), + }, + { + name: "not dualstack 6-4=>as is (used in old)", + enableDualStack: false, + podStatus: makePodStatus([]api.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + oldPodStatus: makePodStatus([]api.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + comparePodStatus: makePodStatus([]api.PodIP{{IP: "::1"}, {IP: "1.1.1.1"}}), + }, + } + + for _, tc := range testCases { + func() { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)() + dropPodStatusDisabledFields(tc.podStatus, tc.oldPodStatus) + + old := tc.oldPodStatus.DeepCopy() + // old pod status should never be changed + if !reflect.DeepEqual(tc.oldPodStatus, old) { + t.Errorf("%v: old pod status changed: %v", tc.name, diff.ObjectReflectDiff(tc.oldPodStatus, old)) + } + + if !reflect.DeepEqual(tc.podStatus, tc.comparePodStatus) { + t.Errorf("%v: unexpected pod status: %v", tc.name, diff.ObjectReflectDiff(tc.podStatus, tc.comparePodStatus)) + } + }() + } +} diff --git a/pkg/registry/core/node/strategy.go b/pkg/registry/core/node/strategy.go index 3f853a51395..8782b78b26f 100644 --- a/pkg/registry/core/node/strategy.go +++ b/pkg/registry/core/node/strategy.go @@ -71,6 +71,7 @@ func (nodeStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { node.Spec.ConfigSource = nil node.Status.Config = nil } + dropDisabledFields(node, nil) } // PrepareForUpdate clears fields that are not allowed to be set by end users on update. @@ -82,6 +83,27 @@ func (nodeStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Objec if !utilfeature.DefaultFeatureGate.Enabled(features.DynamicKubeletConfig) && !nodeConfigSourceInUse(oldNode) { newNode.Spec.ConfigSource = nil } + dropDisabledFields(newNode, oldNode) +} + +func dropDisabledFields(node *api.Node, oldNode *api.Node) { + if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && !multiNodeCIDRsInUse(oldNode) { + if len(node.Spec.PodCIDRs) > 1 { + node.Spec.PodCIDRs = node.Spec.PodCIDRs[0:1] + } + } +} + +// multiNodeCIDRsInUse returns true if Node.Spec.PodCIDRs is greater than one +func multiNodeCIDRsInUse(node *api.Node) bool { + if node == nil { + return false + } + + if len(node.Spec.PodCIDRs) > 1 { + return true + } + return false } // nodeConfigSourceInUse returns true if node's Spec ConfigSource is set(used) diff --git a/pkg/registry/core/node/strategy_test.go b/pkg/registry/core/node/strategy_test.go index 9d1adbaf5ed..8b107f7f726 100644 --- a/pkg/registry/core/node/strategy_test.go +++ b/pkg/registry/core/node/strategy_test.go @@ -17,13 +17,19 @@ limitations under the License. package node import ( + "reflect" "testing" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/util/diff" apitesting "k8s.io/kubernetes/pkg/api/testing" api "k8s.io/kubernetes/pkg/apis/core" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/kubernetes/pkg/features" + // install all api groups for testing _ "k8s.io/kubernetes/pkg/api/testapi" ) @@ -57,3 +63,106 @@ func TestSelectableFieldLabelConversions(t *testing.T) { nil, ) } + +// helper creates a NodeNode with a set of PodCIDRs +func makeNodeWithCIDRs(podCIDRs []string) *api.Node { + return &api.Node{ + Spec: api.NodeSpec{ + PodCIDRs: podCIDRs, + }, + } +} + +func TestDropPodCIDRs(t *testing.T) { + testCases := []struct { + name string + node *api.Node + oldNode *api.Node + compareNode *api.Node + enableDualStack bool + }{ + { + name: "nil pod cidrs", + enableDualStack: false, + node: makeNodeWithCIDRs(nil), + oldNode: nil, + compareNode: makeNodeWithCIDRs(nil), + }, + { + name: "empty pod ips", + enableDualStack: false, + node: makeNodeWithCIDRs([]string{}), + oldNode: nil, + compareNode: makeNodeWithCIDRs([]string{}), + }, + { + name: "single family ipv6", + enableDualStack: false, + node: makeNodeWithCIDRs([]string{"2000::/10"}), + compareNode: makeNodeWithCIDRs([]string{"2000::/10"}), + }, + { + name: "single family ipv4", + enableDualStack: false, + node: makeNodeWithCIDRs([]string{"10.0.0.0/8"}), + compareNode: makeNodeWithCIDRs([]string{"10.0.0.0/8"}), + }, + { + 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"}), + }, + { + 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"}), + }, + { + name: "not dualstack 6-4=>4only", + enableDualStack: false, + node: makeNodeWithCIDRs([]string{"2000::/10", "10.0.0.0/8"}), + oldNode: nil, + compareNode: makeNodeWithCIDRs([]string{"2000::/10"}), + }, + { + 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"}), + }, + { + name: "not dualstack 6-4=>6only", + enableDualStack: false, + node: makeNodeWithCIDRs([]string{"2000::/10", "10.0.0.0/8"}), + oldNode: nil, + compareNode: makeNodeWithCIDRs([]string{"2000::/10"}), + }, + { + 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"}), + }, + } + + for _, tc := range testCases { + func() { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.IPv6DualStack, tc.enableDualStack)() + dropDisabledFields(tc.node, tc.oldNode) + + old := tc.oldNode.DeepCopy() + // old node should never be changed + if !reflect.DeepEqual(tc.oldNode, old) { + t.Errorf("%v: old node changed: %v", tc.name, diff.ObjectReflectDiff(tc.oldNode, old)) + } + + if !reflect.DeepEqual(tc.node, tc.compareNode) { + t.Errorf("%v: unexpected node spec: %v", tc.name, diff.ObjectReflectDiff(tc.node, tc.compareNode)) + } + }() + } +}