kubeadm: skip validating pod subnet against node-cidr-mask when allocate-node-cidrs is set to be false #98984

SataQiu · 2021-02-11T08:13:20Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

kubeadm: skip validating pod subnet against node-cidr-mask when allocate-node-cidrs is set to be false

Which issue(s) this PR fixes:

Ref kubernetes/kubeadm#2391

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: skip validating pod subnet against node-cidr-mask when allocate-node-cidrs is set to be false

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-02-11T08:14:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SataQiu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kubeadm/OWNERS~~ [SataQiu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

SataQiu · 2021-02-11T08:28:30Z

/cc @aojea

If allocate-node-cidrs is already set to be false, the node IPAM controller will not be enabled.
So is it possible to skip the validation when allocate-node-cidrs is set to be false?

aojea · 2021-02-11T08:41:44Z

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

@@ -501,8 +501,10 @@ func ValidateNetworking(c *kubeadm.ClusterConfiguration, fldPath *field.Path) fi
 	}
 	if len(c.Networking.PodSubnet) != 0 {
 		allErrs = append(allErrs, ValidateIPNetFromString(c.Networking.PodSubnet, constants.MinimumAddressesInPodSubnet, isDualStack, field.NewPath("podSubnet"))...)
-		// Pod subnet was already validated, we need to validate now against the node-mask
-		allErrs = append(allErrs, ValidatePodSubnetNodeMask(c.Networking.PodSubnet, c, field.NewPath("podSubnet"))...)
+		if c.ControllerManager.ExtraArgs["allocate-node-cidrs"] != "false" {


we need unit tests cmd/kubeadm/app/phases/controlplane/manifests_test.go

Hi @aojea !
I'm sorry for the delay.
Could you please have a look?

If allocation-node-cidrs is not explicitly configured, it will be defaulted as true by kubeadm.
So we should validate the Pod subnet against the node-mask when c.ControllerManager.ExtraArgs["allocate-node-cidrs"] != "false".

I think is correct but let's double check with @neolit123 ^^^

the KCM flag allocate-node-cidrs is false by default, so it makes sense to validate in kubeadm only if the the user has set the flag to true.

If the PodSubnet is set and the ExtraArgs["allocate-node-cidrs"] is not set, kubeadm will add allocate-node-cidrs=true flag for KCM automatically.
In this case, can we skip this validation?
Or should we not add the allocate-node-cidrs flag for KCM automatically?

kubernetes/cmd/kubeadm/app/phases/controlplane/manifests.go

Lines 307 to 308 in 43e8405

if cfg.Networking.PodSubnet != "" {

defaultArguments["allocate-node-cidrs"] = "true"

the KCM docs say that cluster-cidr requires allocate-node-cidrs, so we should be setting it to true in the default args. adding the flag by default seems correct to me. this make it easier for the user and not require them to pass flags when they wish to set a pod subnet.

extraArgs are merged over default args.
so if the user was explicit about allocate-node-cidrs==false, we should respect it and skip validation in ValidateNetworking.

so the diff here seems valid to me.

aojea · 2021-02-11T08:48:49Z

If allocate-node-cidrs is already set to be false, the node IPAM controller will not be enabled.
So is it possible to skip the validation when allocate-node-cidrs is set to be false?

right

SataQiu · 2021-02-11T10:15:37Z

/test pull-kubernetes-e2e-kind

neolit123 · 2021-02-21T16:51:42Z

/triage accepted
/priority backlog

neolit123 · 2021-02-25T18:57:16Z

@SataQiu @aojea can we proceed with this PR for 1.21?

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

aojea · 2021-02-26T10:08:29Z

this lgtm , defer to @neolit123 to double check this logic #98984 (comment)

neolit123 · 2021-02-26T14:05:11Z

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

@@ -423,7 +423,7 @@ func ValidateServiceSubnetSize(subnetStr string, fldPath *field.Path) field.Erro
 }

 // ValidatePodSubnetNodeMask validates that the relation between podSubnet and node-masks is correct
-func ValidatePodSubnetNodeMask(subnetStr string, c *kubeadm.ClusterConfiguration, fldPath *field.Path) field.ErrorList {
+func ValidatePodSubnetNodeMask(subnetStr string, c *kubeadm.ClusterConfiguration, isDualStack bool, fldPath *field.Path) field.ErrorList {


i think we should leave the signature of ValidatePodSubnetNodeMask the way it is.

"dual stack" is a feature gate, and functions like getClusterNodeMask can continue extracting the value from the ClusterConfiguration:
isDualStack := features.Enabled(c.FeatureGates, features.IPv6DualStack)

isIPv6 is not a feature gate, it is a condition that matters to getClusterNodeMask

with this i also means that we don't have to do the vadation._test.go changes?

Emm, just found isDualStack is already calculated in parent function and a similar function ValidateIPNetFromString has already done like this. 😅

kubernetes/cmd/kubeadm/app/apis/kubeadm/validation/validation.go

Lines 495 to 506 in 43e8405

isDualStack := features.Enabled(c.FeatureGates, features.IPv6DualStack)

if len(c.Networking.ServiceSubnet) != 0 {

allErrs = append(allErrs, ValidateIPNetFromString(c.Networking.ServiceSubnet, constants.MinimumAddressesInServiceSubnet, isDualStack, field.NewPath("serviceSubnet"))...)

// Service subnet was already validated, we need to validate now the subnet size

allErrs = append(allErrs, ValidateServiceSubnetSize(c.Networking.ServiceSubnet, field.NewPath("serviceSubnet"))...)

}

if len(c.Networking.PodSubnet) != 0 {

allErrs = append(allErrs, ValidateIPNetFromString(c.Networking.PodSubnet, constants.MinimumAddressesInPodSubnet, isDualStack, field.NewPath("podSubnet"))...)

// Pod subnet was already validated, we need to validate now against the node-mask

allErrs = append(allErrs, ValidatePodSubnetNodeMask(c.Networking.PodSubnet, c, field.NewPath("podSubnet"))...)

}

ok, this seems like a separate refactor that can be done in the future.

…ate-node-cidrs is set to be false

neolit123 · 2021-03-03T04:24:32Z

/lgtm

pacoxu · 2021-03-03T07:14:01Z

/retest
for subpath flake

k8s-ci-robot requested review from pacoxu and yagonobre February 11, 2021 08:13

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 11, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2021

k8s-ci-robot requested a review from aojea February 11, 2021 08:28

SataQiu force-pushed the fix-kubeadm-20210211 branch from f1abc03 to f8c3dda Compare February 11, 2021 08:38

SataQiu changed the title ~~kubeadm: validate pod subnet against node-cidr-mask only when allocate-node-cidrs is set to be true~~ kubeadm: skip validating pod subnet against node-cidr-mask when allocate-node-cidrs is set to be false Feb 11, 2021

aojea reviewed Feb 11, 2021

View reviewed changes

SataQiu force-pushed the fix-kubeadm-20210211 branch from f8c3dda to 15c43e2 Compare February 26, 2021 02:37

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 26, 2021

pacoxu reviewed Feb 26, 2021

View reviewed changes

cmd/kubeadm/app/apis/kubeadm/validation/validation.go Show resolved Hide resolved

SataQiu force-pushed the fix-kubeadm-20210211 branch from 15c43e2 to 88a404c Compare February 26, 2021 03:17

neolit123 reviewed Feb 26, 2021

View reviewed changes

kubeadm: skip validating pod subnet against node-cidr-mask when alloc…

2ba178c

…ate-node-cidrs is set to be false

SataQiu force-pushed the fix-kubeadm-20210211 branch from 88a404c to 2ba178c Compare March 1, 2021 03:20

k8s-ci-robot assigned neolit123 Mar 3, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2021

k8s-ci-robot merged commit b0ba6c0 into kubernetes:master Mar 3, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: skip validating pod subnet against node-cidr-mask when allocate-node-cidrs is set to be false #98984

kubeadm: skip validating pod subnet against node-cidr-mask when allocate-node-cidrs is set to be false #98984

SataQiu commented Feb 11, 2021 •

edited

k8s-ci-robot commented Feb 11, 2021

SataQiu commented Feb 11, 2021

aojea Feb 11, 2021

SataQiu Feb 26, 2021

SataQiu Feb 26, 2021

aojea Feb 26, 2021

neolit123 Feb 26, 2021

SataQiu Feb 26, 2021

neolit123 Feb 26, 2021

aojea commented Feb 11, 2021

SataQiu commented Feb 11, 2021

neolit123 commented Feb 21, 2021

neolit123 commented Feb 25, 2021

aojea commented Feb 26, 2021

neolit123 Feb 26, 2021 •

edited

SataQiu Feb 26, 2021

neolit123 Feb 26, 2021

neolit123 commented Mar 3, 2021

pacoxu commented Mar 3, 2021

	if cfg.Networking.PodSubnet != "" {
	defaultArguments["allocate-node-cidrs"] = "true"

	isDualStack := features.Enabled(c.FeatureGates, features.IPv6DualStack)

	if len(c.Networking.ServiceSubnet) != 0 {
	allErrs = append(allErrs, ValidateIPNetFromString(c.Networking.ServiceSubnet, constants.MinimumAddressesInServiceSubnet, isDualStack, field.NewPath("serviceSubnet"))...)
	// Service subnet was already validated, we need to validate now the subnet size
	allErrs = append(allErrs, ValidateServiceSubnetSize(c.Networking.ServiceSubnet, field.NewPath("serviceSubnet"))...)
	}
	if len(c.Networking.PodSubnet) != 0 {
	allErrs = append(allErrs, ValidateIPNetFromString(c.Networking.PodSubnet, constants.MinimumAddressesInPodSubnet, isDualStack, field.NewPath("podSubnet"))...)
	// Pod subnet was already validated, we need to validate now against the node-mask
	allErrs = append(allErrs, ValidatePodSubnetNodeMask(c.Networking.PodSubnet, c, field.NewPath("podSubnet"))...)
	}

kubeadm: skip validating pod subnet against node-cidr-mask when allocate-node-cidrs is set to be false #98984

kubeadm: skip validating pod subnet against node-cidr-mask when allocate-node-cidrs is set to be false #98984

Conversation

SataQiu commented Feb 11, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Feb 11, 2021

SataQiu commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Feb 11, 2021

SataQiu commented Feb 11, 2021

neolit123 commented Feb 21, 2021

neolit123 commented Feb 25, 2021

aojea commented Feb 26, 2021

neolit123 Feb 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 commented Mar 3, 2021

pacoxu commented Mar 3, 2021

SataQiu commented Feb 11, 2021 •

edited

neolit123 Feb 26, 2021 •

edited