Kubernetes support externalname service

[regner]

Fixes #1142

Docs will need to be updated but before doing that I wanted to make sure I was at least going about this the right way.

Along with actually supporting the ExternalName service type I added support for the passHostHeader annotation. This is a requirement for actually using ExternalName.

This is basically the first Go code I have written, so very much open to any and all feedback.

[timoreimann]

The log message won't be very useful if we hit the !exists case.

I'd rather use an if/else-if or switch construct to produce two distinct messages.

[regner]

Happy to make the change, but that is how it was before I touched anything. Mostly I don't fully understand what the different between the two is.

[timoreimann]

I'd say let's be good boy scouts and leave the place a bit tidier than how we found it. :-)

Which two do you mean? If/else-if and select?

[regner]

Sorry, I meant the error and exists. Not sure what the log message for each should be as I don't fully understand what they mean.

After a quick look it seems err would mean there was an error in contacting the k8s API and exists would mean there are endpoints in the response. Not 100% sure on that though.

[timoreimann]

Yes, that's basically right: If some error occurs during the GetEndpoints call (which could be prior, during, or after reaching out to the k8s API), the err will be non-nil.

If the API call succeeds but no endpoints were found for the given namespace and name, exists will be false.

One last note: If you split up the OR'ed statement in order to make the error message more specific, be sure to check and handle the err != nil case first: It's conventional in Go that if an error occurs, the remaining return parameters have either arbitrary or zero values.

[timoreimann]

You could add a default case to log a warning. I'd then also add a test making sure that an invalid value does not change the PassHostHeader value.

[regner]

Thanks for the feedback @timoreimann. I have made the requested changes. :)

[timoreimann]

👍

(It would also suffice to just do an if (no else) since we continue if we hit the first case.)

[regner]

Could you expand upon that a little? New to Go and not following. Will also do a bit of reading when I get home tonight.

[timoreimann]

Sure! I'm basically proposing to replace (reducing/simplifying irrelevant parts):

endpoints, exists, err := k8sClient.GetEndpoints(ns, name)
if err != nil {
  log.Errorf("error: %s", err)
  continue
} else if !exists {
  ...
}

by

endpoints, exists, err := k8sClient.GetEndpoints(ns, name)
if err != nil {
  log.Errorf("error: %s", err)
  continue
}

if !exists {
  ...
}

(Note the difference is one less else and an additional line break for readability reasons.)

The logic behind this is as following:

1. If we run into the err != nil case, we'll log an error message and continue to the next loop iteration. Thus, we will never reach the statement checking for !exists.
2. If we run into the err == nil case, we want to check for !exists. In that sense, it's not necessary to use an else if statement because of the previous point: We have already "protected" the err != nil case from doing any additional, unnecessary checks (unnecessary because in the case where an error occurred, no further actions should be taken) by a continue statement. So overall, a simple if suffices.

My rationale here is that it's usually beneficial to turn unnecessary else if into more simple if statements in order to minimize the cognitive load. (No else case means I won't have to bother thinking "what other case are we in now?").

Another reason is that Go programs usually follow a pattern of

1. Calling a function
2. Checking for error
3. in case of an error: return the error
4. Otherwise, proceed to the next function call

The piece of code at hand is a variation of this pattern where we replace 3. by a log and a continue statement.

Hope this helps!

[regner]

It does and makes great sense. Thank you very much for the detailed feedback and explanation. I will make the change now.

[regner]

As this seems to be reasonably acceptable I will go ahead and update some docs when I have a few minutes to spare.

[regner]

@emilevauge @SantoDE @errm @dtomcej @vdemeester

[errm]

LGTM

[dtomcej]

LGTM