-
Notifications
You must be signed in to change notification settings - Fork 511
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
Conversation
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. |
0782870
to
4b7a0e3
Compare
^ Addresses @bcressey 's concern and adds a new template renderer helper that will only add the entry to |
4b7a0e3
to
e1c7834
Compare
^ 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> { |
There was a problem hiding this comment.
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!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - thanks rustfmt
e1c7834
to
7a69c22
Compare
^ rebased on |
There was a problem hiding this 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?
fn resolves_to_localhost_renders_entries() { | ||
let result = setup_and_render_template( | ||
"{{add_unresolvable_hostname name}}", | ||
&json!({"name": "localhost"}), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"}), |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
7a69c22
to
6e14a5d
Compare
^ Added the comments @webern asked for |
6e14a5d
to
bcd0591
Compare
^ Fixed my misspelling. :) |
bcd0591
to
e7194b4
Compare
^ Addressed @bcressey 's comments |
sources/api/schnauzer/src/helpers.rs
Outdated
#[snafu(display("Invalid IP address '{}': {}", ip, source))] | ||
IpFromString { | ||
ip: String, | ||
source: std::net::AddrParseError, | ||
}, | ||
|
There was a problem hiding this comment.
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`.
e7194b4
to
1c25ade
Compare
^ removed the unused error variant |
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`.
Issue number:
Fixes #1700
Description of changes:
Testing done:
Boot an
aws-k8s-1.19
host and observe proper behavior, run a pod, etc./etc/hosts
looks like: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:I followed the same procedure with a VMware variant.
/etc/hosts
before the upgrade:/etc/hosts
after the upgrade (hosts-test
is the hostname of my VM):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.