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

Add implementation of wildcard DNS lookup for container aliases #43444

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

Add implementation of wildcard DNS lookup for container aliases #43444

wants to merge 2 commits into from

Conversation

MFAshby
Copy link

@MFAshby MFAshby commented Mar 31, 2022

Fixes #43442

- What I did

Add implementation of wildcard DNS lookup for container aliases

- How I did it

ResolveName function is updated to perform both exact match for service name to request, and also wildcard match, i.e. entries starting *., strip the * and check if the remainder is a suffix of the request. There is an RFC clarifying how this should work, I think this implementation complies.

I welcome suggestions for faster implementation. I am slightly concerned that I have swapped a map lookup to a loop, which could have a negative performance impact for large numbers of containers.

- How to verify it

Unit test is added. Manual verification can be performed:

docker network create foonetwork
docker run --network foonetwork '--network-alias=*.foo.local' -d nginx
docker run --network foonetwork alpine wget -O - http://my.foo.local
... expect HTML output from nginx ...

- Description for the changelog

Add support for wildcard aliases with --network-alias argument/

- A picture of a cute animal (not mandatory but encouraged)

https://c.files.bbci.co.uk/7853/production/_115230803_i61pmw-q.jpg

@thaJeztah thaJeztah added status/1-design-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/networking impact/changelog labels Apr 1, 2022
@MFAshby
Copy link
Author

MFAshby commented May 5, 2022

one efficient way to do the wildcard lookup might be a trie, but invert the hostnames since we're looking for a suffix not a prefix. This would work for exact matches too, so I don't think it would be necessary to store the set as well as the trie.

@outdooracorn
Copy link

Hey, thanks for creating a PR to add this feature!

I did notice that this only supports wildcard prefixes. It would be great if this could support wildcards anywhere in the alias. E.g. test.*.example.local, test*.example.local, and *.test.*.example.local.

Although, I don't want to hold up this PR; as it's a lot better than the current state of no wildcard handling.

@MFAshby
Copy link
Author

MFAshby commented Nov 22, 2022

I did notice that this only supports wildcard prefixes. It would be great if this could support wildcards anywhere in the alias.

good suggestion, but that's not valid DNS as far as I know.

Wildcards in the DNS are much more limited than other wildcard characters used in other computer systems. Wildcard DNS records have a single * (asterisk) as the leftmost DNS label, such as *.example.com. Asterisks at other places in the domain will not work as a wildcard

https://en.wikipedia.org/wiki/Wildcard_DNS_record#Definitions_of_DNS_wildcards

and here's the RFC (and I agree with the wikipedia author here, it's hard to interpret)

https://datatracker.ietf.org/doc/html/rfc4592#section-2.2.1

@outdooracorn
Copy link

Woops! Thanks for educating me 👍

@DerZade
Copy link

DerZade commented Nov 28, 2022

I would love this feature!! 🥳 What is the holdup on this? It is only a few lines of code and would be a very useful addition. Anything I can do to speed things up?

@DasKoebi
Copy link

I would love this feature too!!! 👍

That would be a great help for my project 🙉 . But in any case, I love your picture 😉

libnetwork/network.go Outdated Show resolved Hide resolved
This allows use of wildcard aliases for container names via
--network-alias command line.

This is useful for running multi-tenant web applications which expect to
expose tenant interfaces on sub-domains.

Fixes #43442

Signed-off-by: Martin Ashby <martin@ashbysoft.com>
Suggestion from @DerZade
Fixes #43444 (comment)
Fixes #43442

Signed-off-by: Martin Ashby <martin@ashbysoft.com>
@khssnv
Copy link

khssnv commented Dec 20, 2022

I need this feature too.

@MFAshby MFAshby closed this by deleting the head repository Dec 28, 2022
@DerZade
Copy link

DerZade commented Dec 28, 2022

Sad, but understandable considering that this PR has been open for almost nine months and has never been looked at by any maintainer. No comments or anything like that. #thebeautyofoss

@MFAshby MFAshby reopened this Dec 28, 2022
@MFAshby
Copy link
Author

MFAshby commented Dec 28, 2022

Sad, but understandable ...

I was tidying my forks and forgot I had this PR open. I still want this, although it's not blocking my own work any longer.

@thrau
Copy link

thrau commented Jan 9, 2023

@MFAshby this feature would be extremely useful for LocalStack, where everything to *.localhost.localstack.cloud needs to resolve to the localstack container. we currently do this with lots of configuration, documentation, and workarounds.

is there anything we can do to push this forward?

@MFAshby
Copy link
Author

MFAshby commented Jan 9, 2023

is there anything we can do to push this forward?

Thanks @thrau . I guess it needs design review from an appropriate maintainer; is this feature something that the moby team are OK with? I don't know who is best to ask, maybe I should check on the discussions page.

It might also need some benchmarking, as I've noted in the original PR description, but I think it's only worth investing the effort to do that if the feature is found to be acceptable.

@maoxuner
Copy link

Could anybody get it reviewed? I'm waiting this feature for a long time.

@MFAshby
Copy link
Author

MFAshby commented Jan 10, 2023

Maybe @thaJeztah ?

@mattjanssen
Copy link

This would be a super useful feature for us, too 🥹

@chrismacp
Copy link

I wish I had the knowledge to review this, would be very handy to have this feature.

MFAshby added a commit to MFAshby/moby that referenced this pull request Feb 6, 2024
Suggestion from @DerZade
Fixes moby#43444 (comment)
Fixes moby#43442

Signed-off-by: Martin Ashby <martin@ashbysoft.com>
@MFAshby
Copy link
Author

MFAshby commented Feb 6, 2024

I tried to rebase this, but since I deleted the original fork I can't change this PR it seems. If changes are required I'll open a new one.

@thrau
Copy link

thrau commented Feb 17, 2024

Could someone from the maintainers and docker folks ( @thaJeztah @vvoland @neersighted ?) let us know whether this is being considered or not? It's the most upvoted PR in the project and has been open for almost two years now. It's understandable if community contributions don't always match with design goals of the project (if that's what's going on), but it would be great to at least understand what the thinking is or what the concerns are. What's blocking the PR?

Screenshot at 2024-02-17 15-38-39

@MFAshby
Copy link
Author

MFAshby commented Feb 19, 2024

let us know whether this is being considered or not?

I'm quite happy to put some more effort into testing and benchmarking this BTW (I noted a possible performance concern in the original description for example), but only if it has a chance of being merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking/dns area/networking impact/changelog kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/1-design-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow wildcard container host aliases
10 participants