Commit 5b0a2957 authored by Thomas Markwalder's avatar Thomas Markwalder
Browse files

[3007] Addressed review comments.

parent b4b1eca7
......@@ -28,91 +28,24 @@ using namespace boost::posix_time;
D2Dhcid::D2Dhcid() {
}
D2Dhcid::~D2Dhcid() {
}
D2Dhcid::D2Dhcid(const std::string& data) {
fromStr(data);
}
void
D2Dhcid::fromStr(const std::string& data) {
const char* buf = data.c_str();
size_t len = data.size();
// String can't be empty and must be an even number of characters.
if (len == 0 || ((len % 2) > 0)) {
isc_throw(NcrMessageError,
"String to byte conversion, invalid data length: " << len);
}
// Iterate over the string of character "digits", combining each pair
// into an unsigned byte and then add the byte to our byte vector.
// This implementation may seem verbose but it is very quick and more
// importantly provides data validation.
bytes_.clear();
for (int i = 0; i < len; i++) {
char ch = buf[i++];
if (ch >= 0x30 && ch <= 0x39) {
// '0' to '9'
ch -= 0x30;
} else if (ch >= 0x41 && ch <= 0x46) {
// 'A' to 'F'
ch -= 0x37;
} else {
// Not a digit, so throw an error.
isc_throw(NcrMessageError, "Invalid data in Dhcid");
}
// Set the upper nibble digit.
uint8_t byte = ch << 4;
ch = buf[i];
if (ch >= 0x30 && ch <= 0x39) {
// '0' to '9'
ch -= 0x30;
} else if (ch >= 0x41 && ch <= 0x46) {
// 'A' to 'F'
ch -= 0x37;
} else {
// Not a digit, so throw an error.
isc_throw(NcrMessageError, "Invalid data in Dhcid");
}
// "OR" in the lower nibble digit.
byte |= ch;
// Add the new byte to the end of the vector.
bytes_.push_back(byte);
try {
isc::util::encode::decodeHex(data, bytes_);
} catch (const isc::Exception& ex) {
isc_throw(NcrMessageError, "Invalid data in Dhcid:" << ex.what());
}
}
std::string
D2Dhcid::toStr() const {
int len = bytes_.size();
char tmp[len+1];
char* chptr = tmp;
unsigned char* bptr = const_cast<unsigned char*>(bytes_.data());
// Iterate over the vector of bytes, converting them into a contiguous
// string of ASCII hexadecimal digits, '0' - '9' and 'A' to 'F'.
// Each byte is split into a pair of digits.
for (int i = 0; i < len; i++) {
uint8_t byte = *bptr++;
char ch = (byte >> 4);
// Turn upper nibble into a digit and append it to char buf.
ch += (ch < 0x0A ? 0x30 : 0x37);
*chptr++ = ch;
// Turn lower nibble into a digit and append it to char buf.
ch = (byte & 0x0F);
ch += (ch < 0x0A ? 0x30 : 0x37);
*chptr++ = ch;
}
return (isc::util::encode::encodeHex(bytes_));
// Null terminate it.
*chptr = 0x0;
return (std::string(tmp));
}
......@@ -120,9 +53,9 @@ D2Dhcid::toStr() const {
/**************************** NameChangeRequest ******************************/
NameChangeRequest::NameChangeRequest()
: change_type_(chgAdd), forward_change_(false),
: change_type_(CHG_ADD), forward_change_(false),
reverse_change_(false), fqdn_(""), ip_address_(""),
dhcid_(), lease_expires_on_(), lease_length_(0), status_(stNew) {
dhcid_(), lease_expires_on_(), lease_length_(0), status_(ST_NEW) {
}
NameChangeRequest::NameChangeRequest(NameChangeType change_type,
......@@ -133,35 +66,31 @@ NameChangeRequest::NameChangeRequest(NameChangeType change_type,
: change_type_(change_type), forward_change_(forward_change),
reverse_change_(reverse_change), fqdn_(fqdn), ip_address_(ip_address),
dhcid_(dhcid), lease_expires_on_(new ptime(lease_expires_on)),
lease_length_(lease_length), status_(stNew) {
lease_length_(lease_length), status_(ST_NEW) {
// Validate the contents. This will throw a NcrMessageError if anything
// is invalid.
validateContent();
}
NameChangeRequest::~NameChangeRequest() {
}
NameChangeRequestPtr
NameChangeRequest::fromFormat(NameChangeFormat format,
isc::util::InputBuffer& buffer) {
// Based on the format requested, pull the marshalled request from
// InputBuffer and pass it into the appropriate format-specific factory.
NameChangeRequestPtr ncr;
switch (format)
{
case fmtJSON: {
switch (format) {
case FMT_JSON: {
try {
// Get the length of the JSON text and create a char buf large
// enough to hold it + 1.
// Get the length of the JSON text.
size_t len = buffer.readUint16();
char string_data[len+1];
// Read len bytes of data from the InputBuffer into local char buf
// and then NULL terminate it.
buffer.readData(&string_data, len);
string_data[len] = 0x0;
// Read the text from the buffer into a vector.
std::vector<uint8_t> vec;
buffer.readVector(vec, len);
// Turn the vector into a string.
std::string string_data(vec.begin(), vec.end());
// Pass the string of JSON text into JSON factory to create the
// NameChangeRequest instance. Note the factory may throw
......@@ -186,12 +115,12 @@ NameChangeRequest::fromFormat(NameChangeFormat format,
void
NameChangeRequest::toFormat(NameChangeFormat format,
isc::util::OutputBuffer& buffer) {
isc::util::OutputBuffer& buffer) const {
// Based on the format requested, invoke the appropriate format handler
// which will marshal this request's contents into the OutputBuffer.
switch (format)
{
case fmtJSON: {
case FMT_JSON: {
// Invoke toJSON to create a JSON text of this request's contents.
std::string json = toJSON();
uint16_t length = json.size();
......@@ -271,22 +200,22 @@ NameChangeRequest::fromJSON(const std::string& json) {
}
std::string
NameChangeRequest::toJSON() {
NameChangeRequest::toJSON() const {
// Create a JSON string of this request's contents. Note that this method
// does NOT use the isc::data library as generating the output is straight
// forward.
std::ostringstream stream;
stream << "{\"change_type\":" << change_type_ << ","
stream << "{\"change_type\":" << getChangeType() << ","
<< "\"forward_change\":"
<< (forward_change_ ? "true" : "false") << ","
<< (isForwardChange() ? "true" : "false") << ","
<< "\"reverse_change\":"
<< (reverse_change_ ? "true" : "false") << ","
<< "\"fqdn\":\"" << fqdn_ << "\","
<< "\"ip_address\":\"" << ip_address_ << "\","
<< "\"dhcid\":\"" << dhcid_.toStr() << "\","
<< (isReverseChange() ? "true" : "false") << ","
<< "\"fqdn\":\"" << getFqdn() << "\","
<< "\"ip_address\":\"" << getIpAddress() << "\","
<< "\"dhcid\":\"" << getDhcid().toStr() << "\","
<< "\"lease_expires_on\":\"" << getLeaseExpiresOnStr() << "\","
<< "\"lease_length\":" << lease_length_ << "}";
<< "\"lease_length\":" << getLeaseLength() << "}";
return (stream.str());
}
......@@ -294,7 +223,9 @@ NameChangeRequest::toJSON() {
void
NameChangeRequest::validateContent() {
// Validate FQDN.
//@TODO This is an initial implementation which provides a minimal amount
// of validation. FQDN, DHCID, and IP Address members are all currently
// strings, these may be replaced with richer classes.
if (fqdn_ == "") {
isc_throw(NcrMessageError, "FQDN cannot be blank");
}
......@@ -326,7 +257,7 @@ NameChangeRequest::validateContent() {
isc::data::ConstElementPtr
NameChangeRequest::getElement(const std::string& name,
const ElementMap& element_map) {
const ElementMap& element_map) const {
// Look for "name" in the element map.
ElementMap::const_iterator it = element_map.find(name);
if (it == element_map.end()) {
......@@ -357,7 +288,7 @@ NameChangeRequest::setChangeType(isc::data::ConstElementPtr element) {
"Wrong data type for change_type: " << ex.what());
}
if (raw_value != chgAdd && raw_value != chgRemove) {
if ((raw_value != CHG_ADD) && (raw_value != CHG_REMOVE)) {
// Value is not a valid change type.
isc_throw(NcrMessageError,
"Invalid data value for change_type: " << raw_value);
......@@ -524,11 +455,11 @@ NameChangeRequest::toText() const {
stream << "Type: " << static_cast<int>(change_type_) << " (";
switch (change_type_) {
case chgAdd:
stream << "chgAdd)\n";
case CHG_ADD:
stream << "CHG_ADD)\n";
break;
case chgRemove:
stream << "chgRemove)\n";
case CHG_REMOVE:
stream << "CHG_REMOVE)\n";
break;
default:
// Shouldn't be possible.
......
......@@ -12,8 +12,8 @@
// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
// PERFORMANCE OF THIS SOFTWARE.
#ifndef _NCR_MSG_H
#define _NCR_MSG_H
#ifndef NCR_MSG_H
#define NCR_MSG_H
/// @file ncr_msg.h
/// @brief This file provides the classes needed to embody, compose, and
......@@ -23,6 +23,7 @@
#include <cc/data.h>
#include <exceptions/exceptions.h>
#include <util/buffer.h>
#include <util/encode/hex.h>
#include <boost/date_time/posix_time/posix_time.hpp>
......@@ -41,28 +42,28 @@ public:
/// @brief Defines the types of DNS updates that can be requested.
enum NameChangeType {
chgAdd = 1,
chgRemove
CHG_ADD,
CHG_REMOVE
};
/// @brief Defines the runtime processing status values for requests.
enum NameChangeStatus {
stNew = 1,
stPending,
stCompleted,
stFailed,
ST_NEW,
ST_PENDING,
ST_COMPLETED,
ST_FAILED,
};
/// @brief Defines the list of data wire formats supported.
enum NameChangeFormat {
fmtJSON = 1
FMT_JSON
};
/// @brief Container class for handling the DHCID value within a
/// NameChangeRequest. It provides conversion to and from string for JSON
/// formatting, but stores the data internally as unsigned bytes.
class D2Dhcid {
public:
public:
/// @brief Default constructor
D2Dhcid();
......@@ -77,9 +78,6 @@ class D2Dhcid {
/// or there is an odd number of digits.
D2Dhcid(const std::string& data);
/// @Destructor
virtual ~D2Dhcid();
/// @brief Returns the DHCID value as a string of hexadecimal digits.
///
/// @return returns a string containing a contiguous stream of digits.
......@@ -93,7 +91,7 @@ class D2Dhcid {
///
/// @throw throws a NcrMessageError if the input data contains non-digits
/// or there is an odd number of digits.
void fromStr(const std::string & data);
void fromStr(const std::string& data);
/// @brief Returns a reference to the DHCID byte vector.
///
......@@ -102,7 +100,7 @@ class D2Dhcid {
return (bytes_);
}
private:
private:
/// @Brief Storage for the DHCID value in unsigned bytes.
std::vector<uint8_t> bytes_;
};
......@@ -124,9 +122,11 @@ typedef boost::shared_ptr<NameChangeRequest> NameChangeRequestPtr;
/// @brief Defines a map of Elements, keyed by their string name.
typedef std::map<std::string, isc::data::ConstElementPtr> ElementMap;
/// @brief This class is used by DHCP-DDNS clients (e.g. DHCP4, DHCP6) to
/// @brief Represents a DHCP-DDNS client request.
/// This class is used by DHCP-DDNS clients (e.g. DHCP4, DHCP6) to
/// request DNS updates. Each message contains a single DNS change (either an
/// add/update or a remove) for a single FQDN. It provides marshalling services/// for moving instances to and from the wire. Currently, the only format
/// add/update or a remove) for a single FQDN. It provides marshalling services
/// for moving instances to and from the wire. Currently, the only format
/// supported is JSON, however the class provides an interface such that other
/// formats can be readily supported.
class NameChangeRequest {
......@@ -137,17 +137,17 @@ public:
/// @brief Constructor. Full constructor, which provides parameters for
/// all of the class members, except status.
///
/// @param change_type is the type of change (Add or Update)
/// @param forward_change indicates if this change should sent to forward
/// @param change_type the type of change (Add or Update)
/// @param forward_change indicates if this change should be sent to forward
/// DNS servers.
/// @param reverse_change indicates if this change should sent to reverse
/// @param reverse_change indicates if this change should be sent to reverse
/// DNS servers.
/// @param fqdn is the domain name whose pointer record(s) should be
/// @param fqdn the domain name whose pointer record(s) should be
/// updated.
/// @param ip_address is the ip address leased to the given FQDN.
/// @param dhcid is the lease client's unique DHCID.
/// @param ptime is a timestamp containing the date/time the lease expires.
/// @param lease_length is the amount of time in seconds for which the
/// @param ip_address the ip address leased to the given FQDN.
/// @param dhcid the lease client's unique DHCID.
/// @param ptime a timestamp containing the date/time the lease expires.
/// @param lease_length the amount of time in seconds for which the
/// lease is valid (TTL).
NameChangeRequest(NameChangeType change_type, bool forward_change,
bool reverse_change, const std::string& fqdn,
......@@ -155,9 +155,6 @@ public:
const boost::posix_time::ptime& lease_expires_on,
uint32_t lease_length);
/// @brief Destructor
virtual ~NameChangeRequest();
/// @brief Static method for creating a NameChangeRequest from a
/// buffer containing a marshalled request in a given format.
///
......@@ -198,7 +195,8 @@ public:
/// @param format indicates the data format to use
/// @param buffer is the output buffer to which the request should be
/// marshalled.
void toFormat(NameChangeFormat format, isc::util::OutputBuffer& buffer);
void
toFormat(NameChangeFormat format, isc::util::OutputBuffer& buffer) const;
/// @brief Static method for creating a NameChangeRequest from a
/// string containing a JSON rendition of a request.
......@@ -214,20 +212,24 @@ public:
/// into a string of JSON text.
///
/// @return returns a string containing the JSON rendition of the request
std::string toJSON();
std::string toJSON() const;
/// @brief Validates the content of a populated request. This method is
/// used by both the full constructor and from-wire marshalling to ensure
/// that the request is content valid. Currently it enforces the
/// following rules:
///
/// 1. FQDN must not be blank.
/// 2. The IP address must be a valid address.
/// 3. The DHCID must not be blank.
/// 4. The lease expiration date must be a valid date/time.
/// 5. That at least one of the two direction flags, forward change and
/// - FQDN must not be blank.
/// - The IP address must be a valid address.
/// - The DHCID must not be blank.
/// - The lease expiration date must be a valid date/time.
/// - That at least one of the two direction flags, forward change and
/// reverse change is true.
///
/// @TODO This is an initial implementation which provides a minimal amount
/// of validation. FQDN, DHCID, and IP Address members are all currently
/// strings, these may be replaced with richer classes.
///
/// @throw throws a NcrMessageError if the request content violates any
/// of the validation rules.
void validateContent();
......@@ -235,7 +237,7 @@ public:
/// @brief Fetches the request change type.
///
/// @return returns the change type
NameChangeType getChangeType() {
NameChangeType getChangeType() const {
return (change_type_);
}
......@@ -255,7 +257,7 @@ public:
/// @brief Checks forward change flag.
///
/// @return returns a true if the forward change flag is true.
const bool isForwardChange() const {
bool isForwardChange() const {
return (forward_change_);
}
......@@ -277,7 +279,7 @@ public:
/// @brief Checks reverse change flag.
///
/// @return returns a true if the reverse change flag is true.
const bool isReverseChange() const {
bool isReverseChange() const {
return (reverse_change_);
}
......@@ -319,7 +321,7 @@ public:
/// @brief Fetches the request IP address.
///
/// @return returns a string containing the IP address
const std::string& getIpAddress_() const {
const std::string& getIpAddress() const {
return (ip_address_);
}
......@@ -371,6 +373,12 @@ public:
/// @brief Fetches the request lease expiration as string.
///
/// The format of the string returned is:
///
/// YYYYMMDDTHHMMSS where T is the date-time separator
///
/// Example: 18:54:54 June 26, 2013 would be: 20130626T185455
///
/// @return returns a ISO date-time string of the lease expiration.
std::string getLeaseExpiresOnStr() const;
......@@ -385,7 +393,11 @@ public:
/// @brief Sets the lease expiration based on the given string.
///
/// @param value is an ISO date-time string from which to set the
/// lease expiration.
/// lease expiration. The format of the input is:
///
/// YYYYMMDDTHHMMSS where T is the date-time separator
///
/// Example: 18:54:54 June 26, 2013 would be: 20130626T185455
///
/// @throw throws a NcrMessageError if the ISO string is invalid.
void setLeaseExpiresOn(const std::string& value);
......@@ -440,7 +452,7 @@ public:
/// @throw throws a NcrMessageError if the element cannot be found within
/// the map
isc::data::ConstElementPtr getElement(const std::string& name,
const ElementMap& element_map);
const ElementMap& element_map) const;
/// @brief Returns a text rendition of the contents of the request.
/// This method is primarily for logging purposes.
......@@ -459,18 +471,20 @@ private:
bool reverse_change_;
/// @brief The domain name whose DNS entry(ies) are to be updated.
/// @TODO Currently, this is a std::string but may be replaced with
/// dns::Name which provides additional validation and domain name
/// manipulation.
std::string fqdn_;
/// @brief The ip address leased to the FQDN.
std::string ip_address_;
/// @brief The lease client's unique DHCID.
/// @TODO Currently, this is uses D2Dhcid it but may be replaced with
/// dns::DHCID which provides additional validation.
D2Dhcid dhcid_;
/// @brief The date-time the lease expires. Note, that a TimePtr currently
/// points to a boost::posix_time::ptime instance. Boost ptimes were chosen
/// because they convert to and from ISO strings in GMT time. The time.h
/// classes can convert to GMT but conversion back assumes local time.
/// @brief The date-time the lease expires.
TimePtr lease_expires_on_;
/// @brief The amount of time in seconds for which the lease is valid (TTL).
......
......@@ -17,6 +17,8 @@
#include <boost/date_time/posix_time/posix_time.hpp>
#include <gtest/gtest.h>
#include <algorithm>
using namespace std;
using namespace isc;
using namespace isc::d2;
......@@ -31,7 +33,7 @@ const char *valid_msgs[] =
{
// Valid Add.
"{"
" \"change_type\" : 1 , "
" \"change_type\" : 0 , "
" \"forward_change\" : true , "
" \"reverse_change\" : false , "
" \"fqdn\" : \"walah.walah.com\" , "
......@@ -42,7 +44,7 @@ const char *valid_msgs[] =
"}",
// Valid Remove.
"{"
" \"change_type\" : 2 , "
" \"change_type\" : 1 , "
" \"forward_change\" : true , "
" \"reverse_change\" : false , "
" \"fqdn\" : \"walah.walah.com\" , "
......@@ -53,7 +55,7 @@ const char *valid_msgs[] =
"}",
// Valid Add with IPv6 address
"{"
" \"change_type\" : 1 , "
" \"change_type\" : 0 , "
" \"forward_change\" : true , "
" \"reverse_change\" : false , "
" \"fqdn\" : \"walah.walah.com\" , "
......@@ -81,7 +83,7 @@ const char *invalid_msgs[] =
"}",
// Invalid forward change.
"{"
" \"change_type\" : 1 , "
" \"change_type\" : 0 , "
" \"forward_change\" : \"bogus\" , "
" \"reverse_change\" : false , "
" \"fqdn\" : \"walah.walah.com\" , "
......@@ -92,7 +94,7 @@ const char *invalid_msgs[] =
"}",
// Invalid reverse change.
"{"
" \"change_type\" : 1 , "
" \"change_type\" : 0 , "
" \"forward_change\" : true , "
" \"reverse_change\" : 500 , "
" \"fqdn\" : \"walah.walah.com\" , "
......@@ -103,7 +105,7 @@ const char *invalid_msgs[] =
"}",
// Forward and reverse change both false.
"{"
" \"change_type\" : 1 , "
" \"change_type\" : 0 , "
" \"forward_change\" : false , "
" \"reverse_change\" : false , "
" \"fqdn\" : \"walah.walah.com\" , "
......@@ -112,10 +114,9 @@ const char *invalid_msgs[] =
" \"lease_expires_on\" : \"19620121T132405\" , "
" \"lease_length\" : 1300 "
"}",
// Invalid forward change.
// Blank FQDN
"{"
" \"change_type\" : 1 , "
" \"change_type\" : 0 , "
" \"forward_change\" : true , "
" \"reverse_change\" : false , "
" \"fqdn\" : \"\" , "
......@@ -126,7 +127,7 @@ const char *invalid_msgs[] =
"}",
// Bad IP address
"{"
" \"change_type\" : 1 , "
" \"change_type\" : 0 , "
" \"forward_change\" : true , "
" \"reverse_change\" : false , "
" \"fqdn\" : \"walah.walah.com\" , "
......@@ -137,7 +138,7 @@ const char *invalid_msgs[] =
"}",
// Blank DHCID
"{"
" \"change_type\" : 1 , "
" \"change_type\" : 0 , "
" \"forward_change\" : true , "
" \"reverse_change\" : false , "
" \"fqdn\" : \"walah.walah.com\" , "
......@@ -148,7 +149,7 @@ const char *invalid_msgs[] =
"}",
// Odd number of digits in DHCID
"{"
" \"change_type\" : 1 , "
" \"change_type\" : 0 , "
" \"forward_change\" : true , "
" \"reverse_change\" : false , "
" \"fqdn\" : \"walah.walah.com\" , "
......@@ -159,7 +160,7 @@ const char *invalid_msgs[] =
"}",
// Text in DHCID
"{"
" \"change_type\" : 1 , "
" \"change_type\" : 0 , "
" \"forward_change\" : true , "
" \"reverse_change\" : false , "
" \"fqdn\" : \"walah.walah.com\" , "
......@@ -170,18 +171,18 @@ const char *invalid_msgs[] =
"}",
// Invalid lease expiration string
"{"
" \"change_type\" : 1 , "
" \"change_type\" : 0 , "
" \"forward_change\" : true , "
" \"reverse_change\" : false , "
" \"fqdn\" : \"walah.walah.com\" , "
" \"ip_address\" : \"192.168.2.1\" , "
" \"dhcid\" : \"010203040A7F8E3D\" , "
" \"lease_expires_on\" : \"19620121132405\" , "
" \"lease_expires_on\" : \"Wed Jun 26 13:46:46 EDT 2013\" , "
" \"lease_length\" : 1300 "
"}",
// Non-integer for lease length.
"{"
" \"change_type\" : 1 , "
" \"change_type\" : 0 , "
" \"forward_change\" : true , "
" \"reverse_change\" : false , "
" \"fqdn\" : \"walah.walah.com\" , "
......@@ -203,46 +204,46 @@ const char *invalid_msgs[] =
/// 6. "Full" constructor, given an invalid lease expiration fails
/// 7. "Full" constructor, given false for both forward and reverse fails
TEST(NameChangeRequestTest, constructionTests) {
NameChangeRequest* ncr = NULL;
// Verify the default constructor works.
ASSERT_TRUE((ncr = new NameChangeRequest()));