server: offer a flag to completely disable the web server This adds a new --webserver_enabled flag, which can be used to completely disable the embedded web server. When disabled, the web server does not listen at all.
The patch required a bit of cleanup to other spots in the code that required all servers to have associated HTTP addresses. A new unit test smoke tests a cluster configured in this manner. Change-Id: I97dacfff075f21cde9248a11fcf19efefb030aec Reviewed-on: http://gerrit.cloudera.org:8080/6216 Tested-by: Kudu Jenkins Reviewed-by: Dan Burkert <[email protected]> (cherry picked from commit 9cb5b24f7f957a3ecca1ae1bf034d3ad9defa03a) Reviewed-on: http://gerrit.cloudera.org:8080/6234 Reviewed-by: Todd Lipcon <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/6cd1e0f9 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/6cd1e0f9 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/6cd1e0f9 Branch: refs/heads/branch-1.3.x Commit: 6cd1e0f9f6ee2f20c743c9c5b24b6d7489168ea6 Parents: 6de257e Author: Todd Lipcon <[email protected]> Authored: Wed Mar 1 15:29:00 2017 -0800 Committer: Todd Lipcon <[email protected]> Committed: Fri Mar 3 01:25:15 2017 +0000 ---------------------------------------------------------------------- .../integration-tests/external_mini_cluster.cc | 17 ++++++--- .../integration-tests/external_mini_cluster.h | 4 +++ src/kudu/integration-tests/security-itest.cc | 38 ++++++++++++++------ src/kudu/master/master.cc | 16 +++++---- src/kudu/master/ts_descriptor.cc | 10 +++--- src/kudu/server/server_base.cc | 31 +++++++++++----- src/kudu/tserver/heartbeater.cc | 12 ++++--- src/kudu/tserver/tablet_server.cc | 4 ++- src/kudu/util/net/net_util.h | 4 +++ src/kudu/util/thread.cc | 8 +++-- src/kudu/util/thread.h | 2 +- 11 files changed, 100 insertions(+), 46 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/6cd1e0f9/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 5ead31b..a57f92a 100644 --- a/src/kudu/integration-tests/external_mini_cluster.cc +++ b/src/kudu/integration-tests/external_mini_cluster.cc @@ -915,7 +915,9 @@ Sockaddr ExternalDaemon::bound_rpc_addr() const { HostPort ExternalDaemon::bound_http_hostport() const { CHECK(status_); - CHECK_GE(status_->bound_http_addresses_size(), 1); + if (status_->bound_http_addresses_size() == 0) { + return HostPort(); + } HostPort ret; CHECK_OK(HostPortFromPB(status_->bound_http_addresses(0), &ret)); return ret; @@ -936,6 +938,7 @@ Status ExternalDaemon::GetInt64Metric(const MetricEntityPrototype* entity_proto, const MetricPrototype* metric_proto, const char* value_field, int64_t* value) const { + CHECK(bound_http_hostport().Initialized()); // Fetch metrics whose name matches the given prototype. string url = Substitute( "http://$0/jsonmetricz?metrics=$1";, @@ -1034,7 +1037,9 @@ Status ExternalMaster::Restart() { } vector<string> flags(GetCommonFlags()); - flags.push_back(Substitute("--webserver_port=$0", bound_http_.port())); + if (bound_http_.Initialized()) { + flags.push_back(Substitute("--webserver_port=$0", bound_http_.port())); + } flags.push_back("--rpc_bind_addresses=" + bound_rpc_.ToString()); return StartProcess(flags); @@ -1141,9 +1146,11 @@ Status ExternalTabletServer::Restart() { flags.push_back("--rpc_bind_addresses=" + bound_rpc_.ToString()); flags.push_back(Substitute("--local_ip_for_outbound_sockets=$0", get_rpc_bind_address())); - flags.push_back(Substitute("--webserver_port=$0", bound_http_.port())); - flags.push_back(Substitute("--webserver_interface=$0", - bound_http_.host())); + if (bound_http_.Initialized()) { + flags.push_back(Substitute("--webserver_port=$0", bound_http_.port())); + flags.push_back(Substitute("--webserver_interface=$0", + bound_http_.host())); + } flags.push_back("--tserver_master_addrs=" + master_addrs_); RETURN_NOT_OK(StartProcess(flags)); return Status::OK(); http://git-wip-us.apache.org/repos/asf/kudu/blob/6cd1e0f9/src/kudu/integration-tests/external_mini_cluster.h ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/external_mini_cluster.h b/src/kudu/integration-tests/external_mini_cluster.h index c4518c3..57fdba1 100644 --- a/src/kudu/integration-tests/external_mini_cluster.h +++ b/src/kudu/integration-tests/external_mini_cluster.h @@ -345,7 +345,11 @@ class ExternalDaemon : public RefCountedThreadSafe<ExternalDaemon> { HostPort bound_rpc_hostport() const; Sockaddr bound_rpc_addr() const; + + // Return the host/port that this daemon is bound to for HTTP. + // May return an uninitialized HostPort if HTTP is disabled. HostPort bound_http_hostport() const; + const NodeInstancePB& instance_id() const; const std::string& uuid() const; http://git-wip-us.apache.org/repos/asf/kudu/blob/6cd1e0f9/src/kudu/integration-tests/security-itest.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/security-itest.cc b/src/kudu/integration-tests/security-itest.cc index abfdaf5..cdac377 100644 --- a/src/kudu/integration-tests/security-itest.cc +++ b/src/kudu/integration-tests/security-itest.cc @@ -39,11 +39,12 @@ namespace kudu { class SecurityITest : public KuduTest { public: + SecurityITest() { + cluster_opts_.enable_kerberos = true; + cluster_opts_.num_tablet_servers = 3; + } void StartCluster() { - ExternalMiniClusterOptions opts; - opts.enable_kerberos = true; - opts.num_tablet_servers = 3; - cluster_.reset(new ExternalMiniCluster(opts)); + cluster_.reset(new ExternalMiniCluster(cluster_opts_)); ASSERT_OK(cluster_->Start()); } @@ -63,6 +64,9 @@ class SecurityITest : public KuduTest { return proxy.SetFlag(req, &resp, &controller); } + // Create a table, insert a row, scan it back, and delete the table. + void SmokeTestCluster(); + Status TryRegisterAsTS() { // Make a new messenger so that we don't reuse any cached connections from // the minicluster startup sequence. @@ -89,16 +93,12 @@ class SecurityITest : public KuduTest { } protected: + ExternalMiniClusterOptions cluster_opts_; unique_ptr<ExternalMiniCluster> cluster_; }; -// Test creating a table, writing some data, reading data, and dropping -// the table. -TEST_F(SecurityITest, SmokeTestAsAuthorizedUser) { +void SecurityITest::SmokeTestCluster() { const char* kTableName = "test-table"; - StartCluster(); - - ASSERT_OK(cluster_->kdc()->Kinit("test-user")); client::sp::shared_ptr<KuduClient> client; ASSERT_OK(cluster_->CreateClient(nullptr, &client)); @@ -127,6 +127,15 @@ TEST_F(SecurityITest, SmokeTestAsAuthorizedUser) { // Delete the table. ASSERT_OK(client->DeleteTable(kTableName)); +} + +// Test creating a table, writing some data, reading data, and dropping +// the table. +TEST_F(SecurityITest, SmokeTestAsAuthorizedUser) { + StartCluster(); + + ASSERT_OK(cluster_->kdc()->Kinit("test-user")); + NO_FATALS(SmokeTestCluster()); // Non-superuser clients should not be able to set flags. Status s = TrySetFlagOnTS(); @@ -188,5 +197,14 @@ TEST_F(SecurityITest, TestAuthorizedSuperuser) { } +// Test that the web UIs can be entirely disabled, for users who feel they +// are a security risk. +TEST_F(SecurityITest, TestDisableWebUI) { + cluster_opts_.extra_master_flags.push_back("--webserver_enabled=0"); + cluster_opts_.extra_tserver_flags.push_back("--webserver_enabled=0"); + StartCluster(); + NO_FATALS(SmokeTestCluster()); +} + } // namespace kudu http://git-wip-us.apache.org/repos/asf/kudu/blob/6cd1e0f9/src/kudu/master/master.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/master.cc b/src/kudu/master/master.cc index eda44c7..1d1d78a 100644 --- a/src/kudu/master/master.cc +++ b/src/kudu/master/master.cc @@ -112,7 +112,9 @@ Status Master::Init() { RETURN_NOT_OK(ServerBase::Init()); - RETURN_NOT_OK(path_handlers_->Register(web_server_.get())); + if (web_server_) { + RETURN_NOT_OK(path_handlers_->Register(web_server_.get())); + } // The certificate authority object is initialized upon loading // CA private key and certificate from the system table when the server @@ -233,12 +235,14 @@ Status Master::InitMasterRegistration() { RETURN_NOT_OK_PREPEND(rpc_server()->GetBoundAddresses(&rpc_addrs), "Couldn't get RPC addresses"); RETURN_NOT_OK(AddHostPortPBs(rpc_addrs, reg.mutable_rpc_addresses())); - vector<Sockaddr> http_addrs; - web_server()->GetBoundAddresses(&http_addrs); - RETURN_NOT_OK(AddHostPortPBs(http_addrs, reg.mutable_http_addresses())); - reg.set_software_version(VersionInfo::GetShortVersionString()); - reg.set_https_enabled(web_server()->IsSecure()); + if (web_server()) { + vector<Sockaddr> http_addrs; + web_server()->GetBoundAddresses(&http_addrs); + RETURN_NOT_OK(AddHostPortPBs(http_addrs, reg.mutable_http_addresses())); + reg.set_https_enabled(web_server()->IsSecure()); + } + reg.set_software_version(VersionInfo::GetShortVersionString()); registration_.Swap(®); registration_initialized_.store(true); http://git-wip-us.apache.org/repos/asf/kudu/blob/6cd1e0f9/src/kudu/master/ts_descriptor.cc ---------------------------------------------------------------------- diff --git a/src/kudu/master/ts_descriptor.cc b/src/kudu/master/ts_descriptor.cc index 26228fa..6f93bb3 100644 --- a/src/kudu/master/ts_descriptor.cc +++ b/src/kudu/master/ts_descriptor.cc @@ -90,11 +90,10 @@ Status TSDescriptor::Register(const NodeInstancePB& instance, std::lock_guard<simple_spinlock> l(lock_); CHECK_EQ(instance.permanent_uuid(), permanent_uuid_); - // TODO(KUDU-418): we don't currently support changing IPs or hosts since the + // TODO(KUDU-418): we don't currently support changing RPC addresses since the // host/port is stored persistently in each tablet's metadata. if (registration_ && - (!HostPortPBsEqual(registration_->rpc_addresses(), registration.rpc_addresses()) || - !HostPortPBsEqual(registration_->http_addresses(), registration.http_addresses()))) { + !HostPortPBsEqual(registration_->rpc_addresses(), registration.rpc_addresses())) { string msg = strings::Substitute( "Tablet server $0 is attempting to re-register with a different host/port. " "This is not currently supported. Old: {$1} New: {$2}", @@ -105,10 +104,9 @@ Status TSDescriptor::Register(const NodeInstancePB& instance, return Status::InvalidArgument(msg); } - if (registration.rpc_addresses().empty() || - registration.http_addresses().empty()) { + if (registration.rpc_addresses().empty()) { return Status::InvalidArgument( - "invalid registration: must have at least one RPC and one HTTP address", + "invalid registration: must have at least one RPC address", SecureShortDebugString(registration)); } http://git-wip-us.apache.org/repos/asf/kudu/blob/6cd1e0f9/src/kudu/server/server_base.cc ---------------------------------------------------------------------- diff --git a/src/kudu/server/server_base.cc b/src/kudu/server/server_base.cc index b67b7f7..772207d 100644 --- a/src/kudu/server/server_base.cc +++ b/src/kudu/server/server_base.cc @@ -76,6 +76,11 @@ TAG_FLAG(min_negotiation_threads, advanced); DEFINE_int32(max_negotiation_threads, 50, "Maximum number of connection negotiation threads."); TAG_FLAG(max_negotiation_threads, advanced); +DEFINE_bool(webserver_enabled, true, "Whether to enable the web server on this daemon. " + "NOTE: disabling the web server is also likely to prevent monitoring systems " + "from properly capturing metrics."); +TAG_FLAG(webserver_enabled, advanced); + DEFINE_string(superuser_acl, "", "The list of usernames to allow as super users, comma-separated. " "A '*' entry indicates that all authenticated users are allowed. " @@ -129,7 +134,6 @@ ServerBase::ServerBase(string name, const ServerBaseOptions& options, metric_entity_(METRIC_ENTITY_server.Instantiate(metric_registry_.get(), metric_namespace)), rpc_server_(new RpcServer(options.rpc_opts)), - web_server_(new Webserver(options.webserver_opts)), result_tracker_(new rpc::ResultTracker(shared_ptr<MemTracker>( MemTracker::CreateTracker(-1, "result-tracker", mem_tracker_)))), is_first_run_(false), @@ -148,6 +152,10 @@ ServerBase::ServerBase(string name, const ServerBaseOptions& options, clock_ = LogicalClock::CreateStartingAt(Timestamp::kInitialTimestamp); } + if (FLAGS_webserver_enabled) { + web_server_.reset(new Webserver(options.webserver_opts)); + } + CHECK_OK(StartThreadInstrumentation(metric_entity_, web_server_.get())); CHECK_OK(codegen::CompilationManager::GetSingleton()->StartInstrumentation( metric_entity_)); @@ -166,6 +174,7 @@ Sockaddr ServerBase::first_rpc_address() const { } Sockaddr ServerBase::first_http_address() const { + CHECK(web_server_); vector<Sockaddr> addrs; WARN_NOT_OK(web_server_->GetBoundAddresses(&addrs), "Couldn't get bound webserver addresses"); @@ -287,7 +296,7 @@ void ServerBase::GetStatusPB(ServerStatusPB* status) const { } // HTTP ports - { + if (web_server_) { vector<Sockaddr> addrs; CHECK_OK(web_server_->GetBoundAddresses(&addrs)); for (const Sockaddr& addr : addrs) { @@ -443,12 +452,14 @@ Status ServerBase::Start() { RETURN_NOT_OK(rpc_server_->Start()); - AddDefaultPathHandlers(web_server_.get()); - AddRpczPathHandlers(messenger_, web_server_.get()); - RegisterMetricsJsonHandler(web_server_.get(), metric_registry_.get()); - TracingPathHandlers::RegisterHandlers(web_server_.get()); - web_server_->set_footer_html(FooterHtml()); - RETURN_NOT_OK(web_server_->Start()); + if (web_server_) { + AddDefaultPathHandlers(web_server_.get()); + AddRpczPathHandlers(messenger_, web_server_.get()); + RegisterMetricsJsonHandler(web_server_.get(), metric_registry_.get()); + TracingPathHandlers::RegisterHandlers(web_server_.get()); + web_server_->set_footer_html(FooterHtml()); + RETURN_NOT_OK(web_server_->Start()); + } if (!options_.dump_info_path.empty()) { RETURN_NOT_OK_PREPEND(DumpServerInfo(options_.dump_info_path, options_.dump_info_format), @@ -466,7 +477,9 @@ void ServerBase::Shutdown() { if (excess_log_deleter_thread_) { excess_log_deleter_thread_->Join(); } - web_server_->Stop(); + if (web_server_) { + web_server_->Stop(); + } rpc_server_->Shutdown(); } http://git-wip-us.apache.org/repos/asf/kudu/blob/6cd1e0f9/src/kudu/tserver/heartbeater.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tserver/heartbeater.cc b/src/kudu/tserver/heartbeater.cc index 892cf84..093399e 100644 --- a/src/kudu/tserver/heartbeater.cc +++ b/src/kudu/tserver/heartbeater.cc @@ -306,13 +306,15 @@ Status Heartbeater::Thread::SetupRegistration(ServerRegistrationPB* reg) { "Failed to add RPC addresses to registration"); addrs.clear(); - RETURN_NOT_OK_PREPEND(CHECK_NOTNULL(server_->web_server())->GetBoundAddresses(&addrs), - "Unable to get bound HTTP addresses"); - RETURN_NOT_OK_PREPEND(AddHostPortPBs(addrs, reg->mutable_http_addresses()), - "Failed to add HTTP addresses to registration"); + if (server_->web_server()) { + RETURN_NOT_OK_PREPEND(server_->web_server()->GetBoundAddresses(&addrs), + "Unable to get bound HTTP addresses"); + RETURN_NOT_OK_PREPEND(AddHostPortPBs(addrs, reg->mutable_http_addresses()), + "Failed to add HTTP addresses to registration"); + reg->set_https_enabled(server_->web_server()->IsSecure()); + } reg->set_software_version(VersionInfo::GetShortVersionString()); - reg->set_https_enabled(server_->web_server()->IsSecure()); return Status::OK(); } http://git-wip-us.apache.org/repos/asf/kudu/blob/6cd1e0f9/src/kudu/tserver/tablet_server.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tserver/tablet_server.cc b/src/kudu/tserver/tablet_server.cc index ea8e1b1..27a0c1d 100644 --- a/src/kudu/tserver/tablet_server.cc +++ b/src/kudu/tserver/tablet_server.cc @@ -88,7 +88,9 @@ Status TabletServer::Init() { RETURN_NOT_OK(ValidateMasterAddressResolution()); RETURN_NOT_OK(ServerBase::Init()); - RETURN_NOT_OK(path_handlers_->Register(web_server_.get())); + if (web_server_) { + RETURN_NOT_OK(path_handlers_->Register(web_server_.get())); + } heartbeater_.reset(new Heartbeater(opts_, this)); http://git-wip-us.apache.org/repos/asf/kudu/blob/6cd1e0f9/src/kudu/util/net/net_util.h ---------------------------------------------------------------------- diff --git a/src/kudu/util/net/net_util.h b/src/kudu/util/net/net_util.h index 3086f4a..863d0f3 100644 --- a/src/kudu/util/net/net_util.h +++ b/src/kudu/util/net/net_util.h @@ -33,6 +33,10 @@ class HostPort { HostPort(std::string host, uint16_t port); explicit HostPort(const Sockaddr& addr); + bool Initialized() const { + return !host_.empty(); + } + // Parse a "host:port" pair into this object. // If there is no port specified in the string, then 'default_port' is used. Status ParseString(const std::string& str, uint16_t default_port); http://git-wip-us.apache.org/repos/asf/kudu/blob/6cd1e0f9/src/kudu/util/thread.cc ---------------------------------------------------------------------- diff --git a/src/kudu/util/thread.cc b/src/kudu/util/thread.cc index 971568e..71aa400 100644 --- a/src/kudu/util/thread.cc +++ b/src/kudu/util/thread.cc @@ -268,9 +268,11 @@ Status ThreadMgr::StartInstrumentation(const scoped_refptr<MetricEntity>& metric METRIC_involuntary_context_switches.InstantiateFunctionGauge(metrics, Bind(&GetInVoluntaryContextSwitches))); - WebCallbackRegistry::PathHandlerCallback thread_callback = - bind<void>(mem_fn(&ThreadMgr::ThreadPathHandler), this, _1, _2); - DCHECK_NOTNULL(web)->RegisterPathHandler("/threadz", "Threads", thread_callback); + if (web) { + WebCallbackRegistry::PathHandlerCallback thread_callback = + bind<void>(mem_fn(&ThreadMgr::ThreadPathHandler), this, _1, _2); + DCHECK_NOTNULL(web)->RegisterPathHandler("/threadz", "Threads", thread_callback); + } return Status::OK(); } http://git-wip-us.apache.org/repos/asf/kudu/blob/6cd1e0f9/src/kudu/util/thread.h ---------------------------------------------------------------------- diff --git a/src/kudu/util/thread.h b/src/kudu/util/thread.h index 5aa7620..46e2505 100644 --- a/src/kudu/util/thread.h +++ b/src/kudu/util/thread.h @@ -354,7 +354,7 @@ class Thread : public RefCountedThreadSafe<Thread> { }; // Registers /threadz with the debug webserver, and creates thread-tracking metrics under -// the given entity. +// the given entity. If 'web' is NULL, does not register the path handler. Status StartThreadInstrumentation(const scoped_refptr<MetricEntity>& server_metrics, WebCallbackRegistry* web); } // namespace kudu
