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

unbound: introducing overwriting of MX records #519

Merged
merged 7 commits into from
Jan 8, 2016

Conversation

8191
Copy link
Member

@8191 8191 commented Dec 10, 2015

First PR to #511:

  • Split unbound host and domain overwrites from Services: DNS Resolver: General to Services: DNS Resolver: Overwrites
  • Introducing resource record types
  • Allow MX records to be overwritten (in addition to already existing A and AAAA)

Put Host and Domain Overrides to separate configuration section of unbound DNS
resolver.
Added double click to edit domain override entry.
Added the possibility to overwrite MX records by unbound DNS resolver.
@8191 8191 changed the title Unbound overrides unbound: introducing overwriting of MX records Dec 10, 2015
<?php

/*
Copyright (C) 2014-2015 Deciso B.V.
Copy link
Member

Choose a reason for hiding this comment

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

This is definitely wrong. Which file did you copy? And where is your copyright? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@fichtner I've copied services_unbound.php. I guess the pfSense copyright should also go along with that file, since parts of the content were moved from an existing file...

Copy link
Member

Choose a reason for hiding this comment

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

Yup, Warren Baker needs to be added.

You can push more commits to the branch to update the pull request.

I will need to get home before I have time to test.

if (!empty($host['descr']) && isset($config['unbound']['txtsupport']))
/* Backwards compatibility for records created before introducing RR types. */
if (!isset($host['rr'])) {
$host['rr'] = (is_ipaddrv6($host['ip'])) ? 'AAAA' : 'A';
Copy link
Member

Choose a reason for hiding this comment

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

Here you're using "AAAA", but later you're using only "A" for both. If we do resource records, we ought to split A and AAAA.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea was, that the user does not need to care about the difference between A and AAAA records, since without this PR the user does not need to care about it neither. So the differentiation between A and AAAA is done programmatically and is shown as "A/AAAA".
I have no problem in separating A and AAAA RRs...

@fichtner
Copy link
Member

Hi Manuel,

Except for the two question the code looks good to me. I am only concerned with backwards compatibility and consistency in the modelling, even though it's just a temporary (0.5 to 1 year by my weak estimate) solution until we can move this to the MVC framework. Sooner or later more people will ask for different records, multiple records of the same type, etc.

Cheers,
Franco

@8191
Copy link
Member Author

8191 commented Jan 4, 2016

Hi Franco,

please see my (in-code) comments above.

Regarding modelling (in)consistency; where exactly are your concerns?
Not directly related, but how do you plan migrating an existing "static PHP" service to MVC? Do you plan to migrate the XML configuration to a new structure or reuse the old one?

Manuel

Previously it was not possible to add two host override entries for the same
host-domain combination. Technically this restriction does not exist neither
within unbound nor DNS.
@8191
Copy link
Member Author

8191 commented Jan 8, 2016

@fichtner I've removed the restriction that a single host cannot be overwritten with two values, since I cannot see any technical justification for that limitation. Also sometimes it might be even required to do so, e.g. DNS round robin.

@fichtner
Copy link
Member

fichtner commented Jan 8, 2016

@8191 cool! I'll try to get this in today :)

@8191
Copy link
Member Author

8191 commented Jan 8, 2016

@fichtner Great... :)
What about A/AAAA differentiation? Shall we split it and let the user choose between A and AAAA? Currently OPNsense determines the type by the specified IP address.

@fichtner
Copy link
Member

fichtner commented Jan 8, 2016

Now that multiple host entries are allowed it doesn't matter. What we should do is get this in now and remodel as soon as we start the MVC code for DNS. It make sense to move the model closer to the specific domain model, so A/AAAA split we will do then.

fichtner added a commit that referenced this pull request Jan 8, 2016
unbound: introducing overwriting of MX records
@fichtner fichtner merged commit a5ddf7f into opnsense:master Jan 8, 2016
@fichtner
Copy link
Member

fichtner commented Jan 8, 2016

In it went, thank you very much! :)

@8191 8191 deleted the unbound-overrides branch January 8, 2016 16:18
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.

2 participants