Skip to content

Conversation

@sudnam
Copy link

@sudnam sudnam commented Oct 12, 2023

Treat a 204 response code as successful when using the dyndns2 provider. Should help with #3585 (by using the custom provider) until we get a proper provider for domeneshop.

@AdSchellevis
Copy link
Member

@sudnam how about 7c6fccd ?

@AdSchellevis AdSchellevis self-assigned this Oct 12, 2023
@fichtner
Copy link
Member

yeah, 2xx range should indicate ok. that would have been my comment as well :)

@sudnam
Copy link
Author

sudnam commented Oct 12, 2023

@sudnam how about 7c6fccd ?

Yeah, that works as well and is a bit nicer I guess. :) Thanks for fixing!

@sudnam sudnam deleted the dyndns-response branch October 12, 2023 18:07
@sudnam
Copy link
Author

sudnam commented Nov 6, 2023

@AdSchellevis Just got the update to 1.16.1 on my home router and still it complains about a 204 response (Account xxx [custom - dydns.domain.com] failed to set new ip [204 - ]). Am I missing something or is there something more that needs to be done?

@fichtner
Copy link
Member

fichtner commented Nov 6, 2023

Will be released in 23.7.8.

@sudnam
Copy link
Author

sudnam commented Nov 6, 2023

So it wont work in <23.7.8 even though it says that 1.16_1 is installed and the changelog mentions the fix for this?

@fichtner
Copy link
Member

fichtner commented Nov 6, 2023

You are right, it’s already in 23.7.7. Native backend? Dyndns2?

@sudnam
Copy link
Author

sudnam commented Nov 6, 2023

Native backend and custom service with 'Custom GET'. My understanding is that that should go through the same code as dyndns2.

@AdSchellevis
Copy link
Member

I would expect so, yes, looking at the code on my end, it seems to include the adjusted line:

# grep 'req.status_code' /usr/local/opnsense/scripts/ddclient/lib/account/dyndns2.py
            if 200 >= req.status_code < 300:
                        self.description, self.current_address, req.status_code, req.text.replace('\n', '')

@fichtner
Copy link
Member

fichtner commented Nov 6, 2023

Old daemon still running?

@sudnam
Copy link
Author

sudnam commented Nov 7, 2023

I also have the expected code in place and tried restarting the daemon but still no luck, getting the same error.

@AdSchellevis
Copy link
Member

@sudnam ok, something is off with my code, I'll take a look

@AdSchellevis
Copy link
Member

@sudnam ok, I'm an idiot, even while reading it twice, I. didn't see the apparent issue here, can you try f6f72bb ?

@sudnam
Copy link
Author

sudnam commented Nov 7, 2023

Right, I totally missed that as well... Is there an easy way to test out that commit in a running system? Without compiling a package and stuff yourself.

@fichtner
Copy link
Member

fichtner commented Nov 7, 2023

# opnsense-patch -c plugins f6f72bb

@sudnam
Copy link
Author

sudnam commented Nov 7, 2023

@fichtner Perfect, thanks!

@AdSchellevis Now I'm getting a raised fatal error (list index out of range) instead when it is trying to update with the same config as before.

@AdSchellevis
Copy link
Member

no clue, you probably have to debug further, nothing else has changed here. the most likely crash point is

self.update_state(address=self.current_address, status=req.text.split()[0])

There's a notice message earlier

@sudnam
Copy link
Author

sudnam commented Nov 7, 2023

Right, that should be it. Since the response is 204, req.text will contain an empty string which will lead to an empty array when split is called on it.

I'll have a look at a fix later tonight.

@AdSchellevis
Copy link
Member

ok, here you go 85e4a25

@sudnam
Copy link
Author

sudnam commented Nov 7, 2023

Thank you very much for all the help!

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants