dns/ddclient hetzner existing record update patch

[jnikodemus]

Fix ddclient update Hetzner for new API
Reference
pull#5082
issue#5187

Problem
The actual update method returns code 200 for an update, because it doesnt use the right entrypoint.

Fix
Changed the API entrypoint "actions" and removed code duplication.

Testing
Tested on a local OPNsense instance with multiple records.

[AdSchellevis]

wouldn't it be better to open a ticket at their end, it feels rather weird to patch the caller in this case (assuming its a bug and not a documented change of behavior)

[jnikodemus]

You are right, I've contacted the hoster again. I'll write it correct.

[Ollienator]

I was just going to open a pull request for the same issue but then I found yours. Since you also removed some code duplication, your changes are ahead of mine (see 7bc1af4).

One thing I have noticed is that you are including the TTL parameter in your request. This should be removed as it's not used in the update_records action (see https://docs.hetzner.cloud/reference/cloud#tag/zone-rrset-actions/update_zone_rrset_records).

[jnikodemus]

I was just going to open a pull request for the same issue but then I found yours. Since you also removed some code duplication, your changes are ahead of mine (see 7bc1af4).

One thing I have noticed is that you are including the TTL parameter in your request. This should be removed as it's not used in the update_records action (see https://docs.hetzner.cloud/reference/cloud#tag/zone-rrset-actions/update_zone_rrset_records).

Thank you, appreciate the hint. I've removed the ttl from the _update_record and tested again -> success.

[TheRealBecks]

Any news on this?

[jnikodemus]

I'm waiting for a merge. Using this fix since 3 weeks without problems.

[Starbase12]

Would also like to see this fix included. Want to keep using ddclient and migrate to Hetzner DNS via API.

[thwien]

I have the same trouble with updating DNS records via new Hetzner API. I can confirm pulling this patch for hetzner.py seems to work properly.

[TheRealBecks]

@jnikodemus I (quick) checked your source code and I'm not sure that I fully understood, so let me try to explain what I would do with the given POST and PUT calls of the Hetzner Cloud API:

1. PUT (Update) the RR (resource record) ❎
2. If PUT fails, do a POST call to create the resource ❎
3. If the POST also fails, then fail ❌

No need to DELETE an RR at all. Also the first PUT (with retries) call will almost always give an HTTP 200 when the credentials and the zone file are correctly setup. So only one API call will always update the RR. The POST call will only used by the very first script call, so the it will cost two API calls.

Doing it that way will be resource and cost efficient.

[becast]

@TheRealBecks The request to update the IP of an RRSet Record is a POST action. (https://docs.hetzner.cloud/reference/cloud#tag/zone-rrset-actions/update_zone_rrset_records)
That's where the current issue lies as it tries to do a PUT which fails.
So for me the Order of operations makes sense according to the (very convoluted) Hetzner API

[jnikodemus]

It was intended as quickfix, to get things running without overengineering.

After reviewieng the logic again I would agree with @TheRealBecks that the API-Call to check for existence is not needed as it creates the record anyway if not exists (not a typocheck as I thought in the first place). However, based on my reading of the documentation, I would still keep this as a POST, as @becast mentioned.

Now that we are trying to improve the implementation in efficiency, I think the API call to retrieve the zone_id may also be unnecessary (not verified in depth yet). The new API supports changes based on zone_name, which is already extracted locally via _get_zone_name().

[TheRealBecks]

@becast Okay, understand, I referenced the calls of type Zone RRSets instead of Zone RRset Actions. So I tested the Python code examples provided by Hetzner and they are working as intended:

1. set_zone_rrset_records (Update an existing RR): ❎
2. If set_zone_rrset_records fails, do a add_zone_rrset_records (Create a new RR) ❎
3. If add_zone_rrset_records also fails, then fail ❌

@jnikodemus No overengineering involved, only two calls as intended (and as you also said in your last post) 🙂

[jnikodemus]

@TheRealBecks I've removed the check for existence and the now unneseccary method _get_record(). I’d appreciate a quick check.

[realizelol]

Code looks good to me, but the permission doesn't seems to be correct by now:
dns/ddclient/src/opnsense/scripts/ddclient/lib/account/hetzner.py
100755 → 100644 (have a look at the file header)
The modified file doesn't seems to be executable at the time of my comment.

[becast]

I can confirm this is working excellently on my installation since last week.

[TheRealBecks]

@AdSchellevis Are you okay with the changes?

[AdSchellevis]

Looking at how the discussion started (a bug elsewhere being worked around here), it feels strange to patch anything. I don't mind that much merging changes if enough people tested it, but in that case it's probably a good idea to sweep this for whitespace changes to limit the amount of lines changed (there's a lot of noise in the PR now).

[TheRealBecks]

@jnikodemus One more iteration without the whole file being auto-formatted (and therefore white spaces being deleted) could lead to a merge 😉

[jnikodemus]

@jnikodemus One more iteration without the whole file being auto-formatted (and therefore white spaces being deleted) could lead to a merge 😉

I added the deleted whitespaces and removed autogenerated Hyperlinks. Hope it's ok to merge now, as the patch would be nice for hetzner-people :)

[AdSchellevis]

@jnikodemus thanks, I've merged it for you

[jnikodemus]

@jnikodemus thanks, I've merged it for you

Great, thanks!

@TheRealBecks thanks for your support :)