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

Add RPC/WS ports to server_info #4427

Merged
merged 3 commits into from
Jun 23, 2023
+121 −4
Merged

Add RPC/WS ports to server_info #4427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1593d05
Add RPC/WebSocket ports to server_info (#4427)
drlongle Feb 17, 2023
7cad80a
Publish peer and grpc ports to server_info
drlongle Feb 22, 2023
65ee0fd
Increase test coverage for RPC ServerInfo
drlongle May 22, 2023
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
7 changes: 7 additions & 0 deletions src/ripple/app/main/Application.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,13 @@ class ApplicationImp : public Application, public BasicApp
return *m_networkOPs;
}

virtual ServerHandlerImp&
getServerHandler() override
{
assert(serverHandler_);
return *serverHandler_;
}

boost::asio::io_service&
getIOService() override
{
Expand Down
3 changes: 3 additions & 0 deletions src/ripple/app/main/Application.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ class Overlay;
class PathRequests;
class PendingSaves;
class PublicKey;
class ServerHandlerImp;
class SecretKey;
class STLedgerEntry;
class TimeKeeper;
Expand Down Expand Up @@ -231,6 +232,8 @@ class Application : public beast::PropertyStream::Source
getOPs() = 0;
virtual OrderBookDB&
getOrderBookDB() = 0;
virtual ServerHandlerImp&
getServerHandler() = 0;
virtual TransactionMaster&
getMasterTransaction() = 0;
virtual perf::PerfLog&
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/app/main/GRPCServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ GRPCServerImpl::GRPCServerImpl(Application& app)
// if present, get endpoint from config
if (app_.config().exists("port_grpc"))
{
Section section = app_.config().section("port_grpc");
const auto& section = app_.config().section("port_grpc");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sneak in a minor improvement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The change you made here definitely is an improvement. You're now taking a reference instead of making a copy. And a Section is a heavy weight thing to copy. Good spotting.

However, not everyone will agree that replacing an explicit type name with auto is an improvement. In my opinion it hides important information that could be visible explicitly in the source code. My personal opinion is that Herb Sutter was dead wrong when he said "almost always auto" years ago. So my preferred declaration here would look like...

Section const& section = ...

Folks who use an IDE with something like intellisense sometimes lose track of those of us who who develop code using just a text editor. Explicit type names (as opposed to auto) also are a big help when viewing code through browsers, like on Github or in code reviews.

I'm not asking for a change here. Just providing a different perspective for you to think about when you make other changes in the future.

auto const optIp = section.get("ip");
if (!optIp)
Expand Down
47 changes: 47 additions & 0 deletions src/ripple/app/misc/NetworkOPs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,11 @@
#include <ripple/rpc/BookChanges.h>
#include <ripple/rpc/DeliveredAmount.h>
#include <ripple/rpc/impl/RPCHelpers.h>
#include <ripple/rpc/impl/ServerHandlerImp.h>
#include <boost/asio/ip/host_name.hpp>
#include <boost/asio/steady_timer.hpp>

#include <algorithm>
#include <mutex>
#include <string>
#include <tuple>
Expand Down Expand Up @@ -2661,6 +2663,51 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters)
info["reporting"] = app_.getReportingETL().getInfo();
}

// This array must be sorted in increasing order.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming this is because of std::set_intersection and Elements are compared using operator< and the ranges must be sorted with respect to the same.

Is there a way to static assert that it is sorted?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great question, and a good idea. Since we're running C++20, std::is_sorted() is now constexpr. So, if all of our standard libraries have that support, it should be easy to check at compile time. Otherwise it's still possible to check, but a bit more code to write.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice job with the static_assert. Looks great!

static constexpr std::array<std::string_view, 7> protocols{
"http", "https", "peer", "ws", "ws2", "wss", "wss2"};
static_assert(std::is_sorted(std::begin(protocols), std::end(protocols)));
{
Json::Value ports{Json::arrayValue};
for (auto const& port : app_.getServerHandler().setup().ports)
{
// Don't publish admin ports for non-admin users
if (!admin &&
!(port.admin_nets_v4.empty() && port.admin_nets_v6.empty() &&
port.admin_user.empty() && port.admin_password.empty()))
continue;
Comment on lines +2675 to +2678
Copy link
Collaborator

Choose a reason for hiding this comment

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

The continue inside the if (!admin && ...) condition is not exercised by the unit tests. Is there a way to tweak your test to exercise that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I modified my unit tests to cover that case.

std::vector<std::string> proto;
std::set_intersection(
std::begin(port.protocol),
std::end(port.protocol),
std::begin(protocols),
std::end(protocols),
std::back_inserter(proto));
if (!proto.empty())
{
auto& jv = ports.append(Json::Value(Json::objectValue));
jv[jss::port] = std::to_string(port.port);
jv[jss::protocol] = Json::Value{Json::arrayValue};
for (auto const& p : proto)
jv[jss::protocol].append(p);
}
}

if (app_.config().exists("port_grpc"))
{
auto const& grpcSection = app_.config().section("port_grpc");
auto const optPort = grpcSection.get("port");
if (optPort && grpcSection.get("ip"))
{
auto& jv = ports.append(Json::Value(Json::objectValue));
jv[jss::port] = *optPort;
jv[jss::protocol] = Json::Value{Json::arrayValue};
jv[jss::protocol].append("grpc");
}
}
info[jss::ports] = std::move(ports);
}

return info;
}

Expand Down
5 changes: 3 additions & 2 deletions src/ripple/protocol/jss.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,13 +459,14 @@ JSS(peers); // out: InboundLedger, handlers/Peers, Overlay
JSS(peer_disconnects); // Severed peer connection counter.
JSS(peer_disconnects_resources); // Severed peer connections because of
// excess resource consumption.
JSS(port); // in: Connect
JSS(port); // in: Connect, out: NetworkOPs
JSS(ports); // out: NetworkOPs
JSS(previous); // out: Reservations
JSS(previous_ledger); // out: LedgerPropose
JSS(proof); // in: BookOffers
JSS(propose_seq); // out: LedgerPropose
JSS(proposers); // out: NetworkOPs, LedgerConsensus
JSS(protocol); // out: PeerImp
JSS(protocol); // out: NetworkOPs, PeerImp
JSS(proxied); // out: RPC ping
JSS(pubkey_node); // out: NetworkOPs
JSS(pubkey_publisher); // out: ValidatorList
Expand Down
61 changes: 60 additions & 1 deletion src/test/rpc/ServerInfo_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
*/
//==============================================================================

#include <ripple/app/misc/NetworkOPs.h>
#include <ripple/beast/unit_test.h>
#include <ripple/protocol/jss.h>
#include <test/jtx.h>
Expand Down Expand Up @@ -55,6 +56,16 @@ class ServerInfo_test : public beast::unit_test::suite
[validators]
%2%
[port_grpc]
ip = 0.0.0.0
port = 50051
[port_admin]
ip = 0.0.0.0
port = 50052
protocol = wss2
admin = 127.0.0.1
)rippleConfig");

p->loadFromString(boost::str(
Expand All @@ -77,15 +88,63 @@ class ServerInfo_test : public beast::unit_test::suite
BEAST_EXPECT(result[jss::result][jss::status] == "success");
BEAST_EXPECT(result[jss::result].isMember(jss::info));
}

{
Env env(*this, makeValidatorConfig());
Env env(*this);

// Call NetworkOPs directly and set the admin flag to false.
// Expect that the admin ports are not included in the result.
auto const result =
env.app().getOPs().getServerInfo(true, false, 0);
auto const& ports = result[jss::ports];
BEAST_EXPECT(ports.isArray() && ports.size() == 0);
}

{
auto config = makeValidatorConfig();
auto const rpc_port =
(*config)["port_rpc"].get<unsigned int>("port");
auto const grpc_port =
(*config)["port_grpc"].get<unsigned int>("port");
auto const ws_port = (*config)["port_ws"].get<unsigned int>("port");
BEAST_EXPECT(grpc_port);
BEAST_EXPECT(rpc_port);
BEAST_EXPECT(ws_port);

Env env(*this, std::move(config));
auto const result = env.rpc("server_info");
BEAST_EXPECT(!result[jss::result].isMember(jss::error));
BEAST_EXPECT(result[jss::result][jss::status] == "success");
BEAST_EXPECT(result[jss::result].isMember(jss::info));
BEAST_EXPECT(
result[jss::result][jss::info][jss::pubkey_validator] ==
validator_data::public_key);

auto const& ports = result[jss::result][jss::info][jss::ports];
BEAST_EXPECT(ports.isArray() && ports.size() == 3);
for (auto const& port : ports)
{
auto const& proto = port[jss::protocol];
BEAST_EXPECT(proto.isArray());
auto const p = port[jss::port].asUInt();
BEAST_EXPECT(p == rpc_port || p == ws_port || p == grpc_port);
if (p == grpc_port)
{
BEAST_EXPECT(proto.size() == 1);
BEAST_EXPECT(proto[0u].asString() == "grpc");
}
if (p == rpc_port)
{
BEAST_EXPECT(proto.size() == 2);
BEAST_EXPECT(proto[0u].asString() == "http");
BEAST_EXPECT(proto[1u].asString() == "ws2");
}
if (p == ws_port)
{
BEAST_EXPECT(proto.size() == 1);
BEAST_EXPECT(proto[0u].asString() == "ws");
}
}
}
}

Expand Down