Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat] annotations for hostnames and without-namespace for services #22

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

iamasmith
Copy link
Contributor

@iamasmith iamasmith commented Mar 9, 2024

Based on the PR by @pl4nty this goes further.

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.

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

Feedback please, quite happy to alter as requested :)

@iamasmith iamasmith changed the title Annotations for hostnames and without-default for services [feat] annotations for hostnames and without-default for services Mar 9, 2024
@iamasmith iamasmith force-pushed the multi-hostnames-by-annotation branch from 9961014 to d98940d Compare March 9, 2024 17:08
@iamasmith
Copy link
Contributor Author

iamasmith commented Mar 9, 2024

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 iamasmith force-pushed the multi-hostnames-by-annotation branch from d98940d to 93417fa Compare March 9, 2024 18:30
@iamasmith
Copy link
Contributor Author

correction pushed to the refactor commit.

@blake blake linked an issue Mar 9, 2024 that may be closed by this pull request
@blake blake self-assigned this Mar 9, 2024
@blake blake added the enhancement New feature or request label Mar 9, 2024
Copy link

@pl4nty pl4nty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just fixed some typos

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
pl4nty added a commit to pl4nty/containers that referenced this pull request Mar 10, 2024
@iamasmith
Copy link
Contributor Author

iamasmith commented Mar 10, 2024

@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 iamasmith force-pushed the multi-hostnames-by-annotation branch from 93417fa to 427ee8e Compare March 10, 2024 09:29
@iamasmith iamasmith changed the title [feat] annotations for hostnames and without-default for services [feat] annotations for hostnames and without-namespace for services Mar 10, 2024
@iamasmith iamasmith force-pushed the multi-hostnames-by-annotation branch from 427ee8e to 8eb4874 Compare March 10, 2024 09:38
@iamasmith
Copy link
Contributor Author

iamasmith commented Mar 10, 2024

@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
Copy link
Contributor Author

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

@blake
Copy link
Owner

blake commented Mar 11, 2024

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 iamasmith force-pushed the multi-hostnames-by-annotation branch from 2b32508 to 189d070 Compare March 11, 2024 17:34
@iamasmith
Copy link
Contributor Author

iamasmith commented Mar 11, 2024

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
Copy link
Contributor Author

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.

@pl4nty
Copy link

pl4nty commented Apr 27, 2024

@iamasmith is your image public? I'd like to use it until this merges

@iamasmith
Copy link
Contributor Author

iamasmith commented Apr 27, 2024

@pl4nty it's not at the moment, it's only in my home lab repo.

I'll see if I can push it up to ghcr soon if you like, it's currently built only for amd64 but I can add an arm64 build and combine them.

EDIT looks like this one I only built for the amd64 nodes at the moment.

@pl4nty
Copy link

pl4nty commented Apr 27, 2024

thanks, I'd appreciate that. most of my nodes are arm64, but I have an amd64 node if needed

@iamasmith
Copy link
Contributor Author

I'll have to spend some time working out what PAT permissions to use. I'm not using Github actions, my build runners run off Gitlab.

@iamasmith
Copy link
Contributor Author

iamasmith commented Apr 27, 2024

@pl4nty I've built the arm64 image as well as amd64 now, pushed them both up and joined them into a multi-arch image at ghcr.io/iamasmith/external-mdns - both archs seem to run fine on my lab clusters - HTH

andrews@AndrewsiMac ~ % manifest-tool inspect ghcr.io/iamasmith/external-mdns:latest
Name:   ghcr.io/iamasmith/external-mdns:latest (Type: application/vnd.docker.distribution.manifest.list.v2+json)
Digest: sha256:2cefed1cc1c46ac6703679b634da32df060c87cf67d8c99e51f46cb499a7d9a1
 * Contains 2 manifest references (2 images, 0 attestation):
[1]     Type: application/vnd.docker.distribution.manifest.v2+json
[1]   Digest: sha256:15b9e6a48ad07ffd9b6b0065ecfaf6b0f0c6030fa97346ea2aeceec18db109d4
[1]   Length: 528
[1] Platform:
[1]    -      OS: linux
[1]    -    Arch: amd64
[1] # Layers: 1
     layer 01: digest = sha256:b535ab8fbd1827d68951ad84a03a7e896982438a9dfe0b493c9d680179206c16
                 type = application/vnd.docker.image.rootfs.diff.tar.gzip

[2]     Type: application/vnd.docker.distribution.manifest.v2+json
[2]   Digest: sha256:f10542605e023c3d3ec36506845c8a4d4b04fe50248ddd30204d1462d626cc44
[2]   Length: 527
[2] Platform:
[2]    -      OS: linux
[2]    -    Arch: arm64
[2] # Layers: 1
     layer 01: digest = sha256:c64a92f31ac5860f846453e9a2cf799fbb99fb3c719233dc81dcc0f87cb5f20b
                 type = application/vnd.docker.image.rootfs.diff.tar.gzip

andrews@AndrewsiMac ~ % 

@iamasmith
Copy link
Contributor Author

iamasmith commented Apr 27, 2024

Apologies for the delay, the Mrs wanted to go out for a walk first.
I decided against running ARC and using Gitlab Actions when I found that it wouldn't work without an Org added to your account (you only find out once you have the operator setup for it and the API path is hardcoded to use an org in the URI) and decided that as I knew Gitlab really well I would use a self hosted Kubernetes runner which works really well as I can do matrix builds for different architectures and they will just build natively on the appropriate nodes. Golang is pretty good for cross compilation but I generally like to test my code on the same arch too.
Anyway, now I've gotten round to doing the PAT I'll probably add a public step to all my pipelines - there are a couple of projects that I've been meaning to open public anyway. I spent ages looking at the PAT options to tie it down to image repo only then found that the options were actually only available on classic PATs and not the new type.

@iamasmith
Copy link
Contributor Author

just checking in @pl4nty to see how you are getting on..?

@pl4nty
Copy link

pl4nty commented Aug 12, 2024

@iamasmith thanks for publishing the image, it's been stable for a few months now. I even implemented DNS-SD on the weekend - might PR if it's a good fit for the project

@strophy
Copy link

strophy commented Nov 25, 2024

@iamasmith thanks for working on this!

I'm using Traefik 3 with Gateway API enabled. I expose a number of services in the cluster, and Traefik sits in front of them with HTTPRoutes defined for each service. Traefik is responsible for handling TLS termination with cert-manager and pointing at the correct service based on the DNS hostname in the request. Only one LoadBalancer type Service actually exists though, so installing the standard version of external-mdns results in only traefik.local mDNS names being created. Would this PR (or the ghcr.io/iamasmith/external-mdns image) allow me to annotate the HTTPRoutes or their backing services and create more mDNS names like frontend.local, or is this something I need to open a separate issue/PR for?

@iamasmith
Copy link
Contributor Author

iamasmith commented Nov 25, 2024

@strophy yes, that's exactly what I added the feature for although my ingress is an istio gateway (which is actually just a service and deployment with it's own Gateway/Ingress type API) however. Annotations go on the service of course.
This would be a reasonable workaround for the lack of gateway API support in external-mdns

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose service with arbitrary hostname
4 participants