diff --git a/katran/lib/IpHelpers.cpp b/katran/lib/IpHelpers.cpp index b127a8bb8..925c4a192 100644 --- a/katran/lib/IpHelpers.cpp +++ b/katran/lib/IpHelpers.cpp @@ -16,7 +16,6 @@ #include "IpHelpers.h" -#include #include #include @@ -25,24 +24,27 @@ namespace katran { constexpr int Uint32_bytes = 4; constexpr uint8_t V6DADDR = 1; -struct beaddr IpHelpers::parseAddrToBe( - const std::string& addr, - bool bigendian) { - auto ipaddr = folly::IPAddress(addr); +struct beaddr IpHelpers::parseAddrToBe(const std::string &addr, + bool bigendian) { + return parseAddrToBe(folly::IPAddress(addr), bigendian); +} + +struct beaddr IpHelpers::parseAddrToBe(const folly::IPAddress &addr, + bool bigendian) { struct beaddr translated_addr = {}; - if (ipaddr.isV4()) { + if (addr.isV4()) { translated_addr.flags = 0; if (bigendian) { - translated_addr.daddr = ipaddr.asV4().toLong(); + translated_addr.daddr = addr.asV4().toLong(); } else { - translated_addr.daddr = ipaddr.asV4().toLongHBO(); + translated_addr.daddr = addr.asV4().toLongHBO(); } } else { for (int partition = 0; partition < 4; partition++) { // bytes() return a ptr to char* array // so we are doing some ptr arithmetics here uint32_t addr_part = - *(uint32_t*)(ipaddr.bytes() + Uint32_bytes * partition); + *(uint32_t *)(addr.bytes() + Uint32_bytes * partition); if (bigendian) { translated_addr.v6daddr[partition] = addr_part; } else { @@ -54,7 +56,11 @@ struct beaddr IpHelpers::parseAddrToBe( return translated_addr; }; -struct beaddr IpHelpers::parseAddrToInt(const std::string& addr) { +struct beaddr IpHelpers::parseAddrToInt(const std::string &addr) { + return parseAddrToBe(addr, false); +}; + +struct beaddr IpHelpers::parseAddrToInt(const folly::IPAddress &addr) { return parseAddrToBe(addr, false); }; diff --git a/katran/lib/IpHelpers.h b/katran/lib/IpHelpers.h index 4a03efc54..23751b663 100644 --- a/katran/lib/IpHelpers.h +++ b/katran/lib/IpHelpers.h @@ -16,6 +16,7 @@ #pragma once +#include #include namespace katran { @@ -33,7 +34,7 @@ struct beaddr { }; class IpHelpers { - public: +public: /** * @param const string addr address to translate * @return struct beaddr representation of given address @@ -42,10 +43,13 @@ class IpHelpers { * of beaddr structure. this function could throw, if given string is not * an ip address. */ - static struct beaddr parseAddrToBe( - const std::string& addr, - bool bigendian = true); - static struct beaddr parseAddrToInt(const std::string& addr); + static struct beaddr parseAddrToBe(const std::string &addr, + bool bigendian = true); + static struct beaddr parseAddrToInt(const std::string &addr); + + static struct beaddr parseAddrToBe(const folly::IPAddress &addr, + bool bigendian = true); + static struct beaddr parseAddrToInt(const folly::IPAddress &addr); }; } // namespace katran diff --git a/katran/lib/KatranLb.cpp b/katran/lib/KatranLb.cpp index 12fb0e06c..ab604bbb9 100644 --- a/katran/lib/KatranLb.cpp +++ b/katran/lib/KatranLb.cpp @@ -147,6 +147,7 @@ AddressType KatranLb::validateAddress( return AddressType::NETWORK; } } + lbStats_.addrValidationFailed++; LOG(ERROR) << "Invalid address: " << addr; return AddressType::INVALID; } @@ -561,11 +562,12 @@ bool KatranLb::modifyRealsForVip( return false; } auto cur_reals = vip_iter->second.getReals(); - for (auto& real : reals) { + for (const auto& real : reals) { if (validateAddress(real.address) == AddressType::INVALID) { LOG(ERROR) << "Invalid real's address: " << real.address; continue; } + folly::IPAddress raddr(real.address); VLOG(4) << folly::format( "modifying real: {} with weight {} for vip {}:{}:{}", real.address, @@ -575,7 +577,7 @@ bool KatranLb::modifyRealsForVip( vip.proto); if (action == ModifyAction::DEL) { - auto real_iter = reals_.find(real.address); + auto real_iter = reals_.find(raddr); if (real_iter == reals_.end()) { LOG(INFO) << "trying to delete non-existing real"; continue; @@ -588,20 +590,20 @@ bool KatranLb::modifyRealsForVip( continue; } ureal.updatedReal.num = real_iter->second.num; - decreaseRefCountForReal(real.address); + decreaseRefCountForReal(raddr); } else { - auto real_iter = reals_.find(real.address); + auto real_iter = reals_.find(raddr); if (real_iter != reals_.end()) { if (std::find( cur_reals.begin(), cur_reals.end(), real_iter->second.num) == cur_reals.end()) { // increment ref count if it's a new real for this vip - increaseRefCountForReal(real.address); + increaseRefCountForReal(raddr); cur_reals.push_back(real_iter->second.num); } ureal.updatedReal.num = real_iter->second.num; } else { - auto rnum = increaseRefCountForReal(real.address); + auto rnum = increaseRefCountForReal(raddr); if (rnum == config_.maxReals) { LOG(INFO) << "exhausted real's space"; continue; @@ -609,7 +611,7 @@ bool KatranLb::modifyRealsForVip( ureal.updatedReal.num = rnum; } ureal.updatedReal.weight = real.weight; - ureal.updatedReal.hash = folly::IPAddress(real.address).hash(); + ureal.updatedReal.hash = raddr.hash(); } ureals.push_back(ureal); } @@ -642,19 +644,21 @@ std::vector KatranLb::getRealsForVip(const VipKey& vip) { int i = 0; for (auto real_id : vip_reals_ids) { reals[i].weight = real_id.weight; - reals[i].address = numToReals_[real_id.num]; + reals[i].address = numToReals_[real_id.num].str(); ++i; } return reals; } int64_t KatranLb::getIndexForReal(const std::string& real) { - auto real_iter = reals_.find(real); - if (real_iter == reals_.end()) { - return kError; - } else { - return real_iter->second.num; + if (validateAddress(real) != AddressType::INVALID) { + folly::IPAddress raddr(real); + auto real_iter = reals_.find(raddr); + if (real_iter != reals_.end()) { + return real_iter->second.num; + } } + return kError; } int KatranLb::addSrcRoutingRule( @@ -713,7 +717,7 @@ int KatranLb::addSrcRoutingRule( // no point to continue. bailing out return kError; } - auto rnum = increaseRefCountForReal(dst); + auto rnum = increaseRefCountForReal(folly::IPAddress(dst)); if (rnum == config_.maxReals) { LOG(ERROR) << "exhausted real's space"; // all src using same dst. no point to continue if we can't add this dst @@ -794,7 +798,7 @@ std::unordered_map KatranLb::getSrcRoutingRule() { auto real = numToReals_[src.second]; auto src_network = folly::sformat("{}/{}", src.first.first.str(), src.first.second); - src_mapping[src_network] = real; + src_mapping[src_network] = real.str(); } return src_mapping; } @@ -808,11 +812,19 @@ KatranLb::getSrcRoutingRuleCidr() { } for (auto& src : lpmSrcMapping_) { auto real = numToReals_[src.second]; - src_mapping[src.first] = real; + src_mapping[src.first] = real.str(); } return src_mapping; } +const std::unordered_map KatranLb::getNumToRealMap() { + std::unordered_map reals; + for (const auto& real: numToReals_) { + reals[real.first] = real.second.str(); + } + return reals; +} + bool KatranLb::stopKatranMonitor() { if (!features_.introspection) { return false; @@ -910,7 +922,8 @@ bool KatranLb::addInlineDecapDst(const std::string& dst) { LOG(ERROR) << "invalid decap destination address: " << dst; return false; } - if (decapDsts_.find(dst) != decapDsts_.end()) { + folly::IPAddress daddr(dst); + if (decapDsts_.find(daddr) != decapDsts_.end()) { LOG(ERROR) << "trying to add already existing decap dst"; return false; } @@ -919,9 +932,9 @@ bool KatranLb::addInlineDecapDst(const std::string& dst) { return false; } VLOG(2) << "adding decap dst " << dst; - decapDsts_.insert(dst); + decapDsts_.insert(daddr); if (!config_.testing) { - modifyDecapDst(ModifyAction::ADD, dst); + modifyDecapDst(ModifyAction::ADD, daddr); } return true; } @@ -931,7 +944,12 @@ bool KatranLb::delInlineDecapDst(const std::string& dst) { LOG(ERROR) << "source based routing is not enabled in forwarding plane"; return false; } - auto dst_iter = decapDsts_.find(dst); + if (validateAddress(dst) == AddressType::INVALID) { + LOG(ERROR) << "provided address in invalid format: " << dst; + return false; + } + auto daddr = folly::IPAddress(dst); + auto dst_iter = decapDsts_.find(daddr); if (dst_iter == decapDsts_.end()) { LOG(ERROR) << "trying to delete non-existing decap dst " << dst; return false; @@ -939,7 +957,7 @@ bool KatranLb::delInlineDecapDst(const std::string& dst) { VLOG(2) << "deleting decap dst " << dst; decapDsts_.erase(dst_iter); if (!config_.testing) { - modifyDecapDst(ModifyAction::DEL, dst); + modifyDecapDst(ModifyAction::DEL, daddr); } return true; } @@ -947,14 +965,14 @@ bool KatranLb::delInlineDecapDst(const std::string& dst) { std::vector KatranLb::getInlineDecapDst() { std::vector dsts; for (auto& dst : decapDsts_) { - dsts.push_back(dst); + dsts.push_back(dst.str()); } return dsts; } bool KatranLb::modifyDecapDst( ModifyAction action, - const std::string& dst, + const folly::IPAddress& dst, uint32_t flags) { auto addr = IpHelpers::parseAddrToBe(dst); if (action == ModifyAction::ADD) { @@ -992,7 +1010,8 @@ void KatranLb::modifyQuicRealsMapping( continue; } VLOG(4) << folly::sformat("modifying quic's real {}", real.address); - auto real_iter = quicMapping_.find(real.address); + auto raddr = folly::IPAddress(real.address); + auto real_iter = quicMapping_.find(raddr); if (action == ModifyAction::DEL) { if (real_iter == quicMapping_.end()) { LOG(ERROR) << folly::sformat( @@ -1000,7 +1019,7 @@ void KatranLb::modifyQuicRealsMapping( real.address); continue; } - decreaseRefCountForReal(real.address); + decreaseRefCountForReal(raddr); quicMapping_.erase(real_iter); } else { if (real_iter != quicMapping_.end()) { @@ -1009,13 +1028,13 @@ void KatranLb::modifyQuicRealsMapping( // or we could silently delete old mapping instead. continue; } - auto rnum = increaseRefCountForReal(real.address); + auto rnum = increaseRefCountForReal(raddr); if (rnum == config_.maxReals) { LOG(ERROR) << "exhausted real's space"; continue; } to_update[real.id] = rnum; - quicMapping_[real.address] = real.id; + quicMapping_[raddr] = real.id; } } if (!config_.testing) { @@ -1038,7 +1057,7 @@ std::vector KatranLb::getQuicRealsMapping() { std::vector reals; QuicReal real; for (auto& mapping : quicMapping_) { - real.address = mapping.first; + real.address = mapping.first.str(); real.id = mapping.second; reals.push_back(real); } @@ -1119,7 +1138,7 @@ bool KatranLb::addHealthcheckerDst( } VLOG(4) << folly::format( "adding healtcheck with so_mark {} to dst {}", somark, dst); - + auto hcaddr = folly::IPAddress(dst); uint32_t key = somark; beaddr addr; @@ -1128,13 +1147,12 @@ bool KatranLb::addHealthcheckerDst( LOG(INFO) << "healthchecker's reals space exhausted"; return false; } - folly::IPAddress dst_addr(dst); // for md bassed tunnels remote_ipv4 must be in host endian format // and v6 in be - if (dst_addr.isV4()) { - addr = IpHelpers::parseAddrToInt(dst); + if (hcaddr.isV4()) { + addr = IpHelpers::parseAddrToInt(hcaddr); } else { - addr = IpHelpers::parseAddrToBe(dst); + addr = IpHelpers::parseAddrToBe(hcaddr); } if (!config_.testing) { auto res = bpfAdapter_.bpfUpdateMap( @@ -1145,7 +1163,7 @@ bool KatranLb::addHealthcheckerDst( return false; } } - hcReals_[somark] = dst; + hcReals_[somark] = hcaddr; return true; } @@ -1177,7 +1195,10 @@ bool KatranLb::delHealthcheckerDst(const uint32_t somark) { std::unordered_map KatranLb::getHealthcheckersDst() { // would be empty map in case if enableHc_ is false - std::unordered_map hcs(hcReals_); + std::unordered_map hcs; + for (const auto& hc : hcReals_) { + hcs[hc.first] = hc.second.str(); + } return hcs; } @@ -1214,7 +1235,7 @@ bool KatranLb::updateVipMap( return true; } -bool KatranLb::updateRealsMap(const std::string& real, uint32_t num) { +bool KatranLb::updateRealsMap(const folly::IPAddress& real, uint32_t num) { auto real_addr = IpHelpers::parseAddrToBe(real); auto res = bpfAdapter_.bpfUpdateMap( bpfAdapter_.getMapFdByName("reals"), &num, &real_addr); @@ -1227,7 +1248,7 @@ bool KatranLb::updateRealsMap(const std::string& real, uint32_t num) { } }; -void KatranLb::decreaseRefCountForReal(const std::string& real) { +void KatranLb::decreaseRefCountForReal(const folly::IPAddress& real) { auto real_iter = reals_.find(real); if (real_iter == reals_.end()) { // it's expected that caller must call this function only after explicit @@ -1244,7 +1265,7 @@ void KatranLb::decreaseRefCountForReal(const std::string& real) { } } -uint32_t KatranLb::increaseRefCountForReal(const std::string& real) { +uint32_t KatranLb::increaseRefCountForReal(const folly::IPAddress& real) { auto real_iter = reals_.find(real); if (real_iter != reals_.end()) { real_iter->second.refCount++; diff --git a/katran/lib/KatranLb.h b/katran/lib/KatranLb.h index 9c132fb34..c4233c5dc 100644 --- a/katran/lib/KatranLb.h +++ b/katran/lib/KatranLb.h @@ -328,9 +328,7 @@ class KatranLb { * * helper function to get internal index to real's ip address mapping. */ - const std::unordered_map& getNumToRealMap() { - return numToReals_; - } + const std::unordered_map getNumToRealMap(); /** * @return uint32_t number of src to dst mappings @@ -499,7 +497,7 @@ class KatranLb { /** * update(add or remove) reals map in forwarding plane */ - bool updateRealsMap(const std::string& real, uint32_t num); + bool updateRealsMap(const folly::IPAddress& real, uint32_t num); /** * helper function to get stats from counter on specified possition @@ -510,12 +508,12 @@ class KatranLb { * helper function to decrease real's ref count and delete it from * internal dicts if rec count became zero */ - void decreaseRefCountForReal(const std::string& real); + void decreaseRefCountForReal(const folly::IPAddress& real); /** * helper function to add new real or increase ref count for existing one */ - uint32_t increaseRefCountForReal(const std::string& real); + uint32_t increaseRefCountForReal(const folly::IPAddress& real); /** * helper function to do initial sanity checking right after bpf programs @@ -593,7 +591,7 @@ class KatranLb { */ bool modifyDecapDst( ModifyAction action, - const std::string& dst, + const folly::IPAddress& dst, const uint32_t flags = 0); /** @@ -627,15 +625,15 @@ class KatranLb { /** * dict of so_mark to real mapping; for healthchecking */ - std::unordered_map hcReals_; + std::unordered_map hcReals_; - std::unordered_map reals_; - std::unordered_map quicMapping_; + std::unordered_map reals_; + std::unordered_map quicMapping_; /** * for reverse real's lookup. get real by num. * used when we are going to delete vip and coresponding reals. */ - std::unordered_map numToReals_; + std::unordered_map numToReals_; std::unordered_map vips_; @@ -647,7 +645,7 @@ class KatranLb { /** * set of destantions, which are used for inline decapsulation. */ - std::unordered_set decapDsts_; + std::unordered_set decapDsts_; /** * flag which indicates if katran is working in "standalone" mode or not. diff --git a/katran/lib/KatranLbStructs.h b/katran/lib/KatranLbStructs.h index 443c762bd..b2c4c440e 100644 --- a/katran/lib/KatranLbStructs.h +++ b/katran/lib/KatranLbStructs.h @@ -190,6 +190,7 @@ struct KatranMonitorStats { /** * @param uint64_t bpfFailedCalls number of failed syscalls + * @param uint64_t addrValidationFailed times provided ipaddress was invalid * * generic userspace related stats to track internals of katran library * such as number of failed bpf syscalls (could happens if we are trying to add @@ -197,6 +198,7 @@ struct KatranMonitorStats { */ struct KatranLbStats { uint64_t bpfFailedCalls{0}; + uint64_t addrValidationFailed{0}; }; /** diff --git a/katran/lib/testing/katran_tester.cpp b/katran/lib/testing/katran_tester.cpp index 360dc9168..7985fa958 100644 --- a/katran/lib/testing/katran_tester.cpp +++ b/katran/lib/testing/katran_tester.cpp @@ -249,8 +249,16 @@ void testLbCounters(katran::KatranLb& lb) { auto lb_stats = lb.getKatranLbStats(); if (lb_stats.bpfFailedCalls != 0) { VLOG(2) << "failed bpf calls: " << lb_stats.bpfFailedCalls; - LOG(INFO) << "incorrect stats about katran library internals"; + LOG(INFO) << "incorrect stats about katran library internals: " + << "number of failed bpf syscalls is non zero"; } + if (lb_stats.addrValidationFailed != 0) { + VLOG(2) << "failed ip address validations: " + << lb_stats.addrValidationFailed; + LOG(INFO) << "incorrect stats about katran library internals: " + << "number of failed ip address validations is non zero"; + } + LOG(INFO) << "Testing of counters is complete"; return; } diff --git a/katran/lib/tests/KatranLbTest.cpp b/katran/lib/tests/KatranLbTest.cpp index fcd7b57ce..e4352f35b 100644 --- a/katran/lib/tests/KatranLbTest.cpp +++ b/katran/lib/tests/KatranLbTest.cpp @@ -293,6 +293,8 @@ TEST_F(KatranLbTest, invalidAddressHandling) { // adding incorrect hc dst res = lb.addHealthcheckerDst(1, "bbb"); ASSERT_FALSE(res); + auto stats = lb.getKatranLbStats(); + ASSERT_EQ(stats.addrValidationFailed, 3); }; TEST_F(KatranLbTest, addInvalidSrcRoutingRule) {