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

The DNS subsystem should manage keeping dnsmasq in sync #16740

Merged

Conversation

smarterclayton
Copy link
Contributor

On startup the DNS server will start a background subsystem that uses
DBus to keep dnsmasq pointing in-addr.arpa and cluster.local at the
local DNS server. If the configuration does not allow that (dnsmasq can
only talk to servers on port 53, for example) then DBus will not be
synced. The subsystem listens for dnsmasq restart events and applies
configuration, so no on-disk configuration should be necessary.

If the subsystem cannot talk to dbus the monitor routine is not
configured.

@sdodson with this change I don't believe any other dnsmasq config is
necessary. The process manages registration on startup, refreshes periodically, and detects restarts.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 7, 2017
@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 7, 2017
@smarterclayton smarterclayton assigned sdodson and unassigned giuseppe and soltysh Oct 8, 2017
@giuseppe
Copy link
Member

giuseppe commented Oct 8, 2017

the change to the system container file LGTM.

@sdodson
Copy link
Member

sdodson commented Oct 9, 2017

What's dnsIP going to be set to? I see it in the test but I'm worried that it'd be dnsIP from node config otherwise which is generally configured to the default interface which would conflict with where dnsmasq is bound.

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Oct 9, 2017 via email

@sdodson
Copy link
Member

sdodson commented Oct 9, 2017

Ok, today we set it to the node's default interface because you can't send dns traffic from pods to 127.0.0.1 and have it hit dnsmasq running on the node. So dnsIP would point at dnsmasq and you'd be programming dnsmasq to send cluster.local and in-addr.arpa to itself.

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Oct 9, 2017 via email

@michaelgugino
Copy link
Contributor

We're planning on using --bind-dynamic for dnsmasq.

http://www.thekelleys.org.uk/dnsmasq/docs/dnsmasq-man.html

bind-dynamic doesn't bind to 0.0.0.0. It binds to individual interfaces, dynamically, except for the exlcuded interfaces.

@smarterclayton
Copy link
Contributor Author

What change is required

@michaelgugino
Copy link
Contributor

@smarterclayton I looked over the 0.0.0.0 portions in this commit. It looks like it will work just fine with bind-dynamic, thanks.

@smarterclayton
Copy link
Contributor Author

@openshift/sig-networking any comments on this?

@smarterclayton
Copy link
Contributor Author

/retest

1 similar comment
@smarterclayton
Copy link
Contributor Author

/retest

On startup the DNS server will start a background subsystem that uses
DBus to keep dnsmasq pointing in-addr.arpa and cluster.local at the
local DNS server. If the configuration does not allow that (dnsmasq can
only talk to servers on port 53, for example) then DBus will not be
synced. The subsystem listens for dnsmasq restart events and applies
configuration, so no on-disk configuration should be necessary.

If the subsystem cannot talk to dbus the monitor routine is not
configured.
@smarterclayton
Copy link
Contributor Author

Silence is consent, discussed in person with a few people.

@smarterclayton smarterclayton added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2017
@sdodson
Copy link
Member

sdodson commented Oct 11, 2017

/lgtm

@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sdodson, smarterclayton

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 16725, 16779, 16798, 16783, 16740).

@openshift-merge-robot openshift-merge-robot merged commit 08889c7 into openshift:master Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants