Random suffix only on collision.

0.16.0 introduced random suffixes to all machine given names
(DNS hostnames) regardless of collisions within a namespace.
This commit 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/

CHANGELOG.md

@@ -18,6 +18,7 @@
 - Add support for generating pre-auth keys with tags [#767](https://github.com/juanfont/headscale/pull/767)
 - Add support for evaluating `autoApprovers` ACL entries when a machine is registered [#763](https://github.com/juanfont/headscale/pull/763)
 - Add config flag to allow Headscale to start if OIDC provider is down [#829](https://github.com/juanfont/headscale/pull/829)
+- Random node DNS suffix only applied if names collide in namespace. [#766](https://github.com/juanfont/headscale/issues/766)
 
 ## 0.16.4 (2022-08-21)
 

grpcv1.go

@@ -1,4 +1,4 @@
-//nolint
+// nolint
 package headscale
 
 import (
@@ -479,7 +479,7 @@ func (api headscaleV1APIServer) DebugCreateMachine(
 		Hostname:    "DebugTestMachine",
 	}
 
-	givenName, err := api.h.GenerateGivenName(request.GetName())
+	givenName, err := api.h.GenerateGivenName(namespace.Name, request.GetKey(), request.GetName())
 	if err != nil {
 		return nil, err
 	}

machine.go

@@ -348,6 +348,22 @@ func (h *Headscale) GetMachine(namespace string, name string) (*Machine, error)
 	return nil, ErrMachineNotFound
 }
 
+// GetMachineByGivenName finds a Machine by given name and namespace and returns the Machine struct.
+func (h *Headscale) GetMachineByGivenName(namespace string, givenName string) (*Machine, error) {
+	machines, err := h.ListMachinesInNamespace(namespace)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, m := range machines {
+		if m.GivenName == givenName {
+			return &m, nil
+		}
+	}
+
+	return nil, ErrMachineNotFound
+}
+
 // GetMachineByID finds a Machine by ID and returns the Machine struct.
 func (h *Headscale) GetMachineByID(id uint64) (*Machine, error) {
 	m := Machine{}
@@ -1018,11 +1034,7 @@ func (machine *Machine) RoutesToProto() *v1.Routes {
 	}
 }
 
-func (h *Headscale) GenerateGivenName(suppliedName string) (string, error) {
-	// If a hostname is or will be longer than 63 chars after adding the hash,
-	// it needs to be trimmed.
-	trimmedHostnameLength := labelHostnameLength - MachineGivenNameHashLength - MachineGivenNameTrimSize
-
+func (h *Headscale) generateGivenName(suppliedName string, randomSuffix bool) (string, error) {
 	normalizedHostname, err := NormalizeToFQDNRules(
 		suppliedName,
 		h.cfg.OIDC.StripEmaildomain,
@@ -1031,18 +1043,40 @@ func (h *Headscale) GenerateGivenName(suppliedName string) (string, error) {
 		return "", err
 	}
 
-	postfix, err := GenerateRandomStringDNSSafe(MachineGivenNameHashLength)
+	if randomSuffix {
+		// Trim if a hostname will be longer than 63 chars after adding the hash.
+		trimmedHostnameLength := labelHostnameLength - MachineGivenNameHashLength - MachineGivenNameTrimSize
+		if len(normalizedHostname) > trimmedHostnameLength {
+			normalizedHostname = normalizedHostname[:trimmedHostnameLength]
+		}
+
+		suffix, err := GenerateRandomStringDNSSafe(MachineGivenNameHashLength)
 		if err != nil {
 			return "", err
 		}
 
-	// Verify that that the new unique name is shorter than the maximum allowed
-	// DNS segment.
-	if len(normalizedHostname) <= trimmedHostnameLength {
-		normalizedHostname = fmt.Sprintf("%s-%s", normalizedHostname, postfix)
-	} else {
-		normalizedHostname = fmt.Sprintf("%s-%s", normalizedHostname[:trimmedHostnameLength], postfix)
+		normalizedHostname += "-" + suffix
 	}
 
 	return normalizedHostname, nil
 }
+
+func (h *Headscale) GenerateGivenName(namespace string, machineKey string, suppliedName string) (string, error) {
+	givenName, err := h.generateGivenName(suppliedName, false)
+	if err != nil {
+		return "", err
+	}
+
+	// Tailscale rules (may differ) https://tailscale.com/kb/1098/machine-names/
+	machine, _ := h.GetMachineByGivenName(namespace, givenName)
+	if machine != nil && machine.MachineKey != machineKey && machine.GivenName == givenName {
+		postfixedName, err := h.generateGivenName(suppliedName, true)
+		if err != nil {
+			return "", err
+		}
+
+		givenName = postfixedName
+	}
+
+	return givenName, nil
+}

machine_test.go

@@ -4,8 +4,8 @@ import (
 	"fmt"
 	"net/netip"
 	"reflect"
+	"regexp"
 	"strconv"
-	"strings"
 	"testing"
 	"time"
 
@@ -346,6 +346,53 @@ func (s *Suite) TestSerdeAddressStrignSlice(c *check.C) {
 	}
 }
 
+func (s *Suite) TestGenerateGivenName(c *check.C) {
+	namespace1, err := app.CreateNamespace("namespace-1")
+	c.Assert(err, check.IsNil)
+
+	namespace2, err := app.CreateNamespace("namespace-2")
+	c.Assert(err, check.IsNil)
+
+	pak, err := app.CreatePreAuthKey(namespace1.Name, false, false, nil)
+	c.Assert(err, check.IsNil)
+
+	_, err = app.GetMachine("namespace-1", "testmachine")
+	c.Assert(err, check.NotNil)
+
+	machine := &Machine{
+		ID:             0,
+		MachineKey:     "machine-key-1",
+		NodeKey:        "node-key-1",
+		DiscoKey:       "disco-key-1",
+		Hostname:       "hostname-1",
+		GivenName:      "hostname-1",
+		NamespaceID:    namespace1.ID,
+		RegisterMethod: RegisterMethodAuthKey,
+		AuthKeyID:      uint(pak.ID),
+	}
+	app.db.Save(machine)
+
+	givenName, err := app.GenerateGivenName(namespace1.Name, "machine-key-2", "hostname-2")
+	comment := check.Commentf("Same namespace, unique machines, unique hostnames, no conflict")
+	c.Assert(err, check.IsNil, comment)
+	c.Assert(givenName, check.Equals, "hostname-2", comment)
+
+	givenName, err = app.GenerateGivenName(namespace1.Name, "machine-key-1", "hostname-1")
+	comment = check.Commentf("Same namespace, same machine, same hostname, no conflict")
+	c.Assert(err, check.IsNil, comment)
+	c.Assert(givenName, check.Equals, "hostname-1", comment)
+
+	givenName, err = app.GenerateGivenName(namespace1.Name, "machine-key-2", "hostname-1")
+	comment = check.Commentf("Same namespace, unique machines, same hostname, conflict")
+	c.Assert(err, check.IsNil, comment)
+	c.Assert(givenName, check.Matches, fmt.Sprintf("^hostname-1-[a-z0-9]{%d}$", MachineGivenNameHashLength), comment)
+
+	givenName, err = app.GenerateGivenName(namespace2.Name, "machine-key-2", "hostname-1")
+	comment = check.Commentf("Unique namespaces, unique machines, same hostname, no conflict")
+	c.Assert(err, check.IsNil, comment)
+	c.Assert(givenName, check.Equals, "hostname-1", comment)
+}
+
 func (s *Suite) TestSetTags(c *check.C) {
 	namespace, err := app.CreateNamespace("test")
 	c.Assert(err, check.IsNil)
@@ -917,15 +964,16 @@ func Test_getFilteredByACLPeers(t *testing.T) {
 	}
 }
 
-func TestHeadscale_GenerateGivenName(t *testing.T) {
+func TestHeadscale_generateGivenName(t *testing.T) {
 	type args struct {
 		suppliedName string
+		randomSuffix bool
 	}
 	tests := []struct {
 		name    string
 		h       *Headscale
 		args    args
-		want    string
+		want    *regexp.Regexp
 		wantErr bool
 	}{
 		{
@@ -939,8 +987,9 @@ func TestHeadscale_GenerateGivenName(t *testing.T) {
 			},
 			args: args{
 				suppliedName: "testmachine",
+				randomSuffix: false,
 			},
-			want:    "testmachine",
+			want:    regexp.MustCompile("^testmachine$"),
 			wantErr: false,
 		},
 		{
@@ -954,23 +1003,9 @@ func TestHeadscale_GenerateGivenName(t *testing.T) {
 			},
 			args: args{
 				suppliedName: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine",
+				randomSuffix: false,
 			},
-			want:    "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine",
-			wantErr: false,
-		},
-		{
-			name: "machine name with 60 chars",
-			h: &Headscale{
-				cfg: &Config{
-					OIDC: OIDCConfig{
-						StripEmaildomain: true,
-					},
-				},
-			},
-			args: args{
-				suppliedName: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine1234567",
-			},
-			want:    "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine",
+			want:    regexp.MustCompile("^testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine$"),
 			wantErr: false,
 		},
 		{
@@ -983,9 +1018,10 @@ func TestHeadscale_GenerateGivenName(t *testing.T) {
 				},
 			},
 			args: args{
-				suppliedName: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine1234567890",
+				suppliedName: "machineeee12345678901234567890123456789012345678901234567890123",
+				randomSuffix: false,
 			},
-			want:    "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			want:    regexp.MustCompile("^machineeee12345678901234567890123456789012345678901234567890123$"),
 			wantErr: false,
 		},
 		{
@@ -998,10 +1034,11 @@ func TestHeadscale_GenerateGivenName(t *testing.T) {
 				},
 			},
 			args: args{
-				suppliedName: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine1234567891",
+				suppliedName: "machineeee123456789012345678901234567890123456789012345678901234",
+				randomSuffix: false,
 			},
-			want:    "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
-			wantErr: false,
+			want:    nil,
+			wantErr: true,
 		},
 		{
 			name: "machine name with 73 chars",
@@ -1013,15 +1050,48 @@ func TestHeadscale_GenerateGivenName(t *testing.T) {
 				},
 			},
 			args: args{
-				suppliedName: "testmaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaachine12345678901234567890",
+				suppliedName: "machineeee123456789012345678901234567890123456789012345678901234567890123",
+				randomSuffix: false,
 			},
-			want:    "",
+			want:    nil,
 			wantErr: true,
 		},
+		{
+			name: "machine name with random suffix",
+			h: &Headscale{
+				cfg: &Config{
+					OIDC: OIDCConfig{
+						StripEmaildomain: true,
+					},
+				},
+			},
+			args: args{
+				suppliedName: "test",
+				randomSuffix: true,
+			},
+			want:    regexp.MustCompile(fmt.Sprintf("^test-[a-z0-9]{%d}$", MachineGivenNameHashLength)),
+			wantErr: false,
+		},
+		{
+			name: "machine name with 63 chars with random suffix",
+			h: &Headscale{
+				cfg: &Config{
+					OIDC: OIDCConfig{
+						StripEmaildomain: true,
+					},
+				},
+			},
+			args: args{
+				suppliedName: "machineeee12345678901234567890123456789012345678901234567890123",
+				randomSuffix: true,
+			},
+			want:    regexp.MustCompile(fmt.Sprintf("^machineeee1234567890123456789012345678901234567890123-[a-z0-9]{%d}$", MachineGivenNameHashLength)),
+			wantErr: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := tt.h.GenerateGivenName(tt.args.suppliedName)
+			got, err := tt.h.generateGivenName(tt.args.suppliedName, tt.args.randomSuffix)
 			if (err != nil) != tt.wantErr {
 				t.Errorf(
 					"Headscale.GenerateGivenName() error = %v, wantErr %v",
@@ -1032,9 +1102,9 @@ func TestHeadscale_GenerateGivenName(t *testing.T) {
 				return
 			}
 
-			if tt.want != "" && strings.Contains(tt.want, got) {
+			if tt.want != nil && !tt.want.MatchString(got) {
 				t.Errorf(
-					"Headscale.GenerateGivenName() = %v, is not a substring of %v",
+					"Headscale.GenerateGivenName() = %v, does not match %v",
 					tt.want,
 					got,
 				)

protocol_common.go

@@ -150,7 +150,7 @@ func (h *Headscale) handleRegisterCommon(
 			Bool("noise", machineKey.IsZero()).
 			Msg("New machine not yet in the database")
 
-		givenName, err := h.GenerateGivenName(registerRequest.Hostinfo.Hostname)
+		givenName, err := h.GenerateGivenName(machine.Namespace.Name, machine.MachineKey, registerRequest.Hostinfo.Hostname)
 		if err != nil {
 			log.Error().
 				Caller().
@@ -374,7 +374,7 @@ func (h *Headscale) handleAuthKeyCommon(
 	} else {
 		now := time.Now().UTC()
 
-		givenName, err := h.GenerateGivenName(registerRequest.Hostinfo.Hostname)
+		givenName, err := h.GenerateGivenName(machine.Namespace.Name, MachinePublicKeyStripPrefix(machineKey), registerRequest.Hostinfo.Hostname)
 		if err != nil {
 			log.Error().
 				Caller().