Random suffix only on hostname collision in namespace.

[shanna]

Implements random suffixes only on hostname collision. Resolves #766.

0.16.0 introduced random suffixes to all machine given names (DNS hostnames) regardless of collisions within a namespace. This PR brings Headscale more inline with Tailscale by only adding a suffix if the hostname will collide within the namespace.

The suffix generation differs from Tailscale.
See https://tailscale.com/kb/1098/machine-names/

• read the CONTRIBUTING guidelines
• raised a GitHub issue or discussed it on the projects chat beforehand
• added unit tests
• added integration tests
• updated documentation if needed
• updated CHANGELOG.md

[restanrm]

Do we really want the machines name to be unique per namespace or the all network ? I'd prefer remove completely the namespace from the DNS name. I think the namespace name in DNS name is killing the magicDNS feature, since we often want to communicate with hosts that are in other namespaces or are tagged. Also, the tagged machines should not be tied to the user that created them.

[shanna]

@restanrm my reasoning is:

1. To stay closer to what I'd expect from regular DNS. Just like I wouldn't expect random suffixes I also wouldn't expect collisions between names on different (sub)domains.
2. If names are unique across the entire headscale instance you'll leak hostname information between namespaces that might not be visible to each other due to ACL if you don't. E.g. In multi-(tenant/team/project) scenarios like mine where I have multiple namespace (groups) of IOT devices that will never interact between namespaces.
3. Personally I would have kept it simple and backed out the random suffix change entirely. Treat MagicDNS like any other hostname collision on a local network leaving it up to network administrator to resolve by renaming one of the machines. I don't know enough about MagicDNS, Headscale or Tailscale though to know how much of an issue this is so I've implemented the direction kradalby wanted to go in the ticket. I'm assuming he knows better here. The Tailscale folks also add a suffix if names collide in a namespace according to the documentation. (edit) Bonjour/zeroconf add a suffix if hostnames collide though so shows what I know.

The Tailscale docs do a pretty good job of explaining why namespaces are useful here and how they handle hostname collisions here.

@juanfont Sorry I didn't realise make test only ran the unit tests so I didn't run any of the integration tests before creating the PR. I'm not sure why this is failing the integration tests, I'm hoping this is more to do with test setup now that the function requires database calls than the change itself. I'm under a time crunch at the moment but I'll take a look as soon as I can.

[restanrm]

@restanrm my reasoning is:

1. To stay closer to what I'd expect from regular DNS. Just like I wouldn't expect random suffixes I also wouldn't expect collisions between names on different (sub)domains.

2. If names are unique across the entire headscale instance you'll leak hostname information between namespaces that might not be visible to each other due to ACL if you don't. E.g. In multi-(tenant/team/project) scenarios like mine where I have multiple namespace (groups) of IOT devices that will never interact between namespaces.

3. Personally I would have kept it simple and backed out the random suffix change entirely. Treat MagicDNS like any other hostname collision on a local network leaving it up to network administrator to resolve by renaming one of the machines. I don't know enough about MagicDNS, Headscale or Tailscale though to know how much of an issue this is so I've implemented the direction [kradalby](https://github.com/kradalby) wanted to go in the ticket. I'm assuming he knows better here. The Tailscale folks also add a suffix if names collide in a namespace according to the documentation. (edit) Bonjour/zeroconf add a suffix if hostnames collide though so shows what I know.

The Tailscale docs do a pretty good job of explaining why namespaces are useful here and how they handle hostname collisions here.

@juanfont Sorry I didn't realise make test only ran the unit tests so I didn't run any of the integration tests before creating the PR. I'm not sure why this is failing the integration tests, I'm hoping this is more to do with test setup now that the function requires database calls than the change itself. I'm under a time crunch at the moment but I'll take a look as soon as I can.

Thanks for your response !

My comment was mainly related to make Headscale closer to Tailscale (in my opinion) because Namespaces are closer to users (in Tailscale) than Tailnet. A Headscale instance should be viewed as a Tailnet as explained here

Although I agree that admin should not join 2 machines with the same hostname on a network it can happen. I don't think that Tailscale's DNS embedded system can handle 2 identical hostnames since they don't offer this functionality with the SaaS version.

For the leak of the name, yes it's possible to rename one's host until we find a collision. But the ACL's would still not allow to obtain the IP address of the other machine if it's not meant to be.

[shanna]

For the leak of the name, yes it's possible to rename one's host until we find a collision. But the ACL's would still not allow to obtain the IP address of the other machine if it's not meant to be.

I meant from a data security perspective more than a network security one. Like when incremental IDs or unique name slugs are used as IDs in URLs to sniff out other customers, customer or machine counts. The sort or thing (however unlikely).

[kradalby]

I think approaching this in incremental steps would be ideal. I am up for discussing having namespace/users in the DNS like @restanrm suggests, but I think the ideal way to tackle this right now would be to make it unique across the whole headscale instance and only add the hash if it isnt.

This allows us to change it later, to only be unique per namespace. It will be way easier to loosen that logic than to go the opposite way.

If we check unique for the whole instance, and clear up the test failures, then I am happy to get this in.
Sounds reasonable?

[dimaqq]

Regarding the definition of the "collision", is there a potential for a data race here?
Let's say we had a host host connected as a node with hostname host.
Then that node lost connection, maybe it was rebooted, reprovisioned, etc., and it connected again.
There's no guarantee that the old node record is gone from headscale, is there?
In which case, the newly connected host will get a suffix due a collision, which is what we wanted to avoid, won't it?

[kradalby]

I am not sure if I understand, but the entry in the database should be the same if it reconnects again?

So for the cases of lost connection, reboot and connected again, then it should be fine.

For reprovisioning, the node will first have to be deleted, which makes sense I think.