Machine rename support #498

bravechamp · 2022-03-13T21:17:13Z

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

Changes:

Machines can have nicknames
If nickname is set, it overrides Machine Name and new name will be used instead
Added nickname on nodes list output
New command nodes rename sets and clears nickname

restanrm

I think you are missing some lock mechanism to forbid update of the name from the machine like the enable autogenerate machine name from https://tailscale.com/kb/1098/machine-names/.
If you don't bring something like that, it would overwritten quickly I think.

bravechamp · 2022-03-13T22:58:32Z

Thanks for the feedback. But, that's the point of the nickname field. It's a new field and can only be set by cli (or api). When set, it overrides machine name. When cleared, machine name is used. So, enable autogenerate machine name checkbox functionality is there.

restanrm · 2022-03-14T11:15:44Z

Thanks for the feedback. But, that's the point of the nickname field. It's a new field and can only be set by cli (or api). When set, it overrides machine name. When cleared, machine name is used. So, enable autogenerate machine name checkbox functionality is there.

Sorry I missed that point. Then, I think you should add a migration script to add the column in the database. See what is done in db.go

bravechamp · 2022-03-14T11:18:41Z

Well, db.AutoMigrate(&Machine{}) already does that, right? During my tests, column appeared as soon as I run the executable.

restanrm · 2022-03-14T13:44:31Z

Neat thanks !

kradalby · 2022-03-16T07:37:57Z

cmd/headscale/cli/nodes.go

@@ -40,6 +40,13 @@ func init() {
 	}
 	nodeCmd.AddCommand(expireNodeCmd)

+	renameNodeCmd.Flags().Uint64P("identifier", "i", 0, "Node identifier (ID)")


general thought: Is this a rename, or is it a setname or something?

I just reused headscale namespaces rename command to keep things familiar. I'm open for suggestions. Right now you can headscale nodes -i 1 rename newmachinename to set the Nickname field or headscale nodes -i 1 rename to clear it.

Ye, its mostly fine, I didnt have a really good name

kradalby · 2022-03-16T07:38:48Z

cmd/headscale/cli/nodes.go

@@ -207,6 +214,54 @@ var expireNodeCmd = &cobra.Command{
 	},
 }

+var renameNodeCmd = &cobra.Command{


These command will need integration tests

kradalby · 2022-03-16T07:40:13Z

machine.go

@@ -367,6 +368,29 @@ func (h *Headscale) ExpireMachine(machine *Machine) {
 	h.db.Save(machine)
 }

+// RenameMachine takes a Machine struct and sets the expire field to now.


This comment seems wrong ;)

Oops. Result of pure laziness. Fixing...

kradalby · 2022-03-16T07:41:24Z

machine.go

@@ -40,6 +40,7 @@ type Machine struct {
 	DiscoKey    string
 	IPAddresses MachineAddresses
 	Name        string
+	Nickname    string


I am a bit conflicted about the naming here; Nickname doesnt just sound completely right, in a way, what is now called Name should become HostName and then Nickname should be Name or maybe GivenName

I agree Nickname is not a good name. I didn't want to change the name of an existing field. Shall I change the Name to Hostname and keep the db column name as name?

I am slightly concerned that the migration software actually understands this tho... I would like to have that sorted. I personally am not interested in having naming things we are not happy with accumulate.

I suspect that using something like RenameColumn (https://gorm.io/docs/migration.html) would just end in an "infinite" loop, renaming it every startup

I think we can do:

Suggested change

Nickname string

GivenName string

and HostName, and add a comment above both with how they are set, and what they mean.

kradalby · 2022-03-16T07:42:35Z

machine.go

+	newNickname, err := NormalizeToFQDNRules(
+		newName,
+		h.cfg.OIDC.StripEmaildomain,
+	)


Since this is a user set name, we should not modify the value, we should throw a "name is not valid error" instead of setting something the user didnt expect.

OK, sounds good. I'll do that

kradalby · 2022-03-16T07:45:32Z

machine.go

+	name := machine.Name
+	if len(machine.Nickname) > 0 {
+		name = machine.Nickname
+	}


I think we need to think about this a bit differently, if we implement this feature, we will have two name fields:

Name

Nickname

Name should be the exact hostname that is passed by the Tailscale client, and should not be used in the DNS config.

Nickname should be set as the Normalized name of Name on registration (e.g. it will always have a value) and then it can be changed by the user later.

Nickname should therefore always be used for the DNS Config.

Then, we have to store the state of "Auto-generate from OS hostname" checkbox in database, right? Otherwise, each client update will overwrite this field.

Yes, so the flow should be:

Machine sends a register request with hostname: TailScaleNode-123

We set the HostName (currently Name) field to TailScaleNode-123

We Normalize TailScaleNode-123 and set the result to GivenName (currently Nickname)

Then if a user wants to rename the host, we let them do that and use the validation part of the Normalization infra.

It does look like the GivenName should be unique?

Yes for the DNS to work properly, normalization function should check if GivenName is already registered in DB and add something to it. This step should behave proerly automatically without the use of an admin or the user to change anything. If the user is not satisfied with the name he could change it manually on the tailscale node.

kradalby · 2022-05-01T21:47:08Z

Closing this in favour of #560

bravechamp added 2 commits March 13, 2022 21:03

Nickname support

663e838

Ability to clear nickname

9901d6b

bravechamp requested review from juanfont and kradalby as code owners March 13, 2022 21:17

Normalize nickname before saving to database

60ee046

restanrm reviewed Mar 13, 2022

View reviewed changes

kradalby reviewed Mar 16, 2022

View reviewed changes

Merge branch 'main' into main

c8aa653

kradalby added this to the v0.16.0 milestone Mar 20, 2022

Merge branch 'main' into main

8845938

kradalby mentioned this pull request Apr 24, 2022

Rework hostname handling #560

Merged

5 tasks

kradalby closed this May 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Machine rename support #498

Machine rename support #498

bravechamp commented Mar 13, 2022 •

edited by juanfont

Loading

restanrm left a comment

bravechamp commented Mar 13, 2022

restanrm commented Mar 14, 2022

bravechamp commented Mar 14, 2022

restanrm commented Mar 14, 2022

kradalby Mar 16, 2022

bravechamp Mar 16, 2022

kradalby Mar 16, 2022

kradalby Mar 16, 2022

bravechamp Mar 16, 2022

kradalby Mar 16, 2022

bravechamp Mar 16, 2022

kradalby Mar 16, 2022

bravechamp Mar 16, 2022

kradalby Mar 16, 2022

kradalby Mar 16, 2022

bravechamp Mar 16, 2022

kradalby Mar 16, 2022

bravechamp Mar 16, 2022

kradalby Mar 16, 2022

kradalby Mar 16, 2022

restanrm Mar 17, 2022

kradalby commented May 1, 2022

Machine rename support #498

Machine rename support #498

Conversation

bravechamp commented Mar 13, 2022 • edited by juanfont Loading

restanrm left a comment

Choose a reason for hiding this comment

bravechamp commented Mar 13, 2022

restanrm commented Mar 14, 2022

bravechamp commented Mar 14, 2022

restanrm commented Mar 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kradalby commented May 1, 2022

bravechamp commented Mar 13, 2022 •

edited by juanfont

Loading