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

Write short form hostname #922

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

Conversation

gabriel-samfira
Copy link

@gabriel-samfira gabriel-samfira commented May 11, 2023

While setting the hostname as a FQDN is perfectly valid, in most cases the expectation is that /etc/hostname holds the short form hostname and the FQDN is added as a canonical hostname, i.e. as the first column in /etc/hosts after the local IP address.

This will allow clients to use:

:~$ hostname -f
arrakis.local

to get the fqdn and still be able to get the short form hostname by calling:

:~$ hostname
arrakis

While setting the hostname as a FQDN is perfectly valid, in most cases
the expectation is that /etc/hostname holds the short form hostname and
the FQDN is added as a canonical hostname, i.e. as the first column in
/etc/hosts after the local IP address.

This will allow clients to use:

  hostname -f

to get the fqdn and still be able to get the host form hostname by
calling:

  hostname

Signed-off-by: Gabriel Adrian Samfira <[email protected]>
@bgilbert
Copy link
Contributor

We arrived at Afterburn's current hostname policy after much debate and debugging, and changing it would likely break existing users. If you'd like to add a command-line option to write only the hostname part, we could consider it.

@gabriel-samfira
Copy link
Author

Fair enough! Would a similar command line option like --short-hostname=/etc/hostname be ok? (I am terrible at naming things).

@bgilbert
Copy link
Contributor

Works for me. Note that there isn't a great way to configure the afterburn-hostname.service service at runtime, since it runs in the initrd. (For example, Ignition can't reconfigure the initrd.) So, if you wanted to use this, you'd have to configure a custom unit that runs in the booted system, which probably means you'd be setting the hostname after things have already started using the old one.

@gabriel-samfira
Copy link
Author

I was a bit sparse on context 😄 .

The need for this came up recently when we noticed that we can't bring up a k8s cluster on OpenStack using Flatcar as a base OS with kops. Here is the relevant issue with more context: kubernetes/kops#15385

For OpenStack we have a fix merged in coreos-cloudinit, but the goal is to eventually use afterburn to set the hostname. The short hostname option will most likely be (eventually) included in future releases of Flatcar.

@bgilbert
Copy link
Contributor

Ah, okay, cool. Happy to ship a command-line option to be invoked by a distro that integrates Afterburn.

The --short command line option is a bool which requires the --hostname
option and which will toggles writing the short form hostname to
/etc/hostname.

Signed-off-by: Gabriel Adrian Samfira <[email protected]>
@gabriel-samfira
Copy link
Author

I added a new boolean flag --short and placed the --hostname in a group. The --short flag requires the --hostname flag and toggles whether or not to write the short form hostname. I also added a new test.

This is my first interaction with rust, so let me know if the changes are not idiomatic or just plain silly 😄.

@gabriel-samfira
Copy link
Author

Here is an example of the new flag in action.

Without the --short flag:

control-plane-nova-dkfvy2 ~ # ./afterburn --provider openstack-metadata --hostname=/root/test
May 16 14:51:15.352 INFO Fetching http://169.254.169.254/latest/meta-data/hostname: Attempt #1
May 16 14:51:15.542 INFO Fetch successful
May 16 14:51:15.542 INFO wrote hostname control-plane-nova-dkfvy2.novalocal to /root/test
control-plane-nova-dkfvy2 ~ # cat /root/test 
control-plane-nova-dkfvy2.novalocal

With the --short flag:

control-plane-nova-dkfvy2 ~ # ./afterburn --provider openstack-metadata --short --hostname=/root/test
May 16 14:51:24.876 INFO Fetching http://169.254.169.254/latest/meta-data/hostname: Attempt #1
May 16 14:51:24.908 INFO Fetch successful
May 16 14:51:24.908 INFO wrote hostname control-plane-nova-dkfvy2 to /root/test
control-plane-nova-dkfvy2 ~ # cat /root/test 
control-plane-nova-dkfvy2

For systems that don't explicitly set the new flag, things work as they did before. Opting to use the new flag will set the short form hostname.

Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

Your first commit is a development checkpoint rather than an incremental logical change, so please squash it. CI also expects a release note to be added in docs/release-notes.md.

hostname_file: Option<String>,
/// Parse the hostname retrieved from metadata as a short hostname
#[arg(long = "short", requires = "hostname")]
short_hostname: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I prefer your original idea of having --short-hostname=/etc/hostname. Rather than adding a mode switch that controls the behavior of another option, let's just support --hostname and --short-hostname as two output switches that operate independently.

assert_eq!(
try_write_hostname("hostname7.example.com", true),
"hostname7"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's at least also test short hostnames where the input value doesn't have an FQDN.

@bgilbert
Copy link
Contributor

bgilbert commented Aug 1, 2023

Thanks for the PR! I won't be able to do future review rounds on this, unfortunately, but hopefully someone else on the team can take a look.

Closing/reopening to kick CI.

@bgilbert bgilbert closed this Aug 1, 2023
@bgilbert bgilbert reopened this Aug 1, 2023
@gabriel-samfira
Copy link
Author

Thanks for the review @bgilbert !

I will make the changes today and push them.

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

Successfully merging this pull request may close these issues.

2 participants