-
Notifications
You must be signed in to change notification settings - Fork 519
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
Improve reliability of settings.network.hostname
generator
#2647
Conversation
sources/api/sundog/src/main.rs
Outdated
if !result.stderr.is_empty() { | ||
let cmd_stderr = String::from_utf8_lossy(&result.stderr); | ||
for line in cmd_stderr.lines() { | ||
info!("{}: {}", command, line); |
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.
Nit: perhaps the log could be a little bit more verbose so we know exactly what we're looking at in the logs. It could get a little confusing in the context of the whole journal.
info!("Setting generator command '{}' stderr: {}", command, line);
490ba85
to
56edc9b
Compare
^ Addresses feedback from @zmrow |
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.
😶🌫️
.ok(); | ||
|
||
// On AWS variants, we fallback to inspecting IMDS. | ||
#[cfg(variant_platform = "aws")] |
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 gives me pause. Elsewhere we have gone out of our way to segregate conditionally-compiled code from unconditionally-compiled code: https://github.com/bottlerocket-os/bottlerocket/blob/d20ea8efe911214ea635da81c2e8fed9c9703c77/sources/api/early-boot-config/src/provider.rs#L8..L21
I'm not sure that makes sense here, but I'm thinking about a case where we would want to query differently for a different provider (vmware? or a different cloud, for example), and ending up with a string of sections of code with compiler guards.
Perhaps a compromise would be something like this:
(maybe it's too much trouble, but it's an idea)
let hostname: Option<String> = Retry::spawn(retry_strategy(), || async { lookup_addr(&ip) })
.await
.or_else(|e| {
eprintln!("Reverse DNS lookup failed: {}", e);
ip_string
Err(e)
})
.ok()
.or_else(|| query_provider_hostname().await)
elsewhere:
async fn query_provider_hostname() -> Option<String> {
#[cfg(variant_platform = "aws")]
query_imds_hostname().await
#[cfg(not(variant_platform = "aws"))]
None
}
#[cfg(variant_platform = "aws")]
/// On AWS variants, we fallback to inspecting IMDS.
async fn query_imds_hostname() -> Option<String> {
todo!()
}
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 had similar worries but didn't give it enough thought to land at this strategy. I like it, I'll do this.
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.
🎉
56edc9b
to
4e8f092
Compare
4e8f092
to
47156dd
Compare
|
47156dd
to
1bebdd8
Compare
^ One more change to add a missing docstring. |
Failure scenarios when resolving a host's hostname are common enough to cause issues. This adds retries to our DNS queries for the current hostname. On AWS variants, we also attempt to query IMDS as a fallback mechanism.
1bebdd8
to
7fa9cca
Compare
I forgot to run clippy 🤦 |
Issue number:
Closes #2585
Closes #2600
Description of Changes:
Alongside the title, this also introduces improved settings-generator logging from sundog.
Just as a note, adding
tokio
andimdsclient
moves netdog's binary size from1.4M
->1.6M
.Testing done:
I blocked DNS resolution in my Bottlerocket VPC at the network level, then ran
netdog generate-hostname
on theaws-ecs-1
variant, noting that hostname generation succeeded despite DNS resolution failing.Note that the first line is output to stderr, and the second to stdout.
I also tested sundog logging by attempting to boot with partial DNS blocking and checking the systemd journal:
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.