-
Notifications
You must be signed in to change notification settings - Fork 0
RDKEMW-11794: Inspect multiple web applications #28
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
base: develop
Are you sure you want to change the base?
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
b'## Copyright scan failure |
|
madanagopalt
left a comment
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.
kindly look at comments
18d44de to
a475ee6
Compare
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.
Pull request overview
This pull request adds support for inspecting multiple web applications running in containers simultaneously. The implementation introduces a WebInspector gateway that sets up port forwarding from host ports (2000-2100) to the WebInspector port (22222) inside containers, enabling external debugging tools to connect to containerized web applications.
Changes:
- Added WebInspector attachment/detachment logic in container lifecycle event handlers
- Introduced new Gateway module with networking and iptables utilities for port forwarding
- Enhanced RuntimeManager with debug port management and container IP address resolution
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| RuntimeManager/RuntimeManagerImplementation.h | Added forward declaration for WebInspector and member variables for tracking WebInspector instances and port availability |
| RuntimeManager/RuntimeManagerImplementation.cpp | Implemented WebInspector attachment on container start and cleanup on container stop |
| RuntimeManager/Gateway/WebInspector.h | Defined WebInspector class for managing debugging connections |
| RuntimeManager/Gateway/WebInspector.cpp | Implemented WebInspector with iptables port forwarding rules |
| RuntimeManager/Gateway/NetFilter.h | Defined NetFilter wrapper class for iptables operations |
| RuntimeManager/Gateway/NetFilter.cpp | Implemented NetFilter with locking and C++ wrapper functions |
| RuntimeManager/Gateway/NetFilterUtils.h | C API header for low-level iptables operations |
| RuntimeManager/Gateway/NetFilterUtils.c | Low-level C implementation of iptables rule management |
| RuntimeManager/Gateway/NetFilterLock.h | File-based locking mechanism for iptables operations |
| RuntimeManager/Gateway/NetFilterLock.cpp | Implementation of NetFilterLock using flock |
| RuntimeManager/Gateway/Debugger.h | Base class for debugger attachments |
| RuntimeManager/Gateway/ContainerUtils.h | Utility functions for container operations |
| RuntimeManager/Gateway/ContainerUtils.cpp | Implementation for entering container namespaces and retrieving IP addresses |
| RuntimeManager/Gateway/CMakeLists.txt | Build configuration for Gateway library |
| RuntimeManager/CMakeLists.txt | Updated to build Gateway module and link iptables libraries |
| .github/workflows/L1-tests.yml | Added required iptables development packages for CI |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
19d2bc6 to
b13636e
Compare
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.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@mounikapalem I've opened a new pull request, #43, to work on those changes. Once the pull request is ready, I'll request review from you. |
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.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
RuntimeManager/CMakeLists.txt
Outdated
| separate_arguments(RUNTIME_MANAGER_INCLUDES) | ||
| include_directories(BEFORE ${RUNTIME_MANAGER_INCLUDES}) | ||
|
|
||
| find_path(LIBIPTC_INCLUDE_DIR NAMES libiptc/libiptc.h) |
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.
Can everylines of this can go inside DEBUG check?
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.
Addressed
RuntimeManager/CMakeLists.txt
Outdated
| endif() | ||
|
|
||
| # IPv4 tables | ||
| find_package_handle_standard_args(LIBIP4TC |
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.
Can everylines of this can go inside DEBUG check?
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.
Addressed
RuntimeManager/CMakeLists.txt
Outdated
| set(LIBIPTC_LIBRARIES ${LIBIPTC_LIBRARY}) | ||
| endif() | ||
|
|
||
| if(LIBIPTC_FOUND AND NOT TARGET LibIptc::LibIptc) |
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.
Can everylines of this can go inside DEBUG check?
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.
Addressed
RuntimeManager/CMakeLists.txt
Outdated
|
|
||
| mark_as_advanced(LIBIPTC_LIBRARY LIBIPTC_INCLUDE_DIR) | ||
|
|
||
| if(LIBIPTC_FOUND) |
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.
Can everylines of this can go inside DEBUG check?
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.
Addressed
madanagopalt
left a comment
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.
kindly update cmake file
Reason for change: Updated WebInspector to support inspection across multiple web applications.
Risks: low
Priority: P2
Refactor NetFilter.cpp to enhance logging and add regex matching for comment removal.
Reason for change: Addressed the review comments. Risks: low Priority: P2
Reason for change: Addressed the review comments.
Risks: low
Priority: P2
Co-authored-by: Copilot <[email protected]>
Reason for change: Addressed the review comments.
Risks: low
Priority: P2
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Reason for change: Addressed the review comments.
Risks: low
Priority: P2
Reason for change: Addressed the review comments.
Risks: low
Priority: P2
Reason for change: Addressed the review comments.
Risks: low
Priority: P2
Reason for change: Addressed the review comments.
Risks: low
Priority: P2
Co-authored-by: Copilot <[email protected]>
120b7d0 to
59c3e32
Compare
Reason for change: Updated the cmake file.
Risks: low
Priority: P2
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.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.