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

desginate: Add mdns as hidden master #2105

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

Conversation

sjamgade
Copy link
Contributor

in crowbar's world the dns-master is the master of all and slaves
forward all queries to dns-master. Under such a configuration there is
no need to query other nameservers as they still forward the query to
dns-master. So designate can just verify on one nameserver(dns-master)
and dns-master will take care of passing that info to all slaves.

Same goes for also_notifies: dns-master will notify all slaves in case
of either zone or recordset is updated/deleted.

this also further simplifies the designate pool config reducing the
time required to create zone and recordsets.

This change is not really required but only eases out the config from
an admin perspective.

Also having multiple nameservers confuses designate in some times as
according to these nameserver designate is not authoritative of these
zones and recordsets.

jgrassler added a commit to jgrassler/crowbar-openstack that referenced this pull request Apr 15, 2019
This reverts commit 64e7ad9.

We've are very close to the release and have multiple pull requests still open
for Designate:

* crowbar#2095
* crowbar#2104
* crowbar#2105
* SUSE-Cloud/doc-cloud#931

There are way too many question marks on this so I am
switching the barclamp back to invisible mode. Lets get these
pull requests squared away carefully and without the rush of
imminent release.
Copy link
Contributor

@cmurphy cmurphy left a comment

Choose a reason for hiding this comment

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

This change is not really required but only eases out the config from
an admin perspective.

If this isn't critical maybe it could wait till the MU?

@@ -36,12 +34,12 @@
# non-hardcoded is high enough
pools = [{
"name" => "default-bind",
"description" => "Default BIND9 Pool",
"description" => "Sample Pool for designate (relies only 1 dns-master)",
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand this correctly, the user doesn't have control over this from the barclamp side so they are stuck with this "sample" in their configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is just a sample file, the user has to edit this file and inject this pool in designate

Copy link
Contributor

Choose a reason for hiding this comment

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

How can the user edit this file if it's being controlled by crowbar? Chef will just overwrite it.

Copy link
Contributor

@dirkmueller dirkmueller Apr 16, 2019

Choose a reason for hiding this comment

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

you can create your own pool rather than the default pool. I think the tricky part here is that you would need to change the pool to use in the designate config. that isn't done yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can the user edit this file if it's being controlled by crowbar? Chef will just overwrite it.

Chef will overwrite file /etc/designate/pools.crowbar.yaml designate relies on /etc/designate/pools.yaml so the user can overwrite the real file or just pass that config to the designate with command documented here SUSE-Cloud/doc-cloud#909

@sjamgade
Copy link
Contributor Author

This code change has no impact on any part of crowbar apart from changing the sample file. But helps greatly in trying to understand what this file really means.

@sjamgade sjamgade requested a review from jgrassler April 16, 2019 14:19
@sjamgade
Copy link
Contributor Author

╰─ irb
irb(main):001:0> dnsmaster='123.123.123.123'
=> "123.123.123.123"
irb(main):002:0> dns = {:fqdn => 'me.exmaple.com' }
=> {:fqdn=>"me.exmaple.com"}
irb(main):003:0> network_settings = {:mdns_bind_host  => dnsmaster }
=> {:mdns_bind_host=>"123.123.123.123"}
irb(main):004:0> pools = [{
irb(main):005:2*   "name" => "default-bind",
irb(main):006:2*   "description" => "Sample Pool for designate (relies only 1 dns-master)",
irb(main):007:2*   "id" => "794ccc2c-d751-44fe-b57f-8894c9f5c842",
irb(main):008:2*   "attributes" => {},
irb(main):009:2*   "ns_records" => [{ "hostname" => "#{dns[:fqdn]}.", "priority" => 1 }],
irb(main):010:2*   "nameservers" => [{ "host" => dnsmaster, "port" => 53 }],
irb(main):011:2*   "also_notifies" => [],
irb(main):012:2*   "targets" => [{
irb(main):013:4*     "type" => "bind9",
irb(main):014:4*     "description" => "BIND9 Server 1",
irb(main):015:4*     "masters" => [{ "host" => network_settings[:mdns_bind_host], "port" => 5354 }],
irb(main):016:4*     "options" => {
irb(main):017:5*       "host" => dnsmaster,
irb(main):018:5*       "port" => 53,
irb(main):019:5*       "rndc_host" => dnsmaster,
irb(main):020:5*       "rndc_port" => 953,
irb(main):021:5*       "rndc_key_file" => "/etc/designate/rndc.key"
irb(main):022:5>     }
irb(main):023:4>   }]
irb(main):024:2> }]
=> [{"name"=>"default-bind", "description"=>"Sample Pool for designate (relies only 1 dns-master)", "id"=>"794ccc2c-d751-44fe-b57f-8894c9f5c842", "attributes"=>{}, "ns_records"=>[{"hostname"=>"me.exmaple.com.", "priority"=>1}], "nameservers"=>[{"host"=>"123.123.123.123", "port"=>53}], "also_notifies"=>[], "targets"=>[{"type"=>"bind9", "description"=>"BIND9 Server 1", "masters"=>[{"host"=>"123.123.123.123", "port"=>5354}], "options"=>{"host"=>"123.123.123.123", "port"=>53, "rndc_host"=>"123.123.123.123", "rndc_port"=>953, "rndc_key_file"=>"/etc/designate/rndc.key"}}]}]
(failed reverse-i-search)`': import^C
irb(main):025:0> require 'yaml'
=> true
irb(main):026:0> pools.to_yaml
=> "---\n- name: default-bind\n  description: Sample Pool for designate (relies only 1 dns-master)\n  id: 794ccc2c-d751-44fe-b57f-8894c9f5c842\n  attributes: {}\n  ns_records:\n  - hostname: me.exmaple.com.\n    priority: 1\n  nameservers:\n  - host: 123.123.123.123\n    port: 53\n  also_notifies: []\n  targets:\n  - type: bind9\n    description: BIND9 Server 1\n    masters:\n    - host: 123.123.123.123\n      port: 5354\n    options:\n      host: 123.123.123.123\n      port: 53\n      rndc_host: 123.123.123.123\n      rndc_port: 953\n      rndc_key_file: \"/etc/designate/rndc.key\"\n"

cmurphy
cmurphy previously approved these changes Apr 16, 2019
@sjamgade
Copy link
Contributor Author

sjamgade commented May 8, 2019

this change is now needed for HA, I have added comments to the code and update the commit message to reflect why this change is required.

guangyee
guangyee previously approved these changes May 8, 2019
Copy link
Contributor

@guangyee guangyee left a comment

Choose a reason for hiding this comment

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

code looks good

@sjamgade
Copy link
Contributor Author

sjamgade commented May 9, 2019

Rebased with HA fix

sjamgade pushed a commit to sjamgade/crowbar-openstack that referenced this pull request May 16, 2019
This reverts commit 64e7ad9.

We've are very close to the release and have multiple pull requests still open
for Designate:

* crowbar#2095
* crowbar#2104
* crowbar#2105
* SUSE-Cloud/doc-cloud#931

There are way too many question marks on this so I am
switching the barclamp back to invisible mode. Lets get these
pull requests squared away carefully and without the rush of
imminent release.
@sjamgade sjamgade force-pushed the rocky_designatesimplify branch from 3fe0370 to c61534d Compare May 20, 2019 18:09
cmurphy
cmurphy previously approved these changes May 29, 2019

network_settings = DesignateHelper.network_settings(node)
# hidden masters are designate-mdns services, in ha this service will be running on multiple
# hosts and any host can be asked to update the zone (when a recordset, corsspoding to a vm is
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to correct the typo here and in the commit message ("corsspoding")

network_settings = DesignateHelper.network_settings(node)
# hidden masters are designate-mdns services, in ha this service will be running on multiple
# hosts and any host can be asked to update the zone (when a recordset, corsspoding to a vm is
# created) on th real-master so all have to be listed as master in the pool.
Copy link
Contributor

Choose a reason for hiding this comment

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

and here ("th") and in the commit message

@sjamgade sjamgade force-pushed the rocky_designatesimplify branch 2 times, most recently from 400008a to 03a5d0b Compare July 3, 2019 09:48
jgrassler
jgrassler previously approved these changes Jul 3, 2019
Copy link
Contributor

@dirkmueller dirkmueller left a comment

Choose a reason for hiding this comment

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

Please at least make the list of nameservers expand all hosts again. Ideally the hostname ns_record would be configurable (as it needs to point to the public hostname), but thats not a new bug introduced here

# dns servers looks like.
# This pool id can be generated by in proposal, but this will change
# with every delete/create cycle of proposal. This might mess
# up the designate configuration. So the advantage of having
# non-hardcoded is high enough
pools = [{
"name" => "default-bind",
"description" => "Default BIND9 Pool",
"description" => "Sample Pool for designate",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? IMHO it should be the default pool, this matches the name line above.

# dns servers looks like.
# This pool id can be generated by in proposal, but this will change
# with every delete/create cycle of proposal. This might mess
# up the designate configuration. So the advantage of having
# non-hardcoded is high enough
pools = [{
"name" => "default-bind",
"description" => "Default BIND9 Pool",
"description" => "Sample Pool for designate",
"id" => "794ccc2c-d751-44fe-b57f-8894c9f5c842",
"attributes" => {},
"ns_records" => [{ "hostname" => "#{dns[:fqdn]}.", "priority" => 1 }],
Copy link
Contributor

Choose a reason for hiding this comment

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

This would need to be configurable in the barclamp.

"id" => "794ccc2c-d751-44fe-b57f-8894c9f5c842",
"attributes" => {},
"ns_records" => [{ "hostname" => "#{dns[:fqdn]}.", "priority" => 1 }],
"nameservers" => dnsservers.map { |ip| { "host" => ip, "port" => 53 } },
"also_notifies" => dnsslaves.map { |ip| { "host" => ip, "port" => 53 } },
"nameservers" => [{ "host" => dnsmaster, "port" => 53 }],
Copy link
Contributor

Choose a reason for hiding this comment

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

this part doesn't make a whole lot of sense to me. this way we have only one DNS server hosting the domain, with other words no High Availability (HA is a requirement, see https://jira.suse.com/browse/SOC-6361 ). We need to list all dns-server masters here, and ideally their public IP address (see SOC-9635 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nameservers is a list where designate will check if the zone and/or ns-record is created successfully, I have limited it to 1 server and that too dns-master. because ....

The DNS barclamp has only 1 dns-server as master. All others are slaves who just forward incoming request to that master (this how bind is configured). This dns-master stores all the zone and ns-records received from designate.

These slaves could also be part of nameservers where designate validates the creation of zones and ns-records, but that wasnt very useful since they are just going to forwards query to dns-master.

"nameservers" => dnsservers.map { |ip| { "host" => ip, "port" => 53 } },
"also_notifies" => dnsslaves.map { |ip| { "host" => ip, "port" => 53 } },
"nameservers" => [{ "host" => dnsmaster, "port" => 53 }],
"also_notifies" => [],
Copy link
Contributor

Choose a reason for hiding this comment

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

leaving this empty by default makes sense to me, however if this is integrated in another hidden master setup (like e.g. SUSE's internal DNS), you need to be able to add additional hosts here. ideally this would be a setting in the barclamp ui.


# the host with VIP will use that ip as the outgoing ip when connecting to the real-master
if node[:designate][:ha][:enabled]
hiddenmasters += [{ "host" => CrowbarPacemakerHelper.cluster_vip(node, "admin"), "port" => 5354 }]
Copy link
Contributor

Choose a reason for hiding this comment

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

this is already in the list above, right? just once via vip and once via the node ip. I don't understand why we need to add it twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

afaik Barclamp::Inventory.get_network_by_type does not return the VIP so the list has admin address,

Also iiuc the that is different from VIP, and the host with VIP will use that ip as the outgoing ip when connecting to the real-master.

@ritesh216
Copy link
Contributor

Addressing the open review comments in the next commit. Note, "nameservers" will be updated via SCRD-9636 to use public FQDNs.

@ritesh216 ritesh216 force-pushed the rocky_designatesimplify branch from 03a5d0b to 82ff9bb Compare August 16, 2019 20:45
@ritesh216 ritesh216 requested a review from dirkmueller August 16, 2019 20:48
@ritesh216 ritesh216 force-pushed the rocky_designatesimplify branch from 82ff9bb to 081e89f Compare August 16, 2019 20:54
@ritesh216 ritesh216 dismissed dirkmueller’s stale review August 16, 2019 21:55

comments addressed.

@ritesh216 ritesh216 force-pushed the rocky_designatesimplify branch 2 times, most recently from b90aeaf to 4757bda Compare August 21, 2019 22:21
@ritesh216 ritesh216 changed the title desginate: simplify sample pool desginate: Add mdns as hidden master Aug 21, 2019
Copy link
Contributor

@ritesh216 ritesh216 left a comment

Choose a reason for hiding this comment

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

WIP

In crowbar's world the dns-master is the master of all and slaves
forward all queries to dns-master. When designate is enabled,
designate-mdns service component(s) become the hidden master(s).

We leave 'also_notifies' empty. It can be configured by the users
via designate-manage utility as and when needed.

designate-mdns service components, in HA, will be
running on multiple hosts and any host can be asked to update a zone
on th real-master. We add the cluster vip as the hidden master in case
HA is enabled otherwise add all servers.
@ritesh216 ritesh216 force-pushed the rocky_designatesimplify branch from 4757bda to a8f2c73 Compare August 23, 2019 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

7 participants