trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From zw...@apache.org
Subject [1/2] trafficserver git commit: TS-4284 Refactors and cleans up the geoip_acl plugin
Date Sat, 26 Mar 2016 03:54:29 GMT
Repository: trafficserver
Updated Branches:
  refs/heads/master 1419f4901 -> 5b2622eac


TS-4284 Refactors and cleans up the geoip_acl plugin


Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/acd06a7d
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/acd06a7d
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/acd06a7d

Branch: refs/heads/master
Commit: acd06a7d230e7c8a7155d42b3f5b818bfe868ddf
Parents: 1419f49
Author: Leif Hedstrom <zwoop@apache.org>
Authored: Fri Mar 18 16:13:20 2016 -0600
Committer: Leif Hedstrom <zwoop@apache.org>
Committed: Fri Mar 25 13:03:06 2016 -0600

----------------------------------------------------------------------
 build/common.m4                             |  24 ----
 configure.ac                                |  12 +-
 plugins/experimental/geoip_acl/Makefile.am  |   6 +-
 plugins/experimental/geoip_acl/acl.cc       | 144 ++++++++++++++++-------
 plugins/experimental/geoip_acl/acl.h        |  32 ++---
 plugins/experimental/geoip_acl/geoip_acl.cc |  41 +++----
 6 files changed, 139 insertions(+), 120 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/trafficserver/blob/acd06a7d/build/common.m4
----------------------------------------------------------------------
diff --git a/build/common.m4 b/build/common.m4
index 42f316c..5b5081a 100644
--- a/build/common.m4
+++ b/build/common.m4
@@ -556,30 +556,6 @@ AC_DEFUN([TS_ARG_ENABLE_VAR],[
   AC_SUBST(m4_join([_], $1, AS_TR_SH($2)))
 ])
 
-dnl
-dnl TS_SEARCH_LIBRARY(function, search-libs, [action-if-found], [action-if-not-found])
-dnl This macro works like AC_SEARCH_LIBS, except that $LIBS is not modified. If the library
-dnl is found, it is cached in the ts_cv_lib_${function} variable.
-dnl
-AC_DEFUN([TS_SEARCH_LIBRARY], [
-  __saved_LIBS="$LIBS"
-
-  AC_SEARCH_LIBS($1, $2, [
-    dnl action-if-found
-    case $ac_cv_search_$1 in
-    "none required"|"no") ts_cv_search_$1="" ;;
-    *) ts_cv_search_$1=$ac_cv_search_$1 ;;
-    esac
-    m4_default([$3], [true])
-  ], [
-    dnl action-if-not-found
-    m4_default([$4], [true])
-  ])
-
-  LIBS="$__saved_LIBS"
-  unset __saved_LIBS
-])
-
 dnl TS_CHECK_SOCKOPT(socket-option, [action-if-found], [action-if-not-found]
 AC_DEFUN([TS_CHECK_SOCKOPT], [
   AC_MSG_CHECKING([for $1 socket option])

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/acd06a7d/configure.ac
----------------------------------------------------------------------
diff --git a/configure.ac b/configure.ac
index 26e446e..052bff9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1406,14 +1406,14 @@ AC_SUBST(use_hwloc)
 # GeoIP as a "helper" plugin, which other plugins can then use. Such a plugin could
 # then manage which libraries to use via explicit dlopen()'s.
 #
-enable_maxmind_geoip=no
-TS_SEARCH_LIBRARY([GeoIP_id_by_code], [GeoIP], [
-  GEOIP_LIBS=$ts_cv_search_GeoIP_id_by_code
-  AC_CHECK_HEADERS([GeoIP.h], [ enable_maxmind_geoip=yes ])
+AC_CHECK_HEADERS([GeoIP.h], [
+  AC_CHECK_LIB([GeoIP], [GeoIP_new], [
+    AC_SUBST([GEO_LIBS], ["-lGeoIP"])
+  ], [
+    AC_SUBST([GEO_LIBS], [""])
+  ])
 ])
 
-AC_SUBST(GEOIP_LIBS)
-AM_CONDITIONAL([BUILD_GEOIP_PLUGIN], [ test "x${enable_maxmind_geoip}" = x"yes" ])
 
 # Right now, the healthcheck plugins requires inotify_init (and friends)
 AM_CONDITIONAL([BUILD_HEALTHCHECK_PLUGIN], [ test "$ac_cv_func_inotify_init" = "yes" ])

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/acd06a7d/plugins/experimental/geoip_acl/Makefile.am
----------------------------------------------------------------------
diff --git a/plugins/experimental/geoip_acl/Makefile.am b/plugins/experimental/geoip_acl/Makefile.am
index 0b4e4ff..34fb0c3 100644
--- a/plugins/experimental/geoip_acl/Makefile.am
+++ b/plugins/experimental/geoip_acl/Makefile.am
@@ -16,11 +16,7 @@
 
 include $(top_srcdir)/build/plugins.mk
 
-if BUILD_GEOIP_PLUGIN
-
 pkglib_LTLIBRARIES = geoip_acl.la
 geoip_acl_la_SOURCES = acl.cc geoip_acl.cc
 geoip_acl_la_LDFLAGS = $(TS_PLUGIN_LDFLAGS)
-geoip_acl_la_LIBADD = $(GEOIP_LIBS)
-
-endif
+geoip_acl_la_LIBADD = $(GEO_LIBS)

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/acd06a7d/plugins/experimental/geoip_acl/acl.cc
----------------------------------------------------------------------
diff --git a/plugins/experimental/geoip_acl/acl.cc b/plugins/experimental/geoip_acl/acl.cc
index c40590b..b7048a1 100644
--- a/plugins/experimental/geoip_acl/acl.cc
+++ b/plugins/experimental/geoip_acl/acl.cc
@@ -24,10 +24,76 @@
 #include "acl.h"
 #include "lulu.h"
 
-// Global GeoIP object.
-GeoIP *gGI;
 
-// Implementation of the ACL base class.
+// Implementation of the ACL base class. This wraps the underlying Geo library
+// that we've found and used.
+GeoDBHandle Acl::_geoip;
+
+// Maxmind v1 APIs
+#if HAVE_GEOIP_H
+bool
+Acl::init()
+{
+  TSDebug(PLUGIN_NAME, "Initialized with Maxmind GeoIP (v1)");
+  _geoip = GeoIP_new(GEOIP_MMAP_CACHE); // GEOIP_STANDARD seems to break threaded apps...
+
+  return true;
+}
+
+int
+Acl::country_id_by_code(const std::string &str) const
+{
+  return GeoIP_id_by_code(str.c_str());
+}
+
+int
+Acl::country_id_by_addr(const sockaddr *addr) const
+{
+  int iso = -1;
+
+  switch (addr->sa_family) {
+  case AF_INET: {
+    uint32_t ip = ntohl(reinterpret_cast<const struct sockaddr_in *>(addr)->sin_addr.s_addr);
+
+    iso = GeoIP_id_by_ipnum(_geoip, ip);
+  } break;
+  case AF_INET6: {
+    TSDebug(PLUGIN_NAME, "eval(): Unsupported protocol, IPv6");
+  } break;
+  default:
+    break;
+  }
+
+  TSDebug(PLUGIN_NAME, "eval(): Client IP seems to come from ISO=%d", iso);
+  return iso;
+}
+#else  /* !HAVE_GEOIP_H */
+
+// No library available, nothing will work :)
+bool
+Acl::init()
+{
+  TSDebug(PLUGIN_NAME, "No Geo library available!");
+  TSError("[%s] No Geo library available!", PLUGIN_NAME);
+
+  return false;
+}
+
+int
+Acl::country_id_by_code(const std::string &str) const
+{
+  return -1;
+}
+
+int
+Acl::country_id_by_addr(const sockaddr *addr) const
+{
+  return -1;
+}
+#endif /* HAVE_GEOIP_H */
+
+
+// This is the rest of the ACL baseclass, which is the same for all underlying Geo libraries.
 void
 Acl::read_html(const char *fn)
 {
@@ -39,7 +105,7 @@ Acl::read_html(const char *fn)
     f.close();
     TSDebug(PLUGIN_NAME, "Loaded HTML from %s", fn);
   } else {
-    TSError("[geoip_acl] Unable to open HTML file %s", fn);
+    TSError("[%s] Unable to open HTML file %s", PLUGIN_NAME, fn);
   }
 }
 
@@ -52,11 +118,13 @@ RegexAcl::parse_line(const char *filename, const std::string &line,
int lineno)
   std::string regex, tmp;
   std::string::size_type pos1, pos2;
 
-  if (line.empty())
+  if (line.empty()) {
     return false;
+  }
   pos1 = line.find_first_not_of(_SEPARATOR);
-  if (line[pos1] == '#' || pos1 == std::string::npos)
+  if (line[pos1] == '#' || pos1 == std::string::npos) {
     return false;
+  }
 
   pos2 = line.find_first_of(_SEPARATOR, pos1);
   if (pos2 != std::string::npos) {
@@ -66,12 +134,12 @@ RegexAcl::parse_line(const char *filename, const std::string &line,
int lineno)
       pos2 = line.find_first_of(_SEPARATOR, pos1);
       if (pos2 != std::string::npos) {
         tmp = line.substr(pos1, pos2 - pos1);
-        if (tmp == "allow")
+        if (tmp == "allow") {
           _acl->set_allow(true);
-        else if (tmp == "deny")
+        } else if (tmp == "deny") {
           _acl->set_allow(false);
-        else {
-          TSError("[geoip_acl] Bad action on in %s:line %d: %s", filename, lineno, tmp.c_str());
+        } else {
+          TSError("[%s] Bad action on in %s:line %d: %s", PLUGIN_NAME, filename, lineno,
tmp.c_str());
           return false;
         }
         // The rest are "tokens"
@@ -102,11 +170,12 @@ RegexAcl::compile(const std::string &str, const char *filename,
int lineno)
   if (NULL != _rex) {
     _extra = pcre_study(_rex, 0, &error);
     if ((NULL == _extra) && error && (*error != 0)) {
-      TSError("[geoip_acl] Failed to study regular expression in %s:line %d at offset %d:
%s", filename, lineno, erroffset, error);
+      TSError("[%s] Failed to study regular expression in %s:line %d at offset %d: %s", PLUGIN_NAME,
filename, lineno, erroffset,
+              error);
       return false;
     }
   } else {
-    TSError("[geoip_acl] Failed to compile regular expression in %s:line %d: %s", filename,
lineno, error);
+    TSError("[%s] Failed to compile regular expression in %s:line %d: %s", PLUGIN_NAME, filename,
lineno, error);
     return false;
   }
 
@@ -134,14 +203,12 @@ CountryAcl::add_token(const std::string &str)
 {
   int iso = -1;
 
-  Acl::add_token(str);
-  iso = GeoIP_id_by_code(str.c_str());
-
+  iso = country_id_by_code(str.c_str());
   if (iso > 0 && iso < NUM_ISO_CODES) {
     _iso_country_codes[iso] = true;
-    TSDebug(PLUGIN_NAME, "Added %s(%d) to remap rule, ACL=%d", str.c_str(), iso, _allow);
+    TSDebug(PLUGIN_NAME, "Added %s(%d) to remap rule, ACL=%s", str.c_str(), iso, _allow ?
"allow" : "deny");
   } else {
-    TSError("[geoip_acl] Tried setting an ISO code (%d) outside the supported range", iso);
+    TSError("[%s] Tried setting an ISO code (%d) outside the supported range", PLUGIN_NAME,
iso);
   }
 }
 
@@ -159,20 +226,22 @@ CountryAcl::read_regex(const char *fn)
     while (!f.eof()) {
       getline(f, line);
       ++lineno;
-      if (!acl)
+      if (!acl) {
         acl = new RegexAcl(new CountryAcl());
+      }
       if (acl->parse_line(fn, line, lineno)) {
-        if (NULL == _regexes)
+        if (NULL == _regexes) {
           _regexes = acl;
-        else
+        } else {
           _regexes->append(acl);
+        }
         acl = NULL;
       }
     }
     f.close();
     TSDebug(PLUGIN_NAME, "Loaded regex rules from %s", fn);
   } else {
-    TSError("[geoip_acl] Unable to open regex file %s", fn);
+    TSError("[%s] Unable to open regex file %s", PLUGIN_NAME, fn);
   }
 }
 
@@ -195,40 +264,22 @@ CountryAcl::eval(TSRemapRequestInfo *rri, TSHttpTxn txnp) const
     } while ((acl = acl->next()));
   }
 
-  // This is a special case for when there are no ISO codes. It got kinda wonky without it.
-  if (0 == _added_tokens)
-    return _allow;
-
   // None of the regexes (if any) matched, so fallback to the remap defaults if there are
any.
-  int iso = -1;
-  const sockaddr *addr = TSHttpTxnClientAddrGet(txnp);
+  int iso = country_id_by_addr(TSHttpTxnClientAddrGet(txnp));
 
-  switch (addr->sa_family) {
-  case AF_INET: {
-    uint32_t ip = ntohl(reinterpret_cast<const struct sockaddr_in *>(addr)->sin_addr.s_addr);
-
-    iso = GeoIP_id_by_ipnum(gGI, ip);
-    if (TSIsDebugTagSet(PLUGIN_NAME)) {
-      const char *c = GeoIP_country_code_by_ipnum(gGI, ip);
-      TSDebug(PLUGIN_NAME, "eval(): IP=%u seems to come from ISO=%d / %s", ip, iso, c);
-    }
-  } break;
-  case AF_INET6:
-    return true;
-  default:
-    break;
-  }
-
-  if ((iso <= 0) || (!_iso_country_codes[iso]))
+  if ((iso <= 0) || (!_iso_country_codes[iso])) {
     return !_allow;
+  }
 
   return _allow;
 }
 
 
-void
+int
 CountryAcl::process_args(int argc, char *argv[])
 {
+  int tokens = 0;
+
   for (int i = 3; i < argc; ++i) {
     if (!strncmp(argv[i], "allow", 5)) {
       _allow = true;
@@ -240,6 +291,9 @@ CountryAcl::process_args(int argc, char *argv[])
       read_html(argv[i] + 6);
     } else { // ISO codes assumed for the rest
       add_token(argv[i]);
+      ++tokens;
     }
   }
+
+  return tokens;
 }

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/acd06a7d/plugins/experimental/geoip_acl/acl.h
----------------------------------------------------------------------
diff --git a/plugins/experimental/geoip_acl/acl.h b/plugins/experimental/geoip_acl/acl.h
index e9be954..3af88d9 100644
--- a/plugins/experimental/geoip_acl/acl.h
+++ b/plugins/experimental/geoip_acl/acl.h
@@ -29,14 +29,14 @@
 #endif
 
 #include <string>
-
 #include "lulu.h"
 
 #if HAVE_GEOIP_H
 #include <GeoIP.h>
-#endif
-
-extern GeoIP *gGI;
+typedef GeoIP *GeoDBHandle;
+#else  /* !HAVE_GEOIP_H */
+typedef void *GeoDBHandle;
+#endif /* HAVE_GEOIP_H */
 
 // See http://www.iso.org/iso/english_country_names_and_code_elements
 // Maxmind allocates 253 country codes,even though there are only 248 according to the above
@@ -47,20 +47,16 @@ static const int NUM_ISO_CODES = 253;
 class Acl
 {
 public:
-  Acl() : _allow(true), _added_tokens(0) {}
+  Acl() : _allow(true) {}
 
   virtual ~Acl() {}
 
   // These have to be implemented for each ACL type
   virtual void read_regex(const char *fn) = 0;
-  virtual void process_args(int argc, char *argv[]) = 0;
+  virtual int process_args(int argc, char *argv[]) = 0;
   virtual bool eval(TSRemapRequestInfo *rri, TSHttpTxn txnp) const = 0;
+  virtual void add_token(const std::string &str) = 0;
 
-  virtual void
-  add_token(const std::string & /* str */)
-  {
-    ++_added_tokens;
-  }
   void
   set_allow(bool allow)
   {
@@ -79,10 +75,16 @@ public:
 
   void read_html(const char *fn);
 
+  int country_id_by_code(const std::string &str) const;
+  int country_id_by_addr(const sockaddr *addr) const;
+
+  static bool init();
+
 protected:
   std::string _html;
   bool _allow;
   int _added_tokens;
+  static GeoDBHandle _geoip;
 };
 
 
@@ -111,10 +113,10 @@ public:
   bool
   match(const char *str, int len) const
   {
-    // TODO: Not 100% sure this is absolutely correct, and not sure why adding
-    // PCRE_NOTEMPTY to the options doesn't work ...
-    if (0 == len)
+    if (0 == len) {
       return false;
+    }
+
     return (pcre_exec(_rex, _extra, str, len, 0, PCRE_NOTEMPTY, NULL, 0) != -1);
   }
 
@@ -138,7 +140,7 @@ public:
   CountryAcl() : _regexes(NULL) { memset(_iso_country_codes, 0, sizeof(_iso_country_codes));
}
 
   void read_regex(const char *fn);
-  void process_args(int argc, char *argv[]);
+  int process_args(int argc, char *argv[]);
   bool eval(TSRemapRequestInfo *rri, TSHttpTxn txnp) const;
   void add_token(const std::string &str);
 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/acd06a7d/plugins/experimental/geoip_acl/geoip_acl.cc
----------------------------------------------------------------------
diff --git a/plugins/experimental/geoip_acl/geoip_acl.cc b/plugins/experimental/geoip_acl/geoip_acl.cc
index ab3058e..1106559 100644
--- a/plugins/experimental/geoip_acl/geoip_acl.cc
+++ b/plugins/experimental/geoip_acl/geoip_acl.cc
@@ -47,11 +47,12 @@ TSRemapInit(TSRemapInterface *api_info, char *errbuf, int errbuf_size)
     return TS_ERROR;
   }
 
-  // gGI = GeoIP_new(GEOIP_STANDARD); // This seems to break on threaded apps
-  gGI = GeoIP_new(GEOIP_MMAP_CACHE);
-
-  TSDebug(PLUGIN_NAME, "remap plugin is successfully initialized");
-  return TS_SUCCESS; /* success */
+  if (Acl::init()) {
+    TSDebug(PLUGIN_NAME, "remap plugin is successfully initialized");
+    return TS_SUCCESS; /* success */
+  } else {
+    return TS_ERROR;
+  }
 }
 
 
@@ -59,21 +60,27 @@ TSReturnCode
 TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf */, int /* errbuf_size
*/)
 {
   if (argc < 3) {
-    TSError("[geoip_acl] Unable to create remap instance, need more parameters");
+    TSError("[%s] Unable to create remap instance, need more parameters", PLUGIN_NAME);
     return TS_ERROR;
   } else {
     Acl *a = NULL;
 
+    // ToDo: Should do better processing here, to make it easier to deal with
+    // rules other then country codes.
     if (!strncmp(argv[2], "country", 11)) {
       TSDebug(PLUGIN_NAME, "creating an ACL rule with ISO country codes");
       a = new CountryAcl();
     }
 
     if (a) {
-      a->process_args(argc, argv);
-      *ih = static_cast<void *>(a);
+      if (a->process_args(argc, argv) > 0) {
+        *ih = static_cast<void *>(a);
+      } else {
+        TSError("[%s] Unable to create remap instance, no geo-identifying tokens provided",
PLUGIN_NAME);
+        return TS_ERROR;
+      }
     } else {
-      TSError("[geoip_acl] Unable to create remap instance, no supported ACL specified as
first parameter");
+      TSError("[%s] Unable to create remap instance, no supported ACL specified as first
parameter", PLUGIN_NAME);
       return TS_ERROR;
     }
   }
@@ -109,19 +116,3 @@ TSRemapDoRemap(void *ih, TSHttpTxn rh, TSRemapRequestInfo *rri)
 
   return TSREMAP_NO_REMAP;
 }
-
-
-/*
-  local variables:
-  mode: C++
-  indent-tabs-mode: nil
-  c-basic-offset: 2
-  c-comment-only-line-offset: 0
-  c-file-offsets: ((statement-block-intro . +)
-  (label . 0)
-  (statement-cont . +)
-  (innamespace . 0))
-  end:
-
-  Indent with: /usr/bin/indent -ncs -nut -npcs -l 120 logstats.cc
-*/


Mime
View raw message