gaborgsomogyi commented on code in PR #22009:
URL: https://github.com/apache/flink/pull/22009#discussion_r1116617650
##########
flink-runtime/src/main/java/org/apache/flink/runtime/security/modules/HadoopModule.java:
##########
@@ -70,10 +71,22 @@ public void install() throws SecurityInstallException {
try {
KerberosLoginProvider kerberosLoginProvider = new
KerberosLoginProvider(securityConfig);
- if (kerberosLoginProvider.isLoginPossible()) {
- kerberosLoginProvider.doLogin();
+ if (kerberosLoginProvider.isLoginPossible(true)) {
+ kerberosLoginProvider.doLogin(true);
loginUser = UserGroupInformation.getLoginUser();
+ if (HadoopUserUtils.isProxyUser((loginUser))
Review Comment:
I think it's good to add this in general not to change the original
behavior. I'm just writing down a possible future consideration so no change
needed for now.
This pointed me to an edge case where a user wants to use proxy user support
w/ non-Hadoop delegation tokens like S3. The original implementation is not
allowing to do this but maybe there is a need for it. I need some more time to
consider this in-depth. Later on we might remove this check when the use-cases
expecting that.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/security/token/hadoop/KerberosLoginProviderITCase.java:
##########
@@ -57,7 +58,7 @@ public void isLoginPossibleMustReturnFalseByDefault() throws
IOException {
UserGroupInformation userGroupInformation =
mock(UserGroupInformation.class);
ugi.when(UserGroupInformation::getCurrentUser).thenReturn(userGroupInformation);
- assertFalse(kerberosLoginProvider.isLoginPossible());
+ assertFalse(kerberosLoginProvider.isLoginPossible(true));
Review Comment:
Such original tests are testing the no proxy cases. I would either set the
new boolean on all non-proxy cases to false or test both proxy and non-proxy
cases (true + false too) to be 100% sure that the original behavior is covered.
It's your choice. Since we're under time pressure I would vote on the first.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/security/token/hadoop/KerberosLoginProvider.java:
##########
@@ -99,6 +101,8 @@ public void doLogin() throws IOException {
LOG.info("Attempting to load user's ticket cache");
UserGroupInformation.loginUserFromSubject(null);
LOG.info("Loaded user's ticket cache successfully");
+ } else if (supportProxyUser) {
+ LOG.info("Delegation token fetch is managed and therefore login is
managed");
Review Comment:
Maybe we can say something like: `Proxy user doesn't need login since it
must have credentials already`. I think `KerberosLoginProvider` shouldn't know
about tokens at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
