Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 29, check for bad client version #40

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Issue 29, check for bad client version #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c80c919
ISSUE-29: Add status code for bad client version.
dreamer-dead Nov 10, 2015
3e434a6
ISSUE-29: Add optional field client_version in Remote.
dreamer-dead Nov 10, 2015
6f8cc4d
ISSUE-29: Check for bad client version in Absorber.
dreamer-dead Nov 10, 2015
3c37592
ISSUE-29: Add new AbsorberTest.BadClientVersion unit test.
dreamer-dead Nov 10, 2015
09cb638
Rename client_version->emitter_version and add config field.
dreamer-dead Nov 11, 2015
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
9 changes: 9 additions & 0 deletions src/daemon/absorber.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,15 @@ bool Absorber::HandleNewMessage(net::ConnectionPtr connection,

if (message->HasExtension(proto::Remote::extension)) {
Message execute(message->ReleaseExtension(proto::Remote::extension));
auto config = conf();
if (execute->emitter_version() < config->absorber().min_emitter_version()) {
Copy link
Owner

Choose a reason for hiding this comment

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

Implement comparison for the complex version.

LOG(WARNING) << "Emitter sent a bad version: "
Copy link
Owner

Choose a reason for hiding this comment

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

Better not to mention emitter and just say about "incompatible" or "deprecated" client version - not the "bad" one. Also it's better to sync wording with the new name of the Status enum.

<< execute->emitter_version();
net::proto::Status bad_version_status;
bad_version_status.set_code(net::proto::Status::BAD_EMITTER_VERSION);
return connection->ReportStatus(bad_version_status);
}

DCHECK(!execute->flags().compiler().has_path());
if (execute->has_source()) {
return tasks_->Push(Task{connection, std::move(execute)});
Expand Down
160 changes: 156 additions & 4 deletions src/daemon/absorber_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,24 @@ namespace daemon {

namespace {

static const ui32 kEmptyEmitterVersion = 0u;
Copy link
Owner

Choose a reason for hiding this comment

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

no need for static in anonymous namespace.

static const ui32 kBadEmitterVersion = 1u;
Copy link
Owner

Choose a reason for hiding this comment

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

Anyway, it's better not to use a module-wide constants outside of test bodies - just for couple of them.

Copy link
Owner

Choose a reason for hiding this comment

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

This constant should be replaced with something like proto::Remote::default().version() - 1

static const ui32 kGoodEmitterVersion = 100u;
Copy link
Owner

Choose a reason for hiding this comment

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

This constant should be replaced with something like proto::Remote::default().version() + 1


// S1..4 types can be one of the following: |String|, |Immutable|, |Literal|
template <typename S1, typename S2, typename S3, typename S4 = String>
net::Connection::ScopedMessage CreateMessage(const S1& source,
const S2& action,
const S3& compiler_version,
const S4& language = S4()) {
net::Connection::ScopedMessage CreateMessage(
const S1& source,
const S2& action,
const S3& compiler_version,
const S4& language = S4(),
ui32 emitter_version = kEmptyEmitterVersion) {
net::Connection::ScopedMessage message(new net::Connection::Message);
auto* extension = message->MutableExtension(proto::Remote::extension);
extension->set_source(source);
if (emitter_version != kEmptyEmitterVersion)
Copy link
Owner

Choose a reason for hiding this comment

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

Forgot braces { }

extension->set_emitter_version(emitter_version);

auto* compiler = extension->mutable_flags()->mutable_compiler();
compiler->set_version(compiler_version);
extension->mutable_flags()->set_action(action);
Expand Down Expand Up @@ -374,5 +383,148 @@ TEST_F(AbsorberTest, BadMessageStatus) {
<< "Daemon must not store references to the connection";
}

TEST_F(AbsorberTest, BadEmitterVersion) {
Copy link
Owner

Choose a reason for hiding this comment

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

Better not to mention "Emitter".

const String expected_host = "fake_host";
const ui16 expected_port = 12345;
const String compiler_version("compiler_version");
const String compiler_path("compiler_path");

conf.mutable_absorber()->mutable_local()->set_host(expected_host);
conf.mutable_absorber()->mutable_local()->set_port(expected_port);
conf.mutable_absorber()->set_min_emitter_version(kGoodEmitterVersion);
auto* version = conf.add_versions();
version->set_version(compiler_version);
version->set_path(compiler_path);

listen_callback =
[&expected_host, expected_port](const String& host, ui16 port, String*) {
EXPECT_EQ(expected_host, host);
EXPECT_EQ(expected_port, port);
return true;
};
connect_callback = [](net::TestConnection* connection) {
connection->CallOnSend([](const net::Connection::Message& message) {
EXPECT_TRUE(message.HasExtension(net::proto::Status::extension));
const auto& status = message.GetExtension(net::proto::Status::extension);
EXPECT_EQ(net::proto::Status::BAD_EMITTER_VERSION, status.code());

EXPECT_FALSE(message.HasExtension(proto::Result::extension));
});
};

absorber.reset(new Absorber(conf));
ASSERT_TRUE(absorber->Initialize());

auto connection1 = test_service->TriggerListen(expected_host, expected_port);
{
SharedPtr<net::TestConnection> test_connection =
std::static_pointer_cast<net::TestConnection>(connection1);

auto message(CreateMessage(""_l, ""_l, ""_l, ""_l, kBadEmitterVersion));
EXPECT_TRUE(
test_connection->TriggerReadAsync(std::move(message), StatusOK()));

UniqueLock lock(send_mutex);
ASSERT_TRUE(send_condition.wait_for(lock, std::chrono::seconds(1),
[this] { return send_count == 1; }));
}

auto connection2 = test_service->TriggerListen(expected_host, expected_port);
Copy link
Owner

Choose a reason for hiding this comment

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

This part doesn't correspond to the test's purpose - according to its name. It's better to leave only the first connection.

{
SharedPtr<net::TestConnection> test_connection =
std::static_pointer_cast<net::TestConnection>(connection2);

test_connection->CallOnSend([](const net::Connection::Message& message) {
EXPECT_TRUE(message.HasExtension(net::proto::Status::extension));
const auto& status = message.GetExtension(net::proto::Status::extension);
EXPECT_EQ(net::proto::Status::OK, status.code());

EXPECT_TRUE(message.HasExtension(proto::Result::extension));
EXPECT_TRUE(message.GetExtension(proto::Result::extension).has_obj());
});
auto message(CreateMessage("source"_l, "action"_l, compiler_version));
auto* extension = message->MutableExtension(proto::Remote::extension);

// Force to use default value from .proto file
extension->clear_emitter_version();
EXPECT_TRUE(
test_connection->TriggerReadAsync(std::move(message), StatusOK()));

UniqueLock lock(send_mutex);
ASSERT_TRUE(send_condition.wait_for(lock, std::chrono::seconds(1),
[this] { return send_count == 2; }));
}

absorber.reset();

EXPECT_EQ(1u, run_count);
EXPECT_EQ(1u, listen_count);
EXPECT_EQ(2u, connect_count);
EXPECT_EQ(2u, connections_created);
EXPECT_EQ(2u, read_count);
EXPECT_EQ(2u, send_count);
EXPECT_EQ(1, connection1.use_count())
<< "Daemon must not store references to the connection";
EXPECT_EQ(1, connection2.use_count())
<< "Daemon must not store references to the connection";
}

TEST_F(AbsorberTest, GoodCustomEmitterVersion) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think, you should modify the existing SuccessfulCompilation test instead of making a new one.

const String expected_host = "fake_host";
const ui16 expected_port = 12345;
const String compiler_version("compiler_version");
const String compiler_path("compiler_path");
const ui32 custom_emitter_version = kGoodEmitterVersion + 1000;

conf.mutable_absorber()->mutable_local()->set_host(expected_host);
conf.mutable_absorber()->mutable_local()->set_port(expected_port);
conf.mutable_absorber()->set_min_emitter_version(custom_emitter_version);
auto* version = conf.add_versions();
version->set_version(compiler_version);
version->set_path(compiler_path);

listen_callback =
[&expected_host, expected_port](const String& host, ui16 port, String*) {
EXPECT_EQ(expected_host, host);
EXPECT_EQ(expected_port, port);
return true;
};
connect_callback = [](net::TestConnection* connection) {
connection->CallOnSend([](const net::Connection::Message& message) {
EXPECT_TRUE(message.HasExtension(net::proto::Status::extension));
const auto& status = message.GetExtension(net::proto::Status::extension);
EXPECT_EQ(net::proto::Status::OK, status.code());

EXPECT_TRUE(message.HasExtension(proto::Result::extension));
EXPECT_TRUE(message.GetExtension(proto::Result::extension).has_obj());
});
};

absorber.reset(new Absorber(conf));
ASSERT_TRUE(absorber->Initialize());

auto connection = test_service->TriggerListen(expected_host, expected_port);
{
SharedPtr<net::TestConnection> test_connection =
std::static_pointer_cast<net::TestConnection>(connection);

auto message(CreateMessage("source"_l, "action"_l, compiler_version, ""_l,
custom_emitter_version));
EXPECT_TRUE(
test_connection->TriggerReadAsync(std::move(message), StatusOK()));

absorber.reset();
}

EXPECT_EQ(1u, run_count);
EXPECT_EQ(1u, listen_count);
EXPECT_EQ(1u, connect_count);
EXPECT_EQ(1u, connections_created);
EXPECT_EQ(1u, read_count);
EXPECT_EQ(1u, send_count);
EXPECT_EQ(1, connection.use_count())
<< "Daemon must not store references to the connection";
}

} // namespace daemon
} // namespace dist_clang
3 changes: 2 additions & 1 deletion src/daemon/configuration.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ message Configuration {
}

message Absorber {
required Host local = 1;
required Host local = 1;
optional uint32 min_emitter_version = 2 [ default = 100 ];
Copy link
Owner

Choose a reason for hiding this comment

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

Use shared message type of version field from Remote message.
Also the default should be 0 - we accept everyone by default.

}

message Collector {
Expand Down
1 change: 1 addition & 0 deletions src/daemon/remote.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package dist_clang.daemon.proto;
message Remote {
optional base.proto.Flags flags = 1;
optional string source = 2;
optional uint32 emitter_version = 3 [ default = 100 ];
Copy link
Owner

Choose a reason for hiding this comment

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

Let it be simply "version" - and the default value should be real, i.e. the current version (790.0). Also the real version is complex and consists of two numbers: better to reflect it as a separate message type, like:

message Version {
  optional uint32 major = 1 [ default = 790 ];
  optional uint32 minor = 2 [ default = 0];
}

This message type should be shared with a its counterpart in Absorber message.


extend net.proto.Universal {
optional Remote extension = 6;
Expand Down
17 changes: 9 additions & 8 deletions src/net/universal.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@ message Universal {

message Status {
enum Code {
OK = 0;
INCONSEQUENT = 1;
NETWORK = 2;
BAD_MESSAGE = 3;
EMPTY_MESSAGE = 4;
EXECUTION = 5;
OVERLOAD = 6;
NO_VERSION = 7;
OK = 0;
INCONSEQUENT = 1;
NETWORK = 2;
BAD_MESSAGE = 3;
EMPTY_MESSAGE = 4;
EXECUTION = 5;
OVERLOAD = 6;
NO_VERSION = 7;
BAD_EMITTER_VERSION = 8;
Copy link
Owner

Choose a reason for hiding this comment

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

Better to choose one or two words, like BAD_VERSION, INCOMPATIBLE, DEPRECATED, etc. and not mention emitter.

}

required Code code = 1 [ default = OK ];
Expand Down