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.

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

Add the ability to migrate CoreDNS configmap in kubeadm #78033

Merged
merged 4 commits into from Aug 21, 2019

Conversation

@rajansandeep
Copy link
Member

commented May 17, 2019

What type of PR is this?

/kind feature

What this PR does / why we need it:
Currently in kubeadm, when upgrading from Kubernetes (Eg. k8s 1.13 -> 1.14), the CoreDNS ConfigMap is retained as-is and applied to the upgraded version. This is done due to the possibility that the user could have customized the Corefile for their use-case.

This approach is disadvantageous because this can lead to an incompatible Corefile which can lead to CoreDNS entering a CrashLoopBackOff state, and hence DNS not working in the cluster.

This PR adds the ability for users to seamlessly migrate their CoreDNS Configuration when they upgrade their clusters using kubeadm. The CoreDNS Migration library helps to handle migrations of CoreDNS Corefiles to be compatible with new versions of CoreDNS.

The PR includes 3 preflight-checks while upgrading to check:

  • If the CoreDNS image is an official release
  • If the Migration of CoreDNS is possible (Checks if there are any errors while migrating the CoreDNS Config from the current version to the next version)
  • If there are any CoreDNS plugins present that are unsupported by the migration library.

The user will be provided Logs and can override these checks.

During Upgrade:

  • There is a check to see if the CoreDNS Config is the default (not edited). In that case, the user will be upgraded to the new default CoreDNS Config.
  • If the CoreDNS Config has been modified, then the user will be migrated to the equivalent compatible Config for the latest version of CoreDNS. This will include handling all deprecations that may be present.

The user will be informed appropriately through logs.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
The first commit includes the code change required for this PR.
The second commit includes the vendor and go mod changes.

This PR also updates the CoreDNS version to 1.5.0
Does this PR introduce a user-facing change?:

Kubeadm now seamlessly migrates the CoreDNS Configuration when upgrading CoreDNS.
@rajansandeep

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

@k8s-ci-robot k8s-ci-robot requested review from chrisohaver and neolit123 May 17, 2019

@neolit123
Copy link
Member

left a comment

thanks for the PR @rajansandeep
i understand the motivation of this change thus i've spent time doing a detailed review.

we might need at least some unit tests and possibly extend the release note that preflight checks are run now.

ideally more eyes from @kubernetes/sig-cluster-lifecycle-pr-reviews are needed here.

@@ -92,6 +93,10 @@ func enforceRequirements(flags *applyPlanFlags, dryRun bool, newK8sVersion strin
return nil, nil, nil, errors.Wrapf(err, "couldn't create a Kubernetes client from file %q", flags.kubeConfigPath)
}

if err := runCoreDNSpreflightChecks(client, ignorePreflightErrorsSet); err != nil {

This comment has been minimized.

Copy link
@neolit123

neolit123 May 17, 2019

Member

the upgrade checks should have a single entry point: runPreflightChecks
the call to runCoreDNSpreflightChecks() should be moved there.
client needs to be passed to runPreflightChecks.

return err
}
if currentDNSType == kubeadmapi.CoreDNS {
fmt.Println("[preflight] Running CoreDNS migration pre-flight checks.")

This comment has been minimized.

Copy link
@neolit123

neolit123 May 17, 2019

Member

we can omit this notification, the preflight package will show check names.

}
if currentDNSType == kubeadmapi.CoreDNS {
fmt.Println("[preflight] Running CoreDNS migration pre-flight checks.")
return upgrade.RunCoreDNSMigrationCheck(client, ignorePreflightErrors)

This comment has been minimized.

Copy link
@neolit123

neolit123 May 17, 2019

Member
currentDNSType, _, err := dns.DeployedDNSAddon(client)
	if err != nil {
		return err
	}
	if currentDNSType == kubeadmapi.CoreDNS {
...

can be inside upgrade.RunCoreDNSMigrationCheck, and optionally make the function a no-op if kube-dns is used.
(i.e. exit early and don't run preflight.RunChecks(migrationChecks, os.Stderr, ignorePreflightErrors))

}
}
// Get the Corefile and installed CoreDNS version.
corefile, currentInstalledCoreDNSVersion, err := GetCoreDNSInfo(client)

This comment has been minimized.

Copy link
@neolit123

neolit123 May 17, 2019

Member

GetCoreDNSInfo() can accept a *ConfigMap as an argument.
if nil it should fetch the CM, if non-nil it should use the given CM.
this avoids fetching the same CM twice.

if !migration.Default("", corefile) {
updatedCorefile, err := migration.Migrate(currentInstalledCoreDNSVersion, kubeadmconstants.CoreDNSVersion, corefile, false)
if err != nil {
return errors.Wrap(err, "unable to migrate CoreDNS Configmap")

This comment has been minimized.

Copy link
@neolit123

neolit123 May 17, 2019

Member

Configmap -> ConfigMap

@@ -297,6 +298,83 @@ func createDNSService(dnsService *v1.Service, serviceBytes []byte, client client
return nil
}

func createCoreDNSConfigMap(client clientset.Interface, cm *v1.ConfigMap) error {

This comment has been minimized.

Copy link
@neolit123

neolit123 May 17, 2019

Member

can we get an unit test for this function?

// checkUnsupportedPlugins checks if there are any plugins included in the current configuration
// that is unsupported for migration.
func checkUnsupportedPlugins(client clientset.Interface) error {
klog.V(1).Infoln("validating if there are any unsupported CoreDNS plugins in the Config")

This comment has been minimized.

Copy link
@neolit123

neolit123 May 17, 2019

Member

Config->Corefile?


}
if UnsupportedPlugins != "" || UnsupportedVersion != "" {
return errors.Errorf("there are unsupported plugins in the CoreDNS Configuration")

This comment has been minimized.

Copy link
@neolit123

neolit123 May 17, 2019

Member

Configuration->Corefile?


// Checks if the CoreDNS image currently installed is an official released version.
func checkRelease(client clientset.Interface) error {
klog.V(1).Infoln("validating if the image installed is the official release of CoreDNS.")

This comment has been minimized.

Copy link
@neolit123

neolit123 May 17, 2019

Member

extra space before official.


isValid := migration.Released(coreDNSImageSha[1])
if !isValid {
return errors.Errorf("the CoreDNS image currently installed is not an official image. If you still wish to proceed, do so at your own risk.")

This comment has been minimized.

Copy link
@neolit123

neolit123 May 17, 2019

Member

this is not a check we have for the control plane or etcd images.
to my understanding, if someone has built coredns from source at any SHA, migration.Released would return false. while this is still not recommended, blocking on it with a preflight check might be a bit too strict.

but possibly needs discussion / feedback from more people.

This comment has been minimized.

Copy link
@rosti

rosti May 29, 2019

Member

+1 for not having this. Users in China may be implicated.

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver May 29, 2019

Contributor

I think it is generally safe to remove this check.

The main reason for adding this check is to prevent external plugin users from inadvertently replacing their custom CoreDNS with a stock one. A stock CoreDNS will crash when it encounters an unknown plugin in the Corefile at start up.

But most users just use a stock version of CoreDNS. And IIUC, those who use CoreDNS compiled with external plugins will upgrade using "phases", and manually upgrade CoreDNS with an updated version compiled with the required external plugins if desired.

@cblecker

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Is this needed/intended to merge before 1.15 code freeze?

@rajansandeep

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

@cblecker Yes! This is needed for 1.15

@cblecker

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@rajansandeep Looks like the caddy dependency needs to be updated.

Can you please run:

hack/pin-dependency.sh github.com/mholt/caddy v1.0.0
hack/pin-dependency.sh github.com/BurntSushi/toml v0.3.1
hack/pin-dependency.sh github.com/golang/mock v1.2.0
hack/pin-dependency.sh github.com/google/uuid v1.1.1
hack/pin-dependency.sh github.com/gorilla/websocket v1.4.0
hack/pin-dependency.sh github.com/miekg/dns v1.1.3
hack/pin-dependency.sh github.com/onsi/ginkgo v1.8.0
hack/pin-dependency.sh github.com/onsi/gomega v1.5.0
hack/pin-dependency.sh github.com/russross/blackfriday v0.0.0-20170610170232-067529f716f4
hack/pin-dependency.sh github.com/stretchr/testify v1.3.0
hack/pin-dependency.sh gopkg.in/natefinch/lumberjack.v2 v2.0.0
hack/pin-dependency.sh gopkg.in/square/go-jose.v2 v2.2.2
hack/pin-dependency.sh gopkg.in/yaml.v2 v2.2.2
GO111MODULE=on go mod edit -dropreplace github.com/natefinch/lumberjack
GO111MODULE=on go mod edit -dropreplace github.com/shurcooL/sanitized_anchor_name
GO111MODULE=on go mod edit -dropreplace gopkg.in/yaml.v1
hack/update-vendor.sh

And commit the changes? Then when complete, include a comment with the output of hack/lint-dependencies.sh

@neolit123

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Can you please run:

looks simple enough 🙄

/priority important-soon
/milestone v1.15

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

/remove-area apiserver cloudprovider

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

/approve
/assign @rosti

@rosti PTAL and LGTM if you think this is good.
at this point i prefer if we merge the PR, run it trough e2e and iterate if needed before 1.16.

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@rajansandeep

if you are changing the version of coredns-kubeadm, make sure all of the following files are updated with the newest version including /home/prow/go/src/k8s.io/kubernetes/build/dependencies.yaml
then run ./hack/verify-external-dependencies-version.sh

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/78033/pull-kubernetes-verify/1161403539239473152

build/dependencies.yaml needs an update at this point.

@rajansandeep

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

This still needs root approval for the changes in vendor/deps...

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

/assign @liggitt

PTAL WRT approval of the /vendor changes.
we can amend on the configmap/upgrade logic if needed in follow ups.

thanks!

@@ -314,9 +314,9 @@ data:
.:53 {
errors
health
ready

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Aug 15, 2019

Member

FYI @aojea the ipv6 config may need tweaking.

@@ -0,0 +1,33 @@
// Copyright 2015 Light Code Labs, LLC

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Aug 15, 2019

Member

is this package a copy from a different upstream? do we have accurate licensing info for this (in Godeps/) ?
is there a reason this isn't imported from the original authors? was it modified?

This comment has been minimized.

Copy link
@neolit123

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Aug 16, 2019

Contributor

Originally I just imported the caddy package. But at the suggestion of reviewers concerned for the number of imported packages, I copied the required pieces locally. The licensing was copied verbatim from the original project. IIRC, I had to modify it slightly (a function moved from one package to another), to reduce the amount of copied code.

do we have accurate licensing info for this (in Godeps/) ?

Sorry, I'm not familiar with the licensing process ... Please let me know what I need to do here to make everyone happy.

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Aug 16, 2019

Member

Originally I just imported the caddy package. But at the suggestion of reviewers concerned for the number of imported packages, I copied the required pieces locally. The licensing was copied verbatim from the original project.

I see, that makes sense I suppose.

IIRC, I had to modify it slightly (a function moved from one package to another), to reduce the amount of copied code.

So you modified one of the source files then? Do we need to denote this somehow?

Sorry, I'm not familiar with the licensing process ... Please let me know what I need to do here to make everyone happy.

I'm not actually sure either, I haven't seen this case before.

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Aug 16, 2019

Contributor

So you modified one of the source files then? Do we need to denote this somehow?

I recall I had to do that to stop the "bleeding" ... otherwise i'd be duplicating most caddy. I don't know if we need to or how to properly denote this (comments in the files? readme in same directory?). IMHO, copying files like this feels a hack to me, and I'd prefer just importing the packages normally. OTOH, I don't have a good understanding of the concern for importing the package in the first place, other than it created a lot of dependencies (which I infer is bad).

This comment has been minimized.

Copy link
@chrisohaver

chrisohaver Aug 20, 2019

Contributor

I propose we just import the packages normally, and remove the copied files. Any objections?

This comment has been minimized.

Copy link
@justaugustus

justaugustus Aug 20, 2019

Member

@chrisohaver -- yeah, I'd feel better about that route.

This comment has been minimized.

Copy link
@justaugustus

justaugustus Aug 20, 2019

Member

/hold
(until that's resolved.)

This comment has been minimized.

Copy link
@rajansandeep

rajansandeep Aug 20, 2019

Author Member

I fixed the part. @justaugustus PTAL and cancel the hold if you feel it's good. Thanks!

This comment has been minimized.

Copy link
@justaugustus

justaugustus Aug 20, 2019

Member

Thanks for fixing the licensing piece, @rajansandeep!
/hold cancel

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

I had the same question w.r.t. that package license

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

/lgtm
/approve
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 20, 2019

@rajansandeep

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

Looks like all hurdles have been crossed and requires one final approval tag.
/assign @liggitt

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, liggitt, neolit123, rajansandeep

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:

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

@k8s-ci-robot k8s-ci-robot merged commit e1c2c67 into kubernetes:master Aug 21, 2019

22 of 23 checks passed

pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation rajansandeep authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.