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.

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

Allow programmatic configuration of unicast relays. #497

Closed
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
40 changes: 20 additions & 20 deletions include/gz/transport/Discovery.hh
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,26 @@ namespace gz
}
}

/// \brief Register a new relay address.
/// \param[in] _ip New IP address.
public: void AddRelayAddress(const std::string &_ip)
{
// Sanity check: Make sure that this IP address is not already saved.
for (auto const &addr : this->relayAddrs)
{
if (addr.sin_addr.s_addr == inet_addr(_ip.c_str()))
return;
}

sockaddr_in addr;
memset(&addr, 0, sizeof(addr));
Copy link
Contributor

Choose a reason for hiding this comment

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

include <cstring>

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 file already includes <string.h> - should I swap the include?

addr.sin_family = AF_INET;
addr.sin_addr.s_addr = inet_addr(_ip.c_str());
addr.sin_port = htons(static_cast<u_short>(this->port));

this->relayAddrs.push_back(addr);
}

/// \brief Broadcast periodic heartbeats.
private: void UpdateHeartbeat()
{
Expand Down Expand Up @@ -1420,26 +1440,6 @@ namespace gz
return true;
}

/// \brief Register a new relay address.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you just moved this function. Out of curiosity, any reason for moving it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it up to sit with the other public functions. ("Group similar declarations together, placing public parts earlier." https://google.github.io/styleguide/cppguide.html#Declaration_Order)

/// \param[in] _ip New IP address.
private: void AddRelayAddress(const std::string &_ip)
{
// Sanity check: Make sure that this IP address is not already saved.
for (auto const &addr : this->relayAddrs)
{
if (addr.sin_addr.s_addr == inet_addr(_ip.c_str()))
return;
}

sockaddr_in addr;
memset(&addr, 0, sizeof(addr));
addr.sin_family = AF_INET;
addr.sin_addr.s_addr = inet_addr(_ip.c_str());
addr.sin_port = htons(static_cast<u_short>(this->port));

this->relayAddrs.push_back(addr);
}

/// \brief Default activity interval value (ms.).
/// \sa ActivityInterval.
/// \sa SetActivityInterval.
Expand Down
14 changes: 14 additions & 0 deletions include/gz/transport/NodeOptions.hh
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include <memory>
#include <string>
#include <vector>

#include "gz/transport/config.hh"
#include "gz/transport/Export.hh"
Expand Down Expand Up @@ -126,6 +127,19 @@ namespace gz
public: bool TopicRemap(const std::string &_fromTopic,
std::string &_toTopic) const;

/// \brief Add relay IPs. This node will send UDP unicast traffic to these
/// addresses to connect networks when UDP multicast traffic is not
/// forwarded.
/// It's also possible to use the environment variable GZ_RELAY to add
/// relays.
/// \param[in] _relayIPs IPv4 addresses of unicast relays to add.
/// \return True if the relay list is valid or false otherwise.
public: bool SetRelays(const std::vector<std::string>& _relayIPs);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rename this function to AddRelays() to be more precise. You're not replacing the current relays, just adding extra ones.


/// \brief Gets the list of relay addresses specified in this NodeOptions.
/// \return The list of relay addresses.
public: const std::vector<std::string>& Relays() const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have two options here:

  1. Do not create a member variable this->dataPtr->relayIPs when adding the relays. Then, just add the relays using AddRelayAddress() as you're doing it right now. Then, in this function, read the entire list of relays from Discovery.
  2. Keep it as it's documenting that the returned vector of relays is the contribution from this node. Other relays could be happening.

I suspect (1) is going to be less confusing and will make debugging easier.


#ifdef _WIN32
// Disable warning C4251 which is triggered by
// std::unique_ptr
Expand Down
6 changes: 6 additions & 0 deletions src/Node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,12 @@ Node::Node(const NodeOptions &_options)

// Save the options.
this->dataPtr->options = _options;

// Add relays from the node options.
for(const auto& addr : _options.Relays()) {
this->dataPtr->shared->dataPtr->msgDiscovery->AddRelayAddress(addr);
this->dataPtr->shared->dataPtr->srvDiscovery->AddRelayAddress(addr);
}
}

//////////////////////////////////////////////////
Expand Down
13 changes: 13 additions & 0 deletions src/NodeOptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include <iostream>
#include <string>
#include <vector>

#include "gz/transport/Helpers.hh"
#include "gz/transport/NodeOptions.hh"
Expand Down Expand Up @@ -136,3 +137,15 @@ bool NodeOptions::TopicRemap(const std::string &_fromTopic,

return topicIt != this->dataPtr->topicsRemap.end();
}

//////////////////////////////////////////////////
bool NodeOptions::SetRelays(const std::vector<std::string>& _relayIPs) {
mbeards marked this conversation as resolved.
Show resolved Hide resolved
this->dataPtr->relayIPs = _relayIPs;
return true;
}

//////////////////////////////////////////////////
const std::vector<std::string>& NodeOptions::Relays() const {
return this->dataPtr->relayIPs;
}

4 changes: 4 additions & 0 deletions src/NodeOptionsPrivate.hh
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include <map>
#include <string>
#include <vector>

#include "gz/transport/config.hh"
#include "gz/transport/NetUtils.hh"
Expand Down Expand Up @@ -50,6 +51,9 @@ namespace gz
/// \brief Table of remappings. The key is the original topic name and
/// its value is the new topic name to be used instead.
public: std::map<std::string, std::string> topicsRemap;

/// \brief List of unicast relay IPs.
public: std::vector<std::string> relayIPs;
};
}
}
Expand Down
7 changes: 7 additions & 0 deletions src/NodeOptions_TEST.cc
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,11 @@ TEST(NodeOptionsTest, accessors)
EXPECT_EQ(opts.Partition(), defaultPartition);
EXPECT_TRUE(opts.SetPartition(aPartition));
EXPECT_EQ(opts.Partition(), aPartition);

// Relay
std::string aRelay = "123.45.67.89";
EXPECT_TRUE(opts.SetRelays({aRelay}));
auto relays = opts.Relays();
ASSERT_EQ(relays.size(), 1);
EXPECT_EQ(relays[0], aRelay);
}