Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[kubeadm]: Remove the deprecated kube-dns as an option in kubeadm #99646

Merged
merged 3 commits into from Mar 4, 2021
Merged

[kubeadm]: Remove the deprecated kube-dns as an option in kubeadm #99646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8a1bd6
remove the deprecated kube-dns as an option in kubeadm
rajansandeep Feb 8, 2021
8901a9b
update vendor to remove unused deps
rajansandeep Mar 2, 2021
62542bb
add validation to indicate kube-dns is no longer supported
rajansandeep Mar 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 0 additions & 3 deletions cmd/kubeadm/app/apis/kubeadm/types.go
View file
Open in desktop
Expand Up @@ -156,9 +156,6 @@ type DNSAddOnType string
const (
// CoreDNS add-on type
CoreDNS DNSAddOnType = "CoreDNS"

// KubeDNS add-on type
KubeDNS DNSAddOnType = "kube-dns"
)

// DNS defines the DNS addon that should be used in the cluster
Expand Down
3 changes: 0 additions & 3 deletions cmd/kubeadm/app/apis/kubeadm/v1beta1/types.go
View file
Open in desktop
Expand Up @@ -145,9 +145,6 @@ type DNSAddOnType string
const (
// CoreDNS add-on type
CoreDNS DNSAddOnType = "CoreDNS"

// KubeDNS add-on type
KubeDNS DNSAddOnType = "kube-dns"
)

// DNS defines the DNS addon that should be used in the cluster
Expand Down
3 changes: 0 additions & 3 deletions cmd/kubeadm/app/apis/kubeadm/v1beta2/types.go
View file
Open in desktop
Expand Up @@ -141,9 +141,6 @@ type DNSAddOnType string
const (
// CoreDNS add-on type
CoreDNS DNSAddOnType = "CoreDNS"

// KubeDNS add-on type
KubeDNS DNSAddOnType = "kube-dns"
Copy link
Member

@neolit123 neolit123 Mar 2, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to double check.

what happens if the user passes kube-dns as the type after this change:
https://github.com/kubernetes/kubernetes/blob/700047303c5414b5fe9b0d1240602cb680274ca1/cmd/kubeadm/app/apis/kubeadm/v1beta2/types.go#L93

is it a NOOP now?

if so, i think we must reflect that in the release note with an additional sentence.

The "kube-dns" type is no longer recognized if passed to ClusterConfiguration.dns.type.

should we error out if anything other than "CoreDNS" is used?

Copy link
Member

@neolit123 neolit123 Mar 4, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajansandeep

should we error out if anything other than "CoreDNS" is used?

i cannot find validation for ClusterConfiguration.DNS.Type.
in EnsureDNSAddon() we just check if Type is coredns first and if not it uses kube-dns.
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/phases/addons/dns/dns.go#L103-L114

this validation should have been originally in:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/apis/kubeadm/validation/validation.go#L59

so...we can instead do this in the release note:

kubeadm: the deprecated kube-dns is no longer supported as an option. Given kubeadm now only supports CoreDNS as the DNS addon, the value of the field "ClusterConfiguration.dns.type" is ignored.

instead of adding the validation in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I updated the release notes

Copy link
Member

@liggitt liggitt Mar 4, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the value of the field "ClusterConfiguration.dns.type" is ignored

That's surprising to me... was that field validated in 1.20? If so, we should error rather than silently accept a config that explicitly requests kube-dns and previously worked and not honor it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently the field was never validated after the API was converted to internal type...
and during runtime the check was if type == "CoreDNS" { // install coredns } else { // install kube-dns }.

having the validation seems better to me overall, but i didn't want to introduce it now.

if we are going it introduce it, it would look like this:

add this:

// ValidateDNS validates the DNS object and collects all encountered errors
func ValidateDNS(dns *kubeadm.DNS, fldPath *field.Path) field.ErrorList {
	allErrs := field.ErrorList{}
	if dns.Type != kubeadm.CoreDNS {
		allErrs = append(allErrs, field.Invalid(fldPath, dns.Type, fmt.Sprintf("only DNS type %q is supported", kubeadm.CoreDNS)))
	}
	return allErrs
}

here https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/apis/kubeadm/validation/validation.go#L493

it should be called like so:

allErrs = append(allErrs, ValidateDNS(&c.DNS, field.NewPath("dns"))...)

here:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/apis/kubeadm/validation/validation.go#L60

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this means that we need to keep the kube-dns type constant and related issues open with a TODO.

Seems like you could inline it as "kube-dns" with a comment that this value is no longer supported. Not sure why you'd need to remove or adjust it in the future.

Copy link
Member

@neolit123 neolit123 Mar 4, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, it can be inlined.

since i logged this ticket to remove the "type" field in v1beta3.
kubernetes/kubeadm#2398
i guess we will have to remove this validation too...EDIT: actually it should be removed with v1beta2 i guess.

@rajansandeep please let me know if you have any questions for this update.

// ValidateDNS validates the DNS object and collects all encountered errors
func ValidateDNS(dns *kubeadm.DNS, fldPath *field.Path) field.ErrorList {
	allErrs := field.ErrorList{}
	const kubeDNSType = "kube-dns"
	if dns.Type == kubeDNSType {
		allErrs = append(allErrs, field.Invalid(fldPath, dns.Type, fmt.Sprintf("DNS type %q is no longer supported", kubeDNSType)))
	}
	return allErrs
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajansandeep
we also need to change the release note to:

kubeadm: the deprecated kube-dns is no longer supported as an option. If "ClusterConfiguration.dns.type" is set to "kube-dns" kubeadm will now throw an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the validation and changed the release notes.
Please let me know if I should squash the commits or leave it as is...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks.

)

// DNS defines the DNS addon that should be used in the cluster
Expand Down
11 changes: 11 additions & 0 deletions cmd/kubeadm/app/apis/kubeadm/validation/validation.go
View file
Open in desktop
Expand Up @@ -57,6 +57,7 @@ func ValidateInitConfiguration(c *kubeadm.InitConfiguration) field.ErrorList {
// ValidateClusterConfiguration validates an ClusterConfiguration object and collects all encountered errors
func ValidateClusterConfiguration(c *kubeadm.ClusterConfiguration) field.ErrorList {
allErrs := field.ErrorList{}
allErrs = append(allErrs, ValidateDNS(&c.DNS, field.NewPath("dns"))...)
allErrs = append(allErrs, ValidateNetworking(c, field.NewPath("networking"))...)
allErrs = append(allErrs, ValidateAPIServer(&c.APIServer, field.NewPath("apiServer"))...)
allErrs = append(allErrs, ValidateAbsolutePath(c.CertificatesDir, field.NewPath("certificatesDir"))...)
Expand Down Expand Up @@ -484,6 +485,16 @@ func getClusterNodeMask(c *kubeadm.ClusterConfiguration, isIPv6 bool) (int, erro
return maskSize, nil
}

// ValidateDNS validates the DNS object and collects all encountered errors
func ValidateDNS(dns *kubeadm.DNS, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
const kubeDNSType = "kube-dns"
if dns.Type == kubeDNSType {
allErrs = append(allErrs, field.Invalid(fldPath, dns.Type, fmt.Sprintf("DNS type %q is no longer supported", kubeDNSType)))
}
return allErrs
}

// ValidateNetworking validates networking configuration
func ValidateNetworking(c *kubeadm.ClusterConfiguration, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
Expand Down
10 changes: 0 additions & 10 deletions cmd/kubeadm/app/cmd/config_test.go
View file
Open in desktop
Expand Up @@ -175,16 +175,6 @@ func TestConfigImagesListRunWithoutPath(t *testing.T) {
},
expectedImages: defaultNumberOfImages,
},
{
name: "kube-dns enabled",
cfg: kubeadmapiv1beta2.ClusterConfiguration{
KubernetesVersion: dummyKubernetesVersionStr,
DNS: kubeadmapiv1beta2.DNS{
Type: kubeadmapiv1beta2.KubeDNS,
},
},
expectedImages: defaultNumberOfImages + 2,
},
}

outputFlags := output.NewOutputFlags(&imageTextPrintFlags{}).WithTypeSetter(outputapischeme.Scheme).WithDefaultOutput(output.TextOutput)
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/cmd/upgrade/apply.go
View file
Open in desktop
Expand Up @@ -104,7 +104,7 @@ func newCmdApply(apf *applyPlanFlags) *cobra.Command {
// - Upgrades the control plane components
// - Applies the other resources that'd be created with kubeadm init as well, like
// - Creating the RBAC rules for the bootstrap tokens and the cluster-info ConfigMap
// - Applying new kube-dns and kube-proxy manifests
// - Applying new CorDNS and kube-proxy manifests
// - Uploads the newly used configuration to the cluster ConfigMap
func runApply(flags *applyFlags, args []string) error {

Expand Down
1 change: 0 additions & 1 deletion cmd/kubeadm/app/cmd/upgrade/plan.go
View file
Open in desktop
Expand Up @@ -177,7 +177,6 @@ func genUpgradePlan(up *upgrade.Upgrade, isExternalEtcd bool) (*outputapi.Upgrad
components = append(components, newComponentUpgradePlan(constants.KubeProxy, up.Before.KubeVersion, up.After.KubeVersion))

components = appendDNSComponent(components, up, kubeadmapi.CoreDNS, constants.CoreDNS)
components = appendDNSComponent(components, up, kubeadmapi.KubeDNS, constants.KubeDNS)

if !isExternalEtcd {
components = append(components, newComponentUpgradePlan(constants.Etcd, up.Before.EtcdVersion, up.After.EtcdVersion))
Expand Down