Copilot commented on code in PR #8473:
URL: https://github.com/apache/hadoop/pull/8473#discussion_r3212205074
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/DockerClientConfigHandler.java:
##########
@@ -80,19 +83,44 @@ private DockerClientConfigHandler() { }
*/
public static Credentials readCredentialsFromConfigFile(Path configFile,
Configuration conf, String applicationId) throws IOException {
+
+ int configFileMaxSizeByte =
conf.getInt(YarnConfiguration.NM_DOCKER_CLIENT_CONFIG_MAX_SIZE_BYTE,
+ YarnConfiguration.DEFAULT_NM_DOCKER_CLIENT_CONFIG_MAX_SIZE_BYTE);
+ if (configFileMaxSizeByte <= 0){
+ configFileMaxSizeByte =
YarnConfiguration.DEFAULT_NM_DOCKER_CLIENT_CONFIG_MAX_SIZE_BYTE;
+ }
+
// Read the config file
String contents = null;
+ String exceptionMessage = null;
configFile = new Path(configFile.toUri());
FileSystem fs = configFile.getFileSystem(conf);
if (fs != null) {
- FSDataInputStream fileHandle = fs.open(configFile);
- if (fileHandle != null) {
- contents = IOUtils.toString(fileHandle, StandardCharsets.UTF_8);
+ FileStatus filestatus = fs.getFileStatus(configFile);
+ if(filestatus.getLen() > configFileMaxSizeByte){
+ exceptionMessage = "configFile (" + filestatus.getLen()
+ + " bytes) is larger than max limitation ("
+ + configFileMaxSizeByte + " bytes): ";
+ } else {
+ FSDataInputStream fileHandle = null;
+ try {
+ fileHandle = fs.open(configFile);
+ if (fileHandle != null) {
+ contents = IOUtils.toString(fileHandle, Charset.forName("UTF-8"));
+ }
+ } catch (IOException e) {
+ throw new IOException("Failed to read Docker client configuration: "
+ + configFile + ": " + e);
+ } finally {
+ if (fileHandle != null) {
+ fileHandle.close();
+ }
+ }
Review Comment:
The file read/exception handling here should preserve the original
IOException as the cause for debuggability (use the IOException constructor
that accepts a cause rather than concatenating `e` into the message). Also,
consider switching to try-with-resources and using `StandardCharsets.UTF_8`
instead of `Charset.forName("UTF-8")` to simplify and avoid unnecessary
lookups/imports.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/DockerClientConfigHandler.java:
##########
@@ -80,19 +83,44 @@ private DockerClientConfigHandler() { }
*/
public static Credentials readCredentialsFromConfigFile(Path configFile,
Configuration conf, String applicationId) throws IOException {
+
+ int configFileMaxSizeByte =
conf.getInt(YarnConfiguration.NM_DOCKER_CLIENT_CONFIG_MAX_SIZE_BYTE,
+ YarnConfiguration.DEFAULT_NM_DOCKER_CLIENT_CONFIG_MAX_SIZE_BYTE);
+ if (configFileMaxSizeByte <= 0){
Review Comment:
The new config max size variable uses inconsistent spacing (`if (...) {`),
which is likely to violate the project’s style/checkstyle rules. Please add a
space before the `{` in the `if (configFileMaxSizeByte <= 0)` block (and keep
formatting consistent with the rest of this file).
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/runtime/TestDockerContainerRuntime.java:
##########
@@ -2512,6 +2513,28 @@ public void
testLaunchContainerWithAdditionalDockerClientConfig(boolean pHttps)
testLaunchContainer(null, getDockerClientConfigFile());
}
+
+ @ParameterizedTest(name = "https={0}")
+ @MethodSource("data")
+ public void testLaunchContainerWithBigDockerClientConfig(boolean pHttps)
+ throws ContainerExecutionException, PrivilegedOperationException,
IOException {
+
+ File file = File.createTempFile("docker-client-config", "big-size");
+ file.deleteOnExit();
+ BufferedWriter bw = new BufferedWriter(new FileWriter(file));
+ bw.write("x".repeat(1025));
+ bw.close();
+
+ initHttps(pHttps);
+ try {
+ testLaunchContainer(null, file);
+ fail();
+ } catch (RuntimeException e) {
+ assertThat(e.getMessage())
+ .contains("configFile (1025 bytes) is larger than max limitation
(1024 bytes)");
+ }
Review Comment:
The failure assertion here is brittle: it hard-codes both the default limit
(1024) and the over-limit size (1025), and uses a manual try/catch + `fail()`.
Prefer `assertThrows` and derive sizes from
`YarnConfiguration.DEFAULT_NM_DOCKER_CLIENT_CONFIG_MAX_SIZE_BYTE` (or
explicitly set the conf property in the test) so the test doesn’t break if the
default changes.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java:
##########
@@ -2726,6 +2726,15 @@ public static boolean isAclEnabled(Configuration conf) {
public static final String NM_DOCKER_DEFAULT_TMPFS_MOUNTS =
DOCKER_CONTAINER_RUNTIME_PREFIX + "default-tmpfs-mounts";
+ /**
+ * Maximum allowed file size in bytes for the Docker client configuration
+ * file (config.json). If the file size exceeds this value, the container
+ * launch will be rejected. Default is 1024 bytes.
+ */
+ public static final String NM_DOCKER_CLIENT_CONFIG_MAX_SIZE_BYTE =
+ DOCKER_CONTAINER_RUNTIME_PREFIX + "client-config-file-max-size-bytes";
+ public static final int DEFAULT_NM_DOCKER_CLIENT_CONFIG_MAX_SIZE_BYTE = 1024;
+
Review Comment:
The new constant name uses singular `...MAX_SIZE_BYTE` while the config key
uses `...max-size-bytes` and other size-related constants typically use a
`..._BYTES` suffix. Consider renaming this to
`NM_DOCKER_CLIENT_CONFIG_MAX_SIZE_BYTES` (and matching default constant naming)
to reduce confusion and improve consistency.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/runtime/TestDockerContainerRuntime.java:
##########
@@ -2512,6 +2513,28 @@ public void
testLaunchContainerWithAdditionalDockerClientConfig(boolean pHttps)
testLaunchContainer(null, getDockerClientConfigFile());
}
+
+ @ParameterizedTest(name = "https={0}")
+ @MethodSource("data")
+ public void testLaunchContainerWithBigDockerClientConfig(boolean pHttps)
+ throws ContainerExecutionException, PrivilegedOperationException,
IOException {
+
+ File file = File.createTempFile("docker-client-config", "big-size");
+ file.deleteOnExit();
+ BufferedWriter bw = new BufferedWriter(new FileWriter(file));
+ bw.write("x".repeat(1025));
+ bw.close();
Review Comment:
This test writes the temp file using a manually managed BufferedWriter.
Please use try-with-resources (or another safe close pattern) so the writer is
always closed even if the write fails.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/DockerClientConfigHandler.java:
##########
@@ -80,19 +83,44 @@ private DockerClientConfigHandler() { }
*/
public static Credentials readCredentialsFromConfigFile(Path configFile,
Configuration conf, String applicationId) throws IOException {
+
+ int configFileMaxSizeByte =
conf.getInt(YarnConfiguration.NM_DOCKER_CLIENT_CONFIG_MAX_SIZE_BYTE,
+ YarnConfiguration.DEFAULT_NM_DOCKER_CLIENT_CONFIG_MAX_SIZE_BYTE);
+ if (configFileMaxSizeByte <= 0){
+ configFileMaxSizeByte =
YarnConfiguration.DEFAULT_NM_DOCKER_CLIENT_CONFIG_MAX_SIZE_BYTE;
+ }
+
// Read the config file
String contents = null;
+ String exceptionMessage = null;
configFile = new Path(configFile.toUri());
FileSystem fs = configFile.getFileSystem(conf);
if (fs != null) {
- FSDataInputStream fileHandle = fs.open(configFile);
- if (fileHandle != null) {
- contents = IOUtils.toString(fileHandle, StandardCharsets.UTF_8);
+ FileStatus filestatus = fs.getFileStatus(configFile);
+ if(filestatus.getLen() > configFileMaxSizeByte){
+ exceptionMessage = "configFile (" + filestatus.getLen()
+ + " bytes) is larger than max limitation ("
+ + configFileMaxSizeByte + " bytes): ";
+ } else {
Review Comment:
There are a few issues in this size-check block: (1) `filestatus` should be
camel-cased as `fileStatus`, (2) the `if` statement is missing spaces (`if (` /
`) {`), and (3) the error text says "max limitation" which is grammatically
awkward—consider changing it to something like "maximum allowed size" (and
update the test expectation accordingly).
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/runtime/DockerLinuxContainerRuntime.java:
##########
@@ -910,7 +910,7 @@ private Credentials
getAdditionalDockerClientCredentials(String clientConfig,
containerIdStr);
} catch (IOException e) {
throw new RuntimeException(
- "Fail to read additional docker client config file from " +
clientConfig);
+ "Fail to read additional docker client config file: " +
e.getMessage());
Review Comment:
This RuntimeException drops the original IOException cause, which makes
failures much harder to diagnose. Please include the caught exception as the
cause (and consider fixing the message grammar to "Failed to read...").
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
