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.

Sign up for GitHub

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

Jump to bottom

nixos/ddclient: make nsupdate protocol usuable #154124

Merged
merged 2 commits into from
Jan 9, 2022
Merged

nixos/ddclient: make nsupdate protocol usuable #154124

merged 2 commits into from
Jan 9, 2022

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Jan 9, 2022 •
edited
Loading

Motivation for this change
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 9, 2022
@Mic92 Mic92 requested a review from SuperSandro2000 January 9, 2022 10:31
@Mic92 Mic92 changed the title Ddclient nixos/ddclient: make nsupdate protocol usuable Jan 9, 2022
Mic92
Mic92 commented Jan 9, 2022
View reviewed changes
nixos/modules/services/networking/ddclient.nix
@@ -29,8 +29,10 @@ let
configFile = if (cfg.configFile != null) then cfg.configFile else configFile';

preStart = ''
install ${configFile} /run/${RuntimeDirectory}/ddclient.conf
${lib.optionalString (cfg.configFile == null) (if (cfg.passwordFile != null) then ''
install --owner ddclient -m600 ${configFile} /run/${RuntimeDirectory}/ddclient.conf
Copy link
Member Author

Choose a reason for hiding this comment

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

This mainly avoids warnings but should have not be strictly necessary.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 9, 2022
@SuperSandro2000
Copy link
Member

Can you fix the manual?

@Mic92
Copy link
Member Author

Mic92 commented Jan 9, 2022

fixed.

@SuperSandro2000 SuperSandro2000 merged commit 8928525 into NixOS:master Jan 9, 2022
@Mic92 Mic92 deleted the ddclient branch January 9, 2022 23:57
@arcnmx
Copy link
Member

arcnmx commented Jan 13, 2022

Can everyone please stop breaking this module without testing it? This is the exact same problem as last time.

Jan 13 10:51:47 shanghai 3jifr0sab601i8l2ka07c2c2cak5csg2-ddclient-prestart[948280]: install: invalid user ‘ddclient’
│ Jan 13 10:51:47 shanghai 3jifr0sab601i8l2ka07c2c2cak5csg2-ddclient-prestart[948284]: sed: can't read /run/ddclient/ddclient.conf: No such file or directory

It's a DynamicUser service, you can't just chown it like that AFAIK

@Mic92
Copy link
Member Author

Mic92 commented Jan 13, 2022 •
edited
Loading

Can everyone please stop breaking this module without testing it? This is the exact same problem as last time.

I tested this module. The reverted commit did not change the owner to ddclient as well, which is what was done here.

Jan 13 10:51:47 shanghai 3jifr0sab601i8l2ka07c2c2cak5csg2-ddclient-prestart[948280]: install: invalid user ‘ddclient’
│ Jan 13 10:51:47 shanghai 3jifr0sab601i8l2ka07c2c2cak5csg2-ddclient-prestart[948284]: sed: can't read /run/ddclient/ddclient.conf: No such file or directory

It's a DynamicUser service, you can't just chown it like that AFAIK

This is true not in general. If it cannot find the user than something is wrong with your nss setup. Systemd creates this user before starting PreStart. Was nscd by chance restarted in the middle?

@Mic92
Copy link
Member Author

Mic92 commented Jan 13, 2022

In 1024571 I fixed some race conditions that broke dynamic user in some cases. What is the full restart log, when you run an upgrade?

@arcnmx
Copy link
Member

arcnmx commented Jan 13, 2022 •
edited
Loading

This is not true. If it cannot find the user than something is wrong with your nss setup.

Thanks for the hint, I'll look into that. I'd been trying to look for documentation on how DynamicUser interacts with exec/etc for ages now .-.

Systemd creates this user before starting PreStart. Was nscd by chance restarted in the middle?

Possibly! It certainly broke a switch :(

What is the full restart log, when you run an upgrade?

I can try to look into it in a bit but if it indeed is related to nscd updating at the same time, I've already performed the update by now ><

EDIT: does that mean units using dynamicuser need an After = nscd or something then..?

@Mic92
Copy link
Member Author

Mic92 commented Jan 13, 2022 •
edited
Loading

It looks like my nscd fix is no longer in nixpkgs. I re-introduced it in this PR: #154928
Can you test it? It should not need After = nscd and this dependency should also not fix the bug you see.

@SuperSandro2000
Copy link
Member

just FYI so that you know: I am the more or less the upstream (core) maintainer of ddclient as of yesterday.

@Mic92
Copy link
Member Author

Mic92 commented Jan 15, 2022

just FYI so that you know: I am the more or less the upstream (core) maintainer of ddclient as of yesterday.

dual-stack support please :)
For this reason I had to use this code instead: https://github.com/Mic92/dotfiles/blob/1348780fa97092731f0b2f483580c8cbd9eff213/nixos/modules/ip-update.nix#L29

@arcnmx
Copy link
Member

arcnmx commented Jan 15, 2022

dual-stack support please :)

wasn't that added some time ago? I'm pretty sure it's working for me.

@Mic92
Copy link
Member Author

Mic92 commented Jan 15, 2022

Ok. Configuration was not clear to me at all. I saw something that looked like an RFC. Might be also that the nsupdate backend does not handle this.

@arcnmx
Copy link
Member

arcnmx commented Jan 15, 2022

I saw something that looked like an RFC

ddclient/ddclient@24ba945 appears to be the result of that and the introduction of proper support, but there hasn't been a tagged release in 2 years, so...

Might be also that the nsupdate backend does not handle this

Maybe yeah, I'm not sure what support for it looks like for individual backends :<

@glittershark
Copy link
Member

glittershark commented Jan 19, 2022 •
edited
Loading

is there a workaround on nixpkgs-unstable (eg without #154928) to get ddclient working again?

@SuperSandro2000
Copy link
Member

dual-stack support please :)

Write me a private message and we can figure this out.

but there hasn't been a tagged release in 2 years, so...

In the meantime I created a fork which I am planning to upstream in the next weeks and then create an RC maybe in a month or so. No promises though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Mic92 @SuperSandro2000 @arcnmx @glittershark