More injectRoute() Preparation Refactors

[aauren]

@mrueg @jnummelin @twz123 @rbrtbnfgl

Another refactor PR that is attempting to pull functionality out of NetworkRoutesController in prep for some work that I want to do with injectRoute().

This time instead of tossing the functionality into the utils package like I did with node.go, I'm attempting to be a bit more Go idiomatic and naming the packages appropriately. This also helps the function names to be a bit shorter as context can be derived from the package name.

Note that with the routes package I'm just barely scratching the surface of what I want to do there long term. Trying to keep these refactors at least a bit controlled so that they don't get too big. Ideally there would probably be a lot more functionality that would get put into that package. And also #1697 would probably get consolidated with that effort.

Any and all feedback is appreciated.

[twz123]

I think the refactoring is good, there's only one thing to address concerning valid port ranges, I suppose.

Some parts of the code that have just been moved around as part of the refactoring caught my 👀 and I left some rather unrelated comments. Maybe worth addressing in follow-up pull requests, if applicable.

[twz123]

Totally unrelated to the refactoring going on here, but the "non-entropic" part got me thinking. This would mean that "21.3.0.4" hashes to the same tunnel as "2.13.0.4". Is that something that kube-router should care about?

Wouldn't it be more straight forward to operate on the IP address bytes, instead of their string representation? E.g. like this:

func GenerateTunnelName(nodeIP net.IP) string {
	if v4 := nodeIP.To4(); v4 != nil {
		return fmt.Sprintf("tun-%x", []byte(v4))
	}

	hash := sha256.Sum256(nodeIP)
	return fmt.Sprintf("tun-%x", hash[:6])[:15]
}

The four bytes of an IPv4 address can be losslessly converted to an eight byte hex string. The 16 bytes of an IPv6 address need to be hashed, as before.

The tunnel names for IPv4 addresses will be shorter, and can be easily converted back to the original IP address. Not sure if that's an undesirable side effect. If yes, the four IPv4 bytes could be hashed, as before, although I suppose that this is not a problem at all?

[aauren]

This is a good catch @twz123!

Unfortunately, changing the tunnel names that we create is something that we need to be very careful about as we rely on consistency for cleaning up tunnels. If we change this we need to either add backwards compatible logic and check tunnels against both, or tell cluster administrators to reboot their machine during rollouts.

Considering that removing the punctuations has a potential to cause a hash collision, I have created a TODO in the code as well as the following issue for tracking and fixing this issue: #1748

[twz123]

Given that this is not doing anything special, did you consider to remove this function and just make the OverlayType fields public?

[aauren]

This might be a good one to have a conversation about. Coming from a different programming background than GoLang, I still have the tendency to not make fields public any time that I don't have to. I guess I have been considering it a forcing function for using functions & interfaces rather than getting used to changing the data on a struct directly.

Not sure if this is a good practice in Go or not though. Do you have any thoughts surrounding this?

[twz123]

I used to have the same tendencies, but I more and more accepted that Go is the way Go is, and that it's better to keep it as straight forward as possible. Certain things can't be avoided anyways (like constructing zero values), so callers will almost always be able to break certain invariants that you try to uphold in your types.

The main benefit of having this function that I can see is that you will get compiler errors at the call sites when new required fields are added to OverlayType, so you can't miss to update all of them. If that's important, then it might be a good idea to keep it. On the other hand, adding optional fields is harder with this approach, but darn easy when you have a struct with public fields. I tend to think of those structs as "factory functions with named parameters".

The amount of work you have to put into your types to make them hard to misuse is (subjectively) higher in Go than in some other popular languages out there. This is a trade-off. Do you have a type that will be used all over the place, or is it something that will only be used a few times, maybe just once? Putting more effort (i.e. implementing patterns that require quite a few more lines of code) into the former will pay off more in the long run than for the latter.

[aauren]

Thanks for taking the time to write out how you think about coding in Go! 😄

In this case, I think that I'll keep this particular one, but in general, I think that I agree with you. It does seem that Go is meant to be simpler and more straight forward. Trying to hide everything inside a struct that is only used rarely, is probably not worth the effort and is likely antithetical to the design of Go.

I'll keep this in mind during the next couple of PRs and see how it works for me.