kubeconfig overhaul #1029

BenTheElder · 2019-10-30T17:44:44Z

Implements most of #850, also #112, #113

This is smaller than it looks, a lot of the lines are boilerplate license etc. because I broke up the implementation into small portions and tried to comment it pretty strongly

There are a few additional not strictly necessary but nice to have features like ensuring that the formatting matches client-go generated kubeconfigs.

This does:

reading in kubeconfig in a structured way with support for (de)serializing arbitrary ignored fields
- modifying server field in a structured way instead of regex
ensuring kind entries are unique (kind-${cluster_name} instead of the ~static kubeadm names) kind create cluster creates same kubeconfig user name for each cluster #112
selecting the kubeconfig file to use in the same way as kubectl Add option to specify kubeconfig when creating kind clusters #113
merging in kubeconfig on kind create cluster
removing kind entries from kubeconfig in kind delete cluster
drops kind get kubeconfig-path as it's no longer relevant
fixes kind get kubeconfig to not depend on any files on the host

This is a mildly breaking change. We'll want a simple guide in the release notes for migration.

If you want kind to use a different config file, you can either set the --kubeconfig option to kind create cluster / kind delete cluster or you can export KUBECONFIG prior to using kind.

This is basically the same as before, just that you decide the path and you do it before kind create cluster instead of after.

k8s-ci-robot · 2019-10-30T17:44:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

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:

~~OWNERS~~ [BenTheElder]

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

BenTheElder · 2019-10-30T17:48:57Z

heh our lints don't like client-go :+)

pkg/internal/util/kubeconfig/merge.go:34:18: Error return value of `unlockFile` is not checked (errcheck)
	defer unlockFile(configPath)
	                ^
pkg/internal/util/kubeconfig/remove.go:37:19: Error return value of `unlockFile` is not checked (errcheck)
		defer unlockFile(configPath)
		                ^
verifying generated ...

BenTheElder · 2019-10-30T17:56:52Z

important reference for expected behavior: https://kubernetes.io/docs/reference/generated/kubectl/kubectl-commands

in addition, from inspecting code (mainly client-go directly) and reading some other sources

for the case of KUBECONFIG being a list of paths, empty paths are discarded, duplicate paths are discarded, THEN the first existing one OR the last file is used
path+".lock" should be used to 'lock' modifications, we've lifted the exact little lockfile creation / deletion routines from client-go
merging clusters into kubeconfig universally sets the current context (kops, gcloud, etc. ...)
removing clusters from kubeconfig universally unsets the current context if the context refers to the cluster being removed

pkg/internal/util/kubeconfig/remove.go

pkg/internal/util/kubeconfig/merge.go

pkg/internal/cluster/kubeconfig/kubeconfig.go

aojea · 2019-10-30T23:05:54Z

pkg/internal/util/kubeconfig/encode_test.go

+	"sigs.k8s.io/kind/pkg/internal/util/assert"
+)
+
+func TestEncodeRoundtrip(t *testing.T) {


can we add a test failing?

not trivially, it only fails if it cannot marshal the type so ...

FYI we have 41.5% statements covered in this package, the rest are mostly error cases that aren't really triggerable like this, or file system interactions (and somewhat unit tests then, but I will probably still add some)

pkg/internal/cluster/create/create.go

pkg/internal/util/kubeconfig/write.go

aojea · 2019-10-30T23:44:28Z

pkg/internal/util/kubeconfig/encode.go

+	if err != nil {
+		return nil, err
+	}
+	// special case: don't write anything when empty


maybe we can detect this first?

not really, the input is fairly arbitrary.

this is a really simple nice to have thing to make the output more like the user would expect. it would be totally valid to not have this entire method.

BenTheElder · 2019-10-31T00:30:57Z

brought coverage up to 59.4% of statements for the kubeconfig package by covering WriteMerged with writing in tempdirs.

BenTheElder · 2019-10-31T00:43:29Z

ok covered removal. this is now at 64.7% statements covered.
the non-covered statements are largely:

the homedir implementation copied from client-go which has a large amount of windows specific code. /shrug we won't be modifying this, only keeping in sync with upstream
error branches that are technically possible but won't really happen (eg failing to marshal a kubeconfig)

BenTheElder · 2019-10-31T00:45:52Z

more specific coverage:
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/1029/pull-kind-unit/1189704250880430080
https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kind/1029/pull-kind-unit/1189704250880430080/artifacts/filtered.html#file96

aojea · 2019-10-31T06:28:21Z

pkg/internal/util/kubeconfig/paths.go

+
+https://kubernetes.io/docs/reference/generated/kubectl/kubectl-commands
+
+- If the --kubeconfig flag is set, then only that file is loaded. The flag may only be set once and no merging takes place.


We should document this later in the quick start guide or the readme

aojea · 2019-10-31T06:31:38Z

/lgtm
this is simply amazing, impressive work

maelvls · 2019-11-05T12:44:21Z

pkg/internal/cluster/create/actions/kubeadminit/init.go

-// matches kubeconfig server entry like:
-//    server: https://172.17.0.2:6443
-// which we rewrite to:
-//    server: https://$ADDRESS:$PORT
-var serverAddressRE = regexp.MustCompile(`^(\s+server:) https://.*:\d+$`)


Since the 'rewrite' doesn't (seem to) happen anymore, the kind get kubeconfig prints the container's host and port instead of 127.0.0.1 + the forwarded port. Or am I wrong?

Or maybe I'm using kind get kubeconfig wrong 😁

The kind get kubeconfig --internal does the same as kind get kubeconfig --internal for now (as of aea9a47)

seems this is failing
https://github.com/kubernetes-sigs/kind/pull/1029/files#diff-8e81c51b562c668c8c80420dd1917316R81
:/

can you open a bug @maelvls ?

Thanks for the quick response 🙂
Issue created: #1051

…G management (kubernetes-sigs/kind#1029)

BenTheElder added 3 commits October 30, 2019 10:33

implement kubeconfig helper package

5ad98b6

move cli to kubeconfig utils

aa3dd90

remove kubeconfig-path

e258374

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 30, 2019

k8s-ci-robot requested review from amwat and munnerz October 30, 2019 17:44

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 30, 2019

ignore "unlocking" for now

b544abd

BenTheElder mentioned this pull request Oct 30, 2019

RFC: CLI UX Improvements #850

Closed

migrate e2e-k8s.sh

ef2111d

BenTheElder force-pushed the kubeconfig branch from 81a545d to ef2111d Compare October 30, 2019 18:14

BenTheElder added 2 commits October 30, 2019 11:18

move homedir logic from client-go into kubeconfig package

9879d7b

add unit tests for kubeconfig

f919591

BenTheElder force-pushed the kubeconfig branch from f11700a to f919591 Compare October 30, 2019 22:45

amwat reviewed Oct 30, 2019

View reviewed changes

pkg/internal/util/kubeconfig/remove.go Outdated Show resolved Hide resolved

pkg/internal/util/kubeconfig/remove.go Outdated Show resolved Hide resolved

pkg/internal/util/kubeconfig/merge.go Show resolved Hide resolved

BenTheElder changed the title ~~[WIP] kubeconfig overhaul~~ kubeconfig overhaul Oct 30, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2019