Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions src/api_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -65,19 +65,13 @@ void MetadataApiServer::Dispatcher::log(const HttpServer::string_type& info) {
LOG(ERROR) << info;
}


MetadataApiServer::MetadataApiServer(const Configuration& config,
const MetadataStore& store,
int server_threads,
const std::string& host, int port)
: config_(config), store_(store), dispatcher_({
{{"GET", "/monitoredResource/"},
[=](const HttpServer::request& request,
std::shared_ptr<HttpServer::connection> conn) {
HandleMonitoredResource(request, conn);
}},
}, config_.VerboseLogging()),
server_(
const std::string& host, int port,
const HandlerMap& handlers)
: config_(config), store_(store),
dispatcher_(handlers, config_.VerboseLogging()), server_(
HttpServer::options(dispatcher_)
.address(host)
.port(std::to_string(port))),
Expand All @@ -88,6 +82,19 @@ MetadataApiServer::MetadataApiServer(const Configuration& config,
}
}

MetadataApiServer::MetadataApiServer(const Configuration& config,
const MetadataStore& store,
int server_threads,
const std::string& host, int port)
: MetadataApiServer(config, store, server_threads, host, port,
std::map<std::pair<std::string, std::string>, Handler>({
Copy link
Contributor

Choose a reason for hiding this comment

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

You didn't have to make this change. First, you can test the Dispatcher class separately, and the HandleMonitoredResource callback separately (that's why the latter was pulled out in the first place). Second, the new code looks a lot uglier because of the explicit specs for the map and pair types. I would back out this whole commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking things over now, I agree. Ill close this pull request and open a new one testing Dispatcher directly

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I meant this commit, not this PR. You could have reused the PR for the dispatch tests (or the HandleMonitoredResource test, which you still can — just reopen the PR).

{std::pair<std::string, std::string>({"GET", "/monitoredResource/"}),
[=](const HttpServer::request& request,
std::shared_ptr<HttpServer::connection> conn) {
HandleMonitoredResource(request, conn);
}
}})) {}

MetadataApiServer::~MetadataApiServer() {
for (auto& thread : server_pool_) {
thread.join();
Expand Down
15 changes: 9 additions & 6 deletions src/api_server.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,20 @@ class MetadataStore;
// A server that implements the metadata agent API.
class MetadataApiServer {
public:
MetadataApiServer(const Configuration& config, const MetadataStore& store,
int server_threads, const std::string& host, int port);
~MetadataApiServer();

private:
class Dispatcher;
using HttpServer = http::server<Dispatcher>;
using Handler = std::function<void(const HttpServer::request&,
std::shared_ptr<HttpServer::connection>)>;
using HandlerMap = std::map<std::pair<std::string, std::string>, Handler>;
MetadataApiServer(const Configuration& config, const MetadataStore& store,
int server_threads, const std::string& host, int port,
const HandlerMap& handlers);
MetadataApiServer(const Configuration& config, const MetadataStore& store,
int server_threads, const std::string& host, int port);
~MetadataApiServer();

class Dispatcher {
public:
using HandlerMap = std::map<std::pair<std::string, std::string>, Handler>;
Dispatcher(const HandlerMap& handlers, bool verbose);
void operator()(const HttpServer::request& request,
std::shared_ptr<HttpServer::connection> conn);
Expand All @@ -63,6 +64,8 @@ class MetadataApiServer {
bool verbose_;
};

private:
friend class ApiServerTest;
// Handler functions.
void HandleMonitoredResource(const HttpServer::request& request,
std::shared_ptr<HttpServer::connection> conn);
Expand Down
24 changes: 16 additions & 8 deletions test/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ YAML_CPP_LIBS=$(YAML_CPP_LIBDIR)/libyaml-cpp.a
CPPFLAGS+= \
-DENABLE_KUBERNETES_METADATA \
-isystem $(GTEST_DIR)/include -I$(GMOCK_DIR)/include \
-I$(YAML_CPP_DIR)/include
-I$(CPP_NETLIB_DIR) -I$(NETWORK_URI_DIR)/include -I$(YAML_CPP_DIR)/include
CXXFLAGS=\
-std=c++11 -g -pthread -Wno-write-strings -Wno-deprecated
LDFLAGS=-L$(CPP_NETLIB_LIBDIR) -L$(NETWORK_URI_LIBDIR) -L$(YAML_CPP_LIBDIR)
Expand All @@ -40,6 +40,7 @@ TEST_DIR=.
TEST_SOURCES=$(wildcard $(TEST_DIR)/*_unittest.cc)
TEST_OBJS=$(TEST_SOURCES:$(TEST_DIR)/%.cc=%.o)
TESTS=\
api_server_unittest \
base64_unittest \
configuration_unittest \
format_unittest \
Expand All @@ -54,7 +55,7 @@ TESTS=\

GTEST_LIB=gtest_lib.a

all: $(TESTS)
all: $(SRC_DIR)/build-cpp-netlib $(SRC_DIR)/build-yaml-cpp $(TESTS)

# TODO: Running the test prints "Running main() from gtest_main.cc".
# Figure out how to fix this.
Expand All @@ -75,13 +76,17 @@ init-submodules:
git submodule update --init $(GTEST_MODULE)
touch init-submodules

$(CPP_NETLIB_LIBS):
cd $(SRC_DIR) && $(MAKE) $@
$(SRC_DIR)/init-submodules:
cd $(SRC_DIR) && $(MAKE) $(@:$(SRC_DIR)/%=%)

$(SRC_DIR)/build-cpp-netlib $(SRC_DIR)/build-yaml-cpp: $(SRC_DIR)/init-submodules
cd $(SRC_DIR) && $(MAKE) $(@:$(SRC_DIR)/%=%)

$(YAML_CPP_LIBS):
cd $(SRC_DIR) && $(MAKE) $@
$(CPP_NETLIB_LIBS): $(SRC_DIR)/build-cpp-netlib

$(SRC_DIR)/%.o: $(SRC_DIR)/%.cc
$(YAML_CPP_LIBS): $(SRC_DIR)/build-yaml-cpp

$(SRC_DIR)/%.o: $(SRC_DIR)/build-cpp-netlib $(SRC_DIR)/build-yaml-cpp $(SRC_DIR)/%.cc
cd $(SRC_DIR) && $(MAKE) $(@:$(SRC_DIR)/%=%)

$(GTEST_SOURCEDIR)/gtest-all.cc $(GTEST_SOURCEDIR)/gtest_main.cc: init-submodules
Expand All @@ -98,8 +103,11 @@ $(GTEST_LIB): gtest-all.o gtest_main.o
$(TESTS): $(GTEST_LIB) $(CPP_NETLIB_LIBS) $(YAML_CPP_LIBS)

# All unittest objects depend on GTEST_LIB.
$(TESTS:%=%.o): $(GTEST_LIB)
# Some headers need CPP_NETLIB_LIBS and YAML_CPP_LIBS.
$(TESTS:%=%.o): $(GTEST_LIB) $(CPP_NETLIB_LIBS) $(YAML_CPP_LIBS)

api_server_unittest: api_server_unittest.o $(SRC_DIR)/api_server.o $(SRC_DIR)/configuration.o $(SRC_DIR)/store.o $(SRC_DIR)/json.o $(SRC_DIR)/resource.o $(SRC_DIR)/logging.o $(SRC_DIR)/time.o
$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
format_unittest: format_unittest.o $(SRC_DIR)/format.o
$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
base64_unittest: base64_unittest.o $(SRC_DIR)/base64.o
Expand Down
87 changes: 87 additions & 0 deletions test/api_server_unittest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
#include "../src/api_server.h"
#include "../src/configuration.h"
#include "../src/store.h"
#include "gtest/gtest.h"

namespace google {

class ApiServerTest : public ::testing::Test {
protected:
static void CallDispatcher(
MetadataApiServer* server,
const MetadataApiServer::HttpServer::request& request,
std::shared_ptr<MetadataApiServer::HttpServer::connection> conn) {
server->dispatcher_(request, conn);
}
};

TEST_F(ApiServerTest, BasicDispacher) {
Configuration config(std::istringstream(""));
MetadataStore store(config);
bool handler_called;
MetadataApiServer::Handler handler = [&handler_called](
const MetadataApiServer::HttpServer::request& request,
std::shared_ptr<MetadataApiServer::HttpServer::connection> conn) {
handler_called = true;
};
MetadataApiServer::Handler badHandler = [&handler_called](
const MetadataApiServer::HttpServer::request& request,
std::shared_ptr<MetadataApiServer::HttpServer::connection> conn) {
handler_called = false;
};
MetadataApiServer::HandlerMap handlers({
{{"GET", "/testPath/"}, handler},
{{"GET", "/badPath/"}, badHandler}
});
MetadataApiServer server(config, store, 0, "TestHost", 1234, handlers);
MetadataApiServer::HttpServer::request request;
request.method = "GET";
request.destination = "/testPath/";
CallDispatcher(&server, request, nullptr);
EXPECT_TRUE(handler_called);
}

TEST_F(ApiServerTest, DispatcherMethodCheck) {
Configuration config(std::istringstream(""));
MetadataStore store(config);
bool handler_called;
MetadataApiServer::Handler handler = [&handler_called](
const MetadataApiServer::HttpServer::request& request,
std::shared_ptr<MetadataApiServer::HttpServer::connection> conn) {
handler_called = true;
};
MetadataApiServer::HandlerMap handlers({{{"GET", "/testPath/"}, handler}});
MetadataApiServer server(config, store, 0, "TestHost", 1234, handlers);
MetadataApiServer::HttpServer::request request;
request.method = "POST";
request.destination = "/testPath/";
CallDispatcher(&server, request, nullptr);
EXPECT_FALSE(handler_called);
}

TEST_F(ApiServerTest, DispatcherSubstringCheck) {
Configuration config(std::istringstream(""));
MetadataStore store(config);
bool handler_called;
MetadataApiServer::Handler handler = [&handler_called](
const MetadataApiServer::HttpServer::request& request,
std::shared_ptr<MetadataApiServer::HttpServer::connection> conn) {
handler_called = true;
};
MetadataApiServer::HandlerMap handlers({{{"GET", "/testPath/"}, handler}});
MetadataApiServer server(config, store, 0, "TestHost", 1234, handlers);
MetadataApiServer::HttpServer::request request;

request.method = "GET";
request.destination = "/testPathFoo/";
CallDispatcher(&server, request, nullptr);
EXPECT_FALSE(handler_called);
request.destination = "/test/";
CallDispatcher(&server, request, nullptr);
EXPECT_FALSE(handler_called);
request.destination = "/testFooPath/";
CallDispatcher(&server, request, nullptr);
EXPECT_FALSE(handler_called);
}

} // namespace google