Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions docs/Test-config-behavior.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# Test Configuration Behavior

Test configuration provides the ability to configure guest/host before/after specific test runs.

## Configuration Options

Test configuration has the following options:

- **tests** - List of tests where config should be applied (string parsed as a regex)
- **secure** - Optional boolean to run guest in Secure Boot
- **parameters** - Array of HLK test parameter name/value pairs
- **pre_test_commands** - Array of commands to run before test execution
- **post_test_commands** - Array of commands to run after test execution

### Pre and Post Test Commands Options

- **desc** - String description of the command
- **host_run** - Optional string command to run on the host
- **guest_run** - Optional string command to run on each client VM
- **guest_reboot** - Ignored in this context
- **files_action** - Array of file operations to perform

### File Action Configuration Options

- **remote_path** - Path to file or directory on Client VM
- **local_path** - Path to file or directory on Client VM

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The description for local_path is incorrect. It states that it's a path on the Client VM, but the implementation shows it's a path on the host machine where AutoHCK is running. This could lead to confusion when configuring file actions. Please update the documentation for clarity.

Suggested change
- **local_path** - Path to file or directory on Client VM
- **local_path** - Path to file or directory on the host running AutoHCK

- **direction** - Direction of the file action (default: `remote-to-local`)
- **move** - Boolean flag to move file/directory (default: `false`, means copy)
- **allow_missing** - Boolean flag to allow missing file/directory (default: `false`)

## Test Configuration Execution Order

Test configuration is executed in the following order:

1. **Pre test commands**
1. Guest command is executed on each client VM
2. Host command is executed on the host
3. Files action is executed
2. **Test execution**
3. **Post test commands**
1. Guest command is executed on each client VM
2. Host command is executed on the host
3. Files action is executed

## File Action Behavior

- If `allow_missing` flag is set to `false` and source file/directory is missing, an error is raised
- If `move` flag is set to `true`, file/directory is moved according to the direction
- If `move` flag is set to `false`, file/directory is copied according to the direction

- If `remote_path` is empty, files will be copied to C:\ drive
- If `local_path` is empty, files will be copied to the current workspace

### Path Replacements

Local path can use the following replacements:

- `@workspace@` - Path to current workspace
- `@safe_test_name@` - Safe name of current test
- `@client_name@` - Client name
17 changes: 17 additions & 0 deletions lib/engines/hcktest/drivers/NetKVM.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,23 @@
{
"desc": "Rename NetKVM ethernet adapter to SupportDevice0",
"guest_run": "Rename-NetAdapter -Name (Get-NetAdapter -InterfaceDescription 'Red Hat VirtIO Ethernet Adapter').Name -NewName 'SupportDevice0'"
},
{
"desc": "Create directory c:\\SupportDevice2",
"guest_run": "New-Item -ItemType Directory -Path 'C:\\SupportDevice2'"
},
{
"desc": "Create File c:\\SupportDevice2\\test.txt with current date and time",
"guest_run": "New-Item -ItemType File -Path 'C:\\SupportDevice2\\test.txt' -Value (Get-Date -Format 'yyyy-MM-dd HH:mm:ss')",
"files_action": [
{
"remote_path": "C:/SupportDevice2/",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency with other paths in this file and the default remote path defined in the code (C:\\), it's better to use backslashes (\\) as path separators for Windows paths in this JSON configuration. While PowerShell often handles forward slashes, being consistent reduces potential ambiguity.

              "remote_path": "C:\\SupportDevice2\\",

"local_path": "@workspace@/@client_name@/@safe_test_name@",
"direction": "remote-to-local",
"move": true,
"allow_missing": false
}
]
}
]
}
Expand Down
101 changes: 92 additions & 9 deletions lib/engines/hcktest/tests.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
RESULTS_YAML = 'results.yaml'
RESULTS_REPORT_SECTIONS = %w[chart guest_info rejected_test url].freeze

DEFAULT_FILE_ACTION_REMOTE_PATH = 'C:\\'
DEFAULT_FILE_ACTION_LOCAL_PATH = '@workspace@'

def initialize(client, support, project, target, tools)
@client = client
@project = project
Expand Down Expand Up @@ -180,20 +183,100 @@
@tools.run_on_machine(client.name, desc, command)
end

def run_guest_test_command(command)
return unless command.guest_run

run_command_on_client(@client, command.guest_run, command.desc)
run_command_on_client(@support, command.guest_run, command.desc) unless @support.nil?
end

def run_host_test_command(command)
return unless command.host_run

@logger.info("Running command (#{command.desc}) on host")
run_cmd(command.host_run)
end

def run_remote_to_local_action_on_client(client_name, files_action)
remote_path = files_action.remote_path

unless @tools.exists_on_machine?(client_name, remote_path)
if files_action.allow_missing
@logger.warn("Remote file #{remote_path} not found on client #{client_name}: skipping download")
return
end

raise EngineError, "Remote file not found on client #{client_name}: #{remote_path}"
end

@tools.download_from_machine(client_name, remote_path, files_action.local_path)
@tools.delete_on_machine(client_name, remote_path) if files_action.move
end

def run_local_to_remote_action_on_client(client_name, files_action)
local_path = files_action.local_path
unless File.exist?(local_path)
if files_action.allow_missing
@logger.warn("Local file #{local_path} not found: skipping upload")
return
end

raise EngineError, "Local file not found: #{local_path}"
end

@tools.upload_to_machine(client_name, local_path, files_action.remote_path)
FileUtils.rm_rf(local_path) if files_action.move

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Using FileUtils.rm_rf with a user-configurable path (local_path) is a significant security risk. A misconfigured path could lead to unintentional deletion of important files or directories on the host system. To mitigate this, you should validate that the local_path is within the project's workspace before performing the delete operation when move is true.

Note: The suggestion uses Pathname, which may require require 'pathname' at the top of the file if it's not already available.

      if files_action.move
        workspace_realpath = Pathname.new(@project.workspace_path).realpath.to_s
        local_realpath = Pathname.new(local_path).realpath.to_s
        unless local_realpath.start_with?(workspace_realpath)
          raise EngineError, "Cannot move file/directory outside of workspace: #{local_path}"
        end
        FileUtils.rm_rf(local_path)
      end

end

sig { params(client_name: String, files_action: Models::FileActionConfig).void }
def run_file_action_on_client(client_name, files_action)
@logger.info("Running file action on #{client_name}" \
"#{files_action.direction} from #{files_action.remote_path} to #{files_action.local_path}")

case files_action.direction
when Models::FileActionDirection::RemoteToLocal
run_remote_to_local_action_on_client(client_name, files_action)
when Models::FileActionDirection::LocalToRemote
run_local_to_remote_action_on_client(client_name, files_action)
end
end

sig do
params(files_actions: T::Array[Models::FileActionConfig], replacement: ReplacementMap).void
end
def run_file_actions(files_actions, replacement)

Check notice

Code scanning / Rubocop

A calculated magnitude based on number of assignments, branches, and conditions. Note test

Metrics/AbcSize: Assignment Branch Condition size for run_file_actions is too high. [<3, 17, 4> 17.72/17]
files_actions.each do |files_action|
if files_action.remote_path.nil? && files_action.local_path.nil?
@logger.warn('Both remote and local paths are nil, skipping file action')
next
end

replacement.merge!({ '@client_name@' => @client.name })
updated_action = files_action.dup_and_replace_path(replacement, DEFAULT_FILE_ACTION_REMOTE_PATH,
DEFAULT_FILE_ACTION_LOCAL_PATH)
run_file_action_on_client(@client.name, updated_action)

next if @support.nil?

replacement.merge!({ '@client_name@' => @support.name })
updated_action = files_action.dup_and_replace_path(replacement, DEFAULT_FILE_ACTION_REMOTE_PATH,
DEFAULT_FILE_ACTION_LOCAL_PATH)
run_file_action_on_client(@support.name, updated_action)
end
end

sig { params(test: Models::HLK::Test, type: Symbol).void }
def run_test_commands(test, type)
select_test_config(test.name, type).each do |command|
guest_cmd = command.guest_run
desc = command.desc
if guest_cmd
run_command_on_client(@client, guest_cmd, desc)
run_command_on_client(@support, guest_cmd, desc) unless @support.nil?
end
run_guest_test_command(command)
run_host_test_command(command)

next unless command.host_run
replacement = ReplacementMap.new({
'@workspace@' => @project.workspace_path,
'@safe_test_name@' => test.safe_name
})

@logger.info("Running command (#{desc}) on host")
run_cmd(command.host_run)
run_file_actions(command.files_action, replacement)
end
end

Expand Down
38 changes: 38 additions & 0 deletions lib/models/command_info.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,43 @@
module AutoHCK
# Models module
module Models
class FileActionDirection < T::Enum
extend T::Sig

enums do
RemoteToLocal = new('remote-to-local')
LocalToRemote = new('local-to-remote')
end

sig { returns(String) }
def to_s
serialize
end
end

class FileActionConfig < T::Struct
extend T::Sig
const :remote_path, T.nilable(String)
const :local_path, T.nilable(String)
const :direction, FileActionDirection, default: FileActionDirection::RemoteToLocal
const :move, T::Boolean, default: false
const :allow_missing, T::Boolean, default: false

sig do
params(replacement: ReplacementMap, default_remote_path: String,
default_local_path: String).returns(FileActionConfig)
end
def dup_and_replace_path(replacement, default_remote_path, default_local_path)
self.class.new(
remote_path: replacement.replace(remote_path || default_remote_path),
local_path: replacement.replace(local_path || default_local_path),
direction: direction,
move: move,
allow_missing: allow_missing
)
end
end

# CommandInfo class
class CommandInfo < T::Struct
extend T::Sig
Expand All @@ -14,6 +51,7 @@ class CommandInfo < T::Struct
const :host_run, T.nilable(String)
const :guest_run, T.nilable(String)
const :guest_reboot, T::Boolean, default: false
const :files_action, T::Array[FileActionConfig], default: []
end
end
end
Loading