Skip to content

HBASE-29775 Allow inspecting log levels in Master UI in read-only mode #7540

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.

Sign up for GitHub

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

Open
jinhyukify wants to merge 3 commits into
base: master
Choose a base branch
from
+190 −72
Open

HBASE-29775 Allow inspecting log levels in Master UI in read-only mode #7540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03444e5
HBASE-29775 Allow inspecting log levels in Master UI in read-only mode
jinhyukify Dec 12, 2025
aad663f
HBASE-29775 Refactor LogLevel CLI to separate response fetching from …
jinhyukify Dec 26, 2025
91167a0
HBASE-29775 Add tests for LogLevel readonly master UI mode
jinhyukify Dec 26, 2025
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
47 changes: 30 additions & 17 deletions hbase-http/src/main/java/org/apache/hadoop/hbase/http/log/LogLevel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.nio.charset.StandardCharsets;
import java.util.Objects;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import javax.net.ssl.HttpsURLConnection;
import javax.net.ssl.SSLSocketFactory;
import javax.servlet.ServletException;
Expand Down Expand Up @@ -60,6 +61,7 @@ public final class LogLevel {
public static final String PROTOCOL_HTTPS = "https";

public static final String READONLY_LOGGERS_CONF_KEY = "hbase.ui.logLevels.readonly.loggers";
public static final String MASTER_UI_READONLY_CONF_KEY = "hbase.master.ui.readonly";

/**
* A command line implementation
Expand Down Expand Up @@ -213,7 +215,11 @@ private int parseProtocolArgs(String[] args, int index) throws HadoopIllegalArgu
* @throws Exception if unable to connect
*/
private void doGetLevel() throws Exception {
process(protocol + "://" + hostName + "/logLevel?log=" + className);
System.out.println(fetchGetLevelResponse());
}

String fetchGetLevelResponse() throws Exception {
return fetchResponse(protocol + "://" + hostName + "/logLevel?log=" + className);
Comment on lines +218 to +222
Copy link
Contributor Author

@jinhyukify jinhyukify Dec 27, 2025

Choose a reason for hiding this comment

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

The previous implementation printed results only to standard output, making it hard to write tests. I refactored the code slightly to separate the logic.

}

/**
Expand All @@ -222,7 +228,12 @@ private void doGetLevel() throws Exception {
* @throws Exception if unable to connect
*/
private void doSetLevel() throws Exception {
process(protocol + "://" + hostName + "/logLevel?log=" + className + "&level=" + level);
System.out.println(fetchSetLevelResponse());
}

String fetchSetLevelResponse() throws Exception {
return fetchResponse(
protocol + "://" + hostName + "/logLevel?log=" + className + "&level=" + level);
}

/**
Expand Down Expand Up @@ -256,11 +267,12 @@ private HttpURLConnection connect(URL url) throws Exception {
}

/**
* Configures the client to send HTTP request to the URL. Supports SPENGO for authentication.
* Send HTTP request and fetch response.
* @param urlString URL and query string to the daemon's web UI
* @return the response from the daemon
* @throws Exception if unable to connect
*/
private void process(String urlString) throws Exception {
private String fetchResponse(String urlString) throws Exception {
URL url = new URL(urlString);
System.out.println("Connecting to " + url);

Expand All @@ -272,16 +284,16 @@ private void process(String urlString) throws Exception {
// disallowed in configuration" in Jetty 9
LogLevelExceptionUtils.validateResponse(connection, 200);

// read from the servlet

try (
InputStreamReader streamReader =
new InputStreamReader(connection.getInputStream(), StandardCharsets.UTF_8);
BufferedReader bufferedReader = new BufferedReader(streamReader)) {
bufferedReader.lines().filter(Objects::nonNull).filter(line -> line.startsWith(MARKER))
.forEach(line -> System.out.println(TAG.matcher(line).replaceAll("")));

return bufferedReader.lines().filter(Objects::nonNull)
.filter(line -> line.startsWith(MARKER)).map(line -> TAG.matcher(line).replaceAll(""))
.collect(Collectors.joining("\n"));
} catch (IOException ioe) {
System.err.println("" + ioe);
return "" + ioe;
Copy link
Contributor

@Apache9 Apache9 Dec 27, 2025

Choose a reason for hiding this comment

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

In the old logic, this will go to stderr, but now it will go to stdout? I'm not very familiar with this area, so I'm not sure whether this is a problem...

Copy link
Contributor Author

@jinhyukify jinhyukify Dec 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the review. Good catch.

One thing I wanted to mention is that the previous behavior already felt a bit inconsistent.
We were catching only IOException here and printing it to standard error, while other exceptions were simply propagated..

That special handling for IOException feels slightly odd to me. Since this is a CLI command, I am considering not catching IOException here at all and letting it propagate, the same way other exceptions do.

I understand that this could technically be a behavior change for users who rely on standard error, but printing only this specific exception to stderr also seems inconsistent from a CLI semantics point of view.

I would like to hear your thoughts on whether there is a strong reason to keep this special case.

}
}
}
Expand All @@ -303,14 +315,6 @@ public void doGet(HttpServletRequest request, HttpServletResponse response)
if (!HttpServer.hasAdministratorAccess(getServletContext(), request, response)) {
return;
}
// Disallow modification of the LogLevel if explicitly set to readonly
Configuration conf =
(Configuration) getServletContext().getAttribute(HttpServer.CONF_CONTEXT_ATTRIBUTE);
if (conf.getBoolean("hbase.master.ui.readonly", false)) {
sendError(response, HttpServletResponse.SC_FORBIDDEN,
"Modification of HBase via the UI is disallowed in configuration.");
return;
}
response.setContentType("text/html");
PrintWriter out;
try {
Expand All @@ -326,6 +330,8 @@ public void doGet(HttpServletRequest request, HttpServletResponse response)
String logName = ServletUtil.getParameter(request, "log");
String level = ServletUtil.getParameter(request, "level");

Configuration conf =
(Configuration) getServletContext().getAttribute(HttpServer.CONF_CONTEXT_ATTRIBUTE);
String[] readOnlyLogLevels = conf.getStrings(READONLY_LOGGERS_CONF_KEY);

if (logName != null) {
Expand All @@ -335,6 +341,13 @@ public void doGet(HttpServletRequest request, HttpServletResponse response)
Logger log = LoggerFactory.getLogger(logName);
out.println(MARKER + "Log Class: <b>" + log.getClass().getName() + "</b><br />");
if (level != null) {
// Disallow modification of the LogLevel if explicitly set to readonly
if (conf.getBoolean(MASTER_UI_READONLY_CONF_KEY, false)) {
sendError(response, HttpServletResponse.SC_FORBIDDEN,
"Modification of HBase via the UI is disallowed in configuration.");
return;
}

if (!isLogLevelChangeAllowed(logName, readOnlyLogLevels)) {
sendError(response, HttpServletResponse.SC_PRECONDITION_FAILED,
"Modification of logger " + logName + " is disallowed in configuration.");
Expand Down
Loading