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

dns - GetDNSVersion does not depend of the version #870

Closed
rajansandeep opened this issue May 29, 2018 · 1 comment · Fixed by kubernetes/kubernetes#64761
Closed

dns - GetDNSVersion does not depend of the version #870

rajansandeep opened this issue May 29, 2018 · 1 comment · Fixed by kubernetes/kubernetes#64761
Labels
documentation/confusing kind/feature Categorizes issue or PR as related to a new feature. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle.

Comments

@rajansandeep
Copy link

Is this a BUG REPORT or FEATURE REQUEST?

BUG REPORT

Reopening the issue from here: kubernetes/kubernetes#64454

Versions

kubeadm version (use kubeadm version):
last one - master for 1.11

What happened?

From that Review : kubernetes/kubernetes#64258

in this piece of code : https://github.com/kubernetes/kubernetes/pull/64258/files/8d8b47596e6e71ae705ed2fd1e946e32a98a42f0#diff-6f6577e723201d3cf7ff85de45a73068

const (
	kubeDNSVersion = "1.14.10"
	coreDNSVersion = "1.1.3"
)

// GetDNSVersion returns the right kube-dns version for a specific k8s version
func GetDNSVersion(kubeVersion *version.Version, dns string) string {
	// v1.9.0+ uses kube-dns 1.14.10, if feature gate "CoreDNS" is disabled.
	// v1.9.0+ uses CoreDNS  1.1.3.

	// In the future when the version is bumped at HEAD; add conditional logic to return the right versions
	// Also, the version might be bumped for different k8s releases on the same branch
	switch dns {
	case kubeadmconstants.CoreDNS:
		// return the CoreDNS version
		return coreDNSVersion
	default:
		return kubeDNSVersion
	}
}

there is a comment:

// In the future when the version is bumped at HEAD; add conditional logic to return the right versions
// Also, the version might be bumped for different k8s releases on the same branch

Obviously, there is no implementation of logic to return the DNS version that as pushed for one version of Kubernetes. (neither for kube-dns, neither for CoreDNS)

for instance:
CoreDNS 1.0.0 was pushed in the release k8s v1.9.0
CoreDNS 1.0.6 was pushed in the release k8s v1.10.0
CoreDNS 1.1.3 is now pushed in the coming release k8s v1.11.0

However, the last version CoreDNS 1.1.3 is compatible with all versions of k8s (1.9 and highers).

What you expected to happen?

A better description on what logic should be encoded in this function. Based on example of above, should that GetDNSVersion function return always 1.1.3 , or check k8s version and return 1.0.0 or 1.0.6 or 1.1.3 ?

Maybe explanation on when this function will not be called with the version of k8s it is released on.

/sig cluster-lifecycle
/cc @luxas
/cc @fturib

@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label May 29, 2018
@rosti
Copy link

rosti commented Jun 1, 2018

I'll give this one a try.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/enhancement labels Jun 5, 2018
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue Jun 6, 2018
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kubeadm: Don't match DNS versions to K8s versions

**What this PR does / why we need it**:

Some code in kubeadm was designed with the intent, that in the future CoreDNS
and kube-dns versions will match to specific K8s versions. This code is not
functional, since it does not perform any version matching. As of this moment,
no version matching is planned and a lot of boilerplate code is left useless.
The solution is simple - remove the unneeded parts to simplify the flow.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes kubernetes/kubeadm#870

**Special notes for your reviewer**:
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/assign @luxas
/assign @timothysc
/kind cleanup

**Release note**:

```release-note
NONE
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation/confusing kind/feature Categorizes issue or PR as related to a new feature. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants