Configure number of thread used by Netty base RESTCONF#2576
Configure number of thread used by Netty base RESTCONF#2576Tobianas wants to merge 2 commits intoPANTHEONtech:mainfrom
Conversation
Summary of ChangesHello @Tobianas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Netty-based RESTCONF module by providing new configuration options for its underlying thread pool. These changes enable users to specify the number of worker threads and a group name, offering greater control over the server's performance and resource utilization. The implementation also involves a refactoring of the Netty endpoint to integrate with OSGi-managed components. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces configurable thread management for the Netty-based RESTCONF server, allowing groupName and workThreads to be specified for performance tuning and resource management. A medium-severity vulnerability was identified where the user-configurable groupName is passed to a BootstrapFactory without sanitization, which could lead to log injection. Furthermore, critical issues related to lifecycle management and configuration consistency need to be addressed, including the improper management of OSGiNettyEndpoint and inconsistencies in NettyRestConfConfiguration's copy constructor and equals/hashCode implementations.
I am having trouble creating individual review comments. Click here to see my feedback.
lighty-modules/lighty-restconf-netty-nb-community/src/main/java/io/lighty/modules/northbound/netty/restconf/community/impl/NettyRestConf.java (58)
The OSGiNettyEndpoint is instantiated in initProcedure() but is not assigned to any field. To ensure proper lifecycle management and resource cleanup, especially during stopProcedure(), the OSGiNettyEndpoint instance should be stored in a private field.
private OSGiNettyEndpoint nettyEndpoint;
lighty-modules/lighty-restconf-netty-nb-community/src/main/java/io/lighty/modules/northbound/netty/restconf/community/impl/NettyRestConf.java (103)
After creating the OSGiNettyEndpoint, it needs to be assigned to the newly introduced nettyEndpoint field to allow for its management and proper closing.
this.nettyEndpoint = new OSGiNettyEndpoint(server, service, mdsalRestconfStreamRegistry, pros);
lighty-modules/lighty-restconf-netty-nb-community/src/main/java/io/lighty/modules/northbound/netty/restconf/community/impl/NettyRestConf.java (107-114)
This block, which was responsible for closing the SimpleNettyEndpoint, has been removed. With the transition to OSGiNettyEndpoint, it is crucial to re-introduce similar logic to properly close the new OSGiNettyEndpoint instance. Failure to do so can lead to resource leaks and improper shutdown. The OSGiNettyEndpoint should be stored in a field and closed here, similar to the following:
if (nettyEndpoint != null) {
try {
nettyEndpoint.close();
} catch (Exception e) {
LOG.error("Failed to stop Netty endpoint!", e);
stopSuccessful = false;
}
}lighty-modules/lighty-restconf-netty-nb-community/src/main/java/io/lighty/modules/northbound/netty/restconf/community/impl/config/NettyRestConfConfiguration.java (54)
The copy constructor NettyRestConfConfiguration(final NettyRestConfConfiguration restConfConfiguration) copies workThreads but omits groupName. To ensure full configuration consistency, groupName should also be copied.
this.workThreads = restConfConfiguration.getWorkThreads();
this.groupName = restConfConfiguration.getGroupName();lighty-modules/lighty-restconf-netty-nb-community/src/main/java/io/lighty/modules/northbound/netty/restconf/community/impl/config/NettyRestConfConfiguration.java (171)
The equals() method should include the groupName field in its comparison to ensure that two NettyRestConfConfiguration objects are correctly evaluated for equality. Without it, objects with different groupName values could be considered equal.
&& Objects.equals(workThreads, that.workThreads)
&& Objects.equals(groupName, that.groupName);lighty-modules/lighty-restconf-netty-nb-community/src/main/java/io/lighty/modules/northbound/netty/restconf/community/impl/config/NettyRestConfConfiguration.java (177)
The hashCode() method should include the groupName field in its calculation. This is essential to maintain the contract between equals() and hashCode(), ensuring that equal objects produce the same hash code.
domMountPointService, inetAddress, httpPort, restconfServletContextPath, workThreads, groupName);lighty-modules/lighty-restconf-netty-nb-community/src/main/java/io/lighty/modules/northbound/netty/restconf/community/impl/NettyRestConf.java (101)
The groupName variable, which can be sourced from an external JSON configuration, is passed directly to the BootstrapFactory constructor. If the BootstrapFactory implementation logs this group name without proper sanitization, this could lead to log injection vulnerabilities. An attacker could provide a malicious group name containing newlines and other characters to forge log entries, potentially obscuring other malicious activities or misleading system administrators. It is recommended to sanitize the groupName to only allow a restricted set of characters (e.g., alphanumeric and dashes) before passing it to the factory.
Tobianas
force-pushed
the
osgi-netty
branch
2 times, most recently
from
066f3ba to
ebe0d2a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Netty-based RESTCONF implementation to allow configuration of the number of worker threads. However, it introduces a significant regression in resource management: the newly introduced OSGiNettyEndpoint is neither stored nor closed, leading to a resource leak and potential Denial of Service (DoS) upon module restarts. Additionally, the workThreads parameter lacks validation, which could be exploited for resource exhaustion attacks. Beyond this, the NettyRestConfConfiguration copy constructor is incomplete as it fails to copy the groupName property, which could lead to incorrect configurations.
| new BootstrapFactory("lighty-restconf-nb-worker", 0), configuration); | ||
| final var bootstrapFactory = new BootstrapFactory(groupName ,workThreads); | ||
| final var pros = OSGiNettyEndpoint.props(bootstrapFactory, configuration); | ||
| new OSGiNettyEndpoint(server, service, mdsalRestconfStreamRegistry, pros); |
There was a problem hiding this comment.
The OSGiNettyEndpoint instance created here is not stored, leading to a resource leak and potential Denial of Service (DoS) upon module restarts. As OSGiNettyEndpoint manages network resources and implements AutoCloseable, it must be explicitly stored and closed to release these resources. The current 'fire-and-forget' approach relies on implicit constructor side-effects for lifecycle management, which is less explicit and harder to maintain compared to the previous SimpleNettyEndpoint implementation. It is recommended to store the instance in a field and manage its lifecycle explicitly.
| new OSGiNettyEndpoint(server, service, mdsalRestconfStreamRegistry, pros); | |
| this.nettyEndpoint = new OSGiNettyEndpoint(server, service, mdsalRestconfStreamRegistry, pros); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Resolved by using SimpleNettyEndpoint in latest patch
| protected boolean stopProcedure() { | ||
| boolean stopSuccessful = true; | ||
| if (nettyEndpoint != null) { | ||
| try { | ||
| nettyEndpoint.close(); | ||
| } catch (Exception e) { | ||
| LOG.error("Failed to stop Netty endpoint!", e); | ||
| stopSuccessful = false; | ||
| } | ||
| } | ||
| if (mdsalRestconfStreamRegistry != null) { |
There was a problem hiding this comment.
The stopProcedure method no longer closes the Netty endpoint. This is a regression from the previous implementation and, combined with the missing reference in initProcedure, leads to a resource leak. The Netty endpoint should be stored in a field and closed here.
| protected boolean stopProcedure() { | |
| boolean stopSuccessful = true; | |
| if (nettyEndpoint != null) { | |
| try { | |
| nettyEndpoint.close(); | |
| } catch (Exception e) { | |
| LOG.error("Failed to stop Netty endpoint!", e); | |
| stopSuccessful = false; | |
| } | |
| } | |
| if (mdsalRestconfStreamRegistry != null) { | |
| protected boolean stopProcedure() { | |
| boolean stopSuccessful = true; | |
| if (nettyEndpoint != null) { | |
| try { | |
| nettyEndpoint.close(); | |
| } catch (Exception e) { | |
| LOG.error("Failed to stop Netty endpoint!", e); | |
| stopSuccessful = false; | |
| } | |
| } | |
| if (mdsalRestconfStreamRegistry != null) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Resolved by using SimpleNettyEndpoint in latest patch
...ghty/modules/northbound/netty/restconf/community/impl/config/NettyRestConfConfiguration.java
Show resolved
Hide resolved
Add the ability to configure groupName and workThreads using json config. JIRA: LIGHTY-398 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
Add tests to verify functionality of reading BootstrapFactory from json file and its default values if no config is provided. JIRA: LIGHTY-398 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
| this.domMountPointService = domMountPointService; | ||
| } | ||
|
|
||
|
|
| private InetAddress inetAddress = InetAddress.getLoopbackAddress(); | ||
| private int httpPort = 8888; | ||
| private String restconfServletContextPath = "restconf"; | ||
| private String groupName = "lighty-netty-worker"; //use as default |
There was a problem hiding this comment.
this is not Java style comment, and comments have no meaning IMO
| return workThreads; | ||
| } | ||
|
|
||
| public void setWorkThreads(int workThreads) { |
| return groupName; | ||
| } | ||
|
|
||
| public void setGroupName(String groupName) { |
2 participants
No description provided.