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

dbus: wait for 'netplan try' to be ready (LP: #1949893) #245

Closed
wants to merge 7 commits into from

Conversation

slyon
Copy link
Collaborator

@slyon slyon commented Nov 24, 2021

Description

netplan-dbus now waits for the spawned 'netplan try' child to touch the /run/netplan/netplan-try.ready stamp file before it returns the DBus call. If no stamp file is detected within up to 5 sec it hits a timeout and returns an error.

Additionally, this PR places ephemeral netplan configs in /run/netplan/config-XXXXXX, to avoid potential exploits of tmpfiles, like https://lwn.net/Articles/250468/

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad. LP#1949893

@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2021

Codecov Report

Merging #245 (0bd35a4) into main (c6ad8e6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #245   +/-   ##
=======================================
  Coverage   99.09%   99.09%           
=======================================
  Files          58       58           
  Lines        9913     9978   +65     
=======================================
+ Hits         9823     9888   +65     
  Misses         90       90           
Impacted Files Coverage Δ
netplan/cli/commands/apply.py 100.00% <100.00%> (ø)
netplan/cli/commands/try_command.py 100.00% <100.00%> (ø)
src/dbus.c 100.00% <100.00%> (ø)
tests/dbus/test_dbus.py 100.00% <100.00%> (ø)
tests/test_cli_units.py 100.00% <100.00%> (ø)
tests/test_utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6ad8e6...0bd35a4. Read the comment docs.

@slyon slyon force-pushed the slyon/lp1949893-dbus-try-wait branch 2 times, most recently from b015bc5 to 99e6190 Compare November 24, 2021 13:35
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Looks good, two nitpicks inside but given the constraints probably the best we can do :) [reading this again I really hope it does not sound negative - it's not meant this way]

netplan/cli/commands/apply.py Outdated Show resolved Hide resolved
src/dbus.c Outdated Show resolved Hide resolved
@mvo5

This comment has been minimized.

@slyon slyon force-pushed the slyon/lp1949893-dbus-try-wait branch 2 times, most recently from 8d58917 to cc37ced Compare November 25, 2021 13:57
@slyon
Copy link
Collaborator Author

slyon commented Nov 25, 2021

Thank you @mvo5 for your comments! Indeed having a "stamp file" is a much more universal approach. I'm not sure why I didn't think about that earlier... I thought about passing signals from child to parent process (like we do from parent to child currently) and also about passing DBus signal directly from the Python code (but that would probably be overkill as we do not have proper DBus integration with the python code, currently and on the netplan-dbus daemon side it would have led to additional sd-bus interrupts and probably the need for async methods.)

Polling for a file is plain and simple solution.
I adopted your recommendation a bit, primarily by having a global /tmp/netplan-try.ready stamp file. As there can only every be one single netplan try call at a time (as it is touching system system config and (re-)starting services). This way we do not have to shuffle it around with the states and it can also be used in a non-DBus environment.

@slyon slyon force-pushed the slyon/lp1949893-dbus-try-wait branch from cc37ced to 0bbc0d5 Compare November 25, 2021 15:30
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Looks very nice! I have some comments/nitpicks/suggestions inline that are hopefully useful but nothing that would bock this. Thanks a lot for working on this!

netplan/cli/commands/try_command.py Outdated Show resolved Hide resolved
netplan/cli/commands/try_command.py Show resolved Hide resolved
netplan/cli/commands/try_command.py Outdated Show resolved Hide resolved
tests/dbus/test_dbus.py Outdated Show resolved Hide resolved
tests/dbus/test_dbus.py Outdated Show resolved Hide resolved
tests/test_cli_units.py Outdated Show resolved Hide resolved
netplan-dbus now waits for the spawned 'netplan try' child to touch the
/tmp/netplan-try.ready stamp file before it returns the DBus call. If no
stamp file is detected within up to 5sec it hits a timeout and returns an
error.
@slyon slyon force-pushed the slyon/lp1949893-dbus-try-wait branch from 9f27143 to 0cf3e7e Compare November 26, 2021 14:31
@slyon slyon marked this pull request as ready for review November 26, 2021 14:31
@slyon slyon force-pushed the slyon/lp1949893-dbus-try-wait branch from 0cf3e7e to 0bd35a4 Compare November 26, 2021 16:11
@slyon slyon requested a review from mvo5 November 26, 2021 16:22
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Looks very nice, thank you! One tiny nitpick inline but feel free to ignore :)

netplan/cli/commands/try_command.py Show resolved Hide resolved
netplan/cli/commands/try_command.py Outdated Show resolved Hide resolved
src/dbus.c Show resolved Hide resolved
@slyon slyon force-pushed the slyon/lp1949893-dbus-try-wait branch from 0bd35a4 to 5aa371b Compare November 26, 2021 16:39
@slyon
Copy link
Collaborator Author

slyon commented Nov 26, 2021

Thank you so much @mvo5 for all your input here. I think this is ready for merging \o/

Let me run some more test on it before merging it early next week.

slyon added a commit that referenced this pull request Nov 29, 2021
netplan-dbus now waits for the spawned 'netplan try' child to touch the /run/netplan/netplan-try.ready stamp file before it returns the DBus call. If no stamp file is detected within up to 5 sec it hits a timeout and returns an error.

Additionally, this PR places ephemeral netplan configs in /run/netplan/config-XXXXXX, to avoid potential exploits of tmpfiles, like https://lwn.net/Articles/250468/

COMMITS:
* test:dbus: some cleanup
* test:utils: add touch() method to MockCmd
* cli:try: add touch/clear_ready_stamp methods
* dbus: wait for 'netplan try' to be ready
netplan-dbus now waits for the spawned 'netplan try' child to touch the
/tmp/netplan-try.ready stamp file before it returns the DBus call. If no
stamp file is detected within up to 5sec it hits a timeout and returns an
error.
* cli:apply: some more time for NM to have all interfaces created
* dbus:cli: move netplan-try.ready stamp file to /run/netplan
to avoid potential expolits of tmpfiles, like https://lwn.net/Articles/250468/
* dbus: move ephemeral netplan config states to /run/netplan
to avoid potential expolits of tmpfiles, like https://lwn.net/Articles/250468/
@slyon
Copy link
Collaborator Author

slyon commented Nov 29, 2021

Merged in e19441c

@slyon slyon closed this Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants