Kubeconfig command features update #255

md2k · 2019-02-28T00:22:01Z

This PR about hetzner-kube cluster kubeconfig my-cluster
What added:

config normalization (rewrite default context/cluster/authinfo names to current cluster-name)
by default if nothing is set, just print current config for selected cluster
save to specific location, by default it saves to ~/.kube/my-cluster.yaml
merge - merges current and picked cluster's config

md2k · 2019-02-28T00:37:12Z

@xetys @mavimo can you take a look.
merge functionality still in progress, but near weekend will be done.
also will be nice if you can point me why go.mod have merge conflicts if i had latest from master before any changes. i'm bit old-school guy and not yet widely use go mod... prefer old way vendor-ing :)

mavimo · 2019-02-28T07:48:09Z

@md2k nice! I'm also working on refactoring the command structure using item as discussed on #234, I'll check it this WE.

md2k · 2019-02-28T09:05:23Z

To perform merge i'm going to use same approach as for sanitization of config (client-go, they have all required api for this), stole this way from official kubectl part, just a bit light weight for our needs. before EOW planing to add merge here.
current merge option simply create backup and exit :)

md2k · 2019-02-28T20:40:18Z

@mavimo @xetys added merge, tested multiple time with my local setup. it is safe to use.
Current behavior simply overwrite if same context already exists. not sure if we need to add force switch, because if i do merge for same cluster-name, probably i did complete rebuild of cluster... or .. do we need verification if i really want overwrite context?

mavimo · 2019-03-01T07:57:45Z

@md2k JM2C: we should not overwrite a context, we should communicate the failure to the user that can rename/delete the context before retry to pull it.

Adding a flag --force should be a nice enhancement but not strictly needed from my PoV.

mavimo

Some minor changes suggested.

@md2k I really like to introduce some tests to this feature, should be easy to test and at the same time since the serialisation/deserialisation can be have different scenario I suppose we should "complicate" the logics behind it on time and without appropriate test-coverage should be hard to evolve it.

PS: I really like this feature, thx for time you spent on this contribution!!

mavimo · 2019-03-01T08:00:57Z

cmd/util.go

+
+// Dump is handy dumps of structure as json (almost any structures)
+func Dump(cls interface{}) {
+	data, err := json.MarshalIndent(cls, "", "    ")


Maybe we can replace it with

fmt.Println(Sdump(cli))

Since I suppose this is a debug utils, I'm not sure we should keep it in our codebase :)

Yeah, this is debug tools, so can be removed before merge. :)

mavimo · 2019-03-01T08:03:21Z

cmd/cluster_kubeconfig.go

+)
+
+const (
+	defaultContext = "kubernetes-admin@kubernetes"


I'm not sure on why we are defining this defaultContext, maybe context should be "cluster specific", like kubernetes-admin@{CLUSTER_NAME}?

this is not what we going to set, this is what we looking to rename. regardless cluster, this context set by default during kubebootstrap, so all our configs with this context. maybe i should change name to default_pattern then.

cmd/cluster_kubeconfig.go

mavimo · 2019-03-01T08:05:20Z

cmd/cluster_kubeconfig.go

-					log.Fatalln("aborted")
-				}
-			}
+		if save, _ := cmd.Flags().GetBool("save"); save {


Ignored error

same here, if they are defined, no error should occur

mavimo · 2019-03-01T08:07:24Z

cmd/cluster_kubeconfig.go


-			ioutil.WriteFile(kubeconfigPath, []byte(kubeConfigContent), 0755)
+			targetPath := fmt.Sprintf("%s/.kube/%s.yaml", GetHome(), provider.GetCluster().Name)
+			if target, _ := cmd.Flags().GetString("target"); target != "" {


Ignored error

mavimo · 2019-03-01T08:07:59Z

cmd/cluster_kubeconfig.go

 	},
 }

+// Write kubeConfig to destination
+func doConfigWrite(dst string, kubeConfig string) (err error) {


Suggested change

func doConfigWrite(dst string, kubeConfig string) (err error) {

func doConfigWrite(dst string, kubeConfig string) error {

mavimo · 2019-03-01T08:08:11Z

cmd/cluster_kubeconfig.go

+}
+
+// Create backup of current kubeCongig
+func doConfigCopyBackUp(src string) (err error) {


Suggested change

func doConfigCopyBackUp(src string) (err error) {

func doConfigCopyBackUp(src string) error {

this done to not assign error directly with :=
for example if you going to use:

if source, err := os.Open(src); err != nil { return }

source will available only inside condition, and we want have access to it outside of condition.
if source declared, then use of := will rise error.
if to remove declaration of err from function then i need also use condition differently as:

source, err := os.Open(src) if err != nil { return }

While both variants completely good, first variant probably looks bit nicer.
But here this is how i do usually. also no need do return with exact parameters :)
If still want me to use your suggestion, let me know

JM2C: the first variant is a bit concise, but make code more hard to understand. We can declare err just before the usage (as we do for source) and return the error (eventually decorated with extra info), like:

var source, destination *os.File var err error if source, err = os.Open(src); err != nil { return fmt.Errorf("unable to get source file: %v", err) }

or use the second proposal:

source, err := os.Open(src) if err != nil { return fmt.Errorf("unable to get source file: %v", err) }

IMHO the second variant is more comprehensible when someone look the code for the first time (new contributors, ppl that try to investigate an issue, the user that write the code after few weeks that write the code..).
Off course is my opinion, maybe a 3rd opinion should be useful ;) cc/ @xetys

I like the second more, as I'm not a fan of C style declaring variables in the header and than doing the code later. The second one is far better to read and understand, and an actual goal of go to make it looking like this

it depends, as i said, all variants is valid in go. but i can agree , i'm always judging from my 5+ experience in go, and for me personally read some-one code in this scope absolutely transparent. but if majority vote for

if err != nil { return err }

not a problem :D

@mavimo @xetys check my comment later below, maybe we really need change way kubeconfig command work, instead of flag usage to print/save/merge , better to change it to sub-command and do kubeconfig save|print|merge and then each of subcommand will have own options if necessary?

md2k · 2019-03-01T11:30:32Z

Hi @mavimo,

we can cover it with tests for sure, only i'm not sure it is truly necessary at this part (but it is my vision).
To make more complex logic..., can you describe a bit in depth what you have in mind about it?
Because i do not want abuse tool with full config management.

My idea to cover next scenario:

i'm already have several clusters i'm working on in my kubeconfig
i'm create new cluster
i want this new cluster be available in my config, so i do a merge
3.1) create backup before merge (since the timestamped with seconds, each run will produce new backup, so user safe because he have history of configs)
3.2) merge my sanitized config into main config (config by this tool always created with prefix hetzner-ClusterName)
I think in this case it hard to break something, because merge logic completely done with native library and follow next steps:
load in 1 or more separate configs to Config structure
if config file you provided empty, not exists or broken - ignore it
if config file have same context,cluster,authinfo name merge them in order they loaded
For example: you merging config1 config2 config3
if config2 have same context as in config1, information of config1 will be overridden by config2.
if apply it to hetzner_kube tool, we can update only what hetnzer_kubeknows, and only if you specify merge flag. if you want to merge , probably you know what you are doing.
One thing only we need to add probably when we destroy cluster also delete it from kubeconfig (but this need to be as additional flag for cluster delete i think).

Another way, is implement basic functionality to manage config by this tool.
But then, kubeconfig command should be moved from cluster -> kubeconfig to standalone and implements next commands:

kubeconfig -|
                      |- save <ClusterName/ContextName> --config(-c) /path/to/save
                      |- delete <ClusterName/ContextName> --config(-c) /path/to/save
                      |- merge <ClusterName/ContextName> --config(-c) /path/to/save
                      |- list --config(-c) /path/to/save

so we can manage kubeconfig(s), but i really do not want do that, because this is out of scope of this tool and all this functionality already built-in to kubectl and for more complex merge management there is plugin for kubectl

md2k · 2019-03-01T11:31:52Z

Other suggestions i'll fix today evening.

md2k · 2019-03-04T11:40:08Z

heh, weekend was busy. i'll try to finalize today-tomorrow this PR

md2k · 2019-03-05T19:44:12Z

found few bugs in current merge logic, so yeah, need to do more complex logic i think. by some reason it not always merge it in right direction. going to dive into a bit more

…kubeconfig_merge

md2k · 2019-03-05T20:14:16Z

nvm here, just a rebase from master and fix for conflicts.

xetys · 2019-05-08T07:58:44Z

Hey,

I'm up to release 0.5 soon and would like to merge this PR.

I see the following two ways:

simple: just fix the merge conflict and wait for @mavimo if he is okay with the current state
advanced: I like the proposal with the sub-command structure, as I would love to add features like user management later (you already can use kubeadm alpha kubeconfig user --client-name=... to add new users to the cluster, which would make sense to pass to the kubeconfig sub-command, too)

Decide which is better for you, how much time you can spend on this right now.

xetys · 2019-11-08T23:21:20Z

hey, what's up here? I would love to see this PR ready for merge

md2k added 5 commits February 27, 2019 19:47

need to play more around

d1b3fd0

implement config normalization, save and merge with backup

6b547c3

fix go.mod

a6fb7b7

fixes to satisfy codeclimate :)

6511784

fixes to satisfy codeclimate

6e52b64

md2k added 2 commits February 28, 2019 01:38

go.mod cleanup with tidy command

3cd9d68

fix typos

3aab92e

added merge functionality

fa32d08

mavimo requested changes Mar 1, 2019

View reviewed changes

md2k added 8 commits March 5, 2019 21:09

need to play more around

f728fe2

implement config normalization, save and merge with backup

417fa3b

fixes to satisfy codeclimate :)

f8e2eb0

fixes to satisfy codeclimate

80d342a

go.mod cleanup with tidy command

7de1328

fix typos

2130c95

added merge functionality

87fb548

Merge branch 'kubeconfig_merge' of github.com:md2k/hetzner-kube into …

b61dea5

…kubeconfig_merge

xetys added this to the 0.5 milestone Mar 31, 2019

xetys modified the milestones: 0.5, 0.6 Nov 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubeconfig command features update #255

Kubeconfig command features update #255

md2k commented Feb 28, 2019

md2k commented Feb 28, 2019

mavimo commented Feb 28, 2019 •

edited

Loading

md2k commented Feb 28, 2019 •

edited

Loading

md2k commented Feb 28, 2019

mavimo commented Mar 1, 2019

mavimo left a comment

mavimo Mar 1, 2019

md2k Mar 1, 2019

mavimo Mar 1, 2019

md2k Mar 1, 2019

mavimo Mar 1, 2019

xetys May 8, 2019

mavimo Mar 1, 2019

mavimo Mar 1, 2019

mavimo Mar 1, 2019

md2k Mar 1, 2019

mavimo Mar 2, 2019

xetys Mar 3, 2019

md2k Mar 4, 2019

md2k Mar 4, 2019

md2k commented Mar 1, 2019 •

edited

Loading

md2k commented Mar 1, 2019

md2k commented Mar 4, 2019

md2k commented Mar 5, 2019

md2k commented Mar 5, 2019

xetys commented May 8, 2019

xetys commented Nov 8, 2019

	func doConfigWrite(dst string, kubeConfig string) (err error) {
	func doConfigWrite(dst string, kubeConfig string) error {

	func doConfigCopyBackUp(src string) (err error) {
	func doConfigCopyBackUp(src string) error {

Kubeconfig command features update #255

Are you sure you want to change the base?

Kubeconfig command features update #255

Conversation

md2k commented Feb 28, 2019

md2k commented Feb 28, 2019

mavimo commented Feb 28, 2019 • edited Loading

md2k commented Feb 28, 2019 • edited Loading

md2k commented Feb 28, 2019

mavimo commented Mar 1, 2019

mavimo left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

md2k commented Mar 1, 2019 • edited Loading

md2k commented Mar 1, 2019

md2k commented Mar 4, 2019

md2k commented Mar 5, 2019

md2k commented Mar 5, 2019

xetys commented May 8, 2019

xetys commented Nov 8, 2019

mavimo commented Feb 28, 2019 •

edited

Loading

md2k commented Feb 28, 2019 •

edited

Loading

md2k commented Mar 1, 2019 •

edited

Loading