Route flannel via existing wireguard interface

[voron]

Untested
Closes #139

[voron]

@xetys @mavimo @JohnnyQQQQ pls advise how to pass codeclimate 4 return statements limit

[mavimo]

@voron IMHO codeclimate check should be ignored in this case. We should "bypass" the check removing the last if and return err, if no error it return nil. This should work but make code less comprehensible so I prefer to keep it as is.
I just suggest to move the wireguard setup in a separate function as we do for flannel.
Other possible solution is to create a new function prepareNetwork that invoke flannel config and wireguard config creating only one return value (but still a workaround)

[xetys]

The latter option is the proper way to do that I think. The motivation behind that comes to mind when someone reads the code for the first time. Reading two functions with less return statements will be easier to understand than one function with more. It feels like codeclimate is a little bit Nazi on that, but the idea behind that still makes sense. So I suggest to split that into two functions and then we need to test it, as you stated it is not tested yet. After that I think we can merge it. Marco Vito Moscaritolo <[email protected]> schrieb am So., 26. Aug. 2018, 15:24:

…

@voron <https://github.com/voron> IMHO codeclimate check should be ignored in this case. We should "bypass" the check removing the last if and return err, if no error it return nil. This should work but make code less comprehensible so I prefer to keep it as is. I just suggest to move the wireguard setup in a separate function as we do for flannel. Other possible solution is to create a new function prepareNetwork that invoke flannel config and wireguard config creating only one return value (but still a workaround) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#178 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACoVc0Gigk6hUKQpbettPPOh7kLsvJ7Vks5uUqFzgaJpZM4WMbgI> .

[mavimo]

@xetys IMHO this rule is too strict in Golang context where the early return approach is a standard, especially for error returning.

Aside the above opinion, on the previous comment my last proposal was:

func (provisioner *NodeProvisioner) preparePackages() error {
	// ...
	err = provisioner.prepareNetwork()
	if err != nil {
		return err
	}

	return nil
}

func (provisioner *NodeProvisioner) prepareNetwork() error {
	err = provisioner.prepareFlannel()
	if err != nil {
		return err
	}

	err = provisioner.prepareWireguard()
	if err != nil {
		return err
	}
	return nil
}

but after consideration I think we should invoke prepareNetwork in the main function and not into preparePackage function, since is not a package but a service configuration, @voron @xetys WDYT?

[voron]

@mavimo @xetys I tried to implement some function separation, but I had to cheat anyway, as prepareAndInstall() already had 4 returns :(
Codeclimate just changed an error anyway. It doesn't like too much returns and doesn't like when I spread returns to functions :)
I'm going to postpone climate for now and test PR instead.

[xetys]

report your test results then please

[voron]

Manual cluster create looks good with 1 master and 1 worker : TX offload disabled on flannel.1, flanneld works via wg0.
But after add-worker / add-external-worker flannel ifaces are missing, I suppose due to wg0 re-creation. We need to add something like kubectl -n kube-system delete pod -l 'app=flannel' after SetupEncryptedNetwork() or maybe inside SetupEncryptedNetwork()

[voron]

Here is corresponding flannel issue. flannel healthz check doesn't look useful too.

I added to PR and tested "delete flannel pod" work-around, now add-worker/add-external-worker perform as expected. But it's just a work-around.

[voron]

@mavimo @xetys WDYT
I don't like options like local PostUp wireguard hook similar to pkill flannel. It may be useful for those who needs to re-configure wg0 frequently, but it's out of project scope.

[xetys]

Hi,

wanna refresh the state on this PR. Is it ready to be reviewed? Does it work for you as expected?

[voron]

@xetys

Is it ready to be reviewed?

Yes, the only issue is codeclimate IDK how to avoid.

Does it work for you as expected?

Yes

[itsmepetrov]

Any updates?

[mavimo]

@itsmepetrov we are working to change the way we create internal network to use the Hetzner network feature.