Skip to content

HADOOP-18014. CallerContext should not include some characters. #3698

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

Merged
tasanuma merged 3 commits into from
Nov 25, 2021
Merged

HADOOP-18014. CallerContext should not include some characters. #3698

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
4d3cec7
HADOOP-18014. CallerContext should not include some characters.
tasanuma Nov 22, 2021
4d1aa67
use try-with-resources
tasanuma Nov 22, 2021
ff11828
Clear CallerContext at the end of the unit test
tasanuma Nov 23, 2021
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
2 changes: 0 additions & 2 deletions hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/CallerContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,6 @@ private void checkFieldSeparator(String separator) {

/**
* Whether the field is valid.
* The field should not contain '\t', '\n', '='.
* Because the context could be written to audit log.
* @param field one of the fields in context.
* @return true if the field is not null or empty.
*/
Expand Down
12 changes: 6 additions & 6 deletions ...roject/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8800,18 +8800,18 @@ public void logAuditEvent(boolean succeeded, String userName,
callerContext != null &&
callerContext.isContextValid()) {
sb.append("\t").append("callerContext=");
if (callerContext.getContext().length() > callerContextMaxLen) {
sb.append(callerContext.getContext().substring(0,
callerContextMaxLen));
String context = escapeJava(callerContext.getContext());
if (context.length() > callerContextMaxLen) {
sb.append(context, 0, callerContextMaxLen);
} else {
sb.append(callerContext.getContext());
sb.append(context);
}
if (callerContext.getSignature() != null &&
callerContext.getSignature().length > 0 &&
callerContext.getSignature().length <= callerSignatureMaxLen) {
sb.append(":")
.append(new String(callerContext.getSignature(),
CallerContext.SIGNATURE_ENCODING));
.append(escapeJava(new String(callerContext.getSignature(),
CallerContext.SIGNATURE_ENCODING)));
}
}
logAuditMessage(sb.toString());
Expand Down
39 changes: 34 additions & 5 deletions ...ect/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestAuditLogger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,10 +256,9 @@ public void testAuditLoggerWithCallContext() throws IOException {
conf.setBoolean(HADOOP_CALLER_CONTEXT_ENABLED_KEY, true);
conf.setInt(HADOOP_CALLER_CONTEXT_MAX_SIZE_KEY, 128);
conf.setInt(HADOOP_CALLER_CONTEXT_SIGNATURE_MAX_SIZE_KEY, 40);
MiniDFSCluster cluster = new MiniDFSCluster.Builder(conf).build();
LogCapturer auditlog = LogCapturer.captureLogs(FSNamesystem.auditLog);

try {
try (MiniDFSCluster cluster = new MiniDFSCluster.Builder(conf).build()) {
LogCapturer auditlog = LogCapturer.captureLogs(FSNamesystem.auditLog);
cluster.waitClusterUp();
final FileSystem fs = cluster.getFileSystem();
final long time = System.currentTimeMillis();
Expand Down Expand Up @@ -414,8 +413,8 @@ public void run() {
assertFalse(auditlog.getOutput().contains("callerContext="));
auditlog.clearOutput();

} finally {
cluster.shutdown();
// clear client context
CallerContext.setCurrent(null);
}
}

Expand Down Expand Up @@ -599,6 +598,36 @@ public void testAuditLogWithRemotePort() throws Exception {
}
}

@Test
public void testCallerContextCharacterEscape() throws IOException {
Configuration conf = new HdfsConfiguration();
conf.setBoolean(HADOOP_CALLER_CONTEXT_ENABLED_KEY, true);
conf.setInt(HADOOP_CALLER_CONTEXT_MAX_SIZE_KEY, 128);
conf.setInt(HADOOP_CALLER_CONTEXT_SIGNATURE_MAX_SIZE_KEY, 40);

try (MiniDFSCluster cluster = new MiniDFSCluster.Builder(conf).build()) {
LogCapturer auditlog = LogCapturer.captureLogs(FSNamesystem.auditLog);
cluster.waitClusterUp();
final FileSystem fs = cluster.getFileSystem();
final long time = System.currentTimeMillis();
final Path p = new Path("/");

assertNull(CallerContext.getCurrent());
Copy link
Contributor

@virajjasani virajjasani Nov 22, 2021

Choose a reason for hiding this comment

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

This seems to be failing as per QA report:

Error Message
expected null, but was:<>
Stacktrace
java.lang.AssertionError: expected null, but was:<>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotNull(Assert.java:756)
	at org.junit.Assert.assertNull(Assert.java:738)
	at org.junit.Assert.assertNull(Assert.java:748)
	at org.apache.hadoop.hdfs.server.namenode.TestAuditLogger.testCallerContextCharacterEscape(TestAuditLogger.java:617)

Maybe we can use separate thread here so that we can expect callerContext to be null (per ThreadLocal)?

Copy link
Member Author

@tasanuma tasanuma Nov 23, 2021

Choose a reason for hiding this comment

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

@virajjasani Thanks for your review. I fixed it to clear the caller context at the end of the unit test. ff11828


CallerContext context = new CallerContext.Builder("c1\nc2").append("c3\tc4")
.setSignature("s1\ns2".getBytes(CallerContext.SIGNATURE_ENCODING)).build();
CallerContext.setCurrent(context);
LOG.info("Set current caller context as {}", CallerContext.getCurrent());
fs.setTimes(p, time, time);
assertTrue(auditlog.getOutput().endsWith(
String.format("callerContext=c1\\nc2,c3\\tc4:s1\\ns2%n")));
auditlog.clearOutput();

// clear client context
CallerContext.setCurrent(null);
}
}

public static class DummyAuditLogger implements AuditLogger {

static boolean initialized;
Expand Down