Skip to content
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.

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

Closes #14. Proxy in workload job to use enable permission checking. #18

Merged
merged 1 commit into from
Apr 5, 2018

Conversation

xkrogen
Copy link
Collaborator

@xkrogen xkrogen commented Mar 28, 2018

No description provided.

Copy link
Collaborator

@chliang71 chliang71 left a comment

Choose a reason for hiding this comment

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

LGTM, only some minor nits

commandQueue = new DelayQueue<>();
Configuration mapperConf = mapperContext.getConfiguration();
String namenodeURI = mapperConf.get(WorkloadDriver.NN_URI);
this.fsCache = fsCache;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: remove 'this.' to be consistent with the rest?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The this is necessary to quality fsCache as a field since fsCache is also the name of a parameter.

@@ -63,6 +64,7 @@ public AuditReplayCommand parse(Text inputLine, Function<Long, Long> relativeToA
String auditMessageSanitized = m.group(2).replace("(options=", "(options:");
Map<String, String> parameterMap = AUDIT_SPLITTER.split(auditMessageSanitized);
return new AuditReplayCommand(relativeToAbsolute.apply(relativeTimestamp),
SPACE_SPLITTER.split(parameterMap.get("ugi")).iterator().next(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

more of a question, does this imply there can be multiple values for ugi, and here we only take the first? I wonder why there can be multiple of them, and why taking just first is enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sort of. Here are a few example UGI strings:

ugi=user (auth:TOKEN) via user/host@REALM (auth:TOKEN)
ugi=user (auth:KERBEROS)
ugi=user@REALM (auth:TOKEN)
ugi=user/host@REALM (auth:TOKEN)

Taking the first is enough because the second is used if the UGI is proxied; the first user is the effective user which is all we care about.

However this made me realize that this current logic is not sufficient to cover the last two cases. Maybe it would be best to do a regex match for the first character string up until whitespace, /, or @. I will update accordingly.

@xkrogen
Copy link
Collaborator Author

xkrogen commented Mar 30, 2018

Hey @chen-liang , mind taking another quick look? I added a comment to address your last question about why we split on space for UGI, and also fixed the issue that UGIs like ugi/host@REALM or ugi@REALM would not be properly handled.

@chliang71
Copy link
Collaborator

LGTM, +1

@xkrogen xkrogen merged commit aadf7c2 into linkedin:master Apr 5, 2018
xkrogen added a commit to xkrogen/dynamometer that referenced this pull request Apr 6, 2018
@xkrogen xkrogen deleted the ekrogen-workload-proxy branch April 11, 2018 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants