Commit 713160c9 authored by Michal 'vorner' Vaner's avatar Michal 'vorner' Vaner
Browse files

Merge #1331

parents 84290dae e9e0f965
......@@ -272,6 +272,22 @@ public:
/// In such cases this method will result in an \c isc::NotImplemented
/// exception unconditionally or when \c replace is false).
///
/// If \c journaling is true, the data source should store a journal
/// of changes. These can be used later on by, for example, IXFR-out.
/// However, the parameter is a hint only. It might be unable to store
/// them and they would be silently discarded. Or it might need to
/// store them no matter what (for example a git-based data source would
/// store journal implicitly). When the \c journaling is true, it
/// requires that the following update be formatted as IXFR transfer
/// (SOA to be removed, bunch of RRs to be removed, SOA to be added,
/// bunch of RRs to be added, and possibly repeated). However, it is not
/// required that the updater checks that. If it is false, it must not
/// require so and must accept any order of changes.
///
/// We don't support erasing the whole zone (by replace being true) and
/// saving a journal at the same time. In such situation, BadValue is
/// thrown.
///
/// \note To avoid throwing the exception accidentally with a lazy
/// implementation, we still keep this method pure virtual without
/// an implementation. All derived classes must explicitly define this
......@@ -282,14 +298,18 @@ public:
/// \exception DataSourceError Internal error in the underlying data
/// source.
/// \exception std::bad_alloc Resource allocation failure.
/// \exception BadValue if both replace and journaling are true.
///
/// \param name The zone name to be updated
/// \param replace Whether to delete existing RRs before making updates
/// \param journaling The zone updater should store a journal of the
/// changes.
///
/// \return A pointer to the updater; it will be NULL if the specified
/// zone isn't found.
virtual ZoneUpdaterPtr getUpdater(const isc::dns::Name& name,
bool replace) const = 0;
bool replace, bool journaling = false)
const = 0;
};
}
}
......
......@@ -838,10 +838,12 @@ DatabaseClient::getIterator(const isc::dns::Name& name,
class DatabaseUpdater : public ZoneUpdater {
public:
DatabaseUpdater(shared_ptr<DatabaseAccessor> accessor, int zone_id,
const Name& zone_name, const RRClass& zone_class) :
const Name& zone_name, const RRClass& zone_class,
bool journaling) :
committed_(false), accessor_(accessor), zone_id_(zone_id),
db_name_(accessor->getDBName()), zone_name_(zone_name.toText()),
zone_class_(zone_class),
zone_class_(zone_class), journaling_(journaling),
diff_phase_(NOT_STARTED),
finder_(new DatabaseClient::Finder(accessor_, zone_id_, zone_name))
{
logger.debug(DBG_TRACE_DATA, DATASRC_DATABASE_UPDATER_CREATED)
......@@ -877,45 +879,97 @@ public:
virtual void commit();
private:
// A short cut typedef only for making the code shorter.
typedef DatabaseAccessor Accessor;
bool committed_;
shared_ptr<DatabaseAccessor> accessor_;
const int zone_id_;
const string db_name_;
const string zone_name_;
const RRClass zone_class_;
const bool journaling_;
// For the journals
enum DiffPhase {
NOT_STARTED,
DELETE,
ADD
};
DiffPhase diff_phase_;
uint32_t serial_;
boost::scoped_ptr<DatabaseClient::Finder> finder_;
// This is a set of validation checks commonly used for addRRset() and
// deleteRRset to minimize duplicate code logic and to make the main
// code concise.
void validateAddOrDelete(const char* const op_str, const RRset& rrset,
DiffPhase prev_phase,
DiffPhase current_phase) const;
};
void
DatabaseUpdater::addRRset(const RRset& rrset) {
DatabaseUpdater::validateAddOrDelete(const char* const op_str,
const RRset& rrset,
DiffPhase prev_phase,
DiffPhase current_phase) const
{
if (committed_) {
isc_throw(DataSourceError, "Add attempt after commit to zone: "
isc_throw(DataSourceError, op_str << " attempt after commit to zone: "
<< zone_name_ << "/" << zone_class_);
}
if (rrset.getRdataCount() == 0) {
isc_throw(DataSourceError, op_str << " attempt with an empty RRset: "
<< rrset.getName() << "/" << zone_class_ << "/"
<< rrset.getType());
}
if (rrset.getClass() != zone_class_) {
isc_throw(DataSourceError, "An RRset of a different class is being "
<< "added to " << zone_name_ << "/" << zone_class_ << ": "
isc_throw(DataSourceError, op_str << " attempt for a different class "
<< zone_name_ << "/" << zone_class_ << ": "
<< rrset.toText());
}
if (rrset.getRRsig()) {
isc_throw(DataSourceError, "An RRset with RRSIG is being added to "
isc_throw(DataSourceError, op_str << " attempt for RRset with RRSIG "
<< zone_name_ << "/" << zone_class_ << ": "
<< rrset.toText());
}
if (journaling_) {
const RRType rrtype(rrset.getType());
if (rrtype == RRType::SOA() && diff_phase_ != prev_phase) {
isc_throw(isc::BadValue, op_str << " attempt in an invalid "
<< "diff phase: " << diff_phase_ << ", rrset: " <<
rrset.toText());
}
if (rrtype != RRType::SOA() && diff_phase_ != current_phase) {
isc_throw(isc::BadValue, "diff state change by non SOA: "
<< rrset.toText());
}
}
}
void
DatabaseUpdater::addRRset(const RRset& rrset) {
validateAddOrDelete("add", rrset, DELETE, ADD);
// It's guaranteed rrset has at least one RDATA at this point.
RdataIteratorPtr it = rrset.getRdataIterator();
if (it->isLast()) {
isc_throw(DataSourceError, "An empty RRset is being added for "
<< rrset.getName() << "/" << zone_class_ << "/"
<< rrset.getType());
}
string columns[DatabaseAccessor::ADD_COLUMN_COUNT]; // initialized with ""
columns[DatabaseAccessor::ADD_NAME] = rrset.getName().toText();
columns[DatabaseAccessor::ADD_REV_NAME] =
rrset.getName().reverse().toText();
columns[DatabaseAccessor::ADD_TTL] = rrset.getTTL().toText();
columns[DatabaseAccessor::ADD_TYPE] = rrset.getType().toText();
string columns[Accessor::ADD_COLUMN_COUNT]; // initialized with ""
columns[Accessor::ADD_NAME] = rrset.getName().toText();
columns[Accessor::ADD_REV_NAME] = rrset.getName().reverse().toText();
columns[Accessor::ADD_TTL] = rrset.getTTL().toText();
columns[Accessor::ADD_TYPE] = rrset.getType().toText();
string journal[Accessor::DIFF_PARAM_COUNT];
if (journaling_) {
journal[Accessor::DIFF_NAME] = columns[Accessor::ADD_NAME];
journal[Accessor::DIFF_TYPE] = columns[Accessor::ADD_TYPE];
journal[Accessor::DIFF_TTL] = columns[Accessor::ADD_TTL];
diff_phase_ = ADD;
if (rrset.getType() == RRType::SOA()) {
serial_ =
dynamic_cast<const generic::SOA&>(it->getCurrent()).
getSerial();
}
}
for (; !it->isLast(); it->next()) {
if (rrset.getType() == RRType::RRSIG()) {
// XXX: the current interface (based on the current sqlite3
......@@ -925,43 +979,53 @@ DatabaseUpdater::addRRset(const RRset& rrset) {
// the interface, but until then we have to conform to the schema.
const generic::RRSIG& rrsig_rdata =
dynamic_cast<const generic::RRSIG&>(it->getCurrent());
columns[DatabaseAccessor::ADD_SIGTYPE] =
columns[Accessor::ADD_SIGTYPE] =
rrsig_rdata.typeCovered().toText();
}
columns[DatabaseAccessor::ADD_RDATA] = it->getCurrent().toText();
columns[Accessor::ADD_RDATA] = it->getCurrent().toText();
if (journaling_) {
journal[Accessor::DIFF_RDATA] = columns[Accessor::ADD_RDATA];
accessor_->addRecordDiff(zone_id_, serial_, Accessor::DIFF_ADD,
journal);
}
accessor_->addRecordToZone(columns);
}
}
void
DatabaseUpdater::deleteRRset(const RRset& rrset) {
if (committed_) {
isc_throw(DataSourceError, "Delete attempt after commit on zone: "
<< zone_name_ << "/" << zone_class_);
}
if (rrset.getClass() != zone_class_) {
isc_throw(DataSourceError, "An RRset of a different class is being "
<< "deleted from " << zone_name_ << "/" << zone_class_
<< ": " << rrset.toText());
}
if (rrset.getRRsig()) {
isc_throw(DataSourceError, "An RRset with RRSIG is being deleted from "
<< zone_name_ << "/" << zone_class_ << ": "
<< rrset.toText());
// If this is the first operation, pretend we are starting a new delete
// sequence after adds. This will simplify the validation below.
if (diff_phase_ == NOT_STARTED) {
diff_phase_ = ADD;
}
validateAddOrDelete("delete", rrset, ADD, DELETE);
RdataIteratorPtr it = rrset.getRdataIterator();
if (it->isLast()) {
isc_throw(DataSourceError, "An empty RRset is being deleted for "
<< rrset.getName() << "/" << zone_class_ << "/"
<< rrset.getType());
}
string params[DatabaseAccessor::DEL_PARAM_COUNT]; // initialized with ""
params[DatabaseAccessor::DEL_NAME] = rrset.getName().toText();
params[DatabaseAccessor::DEL_TYPE] = rrset.getType().toText();
string params[Accessor::DEL_PARAM_COUNT]; // initialized with ""
params[Accessor::DEL_NAME] = rrset.getName().toText();
params[Accessor::DEL_TYPE] = rrset.getType().toText();
string journal[Accessor::DIFF_PARAM_COUNT];
if (journaling_) {
journal[Accessor::DIFF_NAME] = params[Accessor::DEL_NAME];
journal[Accessor::DIFF_TYPE] = params[Accessor::DEL_TYPE];
journal[Accessor::DIFF_TTL] = rrset.getTTL().toText();
diff_phase_ = DELETE;
if (rrset.getType() == RRType::SOA()) {
serial_ =
dynamic_cast<const generic::SOA&>(it->getCurrent()).
getSerial();
}
}
for (; !it->isLast(); it->next()) {
params[DatabaseAccessor::DEL_RDATA] = it->getCurrent().toText();
params[Accessor::DEL_RDATA] = it->getCurrent().toText();
if (journaling_) {
journal[Accessor::DIFF_RDATA] = params[Accessor::DEL_RDATA];
accessor_->addRecordDiff(zone_id_, serial_, Accessor::DIFF_DELETE,
journal);
}
accessor_->deleteRecordInZone(params);
}
}
......@@ -973,6 +1037,9 @@ DatabaseUpdater::commit() {
<< zone_name_ << "/" << zone_class_ << " on "
<< db_name_);
}
if (journaling_ && diff_phase_ == DELETE) {
isc_throw(isc::BadValue, "Update sequence not complete");
}
accessor_->commit();
committed_ = true; // make sure the destructor won't trigger rollback
......@@ -986,7 +1053,13 @@ DatabaseUpdater::commit() {
// The updater factory
ZoneUpdaterPtr
DatabaseClient::getUpdater(const isc::dns::Name& name, bool replace) const {
DatabaseClient::getUpdater(const isc::dns::Name& name, bool replace,
bool journaling) const
{
if (replace && journaling) {
isc_throw(isc::BadValue, "Can't store journal and replace the whole "
"zone at the same time");
}
shared_ptr<DatabaseAccessor> update_accessor(accessor_->clone());
const std::pair<bool, int> zone(update_accessor->startUpdateZone(
name.toText(), replace));
......@@ -995,7 +1068,7 @@ DatabaseClient::getUpdater(const isc::dns::Name& name, bool replace) const {
}
return (ZoneUpdaterPtr(new DatabaseUpdater(update_accessor, zone.second,
name, rrclass_)));
name, rrclass_, journaling)));
}
}
}
......@@ -878,7 +878,8 @@ public:
/// accessor. The returned updater will be able to work separately from
/// the original client.
virtual ZoneUpdaterPtr getUpdater(const isc::dns::Name& name,
bool replace) const;
bool replace,
bool journaling = false) const;
private:
/// \brief The RR class that this client handles.
......
......@@ -815,7 +815,7 @@ InMemoryClient::getIterator(const Name& name, bool) const {
}
ZoneUpdaterPtr
InMemoryClient::getUpdater(const isc::dns::Name&, bool) const {
InMemoryClient::getUpdater(const isc::dns::Name&, bool, bool) const {
isc_throw(isc::NotImplemented, "Update attempt on in memory data source");
}
......
......@@ -284,7 +284,8 @@ public:
/// to update via its updater (this may or may not be a good idea and
/// is subject to further discussions).
virtual ZoneUpdaterPtr getUpdater(const isc::dns::Name& name,
bool replace) const;
bool replace, bool journaling = false)
const;
private:
// TODO: Do we still need the PImpl if nobody should manipulate this class
......
......@@ -32,7 +32,9 @@ public:
virtual FindResult findZone(const isc::dns::Name&) const {
return (FindResult(result::NOTFOUND, ZoneFinderPtr()));
}
virtual ZoneUpdaterPtr getUpdater(const isc::dns::Name&, bool) const {
virtual ZoneUpdaterPtr getUpdater(const isc::dns::Name&, bool, bool)
const
{
return (ZoneUpdaterPtr());
}
};
......
......@@ -13,6 +13,7 @@
// PERFORMANCE OF THIS SOFTWARE.
#include <boost/shared_ptr.hpp>
#include <boost/lexical_cast.hpp>
#include <gtest/gtest.h>
......@@ -37,6 +38,7 @@ using namespace std;
// for some systems.
using boost::shared_ptr;
using boost::dynamic_pointer_cast;
using boost::lexical_cast;
using namespace isc::dns;
namespace {
......@@ -264,6 +266,52 @@ private:
};
/**
* Single journal entry in the mock database.
*
* All the members there are public for simplicity, as it only stores data.
* We use the implicit constructor and operator. The members can't be const
* because of the assignment operator (used in the vectors).
*/
struct JournalEntry {
JournalEntry(int id, uint32_t serial,
DatabaseAccessor::DiffOperation operation,
const std::string (&data)[DatabaseAccessor::DIFF_PARAM_COUNT])
: id_(id), serial_(serial), operation_(operation)
{
data_[DatabaseAccessor::DIFF_NAME] = data[DatabaseAccessor::DIFF_NAME];
data_[DatabaseAccessor::DIFF_TYPE] = data[DatabaseAccessor::DIFF_TYPE];
data_[DatabaseAccessor::DIFF_TTL] = data[DatabaseAccessor::DIFF_TTL];
data_[DatabaseAccessor::DIFF_RDATA] =
data[DatabaseAccessor::DIFF_RDATA];
}
JournalEntry(int id, uint32_t serial,
DatabaseAccessor::DiffOperation operation,
const std::string& name, const std::string& type,
const std::string& ttl, const std::string& rdata):
id_(id), serial_(serial), operation_(operation)
{
data_[DatabaseAccessor::DIFF_NAME] = name;
data_[DatabaseAccessor::DIFF_TYPE] = type;
data_[DatabaseAccessor::DIFF_TTL] = ttl;
data_[DatabaseAccessor::DIFF_RDATA] = rdata;
}
int id_;
uint32_t serial_;
DatabaseAccessor::DiffOperation operation_;
std::string data_[DatabaseAccessor::DIFF_PARAM_COUNT];
bool operator==(const JournalEntry& other) const {
for (size_t i(0); i < DatabaseAccessor::DIFF_PARAM_COUNT; ++ i) {
if (data_[i] != other.data_[i]) {
return false;
}
}
// No need to check data here, checked above
return (id_ == other.id_ && serial_ == other.serial_ &&
operation_ == other.operation_);
}
};
/*
* A virtual database accessor that pretends it contains single zone --
* example.org.
......@@ -288,6 +336,7 @@ public:
readonly_records_ = &readonly_records_master_;
update_records_ = &update_records_master_;
empty_records_ = &empty_records_master_;
journal_entries_ = &journal_entries_master_;
fillData();
}
......@@ -296,6 +345,7 @@ public:
cloned_accessor->readonly_records_ = &readonly_records_master_;
cloned_accessor->update_records_ = &update_records_master_;
cloned_accessor->empty_records_ = &empty_records_master_;
cloned_accessor->journal_entries_ = &journal_entries_master_;
latest_clone_ = cloned_accessor;
return (cloned_accessor);
}
......@@ -544,7 +594,13 @@ public:
*update_records_ = *readonly_records_;
}
return (pair<bool, int>(true, WRITABLE_ZONE_ID));
if (zone_name == "bad.example.org.") {
return (pair<bool, int>(true, -1));
} else if (zone_name == "null.example.org.") {
return (pair<bool, int>(true, 13));
} else {
return (pair<bool, int>(true, WRITABLE_ZONE_ID));
}
}
virtual void commit() {
*readonly_records_ = *update_records_;
......@@ -658,6 +714,30 @@ public:
isc_throw(isc::Unexpected, "Unknown zone ID");
}
}
virtual void addRecordDiff(int id, uint32_t serial,
DiffOperation operation,
const std::string (&data)[DIFF_PARAM_COUNT])
{
if (id == 13) { // The null zone doesn't support journaling
isc_throw(isc::NotImplemented, "Test not implemented behaviour");
} else if (id == -1) { // Bad zone throws
isc_throw(DataSourceError, "Test error");
} else {
journal_entries_->push_back(JournalEntry(id, serial, operation,
data));
}
}
// Check the journal is as expected and clear the journal
void checkJournal(const std::vector<JournalEntry> &expected) {
std::vector<JournalEntry> journal;
// Clean the journal, but keep local copy to check
journal.swap(*journal_entries_);
ASSERT_EQ(expected.size(), journal.size());
for (size_t i(0); i < expected.size(); ++ i) {
EXPECT_TRUE(expected[i] == journal[i]);
}
}
private:
// The following member variables are storage and/or update work space
......@@ -677,6 +757,10 @@ private:
const Domains empty_records_master_;
const Domains* empty_records_;
// The journal data
std::vector<JournalEntry> journal_entries_master_;
std::vector<JournalEntry>* journal_entries_;
// used as temporary storage after searchForRecord() and during
// getNextRecord() calls, as well as during the building of the
// fake data
......@@ -794,6 +878,10 @@ public:
rrset_.reset(new RRset(qname_, qclass_, qtype_, rrttl_));
rrset_->addRdata(rdata::createRdata(rrset_->getType(),
rrset_->getClass(), "192.0.2.2"));
soa_.reset(new RRset(zname_, qclass_, RRType::SOA(), rrttl_));
soa_->addRdata(rdata::createRdata(soa_->getType(), soa_->getClass(),
"ns1.example.org. admin.example.org. "
"1234 3600 1800 2419200 7200"));
// And its RRSIG. Also different from the configured one.
rrsigset_.reset(new RRset(qname_, qclass_, RRType::RRSIG(),
......@@ -895,6 +983,7 @@ public:
const RRTTL rrttl_; // commonly used RR TTL
RRsetPtr rrset_; // for adding/deleting an RRset
RRsetPtr rrsigset_; // for adding/deleting an RRset
RRsetPtr soa_; // for adding/deleting an RRset
// update related objects to be tested
ZoneUpdaterPtr updater_;
......@@ -2703,4 +2792,181 @@ TEST_F(MockDatabaseClientTest, badName) {
DataSourceError);
}
/*
* Test correct use of the updater with a journal.
*/
TEST_F(MockDatabaseClientTest, journal) {
updater_ = client_->getUpdater(zname_, false, true);
updater_->deleteRRset(*soa_);
updater_->deleteRRset(*rrset_);
soa_.reset(new RRset(zname_, qclass_, RRType::SOA(), rrttl_));
soa_->addRdata(rdata::createRdata(soa_->getType(), soa_->getClass(),
"ns1.example.org. admin.example.org. "
"1235 3600 1800 2419200 7200"));
updater_->addRRset(*soa_);
updater_->addRRset(*rrset_);
ASSERT_NO_THROW(updater_->commit());
std::vector<JournalEntry> expected;
expected.push_back(JournalEntry(WRITABLE_ZONE_ID, 1234,
DatabaseAccessor::DIFF_DELETE,
"example.org.", "SOA", "3600",
"ns1.example.org. admin.example.org. "
"1234 3600 1800 2419200 7200"));
expected.push_back(JournalEntry(WRITABLE_ZONE_ID, 1234,
DatabaseAccessor::DIFF_DELETE,
"www.example.org.", "A", "3600",
"192.0.2.2"));
expected.push_back(JournalEntry(WRITABLE_ZONE_ID, 1235,
DatabaseAccessor::DIFF_ADD,
"example.org.", "SOA", "3600",
"ns1.example.org. admin.example.org. "
"1235 3600 1800 2419200 7200"));
expected.push_back(JournalEntry(WRITABLE_ZONE_ID, 1235,
DatabaseAccessor::DIFF_ADD,
"www.example.org.", "A", "3600",
"192.0.2.2"));
current_accessor_->checkJournal(expected);
}
/*
* Push multiple delete-add sequences. Checks it is allowed and all is
* saved.
*/
TEST_F(MockDatabaseClientTest, journalMultiple) {
std::vector<JournalEntry> expected;
updater_ = client_->getUpdater(zname_, false, true);
std::string soa_rdata = "ns1.example.org. admin.example.org. "
"1234 3600 1800 2419200 7200";
for (size_t i(1); i < 100; ++ i) {
// Remove the old SOA
updater_->deleteRRset(*soa_);
expected.push_back(JournalEntry(WRITABLE_ZONE_ID, 1234 + i - 1,
DatabaseAccessor::DIFF_DELETE,
"example.org.", "SOA", "3600",
soa_rdata));
// Create a new SOA
soa_rdata = "ns1.example.org. admin.example.org. " +
lexical_cast<std::string>(1234 + i) + " 3600 1800 2419200 7200";
soa_.reset(new RRset(zname_, qclass_, RRType::SOA(), rrttl_));
soa_->addRdata(rdata::createRdata(soa_->getType(), soa_->getClass(),
soa_rdata));
// Add the new SOA
updater_->addRRset(*soa_);
expected.push_back(JournalEntry(WRITABLE_ZONE_ID, 1234 + i,
DatabaseAccessor::DIFF_ADD,
"example.org.", "SOA", "3600",
soa_rdata));
}
ASSERT_NO_THROW(updater_->commit());
// Check the journal contains everything.
current_accessor_->checkJournal(expected);
}
/*
* Test passing a forbidden sequence to it and expect it to throw.
*
* Note that we implicitly test in different testcases (these for add and
* delete) that if the journaling is false, it doesn't expect the order.
*/
TEST_F(MockDatabaseClientTest, journalBadSequence) {
std::vector<JournalEntry> expected;
{
SCOPED_TRACE("Delete A before SOA");
updater_ = client_->getUpdater(zname_, false, true);
EXPECT_THROW(updater_->deleteRRset(*rrset_), isc::BadValue);
// Make sure the journal is empty now
current_accessor_->checkJournal(expected);
}
{
SCOPED_TRACE("Add before delete");
updater_ = client_->getUpdater(zname_, false, true);
EXPECT_THROW(updater_->addRRset(*soa_), isc::BadValue);
// Make sure the journal is empty now
current_accessor_->checkJournal(expected);
}
{
SCOPED_TRACE("Add A before SOA");
updater_ = client_->getUpdater(zname_, false, true);
// So far OK
EXPECT_NO_THROW(updater_->deleteRRset(*soa_));
// But we miss the add SOA here
EXPECT_THROW(updater_->addRRset(*rrset_), isc::BadValue);
// Make sure the journal contains only the first one
expected.push_back(JournalEntry(WRITABLE_ZONE_ID, 1234,
DatabaseAccessor::DIFF_DELETE,
"example.org.", "SOA", "3600",
"ns1.example.org. admin.example.org. "
"1234 3600 1800 2419200 7200"));
current_accessor_->checkJournal(expected);
}
{
SCOPED_TRACE("Commit before add");
updater_ = client_->getUpdater(zname_, false, true);
// So far OK
EXPECT_NO_THROW(updater_->deleteRRset(*soa_));