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.

Sign up for GitHub

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

Jump to bottom

dns/bind: allow to specify primaryip's port for secondary zone #4445

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

dns/bind: allow to specify primaryip's port for secondary zone #4445

wants to merge 1 commit into from

Conversation

loopway
Copy link

@loopway loopway commented Jan 5, 2025

Closes: #4444

Notes on proposed solution

Bind/Domain.xml:

  • change field type to CSVListField, adopted from net/haproxy HAProxy.xml to allow port specification

Bind/named.conf

  • adjust jinja2 template instead of introducing new fields to avoid have to deal with config migrations of previous opnsense versions
  • if specifying a port for a ipv6 address the following notion has to be used: [address]:port (https://en.wikipedia.org/wiki/IPv6#Address_representation)
  • using negative lookbehind in regular expression to avoid matching ipv6 addresses without port
  • using multiple lookbehind regular expressions since quantifiers are not allowed and ipv6 addresses can be shortened

fichtner
fichtner reviewed Jan 5, 2025
View reviewed changes
dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/Domain.xml
<primaryip type="NetworkField">
<FieldSeparator>,</FieldSeparator>
<asList>Y</asList>
<primaryip type="CSVListField">
Copy link
Member

@fichtner fichtner Jan 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I know we have an address/port field type nowadays.

Copy link
Author

Choose a reason for hiding this comment

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

Found a IPPortField field type. This type would require to specify a port and would therefore invalidate address only entries from previous versions...

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it needs to be extended. My point is no more CSV magic regarding this.

Copy link
Author

Choose a reason for hiding this comment

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

I see your point, but I would argue that should be a separate issue/task, requiring changes in core. A quick github search for type="CSVListField" shows matches in three files in core and 29 files in plugins. Can we move this forward in the meantime? Changing it to an extended IPPortField later should be easy enough. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I’m only saying I’m not at the office until tomorrow to do core changes. Copy+paste is not the best idea here in 2025 😊

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

@fichtner fichtner fichtner left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

os-bind: allow to specify primaryip's port for secondary zone
2 participants
@loopway @fichtner