Uh oh!
There was an error while loading. Please reload this page.
HBASE-26160: Configurable disallowlist for live editing of loglevels - #3549
Merged
Merged
Conversation
bbeaudreaultforce-pushed
the
readonly_loglevels_master
branch
2 times, most recently
from
August 1, 2021 14:40
156fd05 to
7216ff2CompareApache-HBase
commented
Aug 2, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2021
🎊 +1 overall
This message was automatically generated. |
| + "Log Class: <b>" + log.getClass().getName() +"</b><br />"); | ||
| if (level != null) { | ||
| if (!isLogLevelChangeAllowed(logName, readOnlyLogLevels)) { | ||
| response.sendError(HttpServletResponse.SC_PRECONDITION_FAILED, |
Contributor
There was a problem hiding this comment.
After Jetty 9.4.21, sendError() no longer allows a custom message.
You can use setStatus() instead. See https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/server/AuthenticationFilter.java#L622 for reference.
ContributorAuthor
There was a problem hiding this comment.
Thanks for the pointer. It looks like hbase uses a shaded version of netty, and further up in this class uses sendError to send an error response. I could change this if I get another vote from an hbase committer, but I probably should change both in that case. Seems out of scope for this jira, but can do it if we think I should.
ContributorAuthor
There was a problem hiding this comment.
@jojochuang I looked more closely, and there are at least 6 servlets that use the current sendError behavior, as well as a couple filters and a bunch of tests. It seems like we might want to leave this alone here and instead create a jira to update all usages so that they are future proof. Or we could fix those issues as part of a future "upgrade netty" jira.
Does that sound reasonable?
Contributor
There was a problem hiding this comment.
yup make sense to me. It seems HBase doesn't validate the error messages, which is why this problem doesn't surface up when we bumped Jetty version.
Apache-HBase
commented
Aug 2, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2021
🎊 +1 overall
This message was automatically generated. |
| + "Log Class: <b>" + log.getClass().getName() +"</b><br />"); | ||
| if (level != null) { | ||
| if (!isLogLevelChangeAllowed(logName, readOnlyLogLevels)) { | ||
| response.sendError(HttpServletResponse.SC_PRECONDITION_FAILED, |
Contributor
There was a problem hiding this comment.
yup make sense to me. It seems HBase doesn't validate the error messages, which is why this problem doesn't surface up when we bumped Jetty version.
| return true; | ||
| } | ||
| for (String readOnlyLogLevel : readOnlyLogLevels) { | ||
| if (readOnlyLogLevel.startsWith(logger)) { |
Contributor
There was a problem hiding this comment.
i would have thought it's the other way around: logger. startsWith(readOnlyLogLevel)? not sure.
It would be best if we can verify this new feature in TestLogLevel.
ContributorAuthor
There was a problem hiding this comment.
Good catch. Fixed and added a test.
As you may have anticipated in your original comment about response code verification, this meant I needed to update LogLevel to use setStatus as you said :). This meant having to validate the response in the CLI, which is used in in the test. These were minor tweaks, but you'll see them in the latest commit.
bbeaudreaultforce-pushed
the
readonly_loglevels_master
branch
from
August 3, 2021 13:34
c881ee2 to
0401fadCompareApache-HBase
commented
Aug 3, 2021
💔 -1 overall
This message was automatically generated. |
ContributorAuthor
@jojochuang let me know if this looks good with the latest commit. If so, I'll squash and clean up the commits for merging. I'd also like to get this merged into branch-2 and branch-2.4. The LogLevel changes apply cleanly, but TestLogLevel has a bunch of merge conflicts. I'll need to submit a PR which reconciles those once ready. I think the latest test failure is because I force pushed as the build was starting. Hopefully will succeed on re-run. |
Apache-HBase
commented
Aug 3, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 3, 2021
💔 -1 overall
This message was automatically generated. |
bbeaudreaultforce-pushed
the
readonly_loglevels_master
branch
from
August 3, 2021 13:58
0401fad to
55451a8CompareApache-HBase
commented
Aug 3, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 3, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 3, 2021
💔 -1 overall
This message was automatically generated. |
bbeaudreault
commented
Aug 3, 2021
ContributorAuthor
@jojochuang not sure why my latest commit did not kick off a re-build. Would you mind kicking one off when you get a chance? |
Apache-HBase
commented
Aug 3, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 3, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 3, 2021
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
commented
Aug 3, 2021
ContributorAuthor
Looks like all checks are passing now. Thanks whoever fixed that :) |
bbeaudreaultforce-pushed
the
readonly_loglevels_master
branch
from
August 4, 2021 11:51
55451a8 to
69aa2c0Compare
This was referenced Aug 4, 2021
bbeaudreault
commented
Aug 4, 2021
ContributorAuthor
@jojochuang thanks for the review! As promised, I've squashed the commits here and submitted #3558 and #3559 for porting to branch 2 and 2.4. Let me know if there's anything else I need to do to get these merged. |
Apache-HBase
commented
Aug 4, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 4, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 4, 2021
🎊 +1 overall
This message was automatically generated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.