[GitHub] [flink] mridulm commented on a diff in pull request #22509: [FLINK-31983] Add yarn Acls capability to Flink containers

2023-05-04 Thread via GitHub


mridulm commented on code in PR #22509:
URL: https://github.com/apache/flink/pull/22509#discussion_r1185420037


##
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNSessionFIFOITCase.java:
##
@@ -61,6 +61,9 @@
 class YARNSessionFIFOITCase extends YarnTestBase {
 private static final Logger log = 
LoggerFactory.getLogger(YARNSessionFIFOITCase.class);
 
+protected static final String VIEW_ACLS = "user groupUser";
+protected static final String MODIFY_ACLS = "admin groupAdmin";

Review Comment:
   `groupUser` threw me off - `group` would be better :)



-- 
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



[GitHub] [flink] mridulm commented on a diff in pull request #22509: [FLINK-31983] Add yarn Acls capability to Flink containers

2023-05-02 Thread via GitHub


mridulm commented on code in PR #22509:
URL: https://github.com/apache/flink/pull/22509#discussion_r1183111493


##
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNSessionFIFOITCase.java:
##
@@ -61,6 +61,9 @@
 class YARNSessionFIFOITCase extends YarnTestBase {
 private static final Logger log = 
LoggerFactory.getLogger(YARNSessionFIFOITCase.class);
 
+protected static final String VIEW_ACLS = "user groupUser";
+protected static final String MODIFY_ACLS = "admin groupAdmin";

Review Comment:
   Comma separated ?



##
flink-yarn/src/main/java/org/apache/flink/yarn/Utils.java:
##
@@ -425,6 +426,8 @@ static ContainerLaunchContext createTaskExecutorContext(
 
 ctx.setEnvironment(containerEnv);
 
+setAclsFor(ctx, flinkConfig);

Review Comment:
   This is within `createTaskExecutorContext` - so for TM.
   The change in `YarnClusterDescriptor` is within `startAppMaster`, so for JM



##
flink-yarn/src/main/java/org/apache/flink/yarn/Utils.java:
##
@@ -620,4 +623,30 @@ public static YarnConfiguration getYarnConfiguration(
 
 return yarnConfig;
 }
+
+/**
+ * Sets the application ACLs for the given ContainerLaunchContext based on 
the values specified
+ * in the given Flink configuration. Only ApplicationAccessType.VIEW_APP 
and
+ * ApplicationAccessType.MODIFY_APP ACLs are set, and only if they are 
configured in the Flink
+ * configuration.
+ *
+ * @param amContainer the ContainerLaunchContext to set the ACLs for
+ * @param flinkConfig the Flink configuration to read the ACL values from
+ */
+public static void setAclsFor(
+ContainerLaunchContext amContainer,
+org.apache.flink.configuration.Configuration flinkConfig) {
+Map acls = new HashMap<>();
+String viewAcls = 
flinkConfig.getString(YarnConfigOptions.APPLICATION_VIEW_ACLS, null);
+String modifyAcls = 
flinkConfig.getString(YarnConfigOptions.APPLICATION_MODIFY_ACLS, null);
+if (viewAcls != null) {
+acls.put(ApplicationAccessType.VIEW_APP, viewAcls);
+}
+if (modifyAcls != null) {
+acls.put(ApplicationAccessType.MODIFY_APP, modifyAcls);
+}

Review Comment:
   By default include `user.name` always ? (for view and modify acl)



-- 
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