Add external control bridge related infra#851
Add external control bridge related infra#851kostyanf14 wants to merge 4 commits intoHCK-CI:masterfrom
Conversation
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
These logs make NS creation process more visible and allow to easyly debug problems in it. Log to STDERR only for now to simplify code. Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Summary of ChangesHello @kostyanf14, 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 lays the groundwork for a new feature that facilitates external control of virtual machines, likely for specialized testing environments. It involves significant changes to how network resources are provisioned and managed within isolated namespaces, moving towards a more flexible and configurable network setup for client VMs. The updates also include necessary dependency bumps and improved debugging capabilities for network-related operations. 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. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces infrastructure for an external control bridge, which is a significant networking enhancement. The changes are mostly well-implemented, including a new command-line option, a script for network setup, and updates to platform configurations. However, I've identified a few issues that need attention. There's a critical bug in the error handling of the new ns_veth script that could lead to a NameError. Additionally, there's an unsafe command execution pattern in ns.rb that could pose a security risk. I've also made a suggestion to improve dependency management for better build reproducibility. Please review the detailed comments for suggestions on how to address these points.
| command.each do |cmd| | ||
| $stderr.write "[ns_veth #{side}] Running (wait spawn): #{cmd.join(' ')}\n" | ||
| Process.wait spawn(*cmd, out: :err) | ||
| raise $CHILD_STATUS.to_s unless $CHILD_STATUS.success? | ||
| rescue StandardError | ||
| raise "#{cmd}: command failed" | ||
| end |
There was a problem hiding this comment.
There are a couple of issues with this error handling block:
- The
rescueis placed after thecommand.eachloop. In this position, thecmdvariable from the loop is not in scope, which will lead to aNameErrorif an exception occurs within the loop. - The error message
raise "#{cmd}: command failed"is not very informative. It converts an array to a string (e.g.,["ip", "link", ...]), which is not readable, and it swallows the original exception details.
To fix this, the rescue should be inside the loop, and the error message should be improved to be more descriptive and include the original error.
command.each do |cmd|
begin
$stderr.write "[ns_veth #{side}] Running (wait spawn): #{cmd.join(' ')}\n"
Process.wait spawn(*cmd, out: :err)
raise "command failed with status #{$CHILD_STATUS.exitstatus}" unless $CHILD_STATUS.success?
rescue StandardError => e
raise "Command '#{cmd.join(' ')}' failed: #{e.message}"
end
end| sudo = Process.euid.zero? ? [] : ['sudo'] | ||
| cmd = %W[#{sudo} #{chdir}/bin/ns_veth host #{pid} #{run_id} #{ext_bridge}] | ||
| system(cmd.join(' ')) |
There was a problem hiding this comment.
The command is constructed using cmd.join(' ') and executed with system(). This is unsafe and can lead to shell injection vulnerabilities if any of the command arguments (like chdir) contain spaces or other shell metacharacters. It's also inconsistent with other parts of the code (lines 64, 71) that use the safer system(*command) form.
Please construct the command as an array of arguments and use system(*cmd) to execute it safely.
cmd = Process.euid.zero? ? [] : ['sudo']
cmd.concat(["#{chdir}/bin/ns_veth", 'host', pid.to_s, run_id.to_s, ext_bridge])
system(*cmd)| gem 'octokit' | ||
| gem 'openssl', require: false | ||
| gem 'rtoolsHCK', git: 'https://github.com/HCK-CI/rtoolsHCK.git', ref: 'v0.6.1' | ||
| gem 'rtoolsHCK', git: 'https://github.com/HCK-CI/rtoolsHCK.git', ref: 'VIRTWINKVM-1865' |
There was a problem hiding this comment.
For reproducible builds, it's better to pin a dependency to a specific commit hash rather than a branch name. Branch refs can be updated, leading to different versions of the gem being installed over time. Please use the full commit hash from Gemfile.lock to ensure that builds are consistent.
gem 'rtoolsHCK', git: 'https://github.com/HCK-CI/rtoolsHCK.git', ref: 'f59ae987141d8d7192ede862802dbf3d9d0d9105'
| "cpus": 4, | ||
| "memory_gb": 4, | ||
| "winrm_port": 4002, | ||
| "winrm_addr": "192.168.100.2", |
There was a problem hiding this comment.
The json update doesn't match the commit message. Maybe should be separate commit
No description provided.