Add support for static DNS mappings #2129

cbgbt · 2022-05-04T22:37:31Z

Issue number: #2045

Description of changes:

Implement models and settings changes
Write migrations for new settings

This allows users to populate /etc/hosts with static DNS entries via
settings and the Bottlerocket API. Bottlerocket doesn't currently
perform resolver caching, so there is no need to restart networked
services after rendering a new /etc/hosts file.

According to man hosts:

Before the advent of DNS, the host table was the only way of resolving hostnames on the fledgling Internet.

Today, this file is still used to configure a static lookup table for hostnames on Linux. The hosts file is a text file that associates IP addresses (v4 or v6) with hostnames, one line per IP address.

IP_address canonical_hostname [aliases...]

e.g.

10.0.0.1 myhost.example.com myhost

Would ensure that myhost.example.com and myhost both resolve to 10.0.0.1. The man page explicitly calls out the first element as being the canonical_hostname, but there are no technical requirements that differentiate this to aliases — each element must:

Contain only alphanumeric characters, minus signs (“-”) and periods (“.”)
Begin with an alphabetic character
End with an alphanumeric character

The modeled settings enforce these requirements.

Per #921, we don't support DNS caching on Bottlerocket, so we shouldn't need to restart any additional services when we modify static DNS mappings.

Testing done:

Test on aws-ecs-1
Test on aws-k8s-1.22
Test migrations

Example of using the feature:

[ec2-user@admin]$ sudo sheltie
bash-5.1# cat /etc/hosts
127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4 
::1 localhost localhost.localdomain localhost6 localhost6.localdomain6 


bash-5.1# apiclient set -j '{"settings": {"network": {"hosts": [["10.0.0.1", ["test.example.com"]], ["127.0.0.1", ["test.bottlerocket.aws"]], ["10.0.0.1", ["test2.example.com"]]]}}}'
bash-5.1# apiclient get settings.network.hosts
{
  "settings": {
    "network": {
      "hosts": [
        [
          "10.0.0.1",
          [
            "test.example.com"
          ]
        ],
        [
          "127.0.0.1",
          [
            "test.bottlerocket.aws"
          ]
        ],
        [
          "10.0.0.1",
          [
            "test2.example.com"
          ]
        ]
      ]
    }
  }
}
bash-5.1# cat /etc/hosts
127.0.0.1 localhost localhost.localdomain localhost4 localhost4.localdomain4 test.bottlerocket.aws
::1 localhost localhost.localdomain localhost6 localhost6.localdomain6 

10.0.0.1 test.example.com test2.example.com

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.

zmrow · 2022-05-05T20:48:08Z

LGTM so far! Once the migrations are out I will give it another look-see

cbgbt · 2022-05-05T23:54:31Z

Added README documentation
Use BTreeMap to store host entries to preserve ordering

cbgbt · 2022-05-06T21:02:57Z

I'm going to make a few changes to the /etc/hosts template, documentation, and settings name based on some verbal feedback from the team. Should have revision up shortly 😄

cbgbt · 2022-05-17T15:30:07Z

A few changes:

Setting name changed to settings.network.hosts.
Modified /etc/hosts such that it can't contain duplicate line entries for host addresses

cbgbt · 2022-05-17T18:15:28Z

I keep ending up having strange conflicts with develop. Will fix the git tree.

cbgbt · 2022-05-17T18:17:10Z

Fixed, as well as fixed some code clarity issues.

cbgbt · 2022-05-18T20:48:04Z

sources/models/src/lib.rs

+    // Ordering matters in /etc/hosts if multiple domains point to the same IP, so we use BTreeMap to preserve order.
+    hosts: BTreeMap<IpAddr, Vec<ValidLinuxHostname>>,
    https_proxy: Url,


I've been doing some extra reading about BTreeMap and my assumption that it preserves iteration order based on insertion is false: BTreeMap preserves iteration order based on key order. The common solution in the Rust community is to use IndexMap for a map which preserves insertion order.

This also means that I would need to assert that every serializer and deserializer which touches settings objects respects ordering. Fortunately for serde_json and toml this can be enabled by adding a feature flag, but we have a few points e.g. in migrations where helpers assume that a Map can be safely represented via a HashMap.

I'm looking into solving this either by building in order preservation to settings (which could be extremely difficult to continuously enforce in the Bottlerocket repo, even with great testing in the core settings path) or by changing our model here to use an ordered data structure, although this adds some uncomfortable ergonomic questions over how to use the feature.

cbgbt · 2022-05-19T18:27:57Z

I reworked the static DNS feature to use a Vec<(IpAddr, Vec<ValidLinuxHostname>)> as its internal representation, then added a helper method to merge IP address lines together (though this doesn't dedupe hostnames).

While it does make it a bit less ergonomic to specify mappings due to the use of lists to represent pairs, it safely models the behavior of /etc/hosts.

cbgbt · 2022-05-19T21:05:47Z

Force-pushed to rebase atop develop.

bcressey · 2022-05-23T18:16:41Z

sources/api/schnauzer/src/helpers.rs

+    // If our hostname isn't resolveable, add it to the alias list.
+    if hostname.len() > 0 && !hostname_resolveable(hostname) {
+        results.push(hostname.to_owned());
+    }


I think there's an edge case here where the hostname isn't initially resolvable, but should resolve to a non-localhost IP provided in settings.network.hosts, after /etc/hosts is populated.

In that case, we'd incorrectly write /etc/hosts with the hostname as an alias for localhost.

Nice catch! I'll fix that and see if I can't add a unit test to cover that edge case.

bcressey · 2022-05-23T18:20:37Z

sources/api/schnauzer/src/helpers.rs

+                // Cast aside anything that doesn't parse as an IP Address, or any references to localhost.
+                *ip_address != IPV4_LOCALHOST && *ip_address != IPV6_LOCALHOST


Where are we casting aside anything that doesn't parse as an IP address?

Whoops, holdover comment. This used to be much more "stringly typed" but I ended up routing all of the parsing through the models -- much cleaner now.

cbgbt · 2022-05-24T22:55:43Z

Force-pushed to address comments by @bcressey.

Added a unit test for the edge case. Additional testing of that change: https://gist.github.com/cbgbt/d0e7789fe3a9a12e2b59e59b6f2f0278

bcressey · 2022-05-25T15:16:34Z

LGTM once cargo fmt is happy.

cbgbt · 2022-05-25T18:13:52Z

Force-push to fix cargo-fmt issue.

arnaldo2792 · 2022-05-26T16:11:06Z

sources/api/schnauzer/src/helpers.rs

    let template_name = template_name(renderctx);
    trace!("Template name: {}", &template_name);

-    // Check number of parameters, must be exactly one
+    // Check number of parameters, must be exactly two (IP version, hostname, hosts overrides)


nit: should the number be three?

arnaldo2792 · 2022-05-26T16:18:02Z

sources/api/schnauzer/src/helpers.rs

+    // Check number of parameters, must be exactly two (IP version, hostname, hosts overrides)
+    trace!("Number of params: {}", helper.params().len());
+    check_param_count(helper, template_name, 1)?;


nit: another inconsistent count?

arnaldo2792 · 2022-05-26T16:19:45Z

sources/api/schnauzer/src/helpers.rs

@@ -1128,6 +1267,41 @@ fn kube_cpu_helper(num_cores: usize) -> Result<String, TemplateHelperError> {
    ))
 }

+/// Returns whether or not a hostname resolves to a non-loopback IP address.
+///
+/// If `configured_hosts` is Some, the hostname will be considered resolvable if it is listed as an alias for any given IP address.


nit:

Suggested change

/// If `configured_hosts` is Some, the hostname will be considered resolvable if it is listed as an alias for any given IP address.

/// If `configured_hosts` is set, the hostname will be considered resolvable if it is listed as an alias for any given IP address.

arnaldo2792 · 2022-05-26T16:24:09Z

sources/api/schnauzer/src/helpers.rs

+    #[test]
+    fn hostname_resolves_to_static_mapping() {
+        // Given a configured hostname that does not resolve in DNS
+        // and an /etc/hosts configuration which points that contains that hostname as an alias to an IP address,


Nit: I think there are extra words here? Should the line be:

Suggested change

// and an /etc/hosts configuration which points that contains that hostname as an alias to an IP address,

// and an /etc/hosts configuration that contains that hostname as an alias to an IP address,

arnaldo2792 · 2022-05-26T16:29:10Z

sources/models/src/modeled_types/shared.rs

+
+    #[test]
+    fn test_valid_etc_hosts_entries() {
+        assert!(serde_json::from_str::<EtcHostsEntries>(


why can't all these tests be part of the same assert? Same question similar tests below.

I've tried to logically separate different checks into multiple assert lines here, so if one fails it's easier to reason about what behavior the check was testing for.

fgutmann · 2022-05-26T17:47:23Z

README.md

+   Repeated entries are merged (including loopback entries), with the first aliases listed taking precedence. e.g.:
+
+   ```toml
+   [settings.network]
+   hosts = [
+    ["10.0.0.0", ["test.example.com", "test1.example.com"]],
+    ["10.1.1.1", ["test2.example.com"]],
+    ["10.1.1.0", ["test3.example.com"]],
+   ]
+   ```
+   Would result in `/etc/hosts` entries like so:
+   ```
+   10.0.0.0 test.example.com test1.example.com test3.example.com
+   10.1.1.1 test2.example.com
+   ```


Hi I'm trying to validate if the current logic will work for our use-case.

Why in the example test3.example.com (10.1.1.0) is merged with the 10.0.0.0 line and thus effectively rewritten to 10.0.0.0?

Is it possible with the current logic to have one hostname to resolve to multiple IPs? The input syntax seems to support it. Just want to make sure no de-duplication will happen in this case. We will require a final /etc/hosts file that looks something like this:

10.0.2.252 test1.example.com 10.0.2.236 test1.example.com 10.0.3.205 test1.example.com

Hi, thanks for taking a look at this PR.

This is a documentation mistake, I meant for 10.1.1.0 to be a repeat of 10.0.0.0. I'll fix that.

Yes, you can have one hostname resolve to multiple IPs. De-duplication will only occur for IPs, not hostnames. If you submit something like the following, you should end up with /etc/hosts entries like the ones you've given:

hosts = [ ["10.0.2.252", ["test1.example.com"]], ["10.0.2.236", ["test1.example.com"]], ["10.0.3.205", ["test1.example.com]] ]

cbgbt · 2022-05-26T18:24:56Z

Force push to address comments from @arnaldo2792

cbgbt · 2022-05-26T18:36:26Z

Additional push to address README issue pointed out by @fgutmann. Thanks!

webern

I'm cursed with the ability to always see a way to unit test. Otherwise this looks great.

webern · 2022-05-18T17:13:25Z

README.md

@@ -502,11 +502,27 @@ In addition to the container runtime daemons, these credential settings will als

 * `settings.network.hostname`: The desired hostname of the system.
  **Important note for all Kubernetes variants:** Changing this setting at runtime (not via user data) can cause issues with kubelet registration, as hostname is closely tied to the identity of the system for both registration and certificates/authorization purposes.
+* `settings.network.hosts`: A mapping of IP addresses to domain names which should resolve to those IP addresses.
+   This setting results in modifications to the `/etc/hosts` file  for Bottlerocket.
+   Note that this setting does not typically impact name resolution for containers, which usually rely on [orchestrator-specific](https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_HostEntry.html) [mechanisms](https://kubernetes.io/docs/tasks/network/customize-hosts-file-for-pods/) for configuring static resolution.


Just a suggestion...

Suggested change

Note that this setting does not typically impact name resolution for containers, which usually rely on [orchestrator-specific](https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_HostEntry.html) [mechanisms](https://kubernetes.io/docs/tasks/network/customize-hosts-file-for-pods/) for configuring static resolution.

Note that this setting does not typically impact name resolution for containers, which usually rely on orchestrator-specific mechanisms for configuring static resolution.

(See [ECS](https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_HostEntry.html) and [Kubernetes](https://kubernetes.io/docs/tasks/network/customize-hosts-file-for-pods/) documentation for those mechanisms.)

webern · 2022-05-18T17:14:14Z

README.md

+   Entries which point to the loopback address are merged as aliases with the lowest precedence on the existing loopback line.
+
+

 Most users don't need to change this setting as the following defaults work for the majority of use cases.
 If this setting isn't set we attempt to use DNS reverse lookup for the hostname.
 If the lookup is unsuccessful, the IP of the node is used.
-
 ##### Proxy settings


nit: newline crimes were committed

Entries which point to the loopback address are merged as aliases with the lowest precedence on the existing loopback line. Most users don't need to change this setting as the following defaults work for the majority of use cases. If this setting isn't set we attempt to use DNS reverse lookup for the hostname. If the lookup is unsuccessful, the IP of the node is used. ##### Proxy settings

webern · 2022-05-18T17:18:45Z

sources/api/schnauzer/src/helpers.rs

    let template_name = template_name(renderctx);
    trace!("Template name: {}", &template_name);

-    // Check number of parameters, must be exactly one
+    // Check number of parameters, must be exactly two (IP version, hostname, hosts overrides)


Suggested change

// Check number of parameters, must be exactly two (IP version, hostname, hosts overrides)

// Check number of parameters, must be exactly three (IP version, hostname, hosts overrides)

webern · 2022-05-18T17:21:33Z

sources/api/schnauzer/src/helpers.rs

+        .param(1)
+        .map(|v| v.value())
+        .context(error::InternalSnafu {
+            msg: "Missing params after confirming that they exist.",


Maybe since we use this same message all over we could add ParamUnwrapSnafu variant with this message baked in and eliminate the strings from the code.

msg: "Missing params after confirming that they exist.",

webern · 2022-05-18T17:47:35Z

sources/api/schnauzer/src/helpers.rs

+                if let Ok(ip_address) = ip_address.parse::<IpAddr>() {
+                    ip_address == localhost_comparator
+                } else {
+                    false
+                }


Golfable if desired

Suggested change

if let Ok(ip_address) = ip_address.parse::<IpAddr>() {

ip_address == localhost_comparator

} else {

false

}

ip_address.parse::<IpAddr>().map(|ip_address| ip_address == localhost_comparator).unwrap_or_default()

webern · 2022-05-27T19:13:15Z

sources/api/schnauzer/src/helpers.rs

+    if hosts_value.is_null() {
+        // If hosts aren't set, just exit.
+        Ok(())
+    } else {


It's a stylistic preference, but I would return here and unindent the "else" code.

Suggested change

if hosts_value.is_null() {

// If hosts aren't set, just exit.

Ok(())

} else {

if hosts_value.is_null() {

// If hosts aren't set, just exit.

return Ok(())

}

webern · 2022-05-27T19:21:16Z

sources/api/schnauzer/src/helpers.rs

+    trace!("Hosts from template: {:?}", hosts);
+
+    // If our hostname isn't resolveable, add it to the alias list.
+    if hostname.len() > 0 && !hostname_resolveable(hostname, hosts.as_ref()) {


I was trying to think of a way to remove this impurity from this otherwise testable (and complex) function. Alas, I don't see a way to inject this network dependency.

Edit, well... as usual I did think of a way:

localhost_aliases could be a thin wrapper for localhost_aliases_impl.

localhost_aliases_impl takes a functor matching the signature of hostname_resolvable, then localhost_aliases becomes:

pub fn localhost_aliases( helper: &Helper<'_, '_>, h: &Handlebars, c: &Context, renderctx: &mut RenderContext<'_, '_>, out: &mut dyn Output, ) -> Result<(), RenderError> { localhost_aliases_impl(helper, h, c, renderctx, out, &hostname_resolvabe) }

Then I think you could write tests for localhost_aliases_impl that takes a mock of the hostname_resolvable function.

I love function purity. Thanks for the tip, I'll implement this.

I toyed around with implementing something like this, but it ended up making it more challenging to understand and didn't necessarily improve testability, since there are a few tests here already which just use more apparently resolve-able or un-resolve-able hostnames.

webern · 2022-05-27T19:28:54Z

sources/models/src/modeled_types/shared.rs

+                .append(&mut (aliases.clone()));
+        }
+
+        merged.into_iter()


🤯

Nice rust.

cbgbt · 2022-06-02T13:21:32Z

Force push to make some stylistic and comment changes suggested by @webern

etungsten · 2022-06-01T22:44:43Z

README.md

+  If this setting isn't set we attempt to use DNS reverse lookup for the hostname.
+  If the lookup is unsuccessful, the IP of the node is used.
+
+* `settings.network.hosts`: A mapping of IP addresses to domain names which should resolve to those IP addresses.


Should we explain what the default is when this setting is not set? Or maybe it's obvious enough, and it would be too verbose.

I think it might be obvious enough to omit. I noticed that we don't always include the default value, for settings as well.

This allows users to populate /etc/hosts with static DNS entries via settings and the Bottlerocket API. Bottlerocket doesn't currently perform resolver caching, so there is no need to restart networked services after rendering a new /etc/hosts file.

cbgbt · 2022-06-03T00:21:08Z

Force push to rebase.

cbgbt requested review from zmrow, arnaldo2792 and etungsten May 4, 2022 22:37

cbgbt force-pushed the staticdns branch 3 times, most recently from 351f82a to 920e6b2 Compare May 5, 2022 23:53

cbgbt marked this pull request as ready for review May 5, 2022 23:54

cbgbt marked this pull request as draft May 6, 2022 21:03

cbgbt force-pushed the staticdns branch from 920e6b2 to 0c33b8e Compare May 17, 2022 15:28

cbgbt force-pushed the staticdns branch 4 times, most recently from c504f46 to 1b9f780 Compare May 17, 2022 18:13

cbgbt marked this pull request as ready for review May 17, 2022 18:14

cbgbt force-pushed the staticdns branch from 1b9f780 to 41ccf44 Compare May 17, 2022 18:16

cbgbt commented May 18, 2022

View reviewed changes

cbgbt force-pushed the staticdns branch 2 times, most recently from aa83b3b to ad263da Compare May 19, 2022 18:25

cbgbt force-pushed the staticdns branch from ad263da to 862eba2 Compare May 19, 2022 21:05

cbgbt requested review from webern and bcressey May 19, 2022 22:29

cbgbt force-pushed the staticdns branch from 862eba2 to a939cdb Compare May 19, 2022 22:35

bcressey reviewed May 23, 2022

View reviewed changes

cbgbt force-pushed the staticdns branch from a939cdb to cdd797e Compare May 24, 2022 22:53

cbgbt force-pushed the staticdns branch from cdd797e to 91049a9 Compare May 25, 2022 18:13

arnaldo2792 reviewed May 26, 2022

View reviewed changes

fgutmann reviewed May 26, 2022

View reviewed changes

cbgbt force-pushed the staticdns branch from 91049a9 to 7c921d7 Compare May 26, 2022 18:24

cbgbt force-pushed the staticdns branch from 7c921d7 to 6d4c706 Compare May 26, 2022 18:35

arnaldo2792 approved these changes May 27, 2022

View reviewed changes

webern approved these changes May 27, 2022

View reviewed changes

cbgbt force-pushed the staticdns branch 2 times, most recently from fd7577f to d6ab9ae Compare June 2, 2022 13:01

webern approved these changes Jun 3, 2022

View reviewed changes

etungsten approved these changes Jun 3, 2022

View reviewed changes

cbgbt added 3 commits June 3, 2022 00:20

Add support for /etc/hosts mappings

6664a38

This allows users to populate /etc/hosts with static DNS entries via settings and the Bottlerocket API. Bottlerocket doesn't currently perform resolver caching, so there is no need to restart networked services after rendering a new /etc/hosts file.

migrations: add migrations for settings.network.hosts

8dfac50

README: add settings.network.hosts

5cb123d

cbgbt force-pushed the staticdns branch from d6ab9ae to 5cb123d Compare June 3, 2022 00:20

cbgbt merged commit 2a06e2d into bottlerocket-os:develop Jun 3, 2022

cbgbt deleted the staticdns branch August 15, 2023 23:56

		// Cast aside anything that doesn't parse as an IP Address, or any references to localhost.
		ip_address != IPV4_LOCALHOST && ip_address != IPV6_LOCALHOST

	/// If `configured_hosts` is Some, the hostname will be considered resolvable if it is listed as an alias for any given IP address.
	/// If `configured_hosts` is set, the hostname will be considered resolvable if it is listed as an alias for any given IP address.

	// and an /etc/hosts configuration which points that contains that hostname as an alias to an IP address,
	// and an /etc/hosts configuration that contains that hostname as an alias to an IP address,

	Note that this setting does not typically impact name resolution for containers, which usually rely on [orchestrator-specific](https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_HostEntry.html) [mechanisms](https://kubernetes.io/docs/tasks/network/customize-hosts-file-for-pods/) for configuring static resolution.
	Note that this setting does not typically impact name resolution for containers, which usually rely on orchestrator-specific mechanisms for configuring static resolution.
	(See [ECS](https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_HostEntry.html) and [Kubernetes](https://kubernetes.io/docs/tasks/network/customize-hosts-file-for-pods/) documentation for those mechanisms.)

	// Check number of parameters, must be exactly two (IP version, hostname, hosts overrides)
	// Check number of parameters, must be exactly three (IP version, hostname, hosts overrides)

Add support for static DNS mappings #2129

Add support for static DNS mappings #2129

Conversation

cbgbt commented May 4, 2022 • edited Loading

zmrow commented May 5, 2022

cbgbt commented May 5, 2022

cbgbt commented May 6, 2022

cbgbt commented May 17, 2022

cbgbt commented May 17, 2022

cbgbt commented May 17, 2022

Choose a reason for hiding this comment

cbgbt commented May 19, 2022

cbgbt commented May 19, 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

cbgbt commented May 24, 2022

bcressey commented May 25, 2022

cbgbt commented May 25, 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

cbgbt commented May 26, 2022

cbgbt commented May 26, 2022

webern left a 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

cbgbt commented Jun 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbgbt commented Jun 3, 2022

cbgbt commented May 4, 2022 •

edited

Loading