[FLINK-12869] Add yarn acls capability to flink containers #8760

ashangit · 2019-06-17T08:14:02Z

What is the purpose of the change

Provide yarn application acls mechanism on flink containers to be able to provide specific rights to other users than the one running the job (view logs through the resourcemanager/job history, kill the application)

Brief change log

Add 2 parameter (yarn.view.acls and yarn.admin.acls and set ApplicationACLs for all containers

Verifying this change

This change added tests and can be verified as follows:

Manually verified the change by running a WordCount job on a yarn cluster with acls enabled. Verifying that without yarn.admin.acls a user not running the app can't kill the app. Then relaunching the app providing rights to an other user (setting yarn.admin.acls) and check that the specified user can kill the app. The same can be done for yarn.view.acls by checking that the user can now access the application overview from the resourcemanager (<resourcemanager_url>/cluster/app/<app_id>)

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: Affect acls on YARN (currently not managed so no rights can be added)
The S3 file system connector: no

Documentation

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

flinkbot · 2019-06-17T08:16:11Z

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.

Automated Checks

Last check on commit f0cc6b6 (Wed Dec 04 14:57:15 UTC 2019)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

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.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2019-09-18T16:08:30Z

CI report:

8acc0e8 : FAILURE Build
f0cc6b6 : FAILURE Build

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build

GJL

Thank you for your contribution to Apache Flink @ashangit! Sorry that the review took so long.

Currently, the change is untested. Do you think it makes sense to add a new test to YARNSessionFIFOSecuredITCase?

GJL · 2019-09-25T11:02:08Z

@ashangit Do you still want to work on this?

ashangit · 2019-09-26T05:58:31Z

@GJL will look at how I can add some test this week or the next

GJL · 2019-09-26T15:36:15Z

@ashangit Alright, thanks for following up.

GJL · 2019-10-02T13:23:33Z

Thanks for updating the PR. I'll have another look

GJL · 2019-10-04T11:01:31Z

@flinkbot run travis

GJL

I have left some minor comments. Please let me know what you think.

GJL · 2019-10-04T11:26:24Z

flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java

+		for (int nmId = 0; nmId < NUM_NODEMANAGERS; nmId++) {
+			NodeManager nm = yarnCluster.getNodeManager(nmId);
+			nm.getNMContext().getContainers().forEach((k, v) -> {
+				containers.put(k, v.getLaunchContext().getApplicationACLs());


I think this function should go into YARNSessionFIFOSecuredITCase. It's not likely that it will be used elsewhere.

Moreover, you are using a functional style (forEach) with side effects. However, I think when programming in a functional style, one should just return the result. For example:

private static Map<ContainerId, Map<ApplicationAccessType, String>> getRunningContainersAcls() { return nodeManagersStream() .flatMap(toContainersStream()) .collect(Collectors.toMap( Map.Entry::getKey, entry -> getApplicationACLs(entry.getValue()))); } private static Stream<NodeManager> nodeManagersStream() { return IntStream .range(0, NUM_NODEMANAGERS) .mapToObj(i -> yarnCluster.getNodeManager(i)); } private static Function<NodeManager, Stream<Map.Entry<ContainerId, Container>>> toContainersStream() { return nodeManager -> nodeManager.getNMContext().getContainers().entrySet().stream(); } private static Map<ApplicationAccessType, String> getApplicationACLs(final Container container) { return container.getLaunchContext().getApplicationACLs(); }

GJL · 2019-10-04T11:41:03Z

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

@@ -263,6 +265,13 @@ private static LocalResource registerLocalResource(FileSystem fs, Path remoteRsr
 		return localResource;
 	}

+	public static void setAclsFor(ContainerLaunchContext amContainer, org.apache.flink.configuration.Configuration flinkConfig) {
+		amContainer.setApplicationACLs(new HashMap<ApplicationAccessType, String>(){{


Because double brace initialization has some caveats, I wouldn't use it.

GJL · 2019-10-04T11:52:43Z

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

@@ -263,6 +265,13 @@ private static LocalResource registerLocalResource(FileSystem fs, Path remoteRsr
 		return localResource;
 	}

+	public static void setAclsFor(ContainerLaunchContext amContainer, org.apache.flink.configuration.Configuration flinkConfig) {
+		amContainer.setApplicationACLs(new HashMap<ApplicationAccessType, String>(){{


I would feel more comfortable if we only set ApplicationAccessType.VIEW_APP and ApplicationAccessType.MODIFY_APP, if the user actually configured application ACLs. Also, can we make the default values of the new config options null? What do you think?

GJL · 2019-10-04T11:53:42Z

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

+	/**
+	 * Users and groups to give MODIFY access.
+	 */
+	public static final ConfigOption<String> APPLICATION_ADMIN_ACLS =


I was thinking about renaming this to APPLICATION_MODIFY_ACLS so that it is aligned with Hadoop. What do you think?

GJL · 2019-10-04T11:54:43Z

flink-yarn-tests/src/test/java/org/apache/flink/yarn/YarnTestBase.java

@@ -578,6 +579,17 @@ public static int getRunningContainers() {
 		return count;
 	}

+	public static HashMap<ContainerId, Map<ApplicationAccessType, String>> getRunningContainersAcls() {


It's better to declare Map<...> as return type.

GJL · 2019-10-25T07:46:19Z

@ashangit Can I get feedback on the comments I have left? I can finish the remaining work, if you do not have time to work on it.

GJL · 2019-11-04T10:45:38Z

Closing due to inactivity. Feel free to reopen if you find time to work on this. As written above, I can also finalize it.

[FLINK-12869] Add yarn acls capability to flink containers

8acc0e8

rmetzger added review=description? component=Deployment/YARN labels Jun 17, 2019

GJL self-assigned this Sep 18, 2019

GJL reviewed Sep 20, 2019

View reviewed changes

n.fraison and others added 2 commits September 27, 2019 11:55

Add tests

1bfb531

Merge branch 'master' into master

f0cc6b6

GJL requested changes Oct 4, 2019

View reviewed changes

GJL closed this Nov 4, 2019

architgyl mentioned this pull request May 2, 2023

[FLINK-31983] Add yarn Acls capability to Flink containers #22509

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-12869] Add yarn acls capability to flink containers #8760

[FLINK-12869] Add yarn acls capability to flink containers #8760

ashangit commented Jun 17, 2019

flinkbot commented Jun 17, 2019 •

edited

flinkbot commented Sep 18, 2019 •

edited

GJL left a comment

GJL commented Sep 25, 2019

ashangit commented Sep 26, 2019

GJL commented Sep 26, 2019

GJL commented Oct 2, 2019

GJL commented Oct 4, 2019

GJL left a comment

GJL Oct 4, 2019

GJL Oct 4, 2019

GJL Oct 4, 2019

GJL Oct 4, 2019

GJL Oct 4, 2019

GJL commented Oct 25, 2019

GJL commented Nov 4, 2019

[FLINK-12869] Add yarn acls capability to flink containers #8760

[FLINK-12869] Add yarn acls capability to flink containers #8760

Conversation

ashangit commented Jun 17, 2019

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 Jun 17, 2019 • edited

Automated Checks

Review Progress

flinkbot commented Sep 18, 2019 • edited

CI report:

GJL left a comment

Choose a reason for hiding this comment

GJL commented Sep 25, 2019

ashangit commented Sep 26, 2019

GJL commented Sep 26, 2019

GJL commented Oct 2, 2019

GJL commented Oct 4, 2019

GJL left a comment

Choose a reason for hiding this comment

GJL Oct 4, 2019

Choose a reason for hiding this comment

GJL Oct 4, 2019

Choose a reason for hiding this comment

GJL Oct 4, 2019

Choose a reason for hiding this comment

GJL Oct 4, 2019

Choose a reason for hiding this comment

GJL Oct 4, 2019

Choose a reason for hiding this comment

GJL commented Oct 25, 2019

GJL commented Nov 4, 2019

flinkbot commented Jun 17, 2019 •

edited

flinkbot commented Sep 18, 2019 •

edited