Update `change_date` field of records for MySQL and PostgreSQL #34

j-vizcaino · 2017-01-17T15:20:18Z

The change_date field must be updated in order for PowerDNS to be able to generate or regenerate
the SOA serial of the modified zone.

On create, new record gets its change_date field set to the current timestamp (ie. epoch).
On delete, update the change_date field for the SOA record of the associated domain.

Related to issue #22

The `change_date` field must be updated in order for PowerDNS to be able to generate or regenerate the SOA serial of the modified zone. * On create, new record gets its `change_date` field set to the current timestamp (ie. epoch). * On delete, update the `change_date` field for the SOA record of the associated domain.

ekohl

Overall it's looking very good.

ekohl · 2017-01-17T15:31:57Z

lib/smart_proxy_dns_powerdns/backend/mysql.rb

+      ret = connection.affected_rows >= 1
+      if ret
+        connection.query("UPDATE records SET change_date=UNIX_TIMESTAMP() WHERE domain_id=#{domain_id} AND type='SOA'")
+        ret = connection.affected_rows == 1


I think if this is not 1 then it should a log message. Some cases I can imagine:

There is no SOA record: 0 affected rows --> problem)

There are multiple SOA records: > 1 affected rows --> problem)

The change date is already set to the current unix timestamp: 0 affected rows --> race condition, might be a warning but possible problem

In any of those cases it shouldn't affect the return value since a rectify-zone might still be needed in race condition case.

The same goes for the postgresql backend.

You are right, logging would definitely help! The thing is I am not a Rubyist and don't know much about the log interface of the SmartProxy. Any advice would be appreciated.

Also, I think the README needs to be updated to guide to the SOA-EDIT properties defined in PowerDNS. I honestly totally missed this part of the PDNS guide at first and this is a required setting in order to have a working, auto-incremented SOA serial.

Regarding your last usecase I don't think this is true: update will change the SOA record even if the current change_date is already set. My guess is that the backend will still return affected_rows == 1 is this case because nothing in the SQL statement filters the SOA records with "up to date" change_date field.

Another question I have is: do we need to fail hard when update of the SOA fails?
My guess would be yes because usually, if the SOA serial does not get bumped, no other DNS slaves get notified. Also, forwarders would not catch up on changes until the record TTL has expired (since no change in SOA serial means no change to the zone content).

Maybe raising a proper exception with plenty of information would be the best way to go.
All of this would need to be run inside a transaction though because this could lead to the following bad scenario otherwise:

delete record → success

update SOA → fails, raise an exception

user tries again

delete record →no record deleted → does not update SOA :(

You are right, logging would definitely help! The thing is I am not a Rubyist and don't know much about the log interface of the SmartProxy. Any advice would be appreciated.

I'm not a Rubyist either so I'm doing the best I can, but the plugin logs very little. Currently there is only one example in this code base:

smart_proxy_dns_powerdns/lib/smart_proxy_dns_powerdns/backend/rest.rb

Line 87 in 566cdec

logger.debug "Failed to pach records for #{domain_id} with '#{rrset}': #{response.body}"

and I'm not sure the extends is needed there since the main class also does that so first try without it.

Also, I think the README needs to be updated to guide to the SOA-EDIT properties defined in PowerDNS. I honestly totally missed this part of the PDNS guide at first and this is a required setting in order to have a working, auto-incremented SOA serial.

Absolutely.

Regarding your last usecase I don't think this is true: update will change the SOA record even if the current change_date is already set. My guess is that the backend will still return affected_rows == 1 is this case because nothing in the SQL statement filters the SOA records with "up to date" change_date field.

This is not true for MySQL. An UPDATE statement will set affected_rows to the number of rows that changed. If the column already contains the value then it will not increase the affected_rows. Since unix timestamps have a resolution of 1 second there's a decent chance this will happen.

Another question I have is: do we need to fail hard when update of the SOA fails?

That was my initial thinking as well, but the race condition means there's a valid use case where there is no update and we can't really detect. I'm not sure how to solve that.

This is not true for MySQL. An UPDATE statement will set affected_rows to the number of rows that changed. If the column already contains the value then it will not increase the affected_rows. Since unix timestamps have a resolution of 1 second there's a decent chance this will happen.

EDIT: it seems PostgreSQL exhibits the same behavior.

This is really bad news :( We will have to discard the return value of this call then because there is a high chance of this happening when Foreman deletes multiple hosts of the same domain.
Also I am wondering if PowerDNS can serve a zone without SOA record anyway so the problem can be reduced to the "already up to date" race stated above.

Should I discard the return value of the UPDATE statement then?
Maybe, at least issuing a warning would be better than silently ignoring the result.

Should I discard the return value of the UPDATE statement then?
Maybe, at least issuing a warning would be better than silently ignoring the result.

I think it should be an info message. The race condition is quite possible and I'd rather not have false positive warnings. The other cases aren't that likely and I suspect you'll notice the breakage in other ways. If there is no SOA then PowerDNS won't serve the zone and no idea what happens when you have multiple SOA records. Maybe that should be a warning.

ekohl · 2017-01-17T16:59:05Z

Another complication is that strictly speaking it doesn't have to be unix timestamp, just a value that fits in the serial field. It may be enough to document that we do assume it to be and handle it that way though.

* When no SOA record is updated, an info message gets logged * When multiple SOA records are updated, a warning gets logged

j-vizcaino · 2017-01-18T10:20:17Z

Updated the PR with logging + tests.
I need to do more testing regarding the autoserial feature because it is not working on my installation. Once I get this to work, I will update the README with the instructions.

j-vizcaino · 2017-01-18T14:54:30Z

All done. Ready for review.

ekohl

I think it's looking very good. On Friday I'll be back home and have access to a proper test setup. After I've done some testing myself I'll likely merge this as is.

ekohl

Took me a bit longer to get to testing, but I've verified this on MySQL.

Given this builds in a new assumption I consider this a backwards incompatible change. That means I want to release this as 0.4.0 and only build it in nightly for now. Would that be suitable for you?

j-vizcaino · 2017-01-23T15:52:15Z

Yes that's no problem at all. I am currently looking into moving our setup towards using the REST API so there is no rush on my side.

ekohl · 2017-01-23T16:17:34Z

Thanks a lot for you contribution!

ekohl requested changes Jan 17, 2017

View reviewed changes

Log a message on SOA update failure

2fbb46d

* When no SOA record is updated, an info message gets logged * When multiple SOA records are updated, a warning gets logged

README: document SOA autoserial with SQL backends

058bc76

ekohl reviewed Jan 18, 2017

View reviewed changes

ekohl approved these changes Jan 23, 2017

View reviewed changes

ekohl merged commit 3560299 into theforeman:master Jan 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `change_date` field of records for MySQL and PostgreSQL #34

Update `change_date` field of records for MySQL and PostgreSQL #34

j-vizcaino commented Jan 17, 2017

ekohl left a comment

ekohl Jan 17, 2017

j-vizcaino Jan 17, 2017 •

edited

Loading

j-vizcaino Jan 17, 2017 •

edited

Loading

ekohl Jan 17, 2017

ekohl Jan 17, 2017

j-vizcaino Jan 17, 2017 •

edited

Loading

ekohl Jan 17, 2017

ekohl commented Jan 17, 2017

j-vizcaino commented Jan 18, 2017

j-vizcaino commented Jan 18, 2017

ekohl left a comment

ekohl left a comment

j-vizcaino commented Jan 23, 2017

ekohl commented Jan 23, 2017

Update change_date field of records for MySQL and PostgreSQL #34

Update change_date field of records for MySQL and PostgreSQL #34

Conversation

j-vizcaino commented Jan 17, 2017

ekohl left a comment

Choose a reason for hiding this comment

ekohl Jan 17, 2017

Choose a reason for hiding this comment

j-vizcaino Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

j-vizcaino Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

ekohl Jan 17, 2017

Choose a reason for hiding this comment

ekohl Jan 17, 2017

Choose a reason for hiding this comment

j-vizcaino Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

ekohl Jan 17, 2017

Choose a reason for hiding this comment

ekohl commented Jan 17, 2017

j-vizcaino commented Jan 18, 2017

j-vizcaino commented Jan 18, 2017

ekohl left a comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

j-vizcaino commented Jan 23, 2017

ekohl commented Jan 23, 2017

Update `change_date` field of records for MySQL and PostgreSQL #34

Update `change_date` field of records for MySQL and PostgreSQL #34

j-vizcaino Jan 17, 2017 •

edited

Loading

j-vizcaino Jan 17, 2017 •

edited

Loading

j-vizcaino Jan 17, 2017 •

edited

Loading