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 an entry to /etc/hosts for the current hostname #1713

Merged
merged 3 commits into from
Sep 9, 2021

Conversation

zmrow
Copy link
Contributor

@zmrow zmrow commented Aug 18, 2021

Issue number:
Fixes #1700

Description of changes:

    schnauzer: Add a new template helper: `add_unresolvable_hostname`
    
    This change adds a new template helper `add_unresolvable_hostname` that
    will attempt to resolve the hostname given to it in the template (from
    `settings.network.hostname`).  If the hostname is unresolvable or
    resolves to localhost (127.0.0.1 or ::1), the helper adds an alias to
    the hostname for both the IPv4 and IPv6 addresses
    Add a hostname entry to `/etc/hosts` if hostname is unresolvable
    
    Previously, `/etc/hosts` was a static file (in the `release` package)
    that we copied into the image when building Bottlerocket.  In order to
    have an entry in the file corresponding to hostname, a new "hosts"
    service was added to the affected services for
    `settings.network.hostname`.  This new "hosts" service has a templated
    configuration file that uses `settings.network.hostname` to populate the
    entry in `/etc/hosts` if the current hostname is unresolvable in DNS.
    The templated configuration file uses the previously added
    `add_unresolvable_hostname` template renderer helper.
    migrations: Add migrations for `/etc/hosts` "service"
    
    This change includes the 2 migrations necessary to make `/etc/hosts` an
    "affected service" of `settings.network.hostname`: one to add the
    service and configuration file for the hosts service, and another to
    add the hosts service to `settings.network.hostname`.

Testing done:
Boot an aws-k8s-1.19 host and observe proper behavior, run a pod, etc. /etc/hosts looks like:

bash-5.0# cat /etc/hosts 
127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4
::1 localhost localhost.localdomain localhost6 localhost6.localdomain6

Upgrade the same host to a version including these changes, observe proper boot and behavior. /etc/hosts looks the same because the hostname resolves in EC2:

bash-5.0# cat /etc/hosts 
127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4l
::1 localhost localhost.localdomain localhost6 localhost6.localdomain6

I followed the same procedure with a VMware variant. /etc/hosts before the upgrade:

bash-5.0# cat /etc/hosts 
127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4
::1 localhost localhost.localdomain localhost6 localhost6.localdomain6

/etc/hosts after the upgrade (hosts-test is the hostname of my VM):

bash-5.0# cat /etc/hosts 
127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4
::1 localhost localhost.localdomain localhost6 localhost6.localdomain6
127.0.0.1 hosts-test
::1 hosts-test

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@bcressey
Copy link
Contributor

I'd prefer that we scope this more narrowly so that we only add the hosts entry if our configured hostname isn't resolvable through DNS.

That's really the only case where we know it's needed and helpful.

@zmrow
Copy link
Contributor Author

zmrow commented Aug 23, 2021

^ Addresses @bcressey 's concern and adds a new template renderer helper that will only add the entry to /etc/hosts if the hostname isn't resolvable or resolves to localhost.

@zmrow
Copy link
Contributor Author

zmrow commented Aug 23, 2021

^ Splits the template helper into its own commit

@@ -935,8 +1022,9 @@ fn pause_registry<S: AsRef<str>>(region: S) -> String {
}

/// Calculates and returns the amount of CPU to reserve
fn kube_cpu_helper(num_cores: usize) -> Result<String, TemplateHelperError>{
let num_cores = u16::try_from(num_cores).context(error::ConvertUsizeToU16 { number: num_cores })?;
fn kube_cpu_helper(num_cores: usize) -> Result<String, TemplateHelperError> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a rustfmt change

@@ -949,7 +1037,11 @@ fn kube_cpu_helper(num_cores: usize) -> Result<String, TemplateHelperError>{
KUBE_RESERVE_4_CORES + ((num_cores - 4.0) * KUBE_RESERVE_ADDITIONAL)
}
};
Ok(format!("{}{}", cpu_to_reserve.floor().to_string(), millicores_unit))
Ok(format!(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here - thanks rustfmt

@zmrow
Copy link
Contributor Author

zmrow commented Aug 23, 2021

^ rebased on develop

Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

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

Don't we need to add something to README for the new settings. Possibly a warning in k8s docs about kubelet not liking a hostname change?

sources/Cargo.toml Show resolved Hide resolved
fn resolves_to_localhost_renders_entries() {
let result = setup_and_render_template(
"{{add_unresolvable_hostname name}}",
&json!({"name": "localhost"}),
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this test should always work on every system?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it should? I'm willing to be told otherwise though.

fn resolvable_hostname_renders_nothing() {
let result = setup_and_render_template(
"{{add_unresolvable_hostname name}}",
&json!({"name": "amazon.com"}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this unresolvable by DNS? Will it always be unresolvable? Would something completely invalid be better? not*a*valid*hostname?

Copy link
Contributor Author

@zmrow zmrow Aug 25, 2021

Choose a reason for hiding this comment

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

The helper only renders if the name is unresolvable or resolves to 127.0.0.1 or ::1. I realize that amazon.com isn't a realistic hostname, but it should resolve mostly anywhere and that's the behavior I was after.

sources/api/schnauzer/src/lib.rs Show resolved Hide resolved
@zmrow
Copy link
Contributor Author

zmrow commented Aug 25, 2021

^ Added the comments @webern asked for

sources/api/schnauzer/src/lib.rs Outdated Show resolved Hide resolved
@zmrow
Copy link
Contributor Author

zmrow commented Aug 30, 2021

^ Fixed my misspelling. :)

packages/release/release.spec Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Show resolved Hide resolved
@zmrow
Copy link
Contributor Author

zmrow commented Sep 1, 2021

^ Addressed @bcressey 's comments

Comment on lines 157 to 162
#[snafu(display("Invalid IP address '{}': {}", ip, source))]
IpFromString {
ip: String,
source: std::net::AddrParseError,
},

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this error is used any more.

This change adds a new template helper `add_unresolvable_hostname` that
will attempt to resolve the hostname given to it in the template (from
`settings.network.hostname`).  If the hostname is unresolvable or
resolves to localhost (127.0.0.1 or ::1), the helper adds an alias to
the hostname for both the IPv4 and IPv6 addresses.
Previously, `/etc/hosts` was a static file (in the `release` package)
that we copied into the image when building Bottlerocket.  In order to
have an entry in the file corresponding to hostname, a new "hosts"
service was added to the affected services for
`settings.network.hostname`.  This new "hosts" service has a templated
configuration file that uses `settings.network.hostname` to populate the
entry in `/etc/hosts` if the current hostname is unresolvable in DNS.
The templated configuration file uses the previously added
`add_unresolvable_hostname` template renderer helper.
This change includes the 2 migrations necessary to make `/etc/hosts` an
"affected service" of `settings.network.hostname`: one to add the
service and configuration file for the hosts "service", and another to
add the hosts service to `settings.network.hostname`.
@zmrow
Copy link
Contributor Author

zmrow commented Sep 2, 2021

^ removed the unused error variant

@zmrow zmrow merged commit 084fc3b into bottlerocket-os:develop Sep 9, 2021
@zmrow zmrow deleted the etc-hosts-template branch September 9, 2021 15:45
zmrow added a commit to zmrow/bottlerocket that referenced this pull request Sep 14, 2021
The /etc/hosts file was removed and replaced with a template in a prior
change (bottlerocket-os#1713).  This removes the entry from `release-tmpfiles.conf`.
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.

Populate /etc/hosts with an entry for settings.network.hostname
4 participants