-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-28070 Replace javax.servlet.jsp dependency with tomcat-jasper #5607
Conversation
Built code locally, generated assembly, untarred assembly, started local hbase instance. Tested Master/RS Web UI, works fine. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -115,6 +115,13 @@ | |||
<exclude>junit:junit</exclude> | |||
<exclude>org.hamcrest:hamcrest-core</exclude> | |||
<exclude>org.mockito:mockito-core</exclude> | |||
<!-- Exclude transitive dependencies of tomcat-jasper, not needed at runtime --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need this for our release binaries? For client I think it is OK that we do not need to start a web server, but for hbase we do need to start master/rs webs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested out master startup in local mode. Was able to start with just tomcat-jasper, tomcat-jasper-el and tomcat-el-api and hence removed others. Also these 3 jars bring in all those classes which are getting removed from classpath due to removal of javax.el and javax.servlet.jsp.
Can keep these for safer side. Please let me know WDYT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we could just exclude them in the dependencyManagement or dependencies section in our pom file? We need to them in tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @Apache9, these are required at compile time to generate the jsp pages but i was able to load web pages w/o them during runtime. Hence excluding from assembly.
Few others which were not needed at all, I have already excluded in dependencies section for the dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gentle ping @Apache9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Apache9 seems someone is interested in this patch. Could you please have another look at the changes and the above discussion and see if we are good here. Please let me know if otherwise.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Update:
Please let me know if anything else needs to be checked. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Ping @Apache9, any thoughts on this change/approach? Failures are not related. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
All works as expected with the latest tomcat-tasper and post rebasing. Please review! |
This comment has been minimized.
This comment has been minimized.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me, but I'm not an hbase committer, I'd be happy if someone else would take a look.
Gentle ping @Apache9, does this look fine to you? |
Hi @ndimiduk would you have bandwidth to review this one? |
Updated PR description for reviewers to have more context on this PR, for ease of review. |
@NihalJain not at the moment -- maybe someone else will be faster to respond. |
Thank you @ndimiduk for your response. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya @NihalJain thank you for the effort here. I appreciate you reporting your manual testing results here. Can you provide one more bit of information for reviewers -- I'd like to see a diff of the file listing of the hbase assembly with and without this patch (vs. both hadoop versions if you plan to backport to branch-2). That will help us to spot-check for other side-effects of this change. Thank you!
Hey Nick thanks for taking your time to look onto this. Please find diff for master on b161ad5 with hadoop-3 built on JDK17 vs patch rebased to b161ad5: Diff:
Detailed Diff:
Let me create a PR for branch-2 and repeat the manual tests along with diff on both versions of hadoop. |
This comment has been minimized.
This comment has been minimized.
Updated details and PR at #6218 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @NihalJain !!
…pache#5607) Signed-off-by: Istvan Toth <stoty@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> (cherry picked from commit b6394b1)
…pache#6218) (apache#5607) - Also handle changes required for hadoop-2 assembly Signed-off-by: Istvan Toth <stoty@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> (cherry picked from commit 2537690)
…pache#6218) (apache#5607) - Also handle changes required for hadoop-2 assembly Signed-off-by: Istvan Toth <stoty@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> (cherry picked from commit b6394b1) (cherry picked from commit 2537690)
…pache#6218) (apache#5607) - Also handle changes required for hadoop-2 assembly Signed-off-by: Istvan Toth <stoty@apache.org> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> (cherry picked from commit b6394b1) (cherry picked from commit 2537690)
The main driving force behind this PR is the need to remove the org.glassfish:javax.el:jar:3.0.1-b08 dependency from our project, which was added as a direct dependency due to HBASE-18831. Not only has org.glassfish:javax.el reached EOL, but it also has a known vulnerability (CVE-2020-15250). Additionally, it is required by the javax.servlet.jsp dependency, which we are using currently.
To eliminate the org.glassfish:javax.el dependency, this change proposes to replace the javax.servlet.jsp dependency with tomcat-jasper. This change also allows us to use JspC in an Ant task.
tomcat-jasper includes several transitive dependencies. However, we discovered that not all of these are necessary for our specific use case. The dependencies ecj (Eclipse Compiler for Java), tomcat-servlet-api, and tomcat-jsp-api have been excluded as they were not needed at all to build and run code.
The dependencies tomcat-juli, tomcat-api, tomcat-util-scan, and tomcat-util are retained as they are needed during the compilation phase but excluded during assembly to keep our deployment artifacts lean.
These changes have been thoroughly tested to ensure the application behaves as expected. However, additional testing is encouraged to validate these changes in different environments. With this PR, we are able to completely remove org.glassfish:javax.el:jar:3.0.1-b08 from our dependency tree.