[FLINK-8286][Security] Fix kerberos security configuration for YarnTaskExecutor #5896

suez1224 · 2018-04-23T07:02:44Z

What is the purpose of the change

Fix broken YARN kerberos integration for flip-6.

Brief change log

Fix kerberos onfigurations.
Refactor code.
Add unittest.

Verifying this change

This change added tests and can be verified as follows:

*Added test that validates kerberos credentials are set correctly *

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): ( no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no )
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable)

aljoscha · 2018-04-30T17:18:45Z

flink-yarn/src/main/java/org/apache/flink/yarn/YarnTaskExecutorRunnerFactory.java

 			FileSystem.initialize(configuration);
+		} catch (Throwable t) {
+			LOG.error(t.getMessage(), t);


Why is this exception being swallowed?

Good point. Added exceptions to method signature and let caller handle it.

aljoscha · 2018-04-30T17:27:13Z

flink-yarn/src/main/java/org/apache/flink/yarn/YarnTaskExecutorRunnerFactory.java

 			UserGroupInformation currentUser = UserGroupInformation.getCurrentUser();

 			LOG.info("YARN daemon is running as: {} Yarn client user obtainer: {}",
 					currentUser.getShortUserName(), yarnClientUsername);

+			File f = new File(currDir, Utils.KEYTAB_FILE_NAME);


Is the only change really that we always do this instead of having the check on remoteKeytabPath, as the old code had?

The old code had this on line 120:

if (remoteKeytabPath != null) { File f = new File(currDir, Utils.KEYTAB_FILE_NAME); keytabPath = f.getAbsolutePath(); LOG.info("keytab path: {}", keytabPath); }

No, this is a refactoring becaust that part of code is kinda redundant.

The real change is moving this code block below before the call to "new SecurityConfiguration()":

configuration.setString(SecurityOptions.KERBEROS_LOGIN_KEYTAB, f.getAbsolutePath());
configuration.setString(SecurityOptions.KERBEROS_LOGIN_PRINCIPAL, remoteKeytabPrincipal);

I see, could you maybe split this PR in two commits then? One that does the refactoring and one that does the actual fix. This way, it's clearer what exactly the fix is.

@aljoscha done, please take another look. Thanks.

suez1224

@aljoscha thanks for the review. I've replied to the comments, could you please take another look?

suez1224 · 2018-04-30T23:04:30Z

flink-yarn/src/main/java/org/apache/flink/yarn/YarnTaskExecutorRunnerFactory.java

 			FileSystem.initialize(configuration);
+		} catch (Throwable t) {
+			LOG.error(t.getMessage(), t);


Good point. Added exceptions to method signature and let caller handle it.

suez1224 · 2018-04-30T23:07:34Z

flink-yarn/src/main/java/org/apache/flink/yarn/YarnTaskExecutorRunnerFactory.java

 			UserGroupInformation currentUser = UserGroupInformation.getCurrentUser();

 			LOG.info("YARN daemon is running as: {} Yarn client user obtainer: {}",
 					currentUser.getShortUserName(), yarnClientUsername);

+			File f = new File(currDir, Utils.KEYTAB_FILE_NAME);


No, this is a refactoring becaust that part of code is kinda redundant.

The real change is moving this code block below before the call to "new SecurityConfiguration()":

configuration.setString(SecurityOptions.KERBEROS_LOGIN_KEYTAB, f.getAbsolutePath());
configuration.setString(SecurityOptions.KERBEROS_LOGIN_PRINCIPAL, remoteKeytabPrincipal);

aljoscha · 2018-05-08T12:12:29Z

@suez1224 I'll now merge the actual fix, but I'm not 100 % sure the refactoring is correct.

After the fix, we have roughly this path through the code:

if (keytabPath != null && remoteKeytabPrincipal != null) {
	configuration.setString(SecurityOptions.KERBEROS_LOGIN_KEYTAB, keytabPath);
	configuration.setString(SecurityOptions.KERBEROS_LOGIN_PRINCIPAL, remoteKeytabPrincipal);
}

SecurityConfiguration sc = new SecurityConfiguration(configuration);

SecurityUtils.install(sc);

SecurityUtils.getInstalledContext().runSecured(new Callable<Void>() {
	@Override
	public Void call() throws Exception {
		TaskManagerRunner.runTaskManager(configuration, new ResourceID(containerId));
		return null;
	}
});

after the fix, that becomes

// in main()
SecurityUtils.getInstalledContext().runSecured(
   YarnTaskExecutorRunnerFactory.create(System.getenv()));

// in YarnTaskExecutorRunnerFactory.create()
if (keytabPath != null && remoteKeytabPrincipal != null) {
	configuration.setString(SecurityOptions.KERBEROS_LOGIN_KEYTAB, keytabPath);
	configuration.setString(SecurityOptions.KERBEROS_LOGIN_PRINCIPAL, remoteKeytabPrincipal);
}

SecurityConfiguration sc = new SecurityConfiguration(configuration);

SecurityUtils.install(sc);

return new Runner()

Meaning, that if someone messes with how things are called it can happen that SecurityUtils.getInstalledContext() is called before SecurityUtils.install(sc) is called in YarnTaskExecutorRunnerFactory.create. I think this can potentially lead to problems and the old path is much clearer. What do you think?

aljoscha · 2018-05-08T13:55:59Z

Merged, could you please close the PR after discussion of the refactoring?

…urityUtils.install()

suez1224 · 2018-05-09T07:53:06Z

@aljoscha that's a very good catch. I added another commit to make sure the call order is always correct (1298b04). Could you please let me know what you think? I think we should have the unittest in place since this is the second time it breaks. I can create another PR for the refactoring commit.

EronWright · 2018-05-10T00:02:23Z

This PR seems to obfuscate the fix. The issue was with the interpretation of the keytab path, right? But the bulk of the change was to clarify the ordering of context installation vs use?

aljoscha · 2018-05-16T13:03:03Z

@EronWright the fix is really one this: ba3e271 But I haven't yet managed to reproduce a failure on my system without the fix. Still working on it.

aljoscha · 2018-05-16T13:04:19Z

@suez1224 Did you rebase on master? I think that krb5-specific could should not be there anymore.

rmetzger · 2019-01-28T16:11:15Z

@aljoscha @suez1224 This PR appears to be a stale open PR, even though it seem to contain something merged.
I propose to close this PR, and file a JIRA ticket for the refactoring?

tillrohrmann · 2020-06-18T08:17:51Z

Closing this PR as it seems to be abandoned.

aljoscha reviewed Apr 30, 2018

View reviewed changes

suez1224 commented Apr 30, 2018

View reviewed changes

aljoscha mentioned this pull request May 2, 2018

[FLINK-9235][Security] Add integration tests for YARN kerberos integration. #5901

Closed

Shuyi Chen added 2 commits May 2, 2018 12:02

fix YARNTaskExecutor kerberos configurations

d088920

Refactor YarnTaskExecutorRunner and add unittests

c69675b

suez1224 force-pushed the FLINK-8286 branch from 4a62d64 to c69675b Compare May 2, 2018 19:05

ensure SecurityUtils.getInstalledContext() is always called after Sec…

1298b04

…urityUtils.install()

suez1224 force-pushed the FLINK-8286 branch from 16b48be to 1298b04 Compare May 9, 2018 07:48

rmetzger added the component=Deployment/YARN label Mar 18, 2019

tillrohrmann closed this Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-8286][Security] Fix kerberos security configuration for YarnTaskExecutor #5896

[FLINK-8286][Security] Fix kerberos security configuration for YarnTaskExecutor #5896

suez1224 commented Apr 23, 2018

aljoscha Apr 30, 2018

suez1224 Apr 30, 2018

aljoscha Apr 30, 2018

suez1224 Apr 30, 2018

aljoscha May 2, 2018

suez1224 May 2, 2018

suez1224 left a comment

suez1224 Apr 30, 2018

suez1224 Apr 30, 2018

aljoscha commented May 8, 2018

aljoscha commented May 8, 2018

suez1224 commented May 9, 2018

EronWright commented May 10, 2018

aljoscha commented May 16, 2018

aljoscha commented May 16, 2018

rmetzger commented Jan 28, 2019

tillrohrmann commented Jun 18, 2020

[FLINK-8286][Security] Fix kerberos security configuration for YarnTaskExecutor #5896

[FLINK-8286][Security] Fix kerberos security configuration for YarnTaskExecutor #5896

Conversation

suez1224 commented Apr 23, 2018

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suez1224 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljoscha commented May 8, 2018

aljoscha commented May 8, 2018

suez1224 commented May 9, 2018

EronWright commented May 10, 2018

aljoscha commented May 16, 2018

aljoscha commented May 16, 2018

rmetzger commented Jan 28, 2019

tillrohrmann commented Jun 18, 2020