Hostnames annotation feature for services

Instead of a single hostname we add an annotation external-mdns.blakecovarrubias.com/hostnames which can take a comma separated string if present listing short hostnames.

e.g.

Advertising a single name

apiVersion: v1
kind: Service
metadata:
  name: myservice
  namespace: foospace
  annotations:
    external-mdns.blakecovarrubias.com/hostnames: foo
...
spec:
  type: LoadBalancer
...

Advertises the usual foo.foospace.local and foo-foospace.local names instead of myservice.foospace.local

Advertising two names

apiVersion: v1
kind: Service
metadata:
  name: myservice
  namespace: foospace
  annotations:
    external-mdns.blakecovarrubias.com/hostnames: foo, bar
...
spec:
  type: LoadBalancer
...

Advertises the usual foo.foospace.local, foo-foospace.local, bar.foospace.local and bar-foospace.local names instead of myservice.foospace.local

Selective without-namespace annotaton for services

Additionally an annotation external-mdns.blakecovarrubias.com/without-default can be used to conditionally advertise the name as .local mimicing the global option but controlled for the specific service. This annotation is independent of the first annotation giving more flexibility in being able to use without-default less globally. I chose this name to be synonymous with the global value.

e.g.

Use on it's own

apiVersion: v1
kind: Service
metadata:
  name: myservice
  namespace: foospace
  annotations:
    external-mdns.blakecovarrubias.com/without-namespace: "true"
...
spec:
  type: LoadBalancer
...

Causes myservice.local to be additionally advertised alongside myservice.foospace.local and myservice-foospace.local.

Combined use with single hostname

apiVersion: v1
kind: Service
metadata:
  name: myservice
  namespace: foospace
  annotations:
    external-mdns.blakecovarrubias.com/hostnames: foo
    external-mdns.blakecovarrubias.com/without-namespace: "true"
...
spec:
  type: LoadBalancer
...

Advertises the usual foo.local, foo.foospace.local, foo-foospace.local names instead of myservice.foospace.local etc.

Comments

Noting, my instinct would have been not to publish the namespaced based records when using without-namespace but to retain backward compatibility I chose not to change this behaviour and not to further increase the MR by adding a second control for this.

This should cater as a solid workaround for advertising TCP based ingress rules and services behind an Istio Gateway as mentioned in #21

The PR is separated into 4 commits to ease review.

A Refactor tidy up and Adding the Facility to hold multiple names in the resource struct.
Implementation of the first annotation.
Implementation of the second annotation.
Documentation

Feedback please, quite happy to alter as requested :)

iamasmith commented 7 months ago

Additional note, looking at PTR handling there are obvsiouly conflicts in the PTR records generated. Only one can be relevant so advertising multiples in MDNS may have unpredictable behaviour. I made no attempt to correct this as I suspect problems would be edge cases and at least one resolution to a working forward name would be good enough for most dependent services plus the implementation would require concensus if several replicas of the service were deployed. Possibly it would be enough just to build a sorted list and choose one to publish (first or last) which would ensure consistency between additional replicas.

iamasmith commented 7 months ago

correction pushed to the refactor commit.

iamasmith commented 7 months ago

@pl4nty thanks so much! I really, really appreciate your review! You know the pattern, I was swapping annotations back and forth in names for correctness and I actually went with the wrong one, the annotation reallly should have been /without-namespace to complement the global feature. I think I got distracted by the default-namespace global and just got it all a bit muddled. Updates have been rebased and pushed to reflect all of this and the typos that you highlighted. I've kept it to the same 4 commits - all changes are actually on rebased versions of the last 2 commits. I also fixed a shortlived variable name that was annoying me when I saw it.

iamasmith commented 6 months ago

@blake to ensure compatibility with the new changes merged for the security update I've cherry picked @Stelminator and my updates merged to master to this branch so any builds off it should work fine with the new suggested configs.

For this reason, at merge time you might not want to squash as the original commit IDs of these cherry-picked changes will be relevant.

If this is a problem, post test I can rewind the head of this branch and force push again for you to squash as normal.

iamasmith commented 6 months ago

Actually, I'll reorder these commits tonight so the cherry-picked ones appear first and they should disappear from the MR.

blake commented 6 months ago

Switch to your branch and run git rebase master. It'll add your changes on top of the latest commits from the master branch.

iamasmith commented 6 months ago

Of course!, I'm always wary of just doing that our Gitlab instance at work is finicky about merge commits entering branches and rejects anything on the branch at push time if it doesn't have a Jira ticket but I suppose if they are already merged to master then it won't get pushed. TBH, the amount of times I work on stuff outside of work and start the commit message with our teams Jira project ID is crazy. All sorted now.

iamasmith commented 6 months ago

Take your time and review, I'm happy with the version with these changes on my home lab so probably won't check back so much but if you want any changes etc. please just reach out and I'll be happy to take a look.