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.

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

dnsmasq: When dhcp-fqdn is set, there must be a domain without an address set as default #8405

Merged
merged 4 commits into from
Mar 7, 2025

Conversation

Monviech
Copy link
Member

@Monviech Monviech commented Mar 5, 2025

https://thekelleys.org.uk/dnsmasq/docs/dnsmasq-man.html

--dhcp-fqdn

To ensure that all names have a domain part, there must be at least --domain without an address specified when --dhcp-fqdn is set.

Fixes the following error:

2025-03-05T15:44:42 | Critical | dnsmasq | FAILED to start up
2025-03-05T15:44:42 | Critical | dnsmasq | there must be a default domain when --dhcp-fqdn is set

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…ress set as default.
@Monviech Monviech added the bug Production bug label Mar 5, 2025
@Monviech Monviech requested a review from AdSchellevis March 5, 2025 16:02
@Monviech Monviech self-assigned this Mar 5, 2025
@@ -33,6 +33,10 @@ dhcp-lease-max={{dnsmasq.dhcp.lease_max}}
dhcp-fqdn
{% endif %}

{% if dnsmasq.dhcp.domain %}
Copy link
Member

Choose a reason for hiding this comment

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

we could consider using the default one when not specified, but that's also something we can do later if needed. This is a good addition, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed a new commit with a sensible default.

https://en.wikipedia.org/wiki/.internal

Copy link
Member

Choose a reason for hiding this comment

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

@Monviech I was thinking more about the firewall's default domain to be honest, just need some trickery for the hint part in that case. kea has something similar for the hostname:

mapDataToFormUI(data_get_map).done(function(data){
try {
$("#dhcpv4\\.ha\\.this_server_name").attr(
"placeholder",
data.frm_generalsettings.dhcpv4.this_hostname
);
} catch (e) {
null;
}
formatTokenizersUI();
$('.selectpicker').selectpicker('refresh');
updateServiceControlUI('kea');
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks cool I try that out. :)

Copy link
Member Author

@Monviech Monviech Mar 5, 2025

Choose a reason for hiding this comment

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

I've added another commit that adds the same feature.

Edit: I made a mistake there which I still have to fix.

public function getAction()
{
$data = parent::getAction();
$data[self::$internalModelName]['dhcp']['domain'] = (string)Config::getInstance()->object()->system->domain;
Copy link
Member

Choose a reason for hiding this comment

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

you need to place this into a separate container to prevent the value from being persisted.

Copy link
Member Author

Choose a reason for hiding this comment

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

I misunderstood the concept. I have fixed it now.

@@ -31,7 +31,7 @@ dhcp-lease-max={{dnsmasq.dhcp.lease_max}}

{% if dnsmasq.dhcp.fqdn == '1' %}
dhcp-fqdn
domain={{dnsmasq.dhcp.domain|default('internal')}}
domain={{dnsmasq.dhcp.domain}}
Copy link
Member

Choose a reason for hiding this comment

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

domain={{dnsmasq.dhcp.domain|default(system.hostname)}}

Copy link
Member Author

Choose a reason for hiding this comment

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

I've used system.domain instead

@Monviech Monviech requested a review from AdSchellevis March 6, 2025 07:52
Copy link
Member

@AdSchellevis AdSchellevis left a comment

Choose a reason for hiding this comment

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

Nice!

@Monviech Monviech merged commit b11baac into master Mar 7, 2025
@Monviech Monviech deleted the dnsmasq-fqdn-domain branch March 7, 2025 07:19
preethijacobjt pushed a commit to preethijacobjt/opnsense-core that referenced this pull request Mar 26, 2025
…ress set as default (opnsense#8405)

* dnsmasq: When dhcp-fqdn is set, there must be a domain without an address set as default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Production bug
Development

Successfully merging this pull request may close these issues.

2 participants