[FLINK-11088][Security][YARN] Allow YARN to discover pre-installed keytab files #7702

walterddr · 2019-02-14T04:23:57Z

What is the purpose of the change

There're 2 ways of utilizing Kerberos keytab files:

Flink client upload Kerberos keytab files through Yarn local resource bucket.
Flink YARN containers directly load pre-installed Kerberos keytab files from local file system.

Previously Flink only support method #1. This PR introduces two new configuration keys in the YARN configurations to support method #2.

Brief change log

Added two new key with default values
- yarn.security.kerberos.keytab indicates where the keytab file should be on YARN container.
- yarn.security.kerberos.ship-local-keytab: If set to true, Flink will upload the client keytab used in its own section to YARN local resource bucket. if set to false, Flink will assume the path configured in yarn.security.kerberos.keytab already exists and with proper permission.
Changed YarnClusterDescriptor to work with 2 options above.
Changed the YarnTaskExecutorRunner to load keytab configurations differently according to 2 options above.

Verifying this change

This change is already covered by existing tests in flink-yarn-test component.

This change also added tests

added additional test config parsing in YarnTaskExecutorRunnerTest.
Modified YARNSessionFIFOITCase and YARNSessionFIFOSecuredITCase to allow dynamic properties loading during test sections.
Added specific section for pre-installed YARN Kerberos keytab file.

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: yes
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? yes
If yes, how is the feature documented? document regenerated

flinkbot · 2019-02-14T04:24:48Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Review Progress

❌ 1. The [description] looks good.
❌ 2. There is [consensus] that the contribution should go into to Flink.
❔ 3. Needs [attention] from.
❌ 4. The change fits into the overall [architecture].
❌ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve the 1st aspect (similarly, it also supports the consensus, architecture and quality keywords)
@flinkbot approve all to approve all aspects
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval

flinkbot · 2020-01-08T18:27:04Z

CI report:

d4a17be UNKNOWN
0e955bd Travis: PENDING Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

walterddr

@tisonkun @wangyang0918 @tillrohrmann could you guys take a look please?

aljoscha

I added preliminary comments about config names. Could you please rebase this on top of master and add a proper commit title/message?

aljoscha · 2020-02-11T10:30:15Z

flink-yarn/src/main/java/org/apache/flink/yarn/configuration/YarnConfigOptions.java

@@ -225,6 +226,22 @@
 				.noDefaultValue()
 				.withDescription("Specify YARN node label for the YARN application.");

+	public static final ConfigOption<Boolean> REQUIRE_LOCALIZE_KEYTAB =
+		key("yarn.security.kerberos.require.localize.keytab")


I think this should be called yarn.security.kerberos.ship-client-keytab or yarn.security.kerberos.ship-local-keytab

aljoscha · 2020-02-11T10:30:45Z

flink-yarn/src/main/java/org/apache/flink/yarn/configuration/YarnConfigOptions.java

+				" resource bucket with the specified relative path defined in 'yarn.security.kerberos.keytab.path'.");
+
+	public static final ConfigOption<String> LOCALIZED_KEYTAB_PATH =
+		key("yarn.security.kerberos.keytab.path")


To be more in line with existing keytab options this could be simplified to yarn.security.kerberos.keytab

walterddr · 2020-02-24T16:42:44Z

@aljoscha thanks for the suggestions, the config key changes makes much more sense. I have adjusted them and rebased the commits. Please kindly take another look :-)

aljoscha · 2020-03-10T13:48:39Z

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

 	public static final String KEYTAB_PATH = "_KEYTAB_PATH";
+	public static final String REMOTE_KEYTAB_PATH = "_REMOTE_KEYTAB_PATH";


Is it actually necessary to store these keys in the environment, can't we directly use the Flink Configuration to retrieve these settings?

This is a good catch (to also response to other comments related) --> technically speaking there should only be one ConfigKey in the JM/TM perspective: the "REMOTE_KEYTAB_PATH", or previously known as "KEYTAB_PATH" - as you mentioned, it doesn't matter whether it is originated from a remote ship location or pre-installed.

I would revert the changes I've done here.

aljoscha · 2020-03-10T13:53:12Z

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

 					fs,
 					appId,
 					new Path(keytab),
 					localResources,
 					homeDir,
 					"",
 					fileReplication);
+			} else {


Aren't we doing duplicate work here? If we don't have a local keytab and we don't ship it, won't the configuration be already in place and the cluster entrypoint can use it?

aljoscha · 2020-03-10T13:53:38Z

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

-					Utils.KEYTAB_FILE_NAME,
+			boolean	requireLocalizedKeytab = flinkConfiguration.getBoolean(YarnConfigOptions.SHIP_LOCAL_KEYTAB);
+			localizedKeytabPath = flinkConfiguration.getString(YarnConfigOptions.LOCALIZED_KEYTAB_PATH);
+			if (requireLocalizedKeytab) {


I think this can be moved to the outer if, i.e. keytab != null && requireLocalizedKeytab

aljoscha · 2020-03-10T13:55:23Z

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

@@ -941,10 +950,13 @@ private ApplicationReport startAppMaster(
 		// https://github.com/apache/hadoop/blob/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-site/src/site/markdown/YarnApplicationSecurity.md#identity-on-an-insecure-cluster-hadoop_user_name
 		appMasterEnv.put(YarnConfigKeys.ENV_HADOOP_USER_NAME, UserGroupInformation.getCurrentUser().getUserName());

-		if (remotePathKeytab != null) {
-			appMasterEnv.put(YarnConfigKeys.KEYTAB_PATH, remotePathKeytab.toString());
+		if (localizedKeytabPath != null) {


I think we wouldn't have to do this if we simply used the Flink configuration for getting the config values. I think we don't need the environment variables at all and can thus simplify this.

+1, reverting

Just a minor question here. It seems user have to config KERBEROS_LOGIN_KEYTAB. Does it make sense? If the keytab is pre-installed, I think there is no need to define KERBEROS_LOGIN_KEYTAB anymore.

we are not operating under the premise that only one Keytab file is pre-installed. multiple keytab files could be pre-installed and thus user would need to choose

It seems user could choose it by LOCALIZED_KEYTAB_PATH. Am I understand it correctly? If SHIP_LOCAL_KEYTAB is false, then we do not care about the value of KERBEROS_LOGIN_KEYTAB but we still need to set it?

aljoscha · 2020-03-10T13:56:40Z

flink-yarn/src/main/java/org/apache/flink/yarn/entrypoint/YarnEntrypointUtils.java

+				if (f.exists()) { // keytab file exist in working directory.
+					keytabPath = f.getAbsolutePath();
+				} else { // fall back to default keytab file
+					f = new File(workingDirectory, Utils.DEFAULT_KEYTAB_FILE);


Aren't we duplicating the default logic here? If nothing is set the default setting in the Flink Configuration should already be DEFAULT_KEYTAB_FILE.

yes. good catch. this piece of code was there before we use the YarnConfigOption approach. I would refine this.

walterddr

Thanks for the review @aljoscha and good catch on the redundant info shipped to Flink cluster. I would revise the PR soon.

walterddr · 2020-03-11T03:52:52Z

flink-yarn/src/main/java/org/apache/flink/yarn/entrypoint/YarnEntrypointUtils.java

+				if (f.exists()) { // keytab file exist in working directory.
+					keytabPath = f.getAbsolutePath();
+				} else { // fall back to default keytab file
+					f = new File(workingDirectory, Utils.DEFAULT_KEYTAB_FILE);


yes. good catch. this piece of code was there before we use the YarnConfigOption approach. I would refine this.

walterddr · 2020-03-11T03:54:41Z

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

 	public static final String KEYTAB_PATH = "_KEYTAB_PATH";
+	public static final String REMOTE_KEYTAB_PATH = "_REMOTE_KEYTAB_PATH";


This is a good catch (to also response to other comments related) --> technically speaking there should only be one ConfigKey in the JM/TM perspective: the "REMOTE_KEYTAB_PATH", or previously known as "KEYTAB_PATH" - as you mentioned, it doesn't matter whether it is originated from a remote ship location or pre-installed.

I would revert the changes I've done here.

walterddr · 2020-03-11T03:55:12Z

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

@@ -941,10 +950,13 @@ private ApplicationReport startAppMaster(
 		// https://github.com/apache/hadoop/blob/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-site/src/site/markdown/YarnApplicationSecurity.md#identity-on-an-insecure-cluster-hadoop_user_name
 		appMasterEnv.put(YarnConfigKeys.ENV_HADOOP_USER_NAME, UserGroupInformation.getCurrentUser().getUserName());

-		if (remotePathKeytab != null) {
-			appMasterEnv.put(YarnConfigKeys.KEYTAB_PATH, remotePathKeytab.toString());
+		if (localizedKeytabPath != null) {


+1, reverting

walterddr · 2020-03-11T03:55:22Z

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

-					Utils.KEYTAB_FILE_NAME,
+			boolean	requireLocalizedKeytab = flinkConfiguration.getBoolean(YarnConfigOptions.SHIP_LOCAL_KEYTAB);
+			localizedKeytabPath = flinkConfiguration.getString(YarnConfigOptions.LOCALIZED_KEYTAB_PATH);
+			if (requireLocalizedKeytab) {


walterddr · 2020-03-11T03:55:26Z

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

 					fs,
 					appId,
 					new Path(keytab),
 					localResources,
 					homeDir,
 					"",
 					fileReplication);
+			} else {


…ytab files This is to change how keytab files are discovered, * remove hard-coded keytab filenames * extended YARNSessionFIFOITCase to accommodate different types of security setting test cases regenerate configuration documentations

…add logging

aljoscha · 2020-03-11T15:53:21Z

@walterddr I have two more suggestions on top of your PR: https://github.com/aljoscha/flink/tree/pr-7702-yarn-keytab. Could you please take a look there.

aljoscha · 2020-03-11T17:24:49Z

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

+	public static String resolveKeytabPath(String workingDir, String keytabPath) {
+		String keytab = null;
+		if (keytabPath == null) { // keytab not exist
+			LOG.info("keytab path isn't configured!");


I think we shouldn't log here, because now all users will always get this log message, even if they don't do anything with Kerberos, that might be confusing.

walterddr · 2020-03-11T22:56:53Z

@flinkbot run travis

walterddr · 2020-03-11T23:16:25Z

@flinkbot run azure

walterddr · 2020-03-12T21:32:57Z

@flinkbot run azure

aljoscha · 2020-03-13T09:49:04Z

@walterddr Ah, so we were trying to read the actual config key and not the path stored in the environment under that key, right?

aljoscha · 2020-03-13T09:49:19Z

good find! 👌

walterddr · 2020-03-13T11:56:36Z

yes. surprise it didn't triggered any issue originally in the ITCase. I guess that's because there's really no changes in the file when trying to setupLocalResource with miniYarn on the exact same local file system.

…lsTest

aljoscha · 2020-03-13T15:15:09Z

I merged this. Thanks a lot for the good collaboration on this. 😃

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

walterddr force-pushed the FLINK-11088 branch from 467cd32 to 72dd07f Compare January 8, 2020 18:11

walterddr commented Jan 14, 2020

View reviewed changes

aljoscha self-requested a review February 10, 2020 16:38

aljoscha self-assigned this Feb 10, 2020

aljoscha requested changes Feb 11, 2020

View reviewed changes

walterddr force-pushed the FLINK-11088 branch 2 times, most recently from f238728 to 6849601 Compare February 19, 2020 23:10

walterddr force-pushed the FLINK-11088 branch from 6849601 to 0974d56 Compare February 21, 2020 16:21

aljoscha reviewed Mar 10, 2020

View reviewed changes

walterddr commented Mar 11, 2020

View reviewed changes

Rong Rong and others added 3 commits March 11, 2020 15:52

fixup! simplify configuration options/defaults

6aea2e7

[FLINK-11088] In YarnTaskExecutorRunner, simplify keytab loading and …

efef8be

…add logging

[FLINK-11088][yarn] refactor keytab resolve to Utils, regenerate docs

c59f870

walterddr force-pushed the FLINK-11088 branch from d17c4e1 to c59f870 Compare March 11, 2020 17:05

aljoscha reviewed Mar 11, 2020

View reviewed changes

[hotfix] remove excessive logging

d4a17be

[hotfix] fix e2e test exception

ae58c4f

[FLINK-11088] Add unit test for Kerberos parsing in YarnEntrypointUti…

0e955bd

…lsTest

aljoscha closed this Mar 13, 2020

		public static final String KEYTAB_PATH = "_KEYTAB_PATH";
		public static final String REMOTE_KEYTAB_PATH = "_REMOTE_KEYTAB_PATH";

[FLINK-11088][Security][YARN] Allow YARN to discover pre-installed keytab files #7702

[FLINK-11088][Security][YARN] Allow YARN to discover pre-installed keytab files #7702

Conversation

walterddr commented Feb 14, 2019 • edited

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

flinkbot commented Feb 14, 2019

Review Progress

flinkbot commented Jan 8, 2020 • edited

CI report:

walterddr left a comment

Choose a reason for hiding this comment

aljoscha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterddr commented Feb 24, 2020

Choose a reason for hiding this comment

walterddr Mar 11, 2020 • edited

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

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

walterddr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterddr Mar 11, 2020 • edited

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

aljoscha commented Mar 11, 2020

Choose a reason for hiding this comment

walterddr commented Mar 11, 2020

walterddr commented Mar 11, 2020

walterddr commented Mar 12, 2020

aljoscha commented Mar 13, 2020

aljoscha commented Mar 13, 2020

walterddr commented Mar 13, 2020 • edited

aljoscha commented Mar 13, 2020

walterddr commented Feb 14, 2019 •

edited

flinkbot commented Jan 8, 2020 •

edited

walterddr Mar 11, 2020 •

edited

walterddr Mar 11, 2020 •

edited

walterddr commented Mar 13, 2020 •

edited