kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From danburk...@apache.org
Subject kudu git commit: el6: fix krb5 realm workaround for static builds
Date Thu, 17 Nov 2016 21:12:16 GMT
Repository: kudu
Updated Branches:
  refs/heads/master 85404f67b -> b1d02ee1a


el6: fix krb5 realm workaround for static builds

Here's another attempt at enabling the workaround for the numeric realm
problem on el6 (see ba2ae3de4a7c43ff2f5873e822410e066ea99667 for
background).

The previous attempt didn't succeed for static builds because we were
setting LD_PRELOAD to the non-absolute path of the override library.
This worked OK in debug builds because our binary RUNPATHs included the
build/lib/ directory. Static builds didn't have such a RUNPATH and thus
the LD_PRELOAD was unable to locate the specified shared object.

The two options to fix this were (1) determine and specify an absolute
path, or (2) build this workaround into all of our binaries rather than
try to preload it at runtime.

The issue with #1 is that, when we get to Java-based testing, it would
have been inconvenient to determine the absolute path of the override
library. In addition, it would make test binaries non-relocatable, which
is somewhat of a hassle.

So, this takes approach #2. The build incantations are a bit nasty. In
order to make sure that the symbol shows up everywhere, and shows up
before the krb5 library, and doesn't show up more than once (to avoid
ODR violations), we have to manually add it to the link list for the
the executables that need the override. This includes the tests (and
thus the 'test_main' library) as well as any executables which are run
by the tests ((tserver, master, and 'kudu' tool).

In addition, we need to make sure that the linker doesn't entirely skip
the override binary. I accomplished this using the '--undefined' linker
flag.

I tested this patch using both sasl_rpc-test and
external_mini_cluster-test on an el6 box in both debug (dynamic linked)
and release (static-linked) builds. I also fully built both build types
to ensure there were no linker issues in unrelated tests.

Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Reviewed-on: http://gerrit.cloudera.org:8080/5100
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <adar@cloudera.com>
Reviewed-by: Dan Burkert <danburkert@apache.org>


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

Branch: refs/heads/master
Commit: b1d02ee1a6b45745df9668af52c133580defda45
Parents: 85404f6
Author: Todd Lipcon <todd@apache.org>
Authored: Tue Nov 15 16:58:31 2016 -0800
Committer: Dan Burkert <danburkert@apache.org>
Committed: Thu Nov 17 21:12:00 2016 +0000

----------------------------------------------------------------------
 CMakeLists.txt                                  |  10 ++
 src/kudu/integration-tests/CMakeLists.txt       |   4 +-
 .../integration-tests/external_mini_cluster.cc  |   6 --
 src/kudu/master/CMakeLists.txt                  |   1 +
 src/kudu/security/CMakeLists.txt                |  16 +--
 src/kudu/security/krb5_realm_override.cc        | 104 ++++++++++++++++++
 src/kudu/security/test/krb5_realm_override.cc   | 106 -------------------
 src/kudu/tools/CMakeLists.txt                   |   1 +
 src/kudu/tserver/CMakeLists.txt                 |   1 +
 src/kudu/util/CMakeLists.txt                    |   5 +-
 src/kudu/util/test_main.cc                      |   7 --
 src/kudu/util/test_util.cc                      |   2 +
 12 files changed, 132 insertions(+), 131 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/b1d02ee1/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/CMakeLists.txt b/CMakeLists.txt
index b10e3b7..0c4e9df 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -942,6 +942,16 @@ if (NOT NO_TESTS)
   find_package(KerberosPrograms)
 endif()
 
+# The tests as well as any binaries which are run as subprocesses by tests (eg tserver,
+# master, and the 'kudu' CLI tool) need to link this in. We have to set it
+# here so it's accessible by all targets.
+#
+# OSX doesn't support the linker flag below, but also has a new enough krb5 that the
+# override library isn't necessary.
+if (NOT APPLE)
+set(KRB5_REALM_OVERRIDE -Wl,--undefined=krb5_realm_override_loaded krb5_realm_override)
+endif()
+
 ## Boost
 
 # The boost-cmake project hasn't been maintained for years. Let's make sure we

http://git-wip-us.apache.org/repos/asf/kudu/blob/b1d02ee1/src/kudu/integration-tests/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/CMakeLists.txt b/src/kudu/integration-tests/CMakeLists.txt
index 6d58814..0fed394 100644
--- a/src/kudu/integration-tests/CMakeLists.txt
+++ b/src/kudu/integration-tests/CMakeLists.txt
@@ -38,9 +38,7 @@ target_link_libraries(integration-tests
   security-test)
 add_dependencies(integration-tests
   kudu-tserver
-  kudu-master
-  # ExternalMiniCluster uses this shared library as a LD_PRELOAD
-  krb5_realm_override)
+  kudu-master)
 
 # Tests
 set(KUDU_TEST_LINK_LIBS integration-tests ${KUDU_MIN_TEST_LIBS})

http://git-wip-us.apache.org/repos/asf/kudu/blob/b1d02ee1/src/kudu/integration-tests/external_mini_cluster.cc
----------------------------------------------------------------------
diff --git a/src/kudu/integration-tests/external_mini_cluster.cc b/src/kudu/integration-tests/external_mini_cluster.cc
index 95a5d8a..ab69957 100644
--- a/src/kudu/integration-tests/external_mini_cluster.cc
+++ b/src/kudu/integration-tests/external_mini_cluster.cc
@@ -582,12 +582,6 @@ Status ExternalDaemon::EnableKerberos(MiniKdc* kdc, const string&
bind_host) {
   RETURN_NOT_OK_PREPEND(kdc->CreateServiceKeytab(spn, &ktpath),
                         "could not create keytab");
   extra_env_ = kdc->GetEnvVars();
-
-  // Add workaround for krb5 1.10 bug. See krb5_realm_override.cc for details.
-  extra_env_["LD_PRELOAD"] = "libkrb5_realm_override.so";
-  if (getenv("LD_PRELOAD") != nullptr) {
-    extra_env_["LD_PRELOAD"] += ":" + string(getenv("LD_PRELOAD"));
-  }
   extra_flags_.push_back(Substitute("--keytab=$0", ktpath));
   extra_flags_.push_back(Substitute("--kerberos_principal=$0", spn));
   extra_flags_.push_back("--server_require_kerberos");

http://git-wip-us.apache.org/repos/asf/kudu/blob/b1d02ee1/src/kudu/master/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/kudu/master/CMakeLists.txt b/src/kudu/master/CMakeLists.txt
index 695188b..f97140d 100644
--- a/src/kudu/master/CMakeLists.txt
+++ b/src/kudu/master/CMakeLists.txt
@@ -80,5 +80,6 @@ ADD_KUDU_TEST(sys_catalog-test RESOURCE_LOCK "master-web-port")
 # Actual master executable
 add_executable(kudu-master master_main.cc)
 target_link_libraries(kudu-master
+  ${KRB5_REALM_OVERRIDE}
   master
   ${KUDU_BASE_LIBS})

http://git-wip-us.apache.org/repos/asf/kudu/blob/b1d02ee1/src/kudu/security/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/kudu/security/CMakeLists.txt b/src/kudu/security/CMakeLists.txt
index f0b4eff..a4204bc 100644
--- a/src/kudu/security/CMakeLists.txt
+++ b/src/kudu/security/CMakeLists.txt
@@ -15,6 +15,15 @@
 # specific language governing permissions and limitations
 # under the License.
 
+# See the comment in krb5_realm_override.cc for details on this library's usage.
+# The top-level CMakeLists sets a ${KRB5_REALM_OVERRIDE} variable which should
+# be linked first into all Kudu binaries.
+add_library(krb5_realm_override STATIC krb5_realm_override.cc)
+target_link_libraries(krb5_realm_override glog)
+if(NOT APPLE)
+  target_link_libraries(krb5_realm_override dl)
+endif()
+
 add_library(security
   init.cc)
 target_link_libraries(security
@@ -32,11 +41,6 @@ target_link_libraries(security-test
   kudu_test_util
   kudu_util)
 
-# See the comment in krb5_realm_override.cc for details on this library's usage.
-add_library(krb5_realm_override SHARED test/krb5_realm_override.cc)
-add_library(krb5_realm_override_static STATIC test/krb5_realm_override.cc)
-target_link_libraries(krb5_realm_override glog)
-target_link_libraries(krb5_realm_override_static glog)
 
 # Tests
 set(KUDU_TEST_LINK_LIBS
@@ -44,4 +48,4 @@ set(KUDU_TEST_LINK_LIBS
   security-test
   ${KUDU_MIN_TEST_LIBS})
 
-ADD_KUDU_TEST(test/mini_kdc-test)
+ADD_KUDU_TEST(test/mini_kdc-test)
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/kudu/blob/b1d02ee1/src/kudu/security/krb5_realm_override.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/krb5_realm_override.cc b/src/kudu/security/krb5_realm_override.cc
new file mode 100644
index 0000000..3f875f1
--- /dev/null
+++ b/src/kudu/security/krb5_realm_override.cc
@@ -0,0 +1,104 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// This file provides a workaround for tests running with Kerberos 1.11 or earlier.
+// These versions of Kerberos are missing a fix which allows service principals
+// to use IP addresses in their host component:
+//
+//   http://krbdev.mit.edu/rt/Ticket/Display.html?id=7603
+//
+// We use such principals in external minicluster tests, where servers have IP addresses
+// like 127.x.y.z that have no corresponding reverse DNS.
+//
+// The file contains an implementation of krb5_get_host_realm which wraps the one
+// in the Kerberos library. It detects the return code that indicates the
+// above problem and falls back to the default realm/
+//
+// The wrapper is injected via linking it into tests as well as the
+// "security" library. The linkage invocation uses the '-Wl,--undefined'
+// linker flag to force linking even though no symbol here is explicitly
+// referenced.
+
+#include <dlfcn.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <krb5/krb5.h>
+#include <glog/logging.h>
+
+extern "C" {
+
+// This symbol is exported from the static library so that other static-linked binaries
+// can reference it and force this compilation unit to be linked. Otherwise the linker
+// thinks it's unused and doesn't link it.
+int krb5_realm_override_loaded = 1;
+
+// Save the original function from the Kerberos library itself.
+// We use dlsym() to load all of them, since this file gets linked into
+// some test binaries that themselves may not link against libkrb5.so at all.
+static void* g_orig_krb5_get_host_realm;
+static void* g_orig_krb5_get_default_realm;
+static void* g_orig_krb5_free_default_realm;
+
+// We only enable our workaround if this environment variable is set.
+constexpr static const char* kEnvVar = "KUDU_ENABLE_KRB5_REALM_FIX";
+
+#define CALL_ORIG(func_name, ...) \
+  ((decltype(&func_name))g_orig_ ## func_name)(__VA_ARGS__)
+
+__attribute__((constructor))
+static void init_orig_func() {
+  g_orig_krb5_get_host_realm = dlsym(RTLD_NEXT, "krb5_get_host_realm");
+  g_orig_krb5_get_default_realm = dlsym(RTLD_NEXT, "krb5_get_default_realm");
+  g_orig_krb5_free_default_realm = dlsym(RTLD_NEXT, "krb5_free_default_realm");
+}
+
+krb5_error_code krb5_get_host_realm(krb5_context context, const char* host, char*** realmsp)
{
+  CHECK(g_orig_krb5_get_host_realm);
+  CHECK(g_orig_krb5_get_default_realm);
+  CHECK(g_orig_krb5_free_default_realm);
+
+  krb5_error_code rc = CALL_ORIG(krb5_get_host_realm, context, host, realmsp);
+  if (rc != KRB5_ERR_NUMERIC_REALM || getenv(kEnvVar) == nullptr) {
+    return rc;
+  }
+  // If we get KRB5_ERR_NUMERIC_REALM, this is indicative of a Kerberos version
+  // which has not provided support for numeric addresses as service host names
+  // So, we fill in the default realm instead.
+  char* default_realm;
+  rc = CALL_ORIG(krb5_get_default_realm, context, &default_realm);
+  if (rc != 0) {
+    return rc;
+  }
+
+  char** ret_realms;
+  ret_realms = static_cast<char**>(malloc(2 * sizeof(*ret_realms)));
+  if (ret_realms == nullptr) return ENOMEM;
+  ret_realms[0] = strdup(default_realm);
+  if (ret_realms[0] == nullptr) {
+    free(ret_realms);
+    return ENOMEM;
+  }
+  ret_realms[1] = 0;
+  *realmsp = ret_realms;
+
+  CALL_ORIG(krb5_free_default_realm, context, default_realm);
+  return 0;
+}
+
+} // extern "C"

http://git-wip-us.apache.org/repos/asf/kudu/blob/b1d02ee1/src/kudu/security/test/krb5_realm_override.cc
----------------------------------------------------------------------
diff --git a/src/kudu/security/test/krb5_realm_override.cc b/src/kudu/security/test/krb5_realm_override.cc
deleted file mode 100644
index d506cf1..0000000
--- a/src/kudu/security/test/krb5_realm_override.cc
+++ /dev/null
@@ -1,106 +0,0 @@
-// Licensed to the Apache Software Foundation (ASF) under one
-// or more contributor license agreements.  See the NOTICE file
-// distributed with this work for additional information
-// regarding copyright ownership.  The ASF licenses this file
-// to you under the Apache License, Version 2.0 (the
-// "License"); you may not use this file except in compliance
-// with the License.  You may obtain a copy of the License at
-//
-//   http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing,
-// software distributed under the License is distributed on an
-// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-// KIND, either express or implied.  See the License for the
-// specific language governing permissions and limitations
-// under the License.
-
-// This file provides a workaround for tests running with Kerberos 1.11 or earlier.
-// These versions of Kerberos are missing a fix which allows service principals
-// to use IP addresses in their host component:
-//
-//   http://krbdev.mit.edu/rt/Ticket/Display.html?id=7603
-//
-// We use such principals in external minicluster tests, where servers have IP addresses
-// like 127.x.y.z that have no corresponding reverse DNS.
-//
-// The file contains an implementation of krb5_get_host_realm which wraps the one
-// in the Kerberos library. It detects the return code that indicates the
-// above problem and falls back to the default realm/
-//
-// The wrapper is injected in two ways:
-// 1) kudu_test_main static-links against this source so that all test
-//    binaries load our wrapped function ahead of the library-provided one.
-// 2) ExternalMiniCluster sets LD_PRELOAD to load a shared library built from
-//    this source before it forks server processes.
-//
-// Thus, we don't end up polluting our shipped binaries with this wrapper, but
-// our tests which depend on numeric host names can still pass on Kerberos 1.10.
-
-#include <dlfcn.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-
-#include <krb5/krb5.h>
-#include <glog/logging.h>
-
-extern "C" {
-
-// This symbol is exported from the static library so that
-// test_main.cc can reference it and force this compilation unit
-// to be linked. Otherwise the linker thinks it's unused and doesn't
-// link it.
-int krb5_realm_override_loaded = 1;
-
-// Save the original function from the Kerberos library itself.
-// We use dlsym() to load all of them, since this file gets linked into
-// some test binaries that themselves may not link against libkrb5.so at all.
-static void* g_orig_krb5_get_host_realm;
-static void* g_orig_krb5_get_default_realm;
-static void* g_orig_krb5_free_default_realm;
-
-#define CALL_ORIG(func_name, ...) \
-  ((decltype(&func_name))g_orig_ ## func_name)(__VA_ARGS__)
-
-__attribute__((constructor))
-static void init_orig_func() {
-  g_orig_krb5_get_host_realm = dlsym(RTLD_NEXT, "krb5_get_host_realm");
-  g_orig_krb5_get_default_realm = dlsym(RTLD_NEXT, "krb5_get_default_realm");
-  g_orig_krb5_free_default_realm = dlsym(RTLD_NEXT, "krb5_free_default_realm");
-}
-
-krb5_error_code krb5_get_host_realm(krb5_context context, const char* host, char*** realmsp)
{
-  CHECK(g_orig_krb5_get_host_realm);
-  CHECK(g_orig_krb5_get_default_realm);
-  CHECK(g_orig_krb5_free_default_realm);
-
-  krb5_error_code rc = CALL_ORIG(krb5_get_host_realm, context, host, realmsp);
-  if (rc != KRB5_ERR_NUMERIC_REALM) {
-    return rc;
-  }
-  // If we get KRB5_ERR_NUMERIC_REALM, this is indicative of a Kerberos version
-  // which has not provided support for numeric addresses as service host names
-  // So, we fill in the default realm instead.
-  char* default_realm;
-  rc = CALL_ORIG(krb5_get_default_realm, context, &default_realm);
-  if (rc != 0) {
-    return rc;
-  }
-
-  char** ret_realms;
-  ret_realms = static_cast<char**>(malloc(2 * sizeof(*ret_realms)));
-  if (ret_realms == nullptr) return ENOMEM;
-  ret_realms[0] = strdup(default_realm);
-  if (ret_realms[0] == nullptr) {
-    free(ret_realms);
-    return ENOMEM;
-  }
-  ret_realms[1] = 0;
-  *realmsp = ret_realms;
-
-  CALL_ORIG(krb5_free_default_realm, context, default_realm);
-  return 0;
-}
-
-} // extern "C"

http://git-wip-us.apache.org/repos/asf/kudu/blob/b1d02ee1/src/kudu/tools/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/kudu/tools/CMakeLists.txt b/src/kudu/tools/CMakeLists.txt
index 53886df..c38d2da 100644
--- a/src/kudu/tools/CMakeLists.txt
+++ b/src/kudu/tools/CMakeLists.txt
@@ -71,6 +71,7 @@ add_executable(kudu
   tool_main.cc
 )
 target_link_libraries(kudu
+  ${KRB5_REALM_OVERRIDE}
   consensus
   gutil
   krpc

http://git-wip-us.apache.org/repos/asf/kudu/blob/b1d02ee1/src/kudu/tserver/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/kudu/tserver/CMakeLists.txt b/src/kudu/tserver/CMakeLists.txt
index 77b890d..0245428 100644
--- a/src/kudu/tserver/CMakeLists.txt
+++ b/src/kudu/tserver/CMakeLists.txt
@@ -141,6 +141,7 @@ target_link_libraries(tserver
 
 add_executable(kudu-tserver tablet_server_main.cc)
 target_link_libraries(kudu-tserver
+  ${KRB5_REALM_OVERRIDE}
   tserver
   ${KUDU_BASE_LIBS})
 

http://git-wip-us.apache.org/repos/asf/kudu/blob/b1d02ee1/src/kudu/util/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/kudu/util/CMakeLists.txt b/src/kudu/util/CMakeLists.txt
index ca793e3..0977461 100644
--- a/src/kudu/util/CMakeLists.txt
+++ b/src/kudu/util/CMakeLists.txt
@@ -262,13 +262,12 @@ endif()
 add_library(kudu_test_main
   test_main.cc)
 target_link_libraries(kudu_test_main
+  ${KRB5_REALM_OVERRIDE}
   gflags
   glog
   gmock
   kudu_util
-  kudu_test_util
-  # See the comment in kudu/security/test/krb5_realm_override.cc for details.
-  krb5_realm_override_static)
+  kudu_test_util)
 
 if(NOT APPLE)
   target_link_libraries(kudu_test_main

http://git-wip-us.apache.org/repos/asf/kudu/blob/b1d02ee1/src/kudu/util/test_main.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/test_main.cc b/src/kudu/util/test_main.cc
index 9d8e4a8..2935b24 100644
--- a/src/kudu/util/test_main.cc
+++ b/src/kudu/util/test_main.cc
@@ -34,13 +34,6 @@ DEFINE_int32(stress_cpu_threads, 0,
              "Number of threads to start that burn CPU in an attempt to "
              "stimulate race conditions");
 
-// Force linking of krb5_realm_override.cc. Without using any of its
-// symbols explicitly, the compilation unit would be skipped.
-extern "C" {
-extern int krb5_realm_override_loaded;
-int force_load_krb5_workaround = krb5_realm_override_loaded;
-}
-
 namespace kudu {
 
 // Start thread that kills the process if --test_timeout_after is exceeded before

http://git-wip-us.apache.org/repos/asf/kudu/blob/b1d02ee1/src/kudu/util/test_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/test_util.cc b/src/kudu/util/test_util.cc
index 3414797..e143a42 100644
--- a/src/kudu/util/test_util.cc
+++ b/src/kudu/util/test_util.cc
@@ -101,6 +101,8 @@ void KuduTest::OverrideKrb5Environment() {
   setenv("KRB5_CONFIG", kInvalidPath, 1);
   setenv("KRB5_KTNAME", kInvalidPath, 1);
   setenv("KRB5CCNAME", kInvalidPath, 1);
+  // Enable the workaround for MIT krb5 1.10 bugs from krb5_realm_override.cc.
+  setenv("KUDU_ENABLE_KRB5_REALM_FIX", "yes", 1);
 }
 
 ///////////////////////////////////////////////////


Mime
View raw message