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

systemd-networkd: Conditionalize hostnamed/timedated DBUS calls #3232

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

zmrow
Copy link
Contributor

@zmrow zmrow commented Jun 27, 2023

Issue number:
Related to #2449

Description of changes:

    systemd-networkd registers a function to call when first connecting to
    DBUS.  This function makes three calls to other DBUS services (hostnamed
    and timedated) which aren't used in Bottlerocket.  This change makes a
    patch to systemd-networkd that conditionalizes these calls based on the
    underlying service being built.
    
    Removing the calls to non-existent services cleans up some confusing and
    inconsequential messages in the journal on boot.

An example of messages logged on boot because of nonexistent hostnamed DBUS service:

Jun 26 20:54:47 localhost systemd-networkd[968]: Could not set hostname: The name is not activatable 

Testing done:

  • Build an aws-k8s-1.24 image to ensure nothing breaks
  • Build an aws-dev image with a hard-coded network config file and a dependency on console-getty.service in preconfigured.target. Outside of the expected boot failures, observe that the above logs don't show up in the journal, and systemd-networkd is running, and networkctl` is happy. Observe no other related messages are logged in the journal.

Testing of this change will be ongoing during development, but this patch avoids the most obvious and inconsequential failure messages.

bash-5.1# networkctl status
●        State: routable                       
  Online state: online                         
       Address: 172.31.61.130 on eth0
                fe80::c00:dff:fe26:6e27 on eth0
       Gateway: 172.31.48.1 on eth0
           DNS: 172.31.0.2

Jun 27 21:06:57 localhost systemd-networkd[1006]: Enumeration completed
Jun 27 21:06:57 localhost systemd-networkd[1006]: eth0: found matching network '/x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/systemd/networ.
Jun 27 21:06:57 localhost systemd-networkd[1006]: eth0: Link UP
Jun 27 21:06:57 localhost systemd-networkd[1006]: eth0: Gained carrier
Jun 27 21:06:57 localhost systemd-networkd[1006]: eth0: found matching network '/x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/systemd/networ.
Jun 27 21:06:57 localhost systemd[1]: Started Network Configuration.
Jun 27 21:06:57 localhost systemd-networkd[1006]: eth0: DHCPv4 address 172.31.61.130/20, gateway 172.31.48.1 acquired from 172.31.48.1
Jun 27 21:06:58 localhost systemd[1]: Starting Wait for Network to be Configured...
Jun 27 21:06:59 localhost systemd-networkd[1006]: eth0: Gained IPv6LL
Jun 27 21:06:59 localhost systemd[1]: Finished Wait for Network to be Configured.

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.

Copy link
Contributor

@foersleo foersleo left a comment

Choose a reason for hiding this comment

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

The patch as is will work for the case of startup. Looking at the code again, I am wondering about the other call sites of these functions.

The manager_set_timezone and manager_set_hostname functions are also called when handling dhcp leases. The tz and hostname data can also be configured through a dhcp lease it seems (see functions dhcp_lease_acquired, dhcp_reset_hostname, dhcp6_address_acquired in src/network/networkd-dhcp4.c and src/network/networkd-dhcp6.c).

Do we plan to not support such use cases where a user wants to use this? As far as I can tell those are non-essential features not implemented by quite some DHCP clients, but we should be sure we are doing the right thing here and are not in need of a service (hostnamed or something else) to service these requests.

I am no DHCP expert myself though, so I might be over-cautious here.

@markusboehme
Copy link
Member

If we want to make this conditional on build-time options (good idea!), a more universal approach that targets the other call sites as well and is robust against future changes would be to provide stub implementations in the networkd-manager.h header. For example,

#if ENABLE_HOSTNAMED
int manager_set_hostname(Manager *m, const char *hostname);
#else
static inline int manager_set_hostname(_unused_ Manager *m, _unused_ const char *hostname) {
        return 0;
}
#endif /* ENABLE_HOSTNAMED */

while also wrapping the real implementation of manager_set_hostname in a corresponding pre-processor branch.

@zmrow
Copy link
Contributor Author

zmrow commented Jun 30, 2023

The manager_set_timezone and manager_set_hostname functions are also called when handling dhcp leases. The tz and hostname data can also be configured through a dhcp lease it seems (see functions dhcp_lease_acquired, dhcp_reset_hostname, dhcp6_address_acquired in src/network/networkd-dhcp4.c and src/network/networkd-dhcp6.c).

Do we plan to not support such use cases where a user wants to use this? As far as I can tell those are non-essential features not implemented by quite some DHCP clients, but we should be sure we are doing the right thing here and are not in need of a service (hostnamed or something else) to service these requests.

I am no DHCP expert myself though, so I might be over-cautious here.

@foersleo I also found this after I put out the PR and was in the middle of fixing it up - thanks for keeping me honest! :) .

At least currently, we don't allow the configuration of tz and hostname via DHCP. Timezone isn't something currently configurable, and hostname is a configurable API setting. I'd expect it would be a decent re-architecture for us to support tz/hostname via DHCP lease. Because of that, it feels pretty safe to make the manager_set_timezone and manager_set_hostname calls a no-op in the DHCP clients.

I'll plan to fix that up and put out a new revision.

@zmrow
Copy link
Contributor Author

zmrow commented Jun 30, 2023

If we want to make this conditional on build-time options (good idea!), a more universal approach that targets the other call sites as well and is robust against future changes would be to provide stub implementations in the networkd-manager.h header. For example,

This is a great idea - I'll dig into that!

@zmrow
Copy link
Contributor Author

zmrow commented Jul 6, 2023

^ Addresses @markusboehme and @foersleo 's comments

@zmrow zmrow requested a review from foersleo July 6, 2023 18:17
systemd-networkd registers a function to call when first connecting to
DBUS.  This function makes three calls to other DBUS services (hostnamed
and timedated) which aren't used in Bottlerocket.  Calls to the same
DBUS services are also made in the DHCP clients.  This change makes a
patch to systemd-networkd that conditionalizes these calls based on the
underlying service being built, return 0 if the services isn't built.

Removing the calls to non-existent services cleans up some confusing and
inconsequential messages in the journal on boot.
@zmrow
Copy link
Contributor Author

zmrow commented Jul 6, 2023

^ Uses ENABLE_TIMEDATED for timezone calls

@zmrow zmrow requested a review from bcressey July 6, 2023 18:43
Copy link
Contributor

@foersleo foersleo left a comment

Choose a reason for hiding this comment

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

Nice! This looks a lot cleaner and more robust.

@zmrow zmrow changed the title systemd-networkd: Conditionalize top-level DBUS calls systemd-networkd: Conditionalize hostnamed/timedated DBUS calls Jul 7, 2023
@zmrow zmrow merged commit a5f5df2 into bottlerocket-os:develop Jul 7, 2023
@zmrow zmrow deleted the patch-networkd-calls branch July 7, 2023 15:28
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.

5 participants