Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.
Merged
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
21 changes: 3 additions & 18 deletions src/api_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

#include "environment.h"
#include "format.h"
#include "http_common.h"
#include "json.h"
#include "logging.h"
#include "oauth2.h"
Expand Down Expand Up @@ -57,8 +58,6 @@ class MetadataApiServer {
private:
const MetadataAgent& agent_;
};
friend std::ostream& operator<<(
std::ostream& o, const HttpServer::request::headers_container_type& hv);

Handler handler_;
HttpServer server_;
Expand Down Expand Up @@ -87,20 +86,6 @@ class MetadataReporter {
std::thread reporter_thread_;
};


// To allow logging headers. TODO: move to a common location.
std::ostream& operator<<(
std::ostream& o,
const MetadataApiServer::HttpServer::request::headers_container_type& hv) {
o << "[";
for (const auto& h : hv) {
o << " " << h.name << ": " << h.value;
}
o << " ]";
return o;
}


MetadataApiServer::Handler::Handler(const MetadataAgent& agent)
: agent_(agent) {}

Expand Down Expand Up @@ -155,7 +140,7 @@ MetadataApiServer::MetadataApiServer(const MetadataAgent& agent,
server_pool_()
{
for (int i : boost::irange(0, server_threads)) {
server_pool_.emplace_back(std::bind(&HttpServer::run, &server_));
server_pool_.emplace_back(&HttpServer::run, &server_);
}
}

Expand All @@ -170,7 +155,7 @@ MetadataReporter::MetadataReporter(const MetadataAgent& agent, double period_s)
environment_(agent.config_),
auth_(environment_),
period_(period_s),
reporter_thread_(std::bind(&MetadataReporter::ReportMetadata, this)) {}
reporter_thread_(&MetadataReporter::ReportMetadata, this) {}

MetadataReporter::~MetadataReporter() {
reporter_thread_.join();
Expand Down
51 changes: 51 additions & 0 deletions src/http_common.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Copyright 2017 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
**/
#ifndef HTTP_COMMON_H_
#define HTTP_COMMON_H_

#include <boost/network/protocol/http/client.hpp>
#include <boost/network/protocol/http/server.hpp>
#include <iostream>

namespace google {

// Allow logging request headers.

Choose a reason for hiding this comment

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

What does "allow" mean in this context? The method seems straight forward though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means "enable" -- without this method, using log << req.headers() would fail to compile. However, "enable" is overloaded (e.g., could mean a config option), so I went with a more neutral word.

Choose a reason for hiding this comment

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

Ah, I see. Thanks for the clarification.

inline std::ostream& operator<<(
std::ostream& o,
const boost::network::http::basic_request<boost::network::http::tags::http_server>::headers_container_type& hv) {
o << "[";
for (const auto& h : hv) {
o << " " << h.name << ": " << h.value;
}
o << " ]";
return o;
}

// Allow logging response headers.
inline std::ostream& operator<<(
std::ostream& o,
const boost::network::http::basic_response<boost::network::http::BOOST_NETWORK_HTTP_CLIENT_DEFAULT_TAG>::headers_container_type& hv) {
o << "[";
for (const auto& h : hv) {
o << " " << h.first << ": " << h.second;
}
o << " ]";
return o;
}

}

#endif // HTTP_COMMON_H_
59 changes: 41 additions & 18 deletions src/json.cc
Original file line number Diff line number Diff line change
Expand Up @@ -223,17 +223,13 @@ class TopLevelContext : public Context {
public:
TopLevelContext() : Context(nullptr) {}
void AddValue(std::unique_ptr<Value> value) override {
if (value_ != nullptr) {
std::cerr << "Replacing " << *value_
<< " with " << *value << std::endl;
}
value_ = std::move(value);
values_.emplace_back(std::move(value));
}
std::unique_ptr<Value> value() {
return std::move(value_);
std::vector<std::unique_ptr<Value>> values() {
return std::move(values_);
}
private:
std::unique_ptr<Value> value_;
std::vector<std::unique_ptr<Value>> values_;
};

class ArrayContext : public Context {
Expand Down Expand Up @@ -320,13 +316,12 @@ class JSONBuilder {
}

// Top-level context only.
std::unique_ptr<Value> value() {
std::vector<std::unique_ptr<Value>> values() throw(Exception) {
TopLevelContext* top_level = dynamic_cast<TopLevelContext*>(context_);
if (top_level == nullptr) {
std::cerr << "value() called for an inner context" << std::endl;
return nullptr;
throw Exception("values() called for an inner context");
}
return top_level->value();
return top_level->values();
}

private:
Expand Down Expand Up @@ -424,13 +419,17 @@ yajl_callbacks callbacks = {

}

std::unique_ptr<Value> Parser::FromStream(std::istream& stream) {
std::vector<std::unique_ptr<Value>> Parser::AllFromStream(std::istream& stream)
throw(Exception)
{
JSONBuilder builder;

const int kMax = 65536;
unsigned char data[kMax];
yajl_handle handle = yajl_alloc(&callbacks, NULL, (void*) &builder);
yajl_config(handle, yajl_allow_comments, 1);
yajl_config(handle, yajl_allow_multiple_values, 1);
//yajl_config(handle, yajl_allow_trailing_garbage, 1);

Choose a reason for hiding this comment

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

Are we commenting out these two lines on purpose? If so, maybe leave a comment for the reason and under what circumstances we might want to uncomment them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. These were robustness changes that would allow us to talk to less formalized APIs. I decided we should instead do some validation on the Kubernetes API, but if it turns out to be buggy or unstable, we may end up uncommenting these.
I thought that the option names (allow_trailing_garbage and dont_validate_strings) were self-descriptive enough that they didn't merit a separate comment.

Choose a reason for hiding this comment

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

Ah, I see. Now I understand.

//yajl_config(handle, yajl_dont_validate_strings, 1);

for (;;) {
Expand All @@ -446,17 +445,41 @@ std::unique_ptr<Value> Parser::FromStream(std::istream& stream) {

if (stat != yajl_status_ok) {
unsigned char* str = yajl_get_error(handle, 1, data, kMax);
// TODO cerr << str << endl;
std::string error_str((const char*)str);
yajl_free_error(handle, str);
return nullptr;
throw Exception(error_str);
}

yajl_free(handle);
return builder.value();
return builder.values();
}

std::unique_ptr<Value> Parser::FromStream(std::istream& stream)
throw(Exception)
{
std::vector<std::unique_ptr<Value>> all_values = AllFromStream(stream);
if (all_values.empty()) {
return nullptr;
}
if (all_values.size() > 1) {
std::ostringstream out;
out << "Single value expected, " << all_values.size() << " values seen";
throw Exception(out.str());
}
return std::move(all_values[0]);
}

std::vector<std::unique_ptr<Value>> Parser::AllFromString(
const std::string& input) throw(Exception)
{
std::istringstream stream(input);
return AllFromStream(stream);
}

std::unique_ptr<Value> Parser::FromString(const std::string& input) {
std::stringstream stream(input);
std::unique_ptr<Value> Parser::FromString(const std::string& input)
throw(Exception)
{
std::istringstream stream(input);
return FromStream(stream);
}

Expand Down
25 changes: 17 additions & 8 deletions src/json.h
Original file line number Diff line number Diff line change
Expand Up @@ -247,13 +247,13 @@ class Object : public Value, public std::map<std::string, std::unique_ptr<Value>
const T* GetField(const std::string& field) const throw(Exception) {
auto value_it = find(field);
if (value_it == end()) {
throw json::Exception("There is no " + field + " in " + ToString());
throw Exception("There is no " + field + " in " + ToString());
}
if (value_it->second->type() != internal::TypeHelper<T>::type) {
constexpr const char* name = internal::TypeHelper<T>::name;
constexpr const char* article = internal::ArticleHelper<name[0]>::value();
throw json::Exception(field + " " + value_it->second->ToString() +
" is not " + article + " " + name);
throw Exception(field + " " + value_it->second->ToString() +
" is not " + article + " " + name);
}
return value_it->second->As<T>();
}
Expand All @@ -267,7 +267,8 @@ class Object : public Value, public std::map<std::string, std::unique_ptr<Value>
struct FieldGetter {
using return_type = const T*;
static const T* GetField(const Object* obj, const std::string& field)
throw(Exception) {
throw(Exception)
{
return obj->GetField<T>(field);
}
};
Expand All @@ -277,7 +278,8 @@ class Object : public Value, public std::map<std::string, std::unique_ptr<Value>
using value_type = typename std::result_of<decltype(&T::value)(T)>::type;
using return_type = typename std::remove_reference<value_type>::type;
static return_type GetField(const Object* obj, const std::string& field)
throw(Exception) {
throw(Exception)
{
return obj->GetField<T>(field)->value();
}
};
Expand All @@ -288,7 +290,8 @@ class Object : public Value, public std::map<std::string, std::unique_ptr<Value>
// Field accessors.
template<class T>
typename FieldGetter<T>::return_type Get(const std::string& field) const
throw(Exception) {
throw(Exception)
{
return FieldGetter<T>::GetField(this, field);
}

Expand Down Expand Up @@ -328,8 +331,14 @@ inline std::unique_ptr<Value> object(

class Parser {
public:
static std::unique_ptr<Value> FromStream(std::istream& stream);
static std::unique_ptr<Value> FromString(const std::string& input);
static std::vector<std::unique_ptr<Value>> AllFromStream(
std::istream& stream) throw(Exception);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we limit the throw to a json::Exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is json::Exception. This code is within the json namespace.

Copy link
Contributor

@bmoyles0117 bmoyles0117 Dec 19, 2017

Choose a reason for hiding this comment

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

Ahh I was confused, why is it that we need to include json:: here despite also being in the json namespace?

throw json::Exception("There is no " + field + " in " + ToString());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a typo. Fixed (there and in json.cc).

static std::vector<std::unique_ptr<Value>> AllFromString(
const std::string& input) throw(Exception);
static std::unique_ptr<Value> FromStream(std::istream& stream)
throw(Exception);
static std::unique_ptr<Value> FromString(const std::string& input)
throw(Exception);
};

}
Expand Down
14 changes: 1 addition & 13 deletions src/oauth2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <openssl/pkcs12.h>

#include "base64.h"
#include "http_common.h"
#include "json.h"
#include "logging.h"

Expand Down Expand Up @@ -145,19 +146,6 @@ double SecondsSinceEpoch(

}

// To allow logging headers. TODO: move to a common location.
std::ostream& operator<<(
std::ostream& o,
const http::client::request::headers_container_type& hv) {
o << "[";
for (const auto& h : hv) {
o << " " << h.first << ": " << h.second;
}
o << " ]";
return o;
}


json::value OAuth2::ComputeTokenFromCredentials() const {
const std::string service_account_email =
environment_.CredentialsClientEmail();
Expand Down
2 changes: 1 addition & 1 deletion src/updater.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ void PollingMetadataUpdater::start() {
LOG(INFO) << "Timer locked";
}
reporter_thread_ =
std::thread(std::bind(&PollingMetadataUpdater::PollForMetadata, this));
std::thread(&PollingMetadataUpdater::PollForMetadata, this);
}

void PollingMetadataUpdater::stop() {
Expand Down