BIND 10 trac1539-2, updated. 3aa296cf17e158576175ca0c349a6dee19a97af1 [1539] supported operator<< for IOEndpoint for unified logging format.
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue May 22 06:50:19 UTC 2012
The branch, trac1539-2 has been updated
via 3aa296cf17e158576175ca0c349a6dee19a97af1 (commit)
via 434d67fede28c9e19127395b115dc3551d25692b (commit)
via d70c06df29e24e8a7a258728ce419e27372c91f9 (commit)
via 634994e89a83dac53318ac32dc9a78688d5c5129 (commit)
via fedb6edd4f302d8be91ea704ac71ded40ed921f9 (commit)
from ad6c12af58714d9c72f7726a598b24fc81ba03fd (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 3aa296cf17e158576175ca0c349a6dee19a97af1
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon May 21 23:48:17 2012 -0700
[1539] supported operator<< for IOEndpoint for unified logging format.
the formatEndpoint() function locally defined in auth_srv was essentially
moved there. also added the test of the operator.
commit 434d67fede28c9e19127395b115dc3551d25692b
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon May 21 21:22:29 2012 -0700
[1539] hide everything except push() inside SocketSessionForwarderHolder.
this will make the caller side much simpler. in particular, the caller
now doesn't have to handle exceptions. connect() and close() can now
be private.
To handle various types of requests, the constructor is extended to take
a message type, which will be used in the log message on failure.
log message ID and description were made generic.
commit d70c06df29e24e8a7a258728ce419e27372c91f9
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon May 21 18:15:42 2012 -0700
[1539] updated the comment in unsupportedRequest about supported opcodes.
so that it matches latest implementation status (and the statement is
generic and won't have to be updated this way again)
commit 634994e89a83dac53318ac32dc9a78688d5c5129
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon May 21 17:49:11 2012 -0700
[1539] updated the comment about getDDNSSocketPath based on review comment.
so that it doesn't necessarily mean we'll get the path from a configuration.
commit fedb6edd4f302d8be91ea704ac71ded40ed921f9
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Mon May 21 17:44:59 2012 -0700
[1539] use getSALength() instead of sa_len to calculate the length of sockaddr.
the latter isn't very portable.
-----------------------------------------------------------------------
Summary of changes:
src/bin/auth/auth_messages.mes | 22 ++--
src/bin/auth/auth_srv.cc | 132 ++++++++++++------------
src/bin/auth/common.h | 6 +-
src/bin/auth/tests/auth_srv_unittest.cc | 5 +-
src/lib/asiolink/io_endpoint.cc | 24 ++++-
src/lib/asiolink/io_endpoint.h | 30 +++++-
src/lib/asiolink/tests/io_endpoint_unittest.cc | 58 ++++++++++-
src/lib/testutils/srv_test.cc | 2 +-
8 files changed, 187 insertions(+), 92 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/auth/auth_messages.mes b/src/bin/auth/auth_messages.mes
index 382d72c..3d1a6d6 100644
--- a/src/bin/auth/auth_messages.mes
+++ b/src/bin/auth/auth_messages.mes
@@ -255,16 +255,18 @@ processed by the authoritative server has been found to contain an
unsupported opcode. (The opcode is included in the message.) The server
will return an error code of NOTIMPL to the sender.
-% AUTH_UPDATE_FORWARD_ERROR failed to forward update request from %1: %2
-The authoritative server receives a dynamic update request, tried to
-forward it to a separate process (which is usually b10-ddns) to handle it,
-but it failed. It could be configuration mismatch between b10-auth and
-b10-ddns, or it may because update requests are coming too fast and b10-ddns
-cannot keep up with the rate, or some system level failure. In either case
-this means the BIND 10 system is not working as expected, so the administrator
-should look into the cause and address the issue. The log message
-includes the client's address (and port), and the error message sent
-from the lower layer that detects the failure.
+% AUTH_MESSAGE_FORWARD_ERROR failed to forward %1 request from %2: %3
+The authoritative server tried to forward some type DNS request
+message to a separate process (e.g., forwarding dynamic update
+requests to b10-ddns) to handle it, but it failed. It could be
+configuration mismatch between b10-auth and the forwarded program, or
+it may because the requests are coming too fast and the forwarded
+program cannot keep up with the rate, or some system level failure.
+In either case this means the BIND 10 system is not working as
+expected, so the administrator should look into the cause and address
+the issue. The log message includes the client's address (and port),
+and the error message sent from the lower layer that detects the
+failure.
% AUTH_XFRIN_CHANNEL_CREATED XFRIN session channel created
This is a debug message indicating that the authoritative server has
diff --git a/src/bin/auth/auth_srv.cc b/src/bin/auth/auth_srv.cc
index 366108c..35a0dcc 100644
--- a/src/bin/auth/auth_srv.cc
+++ b/src/bin/auth/auth_srv.cc
@@ -17,6 +17,7 @@
#include <util/io/socketsession.h>
#include <asiolink/asiolink.h>
+#include <asiolink/io_endpoint.h>
#include <config/ccsession.h>
@@ -115,28 +116,77 @@ private:
// A helper container of socket session forwarder.
//
// This class provides a simple wrapper interface to SocketSessionForwarder
-// so that the caller doesn't have to worry about exception handling or
-// parameter building.
+// so that the caller doesn't have to worry about connection management,
+// exception handling or parameter building.
//
// It internally maintains whether the underlying forwarder establishes a
-// connection to the receiver. Its connect() wrapper tries connection
-// setup only when necessary, so the caller can just always call connect().
-// It also closes the connection on destruction, when necessary, automatically.
-//
-// Its push() wrapper takes the session data to be forwarded in the form of
-// IOMessage object, and converts it to the parameters that the underlying
-// SocketSessionForwarder expects.
+// connection to the receiver. On a forwarding request, if the connection
+// hasn't been established yet, it automatically opens a new one, then
+// pushes the session over it. It also closes the connection on destruction,
+// or a non-recoverable error happens, automatically. So the only thing
+// the application has to do is to create this object and push any session
+// to be forwarded.
class SocketSessionForwarderHolder {
public:
- SocketSessionForwarderHolder(BaseSocketSessionForwarder& forwarder) :
- forwarder_(forwarder), connected_(false)
+ /// \brief The constructor.
+ ///
+ /// \param message_name Any string that can identify the type of messages
+ /// to be forwarded via this session. It will be only used as part of
+ /// log message, so it can be anything, but in practice something like
+ /// "update" or "xfr" is expected.
+ /// \param forwarder The underlying socket session forwarder.
+ SocketSessionForwarderHolder(const string& message_name,
+ BaseSocketSessionForwarder& forwarder) :
+ message_name_(message_name), forwarder_(forwarder), connected_(false)
{}
+
~SocketSessionForwarderHolder() {
if (connected_) {
forwarder_.close();
}
}
+ /// \brief Push a socket session corresponding to given IOMessage.
+ ///
+ /// If the connection with the receiver process hasn't been established,
+ /// it automatically establishes one, then push the session over it.
+ ///
+ /// If either connect or push fails, the underlying forwarder object should
+ /// throw an exception. This method logs the event, and propagates the
+ /// exception to the caller, which will eventually result in SERVFAIL.
+ /// The connection, if established, is automatically closed, so the next
+ /// forward request will trigger reopening a new connection.
+ ///
+ /// \note: Right now, there's no API to retrieve the local address from
+ /// the IOMessage. Until it's added, we pass the remote address as
+ /// local.
+ ///
+ /// \param io_message The request message to be forwarded as a socket
+ /// session. It will be converted to the parameters that the underlying
+ /// SocketSessionForwarder expects.
+ void push(const IOMessage& io_message) {
+ const IOEndpoint& remote_ep = io_message.getRemoteEndpoint();
+ const int protocol = remote_ep.getProtocol();
+ const int sock_type = getSocketType(protocol);
+ try {
+ connect();
+ forwarder_.push(io_message.getSocket().getNative(),
+ remote_ep.getFamily(), sock_type, protocol,
+ remote_ep.getSockAddr(), remote_ep.getSockAddr(),
+ io_message.getData(), io_message.getDataSize());
+ } catch (const SocketSessionError& ex) {
+ LOG_ERROR(auth_logger, AUTH_MESSAGE_FORWARD_ERROR).
+ arg(message_name_).arg(remote_ep).arg(ex.what());
+ close();
+ throw;
+ }
+ }
+
+private:
+ const string message_name_;
+ BaseSocketSessionForwarder& forwarder_;
+ bool connected_;
+
void connect() {
if (!connected_) {
forwarder_.connectToReceiver();
@@ -151,28 +201,6 @@ public:
}
}
- // Push a socket session corresponding to given IOMessage.
- //
- // NOTE: Right now, there's no API to retrieve the local address from
- // the IOMessage. Until it's added, we pass the remote address as
- // local.
- //
- // If the underlying forwarder throws on push(), the exception will be
- // propagated to the caller.
- void push(const IOMessage& io_message) {
- const IOEndpoint& remote_ep = io_message.getRemoteEndpoint();
- const int protocol = remote_ep.getProtocol();
- const int sock_type = getSocketType(protocol);
- forwarder_.push(io_message.getSocket().getNative(),
- remote_ep.getFamily(), sock_type, protocol,
- remote_ep.getSockAddr(), remote_ep.getSockAddr(),
- io_message.getData(), io_message.getDataSize());
- }
-
-private:
- BaseSocketSessionForwarder& forwarder_;
- bool connected_;
-
static int getSocketType(int protocol) {
switch (protocol) {
case IPPROTO_UDP:
@@ -185,23 +213,6 @@ private:
}
}
};
-
-// A helper function to log an address/port in the form of IOEndpoint
-// in our preferred format: [<ipv6_addr>]:port or <ipv4_addr>:port
-string
-formatEndpoint(const IOEndpoint& ep) {
- string addr_port;
- if (ep.getFamily() == AF_INET6) {
- addr_port = "[" + ep.getAddress().toText() + "]";
- } else if (ep.getFamily() == AF_INET) {
- addr_port = ep.getAddress().toText();
- } else {
- addr_port = "(unknown address)";
- }
- addr_port += ":" + boost::lexical_cast<string>(ep.getPort());
-
- return (addr_port);
-}
}
class AuthSrvImpl {
@@ -309,7 +320,7 @@ AuthSrvImpl::AuthSrvImpl(const bool use_cache,
keyring_(NULL),
xfrout_connected_(false),
xfrout_client_(xfrout_client),
- ddns_forwarder_(ddns_forwarder)
+ ddns_forwarder_("update", ddns_forwarder)
{
// cur_datasrc_ is automatically initialized by the default constructor,
// effectively being an empty (sqlite) data source. once ccsession is up
@@ -848,21 +859,10 @@ AuthSrvImpl::processNotify(const IOMessage& io_message, Message& message,
bool
AuthSrvImpl::processUpdate(const IOMessage& io_message) {
- try {
- ddns_forwarder_.connect();
- ddns_forwarder_.push(io_message);
- } catch (const SocketSessionError& ex) {
- // If either connect or push fails, the forwarder object should throw
- // an exception. We log the event, and propagate the exception to
- // the caller, which will result in SERVFAIL.
- LOG_ERROR(auth_logger, AUTH_UPDATE_FORWARD_ERROR).
- arg(formatEndpoint(io_message.getRemoteEndpoint())).
- arg(ex.what());
- ddns_forwarder_.close();
- throw;
- }
-
- // On successful push, the request shouldn't be responded from b10-auth.
+ // Push the update request to a separate process via the forwarder.
+ // On successful push, the request shouldn't be responded from b10-auth,
+ // so we return false.
+ ddns_forwarder_.push(io_message);
return (false);
}
diff --git a/src/bin/auth/common.h b/src/bin/auth/common.h
index 48a47d1..9a1942c 100644
--- a/src/bin/auth/common.h
+++ b/src/bin/auth/common.h
@@ -46,8 +46,10 @@ std::string getXfroutSocketPath();
///
/// The logic should be the same as in b10-ddns, so they find each other.
///
-/// Note: eventually this should be retrieved from the ddns configuration,
-/// at which point this function should be deprecated.
+/// Note: eventually we should find a better way so that we don't have to
+/// repeat the same magic value (and how to tweak it with some magic
+/// environment variable) twice, at which point this function may be able
+/// to be deprecated.
std::string getDDNSSocketPath();
/// \brief The name used when identifieng the process
diff --git a/src/bin/auth/tests/auth_srv_unittest.cc b/src/bin/auth/tests/auth_srv_unittest.cc
index 48756ac..0da8911 100644
--- a/src/bin/auth/tests/auth_srv_unittest.cc
+++ b/src/bin/auth/tests/auth_srv_unittest.cc
@@ -16,6 +16,8 @@
#include <gtest/gtest.h>
+#include <util/io/sockaddr_util.h>
+
#include <dns/message.h>
#include <dns/messagerenderer.h>
#include <dns/name.h>
@@ -56,6 +58,7 @@ using namespace std;
using namespace isc::cc;
using namespace isc::dns;
using namespace isc::util;
+using namespace isc::util::io::internal;
using namespace isc::util::unittests;
using namespace isc::dns::rdata;
using namespace isc::data;
@@ -1428,7 +1431,7 @@ checkAddrPort(const struct sockaddr& actual_sa,
const string& expected_addr, uint16_t expected_port)
{
char hbuf[NI_MAXHOST], sbuf[NI_MAXSERV];
- const int error = getnameinfo(&actual_sa, actual_sa.sa_len, hbuf,
+ const int error = getnameinfo(&actual_sa, getSALength(actual_sa), hbuf,
sizeof(hbuf), sbuf, sizeof(sbuf),
NI_NUMERICHOST | NI_NUMERICSERV);
if (error != 0) {
diff --git a/src/lib/asiolink/io_endpoint.cc b/src/lib/asiolink/io_endpoint.cc
index 63830a5..2354521 100644
--- a/src/lib/asiolink/io_endpoint.cc
+++ b/src/lib/asiolink/io_endpoint.cc
@@ -14,10 +14,6 @@
#include <config.h>
-#include <unistd.h> // for some IPC/network system calls
-#include <sys/socket.h>
-#include <netinet/in.h>
-
#include <asio.hpp>
#include <asiolink/io_address.h>
@@ -26,6 +22,13 @@
#include <asiolink/tcp_endpoint.h>
#include <asiolink/udp_endpoint.h>
+#include <boost/lexical_cast.hpp>
+
+#include <cassert>
+#include <unistd.h> // for some IPC/network system calls
+#include <sys/socket.h>
+#include <netinet/in.h>
+
using namespace std;
namespace isc {
@@ -58,5 +61,18 @@ IOEndpoint::operator!=(const IOEndpoint& other) const {
return (!operator==(other));
}
+ostream&
+operator<<(ostream& os, const IOEndpoint& endpoint) {
+ if (endpoint.getFamily() == AF_INET6) {
+ os << "[" << endpoint.getAddress().toText() << "]";
+ } else {
+ // In practice this should be AF_INET, but it's not guaranteed by
+ // the interface. We'll use the result of textual address
+ // representation opaquely.
+ os << endpoint.getAddress().toText();
+ }
+ os << ":" << boost::lexical_cast<string>(endpoint.getPort());
+ return (os);
+}
} // namespace asiolink
} // namespace isc
diff --git a/src/lib/asiolink/io_endpoint.h b/src/lib/asiolink/io_endpoint.h
index 11ea97b..dd74036 100644
--- a/src/lib/asiolink/io_endpoint.h
+++ b/src/lib/asiolink/io_endpoint.h
@@ -18,9 +18,6 @@
// IMPORTANT NOTE: only very few ASIO headers files can be included in
// this file. In particular, asio.hpp should never be included here.
// See the description of the namespace below.
-#include <unistd.h> // for some network system calls
-
-#include <sys/socket.h> // for sockaddr
#include <functional>
#include <string>
@@ -28,6 +25,12 @@
#include <exceptions/exceptions.h>
#include <asiolink/io_address.h>
+# include <ostream>
+
+#include <unistd.h> // for some network system calls
+
+#include <sys/socket.h> // for sockaddr
+
namespace isc {
namespace asiolink {
@@ -158,6 +161,27 @@ public:
const unsigned short port);
};
+/// \brief Insert the \c IOEndpoint as a string into stream.
+///
+/// This method converts \c endpoint into a string and inserts it into the
+/// output stream \c os.
+///
+/// This method converts the address and port of the endpoint in the textual
+/// format that other BIND 10 modules would use in logging, i.e.,
+/// - For IPv6 address: [<address>]:port (e.g., [2001:db8::5300]:53)
+/// - For IPv4 address: <address>:port (e.g., 192.0.2.53:5300)
+///
+/// If it's neither IPv6 nor IPv4, it converts the endpoint into text in the
+/// same format as that for IPv4, although in practice such a case is not
+/// really expected.
+///
+/// \param os A \c std::ostream object on which the insertion operation is
+/// performed.
+/// \param endpoint A reference to an \c IOEndpoint object output by the
+/// operation.
+/// \return A reference to the same \c std::ostream object referenced by
+/// parameter \c os after the insertion operation.
+std::ostream& operator<<(std::ostream& os, const IOEndpoint& endpoint);
} // namespace asiolink
} // namespace isc
#endif // __IO_ENDPOINT_H
diff --git a/src/lib/asiolink/tests/io_endpoint_unittest.cc b/src/lib/asiolink/tests/io_endpoint_unittest.cc
index 948e708..2170112 100644
--- a/src/lib/asiolink/tests/io_endpoint_unittest.cc
+++ b/src/lib/asiolink/tests/io_endpoint_unittest.cc
@@ -13,18 +13,22 @@
// PERFORMANCE OF THIS SOFTWARE.
#include <config.h>
+
+#include <asiolink/io_endpoint.h>
+#include <asiolink/io_error.h>
+
#include <gtest/gtest.h>
+#include <boost/shared_ptr.hpp>
+
+#include <sstream>
+#include <string>
+
#include <sys/types.h>
#include <sys/socket.h>
#include <netdb.h>
#include <string.h>
-#include <boost/shared_ptr.hpp>
-
-#include <asiolink/io_endpoint.h>
-#include <asiolink/io_error.h>
-
using namespace isc::asiolink;
namespace {
@@ -240,4 +244,48 @@ TEST(IOEndpointTest, getSockAddr) {
sockAddrMatch(ep->getSockAddr(), "2001:db8::5300", "35");
}
+// A faked IOEndpoint for an uncommon address family. It wouldn't be possible
+// to create via the normal factory, so we define a special derived class
+// for it.
+class TestIOEndpoint : public IOEndpoint {
+ virtual IOAddress getAddress() const {
+ return IOAddress("2001:db8::bad:add");
+ }
+ virtual uint16_t getPort() const { return (42); }
+ virtual short getProtocol() const { return (IPPROTO_UDP); }
+ virtual short getFamily() const { return (AF_UNSPEC); }
+ virtual const struct sockaddr& getSockAddr() const {
+ static struct sockaddr sa_placeholder;
+ return (sa_placeholder);
+ }
+};
+
+void
+checkEndpointText(const std::string& expected, const IOEndpoint& ep) {
+ std::ostringstream oss;
+ oss << ep;
+ EXPECT_EQ(expected, oss.str());
+}
+
+// test operator<<. We simply confirm it appends the result of toText().
+TEST(IOEndpointTest, LeftShiftOperator) {
+ // UDP/IPv4
+ ConstIOEndpointPtr ep(IOEndpoint::create(IPPROTO_UDP,
+ IOAddress("192.0.2.1"), 53210));
+ checkEndpointText("192.0.2.1:53210", *ep);
+
+ // UDP/IPv6
+ ep.reset(IOEndpoint::create(IPPROTO_UDP, IOAddress("2001:db8::53"), 53));
+ checkEndpointText("[2001:db8::53]:53", *ep);
+
+ // Same for TCP: shouldn't be different
+ ep.reset(IOEndpoint::create(IPPROTO_TCP, IOAddress("192.0.2.1"), 53210));
+ checkEndpointText("192.0.2.1:53210", *ep);
+ ep.reset(IOEndpoint::create(IPPROTO_TCP, IOAddress("2001:db8::53"), 53));
+ checkEndpointText("[2001:db8::53]:53", *ep);
+
+ // Uncommon address family. The actual behavior doesn't matter much
+ // in practice, but we check such input doesn't make it crash.
+ checkEndpointText("2001:db8::bad:add:42", TestIOEndpoint());
+}
}
diff --git a/src/lib/testutils/srv_test.cc b/src/lib/testutils/srv_test.cc
index 12f71a1..d686da6 100644
--- a/src/lib/testutils/srv_test.cc
+++ b/src/lib/testutils/srv_test.cc
@@ -100,7 +100,7 @@ void
SrvTestBase::unsupportedRequest() {
for (unsigned int i = 0; i < 16; ++i) {
// set Opcode to 'i', which iterators over all possible codes except
- // the standard query and notify
+ // the standard opcodes we support.
if (i == isc::dns::Opcode::QUERY().getCode() ||
i == isc::dns::Opcode::NOTIFY().getCode() ||
i == isc::dns::Opcode::UPDATE().getCode()) {
More information about the bind10-changes
mailing list