RFC2136 RecordType Option #1457

ghost · 2017-03-09T13:19:54Z

As explained in #1456 this PR makes it possible to control whether A, AAAA or both record types are changed when updating using RFC2136.

Tasks

Modify edit page
Modify service function
Modify table overview

Open Questions

pfSense do not changed their table overview template, so you cannot see which record types are updated. Should we add this in OPNsense as it might be useful because you do not have to open the edit page for each update task? (see discussion)

Ported from 'pfsense/pfsense#1578', contributed by Robert Nelson.

ghost · 2017-03-09T13:20:54Z

I will try to change the overview of all RFC2136 update tasks!

ghost · 2017-03-09T13:37:51Z

One should keep it as simple as possible. In order to follow this idea I have added an extra check so only ip address types which should be updated are displayed in the overview table.

fichtner · 2017-03-09T13:37:45Z

src/www/services_rfc2136_edit.php

@@ -58,6 +58,9 @@
    $pconfig['usetcp'] = isset($a_rfc2136[$id]['usetcp']);
    $pconfig['usepublicip'] = isset($a_rfc2136[$id]['usepublicip']);

+    $pconfig['recordtype'] = $a_rfc2136[$id]['recordtype'];
+    if (!$pconfig['recordtype']) $pconfig['recordtype'] = "both";


For compatibility the record type should remain empty in the case of "both".

Sorry, if I did not get the point but if there is no recordtype set, you are currently updating A and AAAA records which is reflected by "both".

yes. the config semantics are changed here so this imposes a silent migration only when the entry is edited. we cannot guarantee this won't be problematic in the future so we need to keep the empty value as default. :)

Ok, I will change it! :)

Cool. If you need anything or have questions I'm happy to help. :)

@fichtner I want to test my changes but there is a problem: opnsense/tools#46

From a working OPNsense VM you can install the repository and upgrade from there:

# opnsense-code -a wevcode opnsense-core # cd /usr/opnsense-core # git checkout rfc2136-recordtype-option # opnsense-update -t opnsense-devel # make upgrade

@fichtner I cannot find this in the tools repository as a documentation, please add this tip for other contributors.

Most of it is in the handbook: https://docs.opnsense.org/development/workflow.html

But it needs some updates on the refined tools :)

fichtner · 2017-03-09T13:39:25Z

src/www/services_rfc2136_edit.php

+                    <td class="vtable">
+                      <input name="recordtype" type="radio" value="A" <?php if ($pconfig['recordtype'] == "A") echo "checked=\"checked\""; ?> /> <?=gettext("A (IPv4)");?> &nbsp;
+                      <input name="recordtype" type="radio" value="AAAA" <?php if ($pconfig['recordtype'] == "AAAA") echo "checked=\"checked\""; ?> /> <?=gettext("AAAA (IPv6)");?> &nbsp;
+                      <input name="recordtype" type="radio" value="both" <?php if ($pconfig['recordtype'] == "both") echo "checked=\"checked\""; ?> /> <?=gettext("Both");?>


value="" and empty($pconfig['recordtype']) is better, also the default selection should be the first in the list.

I will remove the "values", for the second change please read my comment above.

fichtner · 2017-03-09T13:40:56Z

src/etc/inc/services.inc

@@ -1748,7 +1748,7 @@ EOD;
                $need_update = false;

                /* Update IPv4 if we have it. */
-                if (is_ipaddrv4($wanip)) {
+                if (is_ipaddrv4($wanip) && $dnsupdate['recordtype'] != "AAAA") {


logic works, but may not be resilient against broadening recordtype to other types, maybe:

is_ipaddrv4($wanip) && (empty($dnsupdate['recordtype']) || $dnsupdate['recordtype'] == 'A')

Well I see yout point. Do you think, we should rename "both" to "ALL" which represents all normal DNS records as A, AAAA (and maybe AAAAAAAA in some years)?

Any other record type has to be set by an extra configuration option as MX, right?

That's the idea, exactly. And indeed "All" sounds best for future use.

fichtner · 2017-03-09T13:42:07Z

And BTW: thanks for working on this 👍

ghost · 2017-03-09T13:49:41Z

OpenSource: Everyone can use it, but you should also try to give something back! 👍

In order to provide support for additional recorytypes, 'both' is changed to 'all', which is represented by an empty value.

To provide support for additional recordtypes all checks had to be changed.

ghost · 2017-03-10T11:47:27Z

@fichtner Updated branch; I will test it now. 🏁

fichtner · 2017-03-10T11:48:02Z

src/www/services_rfc2136_edit.php

@@ -58,6 +58,8 @@
    $pconfig['usetcp'] = isset($a_rfc2136[$id]['usetcp']);
    $pconfig['usepublicip'] = isset($a_rfc2136[$id]['usepublicip']);

+    $pconfig['recordtype'] = $a_rfc2136[$id]['recordtype'];


This needs something like:

$pconfig['server'] = isset($id) &&!empty($a_rfc2136[$id]['server']) ? $a_rfc2136[$id]['server'] : null;

You are right, I will change it!

fichtner · 2017-03-10T11:49:04Z

src/www/services_rfc2136.php

@@ -140,7 +140,7 @@
                      <td>
 <?php
                        $filename = "/conf/dyndns_{$rfc2136['interface']}_rfc2136_" . escapeshellarg($rfc2136['host']) . "_{$rfc2136['server']}.cache";
-                        if (file_exists($filename) && !empty($rfc2136['enable'])) {
+                        if (file_exists($filename) && !empty($rfc2136['enable']) && $rfc2136['recordtype'] != "AAAA") {


same logic as in services.inc should be used here. We cannot expect it to be set, it would probably produce a warning now.

Missed that, sorry.

ghost · 2017-03-10T12:01:06Z

@fichtner Should be fixed! :)

ghost · 2017-03-10T12:46:53Z

@fichtner Also added a help text to undestand the new 'all' option.

ghost · 2017-03-10T12:49:25Z

Ready for review.

fichtner · 2017-03-10T12:58:27Z

Merged, thanks!

fichtner · 2017-03-10T13:18:39Z

Two small follow-ups committed. I think this will land in 17.1.3 next week. Thanks again.

bephinix added 2 commits March 9, 2017 14:13

Add 'recordtype' as a config option for rfc2136 updates.

7ce54fc

Ported from 'pfsense/pfsense#1578', contributed by Robert Nelson.

Only update IPv4/IPv6 in rfc2316 updates if wanted.

c6e48e9

Ported from 'pfsense/pfsense#1578', contributed by Robert Nelson.

Only show ip address types which should be updated.

5aeaeff

fichtner self-assigned this Mar 9, 2017

fichtner reviewed Mar 9, 2017

View reviewed changes

bephinix added 2 commits March 10, 2017 12:41

Set 'All' as default recordtype and first choice.

657d50c

In order to provide support for additional recorytypes, 'both' is changed to 'all', which is represented by an empty value.

Recordtype checks for nsupdate changed to work with empty values.

ff4f087

To provide support for additional recordtypes all checks had to be changed.

fichtner reviewed Mar 10, 2017

View reviewed changes

bephinix added 2 commits March 10, 2017 12:53

Use same logic for recordtype checks as the edit page.

ac0782b

Modify variable assignment to work with empty values.

e50a9df

Add help text for record type option.

b373957

fichtner merged commit fb7c461 into opnsense:master Mar 10, 2017

fichtner added a commit that referenced this pull request Mar 10, 2017

dns: small follow-up for #1457

2acd459

fichtner added a commit that referenced this pull request Mar 10, 2017

dns: do not store empty recordtype #1457

6e57f40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC2136 RecordType Option #1457

RFC2136 RecordType Option #1457

ghost commented Mar 9, 2017 •

edited by fichtner

Loading

ghost commented Mar 9, 2017

ghost commented Mar 9, 2017

fichtner Mar 9, 2017

ghost Mar 9, 2017

fichtner Mar 9, 2017

ghost Mar 9, 2017

fichtner Mar 9, 2017

ghost Mar 9, 2017

fichtner Mar 10, 2017

ghost Mar 10, 2017

fichtner Mar 10, 2017

fichtner Mar 9, 2017

ghost Mar 9, 2017

fichtner Mar 9, 2017

ghost Mar 9, 2017

fichtner Mar 9, 2017

fichtner commented Mar 9, 2017

ghost commented Mar 9, 2017

ghost commented Mar 10, 2017

fichtner Mar 10, 2017

ghost Mar 10, 2017

fichtner Mar 10, 2017

ghost Mar 10, 2017

ghost commented Mar 10, 2017

ghost commented Mar 10, 2017

ghost commented Mar 10, 2017

fichtner commented Mar 10, 2017

fichtner commented Mar 10, 2017

RFC2136 RecordType Option #1457

RFC2136 RecordType Option #1457

Conversation

ghost commented Mar 9, 2017 • edited by fichtner Loading

Tasks

Open Questions

ghost commented Mar 9, 2017

ghost commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fichtner commented Mar 9, 2017

ghost commented Mar 9, 2017

ghost commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 10, 2017

ghost commented Mar 10, 2017

ghost commented Mar 10, 2017

fichtner commented Mar 10, 2017

fichtner commented Mar 10, 2017

ghost commented Mar 9, 2017 •

edited by fichtner

Loading