YARN-8972. [Router] Add support to prevent DoS attack over ApplicationSubmissionContext size. #5382
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nSubmissionContext size.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Can you help review this pr? Thank you very much! |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
goiri
reviewed
Feb 17, 2023
| <name>yarn.router.asc-interceptor-max-size</name> | ||
| <value>1048576</value> | ||
| <description> | ||
|
|
There was a problem hiding this comment.
Can we add a description and fix the blank warning?
There was a problem hiding this comment.
Thank you very much for your help to review the code, I will modify the code.
| private static void logContainerLaunchContext(ApplicationSubmissionContextPBImpl appContext) | ||
| throws IOException { | ||
|
|
||
| if (appContext != null) { |
There was a problem hiding this comment.
Can we do early exits instead of these long nested ifs?
There was a problem hiding this comment.
Thanks for your suggestion, I will fix it.
|
💔 -1 overall
This message was automatically generated. |
goiri
reviewed
Feb 17, 2023
| @Unstable | ||
| private static void logContainerLaunchContext(ApplicationSubmissionContextPBImpl appContext) | ||
| throws IOException { | ||
| if (appContext != null && appContext.getAMContainerSpec() != null) { |
There was a problem hiding this comment.
if (appContext == null || appContext.getAMContainerSpec() == null) {
return;
}
There was a problem hiding this comment.
Thanks for your suggestion, I will modify the code.
| throws IOException { | ||
| if (appContext != null && appContext.getAMContainerSpec() != null) { | ||
| ContainerLaunchContext launchContext = appContext.getAMContainerSpec(); | ||
| ContainerLaunchContextPBImpl clc = (ContainerLaunchContextPBImpl) launchContext; |
There was a problem hiding this comment.
I will modify the code.
| We will throw an exception. the default value is 1MB, which is 1048576 Bytes. | ||
| </description> | ||
| </property> | ||
|
|
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Can you help to review this PR again? Thank you very much! |
goiri
approved these changes
Feb 24, 2023
...g/apache/hadoop/yarn/server/router/clientrm/TestApplicationSubmissionContextInterceptor.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
Feb 27, 2023
| **/ | ||
| public static final String ROUTER_ASC_INTERCEPTOR_MAX_SIZE = | ||
| ROUTER_PREFIX + "asc-interceptor-max-size"; | ||
| public static final long DEFAULT_ROUTER_ASC_INTERCEPTOR_MAX_SIZE = 1024 * 1024; |
There was a problem hiding this comment.
What if we use Configuration#getStorageSize()?
There was a problem hiding this comment.
Thank you very much for your suggestion, I will modify the code.
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
Feb 28, 2023
| double maxAscSize = conf.getStorageSize(YarnConfiguration.ROUTER_ASC_INTERCEPTOR_MAX_SIZE, | ||
| YarnConfiguration.DEFAULT_ROUTER_ASC_INTERCEPTOR_MAX_SIZE, StorageUnit.KB); | ||
| if (appContext != null) { | ||
| int size = appContext.getProto().getSerializedSize(); |
There was a problem hiding this comment.
Is this KB? can you add it to the variable?
int sizeKB
There was a problem hiding this comment.
From the previous stuff I was expecting bytes.
There was a problem hiding this comment.
Thank you for your suggestion, the unit is bytes not KB, I will modify the code.
There was a problem hiding this comment.
Can we add units to the variables?
size and maxAscSize
There was a problem hiding this comment.
Thanks for your suggestion, I will modify the code.
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Can you help review this PR again? Thank you very much! I will continue to follow up YARN-11376, YARN-11445. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
Mar 7, 2023
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Thank you very much for your help in reviewing the code! I will continue to follow up YARN-11376, YARN-11445. |
ferdelyi
pushed a commit
to ferdelyi/hadoop
that referenced
this pull request
May 26, 2023
…nSubmissionContext size. (apache#5382)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JIRA: YARN-8972. [Router] Add support to prevent DoS attack over ApplicationSubmissionContext size.