-
Notifications
You must be signed in to change notification settings - Fork 656
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
dns/rfc2136: delete cache files if nsupdate failed #4057
base: master
Are you sure you want to change the base?
Conversation
@AdSchellevis already mentioned in the issue that here a background process should be used. How can I use a background process here and get the exit code to decide if the cache file should be deleted? Does $cmd .= " /var/etc/nsupdatecmds{$i}";
$cmd .= " || rm -f ".rfc2136_cache_file($dnsupdate, 4)." ".rfc2136_cache_file($dnsupdate, 6); |
/* invoke nsupdate */ | ||
$cmd = "/usr/local/bin/nsupdate -k {$keyfile}"; | ||
if (isset($dnsupdate['usetcp'])) { | ||
$cmd .= " -v"; | ||
} | ||
|
||
$cmd .= " /var/etc/nsupdatecmds{$i}"; |
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.
Looking at the code, this stuff could really use a rewrite. The current code actually has a race condition if you ask me, the cache files(s) should only be written after an successful update, which currently isn't the case.
A short term solution could be to keep the mwexec_bg($cmd);
and wrap the update action in a new shell script (src/bin/nsupdate.sh
), although Ideally one should then also change the rfc2136_cache_file
function to something easily predictable using the same key. If these cache files are not used anywhere else, it might be an option to calculate one key which is then used for all files (e.g. {$dnsupdate['interface']}_{$dnsupdate['host']}_{$dnsupdate['server']}{$ipver}
).
The files being /var/etc/nsupdatecmds{$key}
, /var/etc/nsupdatekey{$key}
, /var/cache/rfc2136_{$key}_4.cache
and /var/cache/rfc2136_{$key}_6.cache
.
I'm not sure how to feed the complete action to /usr/sbin/daemon
, but if we push it in a simple script, we only need to pass the new key and the value of usetcp
, after which we could do something like:
/usr/local/bin/nsupdate -k ... || rm {both cache files}
I'm not using this myself (also don't have a test setup), but this is a direction that could work in my opinion.
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.
You're right, creating the cache files after the nsupdate was successful would be a better way. I try to implement that in the next week.
I added now a wrapper for handling the cache files in background, but now it is not updating on bootup or newwanip hook. I added in addition to that a cron: The cron was executed 13 minutes after newwanip and then the update ran through. OPNSense "General" Log:
Output of
First I thought it is a DNS resolution issue and I added my DNS server as override to unbound. But it's still not working when the hooks are triggering the nsupdate. For me it looks like as the hooks are having race conditions.
DNS setup on my OPNSense box:
What could be the issue here? What could I do here? |
At first glance I'm afraid your overcomplicating the code now, when the hook fires multiple times, the command executed maybe different know due the conditionals. I would really opt for a simpler setup, less code, less chance of regressions. Maybe keeping it as simple as removing the cache files when execution failed is an easier approach (when restructuring the filenames, the script is almost a one-liner). |
No, it triggers extactly as before. See #4055. I think the problem was from the beginning, that the logic does not wait for a working internet connection. The cron is just a workaround for that problem.
Definitively. But not sure how to archieve that, as the nsupdate should not block the bootup / hook execution.
I will try that, but I wanted to finish here first. Is there a function in the opnsense code where I can check if there is a working internet connection? Should this maybe the case when the |
I see. Maybe it's better to use a local workaround for your use-case, maybe a late syshook start script (https://docs.opnsense.org/development/backend/autorun.html) removing the cache files and manually restarting the process. To guide this into a mergable state will likely cost more time than I can offer (maybe even more than engineering a solution myself, in which case adding the functionality to our native backend in the ddclient plugin likely would make more sense).
Not really, working is a fluid concept, when |
I just want to throw this into the mix as alternative: |
Interesting, will try that. What about the potential race condition in the If there is no easy fix, can we at least merge one of the two workarounds I provided for deleting / not creating the cache files? The plugin would be still dirty, but at least it can work in combination with the cron created in the OPNSense settings. Right now the plugin is broken for me. |
I'm afraid not as it moves the action in the foreground, peoples setups might freeze during boot as unintended side affect.
I don't mind the plugin being bad code, as we don't support it officially anyway, but when we merge changes which breaks it for others, it will cost time which would be better spend elsewhere. For us this currently just isn't a priority, if we could push it forward with some tips and review work, that's ok, but only within reasonable limits. |
See #4055.