radvd: allow disabling DNSSL per interface #10282#10284
Conversation
| <IpAllowed>N</IpAllowed> | ||
| <AsList>Y</AsList> | ||
| </DNSSL> | ||
| <DNSSL type="CSVListField"/> |
There was a problem hiding this comment.
I see your issue and we probably shouldn't demote this to CSVListField which is more of a last resort for rapid implementation than it is good for integrity especially replacing it it blatantly like this. Would be nicer if IsDNSName recognises "." as a valid input. In theory it's a valid DNS name ... or make room for optionally allowing it as well.
I'm happy to do this PR. It's difficult to steer people steering AI.
There was a problem hiding this comment.
IsDNSName has been made more strict a few times if I remember correctly.
Would be better to create a custom hostnamefield like dnsmasq has for an issue like this.
There was a problem hiding this comment.
Would be nicer if IsDNSName recognises "." as a valid input.
Well, to my knowledge IsDNSName is supposed to be based on the implementation of FILTER_VALIDATE_DOMAIN which does not consider . a valid domain:
% php -r "var_dump(filter_var('.', FILTER_VALIDATE_DOMAIN));"
bool(false)`
It's difficult to steer people steering AI.
I'm not a vibe coder, in the before times I would have used Stack Overflow, PHP documentation and copy and paste of some existing code snippet making it work.
It was my own opinion that treating this as a CSVListField was the cleanest way to build it as-in the most local scope, least LOC touched and no surprising side-effects as I tend to prefer for projects I am not intricately familiar with.
There was a problem hiding this comment.
@fichtner wouldnt a simple boolean field do the job here as well? Imo its better than magic values, but its your choice here. Just wanted to give my two cents xD
There was a problem hiding this comment.
I have to say for the record if you think you didn’t degrade the code you definitely did. We value review from peers and we are often quite picky amongst ourselves as well which produces better quality code overall. It’s our process and I want you to respect it and avoid defending yourself for things you couldn’t know.
There was a problem hiding this comment.
I have to say for the record if you think you didn’t degrade the code you definitely did. We value review from peers and we are often quite picky amongst ourselves as well which produces better quality code overall. It’s our process and I want you to respect it and avoid defending yourself for things you couldn’t know.
Would you like to take over from here or give me a hint on how you prefer it being done and I will do it 100% by hand.
My point regarding FILTER_VALIDATE_DOMAIN stands, I would avoid adding an unexpected layer on top just for this behaviour.
My original preferred shape was setting DNS Search List Lifetime to -1 because it is pretty minimal and less of a validation nightmare.
Thank you for your efforts.
Important notices
Before you submit a pull request, we ask you kindly to acknowledge the following:
If AI was used, please disclose:
Describe the problem
The DNS Search List (DNSSL) can't be disabled without also disabling Recursive DNS Servers (RDNSS) which may lead to adverse effects with certain devices, I only observed weirdness with Android phones.
Describe the proposed solution
Entering
.into the DNS Search List (DNSSL) field will omit the relevant section for that interface in radvd.conf entirely per #10282 (comment).Related issue
#10282
I already tried the patch for a brief period in isolation, it works as expected.