-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[FLINK-29761][runtime][security] Simplify HadoopModule #21160
Conversation
&& !StringUtils.isBlank(securityConfig.getPrincipal())) { | ||
String keytabPath = (new File(securityConfig.getKeytab())).getAbsolutePath(); | ||
|
||
UserGroupInformation.loginUserFromKeytab(securityConfig.getPrincipal(), keytabPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steveloughran since you have quite some XP w/ UGI a quick question:
AFAIK UserGroupInformation.loginUserFromKeytab
does the same just like
ugi = UserGroupInformation.loginUserFromKeytabAndReturnUGI
UserGroupInformation.setLoginUser(ugi);
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, plus sets up a renewer thread to do relogin on a regular basis.
Method loginUserFromSubjectMethod = | ||
UserGroupInformation.class.getMethod( | ||
"loginUserFromSubject", Subject.class); | ||
loginUserFromSubjectMethod.invoke(null, (Subject) null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steveloughran here is another similar question:
AFAIK UserGroupInformation.loginUserFromSubject
does the same just like
ugi = UserGroupInformation.getUGIFromTicketCache
UserGroupInformation.setLoginUser(ugi);
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh, its more complex than that. look at the source.
@steveloughran So if I understand correctly it would be better to use the original UGI calls instead of the other APIs because:
Correct me if I'm wrong. |
6d3989d
to
0b3f379
Compare
Based on the fact that the UGI APIs are not doing the same things I've refactored the original PR and using the exact same API calls. Re-tested it manually and works fine. |
0b3f379
to
4b49fea
Compare
@flinkbot run azure |
4b49fea
to
1cfa567
Compare
@flinkbot run azure |
1cfa567
to
f2b1644
Compare
flink-runtime/src/test/java/org/apache/flink/runtime/hadoop/HadoopUserUtilsITCasae.java
Outdated
Show resolved
Hide resolved
9429c05
to
2b4cae1
Compare
2b4cae1
to
83dd347
Compare
@flinkbot run azure |
// UserGroupInformation.loginUserFromSubject(null); | ||
Method loginUserFromSubjectMethod = | ||
UserGroupInformation.class.getMethod( | ||
"loginUserFromSubject", Subject.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to help reviewers this API has been added in Hadoop v2.3.0. Please see this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @gaborgsomogyi. Looks good, added a minor javadoc comment which I will squash into your commit on merge.
Merging later today EU time unless anyone has objections.
What is the purpose of the change
HadoopModule
is quite complex and contains reflection which can be simplified. In this PR I've made this simplification keeping the original functionality.The other important intention is that
flink-runtime
module has used several things fromflink-hadoop-fs
module which is just bad from architectural perspective. In this PR this I've eliminated this.Brief change log
KerberosLoginProvider
inHadoopModule
Verifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation