feat(image): implement one.image.resize API#7507
feat(image): implement one.image.resize API#7507lexfrei wants to merge 18 commits intoOpenNebula:masterfrom
Conversation
Add Resize/ResizeContext methods to ImageController, ImageResize to RPCCaller interface, and XML-RPC/gRPC client implementations. Test verifies that resize rejects downsizing (server must return "greater than current" error). Part of: OpenNebula#5251 Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Implement the ImageManager::resize_image() method and _resize callback for the new one.image.resize operation. The resize follows the standard async driver pattern: validate (type, state, size), reserve quota for the size delta, lock the image, send RESIZE to the datastore driver, and handle the callback to update size or rollback quota on error. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Register the resize operation in the ImageAPI layer, XML-RPC binding (ImageResizeXRPC), and gRPC binding (ImageResizeGRPC). The XML-RPC method signature is "A:sis" (session, image_id, size_string). The protobuf ResizeRequest message and Resize RPC are added to the image service definition. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Replace the ImageResize gRPC stub with a real implementation that calls the Resize RPC. Regenerate Go protobuf bindings from updated image.proto to include ResizeRequest message and Resize service method. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add the RESIZE action to the datastore driver framework and implement backend-specific resize scripts: - dummy: echo requested size (for testing) - fs: qemu-img resize (local or via SSH bridge) - ceph: rbd resize - dev: symlink to not_supported.sh (block devices) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add Image#resize(size) to the Ruby OCA library and the corresponding "oneimage resize <id> <size>" CLI command. Size accepts suffixes (T/G/M, default MiB) consistent with the existing disk-resize command. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Register resize scripts in install.sh for all 8 datastore backends - Add not_supported.sh symlinks for iscsi_libvirt, restic, rsync, virtiofs - Fix XML injection: use to_string(new_size) instead of raw user input - Add resize to bash completion for oneimage - Add iss.fail() validation for non-numeric size input - Fix doc comment: @param error_str → @param error - Consistent MiB units across docs and error messages Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
src/image/ImageManagerActions.cc
Outdated
|
|
||
| int ds_id; | ||
|
|
||
| if ( auto img = ipool->get_ro(iid) ) |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
We observed that you are using unintended operations or assignments that could lead to logical errors, vulnerabilities, or undefined behavior. To prevent these issues, ensure that you follow best practices, such as using explicit comparisons and clear operator usage, and using parentheses to clarify intent. Learn more - https://cvefeed.io/cwe/detail/cwe-480-use-of-incorrect-operator
There was a problem hiding this comment.
False positive. This is standard C++ code — ipool->get_ro(iid) returns a smart pointer checked via implicit bool conversion, which is idiomatic C++. This code has since been refactored to use a single ipool->get(iid) call to eliminate the TOCTOU race.
|
|
||
| string ds_data; | ||
|
|
||
| if (auto ds = dspool->get_ro(ds_id)) |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
We observed that you are using unintended operations or assignments that could lead to logical errors, vulnerabilities, or undefined behavior. To prevent these issues, ensure that you follow best practices, such as using explicit comparisons and clear operator usage, and using parentheses to clarify intent. Learn more - https://cvefeed.io/cwe/detail/cwe-480-use-of-incorrect-operator
There was a problem hiding this comment.
Same as above — this pattern has been refactored in a subsequent commit. The get_ro() + get() pair was replaced with a single get() write lock.
| if (new_size <= cur_size) | ||
| { | ||
| ostringstream oss; | ||
| oss << "New size (" << new_size |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
A cross-site scripting (XSS) vulnerability was detected. User-controlled data is being output without proper sanitization, allowing an attacker to inject malicious scripts. This could lead to session hijacking, malware installation, or phishing attacks. Always sanitize and encode user inputs before including them in the output, using context-appropriate encoding methods. Learn More - https://cwe.mitre.org/data/definitions/79.html.
There was a problem hiding this comment.
False positive. This is server-side C++ code building an error string returned via XML-RPC/gRPC — not rendered in a browser. new_size is a long long parsed from the input string via istringstream, so it is guaranteed to be a plain integer.
|
|
||
| image_msg_t msg(ImageManagerMessages::RESIZE, "", iid, drv_msg); | ||
|
|
||
| imd->write(msg); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
A cross-site scripting (XSS) vulnerability was detected. User-controlled data is being output without proper sanitization, allowing an attacker to inject malicious scripts. This could lead to session hijacking, malware installation, or phishing attacks. Always sanitize and encode user inputs before including them in the output, using context-appropriate encoding methods. Learn More - https://cwe.mitre.org/data/definitions/79.html.
There was a problem hiding this comment.
Valid concern — this was the XML injection vector where raw user input size was interpolated into <SIZE> XML. Already fixed in a subsequent commit: now uses to_string(new_size) (the parsed integer) instead of the raw string, and the value is also wrapped in <EXTRA_DATA>.
| if (new_size <= cur_size) | ||
| { | ||
| ostringstream oss; | ||
| oss << "New size (" << new_size |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Potential log injection detected. Ensure all untrusted input is properly sanitized before logging. Use parameterized logging or validate input against an allow list to prevent log injection vulnerabilities. Consider using a dedicated logging library's built-in sanitization features when available. Learn more - https://cwe.mitre.org/data/definitions/117.html
There was a problem hiding this comment.
False positive. The logged value new_size is a long long — a plain integer that cannot contain log control characters. The size string is validated via istringstream with iss.fail() || !iss.eof() checks before this point is reached.
|
|
||
| image_msg_t msg(ImageManagerMessages::RESIZE, "", iid, drv_msg); | ||
|
|
||
| imd->write(msg); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Potential log injection detected. Ensure all untrusted input is properly sanitized before logging. Use parameterized logging or validate input against an allow list to prevent log injection vulnerabilities. Consider using a dedicated logging library's built-in sanitization features when available. Learn more - https://cwe.mitre.org/data/definitions/117.html
There was a problem hiding this comment.
Same as comment 4 — the raw size string is no longer used in XML construction. to_string(new_size) is used instead, which produces a sanitized integer string.
|
|
||
| long long resize_delta = 0; | ||
| image->get_template_attribute("RESIZE_DELTA", resize_delta); | ||
| image->remove_template_attribute("RESIZE_DELTA"); |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
User input is being used directly in C++ file system operations without proper validation. This can allow attackers to access files outside the intended directory through path traversal attacks, potentially exposing sensitive information or enabling system compromise. Use fs::canonical() to resolve paths and validate they start with an allowed base directory, implement allowlist validation using std::find() to check against permitted directories, or sanitize input by removing dangerous sequences like ".." using string operations. Learn more
There was a problem hiding this comment.
False positive. get_template_attribute("RESIZE_DELTA") reads from an in-memory XML template object, not from the filesystem. There is no file path involved.
- Eliminate TOCTOU race by using single get() write lock instead of separate get_ro() then get() calls in resize_image() - Quote DISK_SRC in fs/resize to prevent shell injection with crafted paths - Add SPARSE/preallocation handling in fs/resize for non-sparse raw images - Remove unused FORMAT xpath extraction from fs/resize - Add explicit M (MiB) suffix to rbd resize --size in ceph/resize - Handle malformed driver payload in _resize callback: treat invalid size as error with quota rollback instead of silently succeeding - Expand Go test coverage: invalid input, empty string, and positive resize with size verification Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add !iss.eof() check to istringstream parsing so inputs like "100abc" or "100GB" are rejected instead of silently truncating to 100. The CLI path was already protected by regex validation in size_in_mb(), but direct XML-RPC/gRPC API callers were not. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
| { | ||
| monitor_datastore(ds_id); | ||
| } | ||
| else if (resize_delta > 0) |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
We observed that you are using unintended operations or assignments that could lead to logical errors, vulnerabilities, or undefined behavior. To prevent these issues, ensure that you follow best practices, such as using explicit comparisons and clear operator usage, and using parentheses to clarify intent. Learn more - https://cvefeed.io/cwe/detail/cwe-480-use-of-incorrect-operator
There was a problem hiding this comment.
False positive. else if (resize_delta > 0) is an intentional comparison — quota rollback should only happen when a positive delta was reserved. The else branch (when resize_delta == 0) logs a warning about missing RESIZE_DELTA attribute.
- Add iss.fail()/iss.eof() checks to _resize callback payload parsing, matching the strict validation already applied in resize_image() input - Quote $RBD_SRC in ceph/resize to handle source paths with spaces Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Check returned size >= current size in _resize callback to reject
drivers returning a smaller size than requested
- Quote CEPH_USER, CEPH_KEY, CEPH_CONF in ceph/resize for paths
with spaces or special characters
- Quote ${SIZE}M in fs/resize for defense-in-depth
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Use relative DRIVER_PATH in dummy/resize instead of hardcoded path
- Remove dead RBD reassignment in ceph/resize heredoc (variable
already expanded at heredoc creation time with unquoted EOF)
- Quote ${SIZE}M in ceph/resize heredoc for defense-in-depth
- Log warning when RESIZE_DELTA attribute is missing in error callback
to aid debugging quota leaks
- Fix grammar in CLI resize help text
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
|
||
| if (new_size > 0 && new_size < cur_size) | ||
| { | ||
| oss << "Error resizing image: driver returned size (" |
There was a problem hiding this comment.
Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.
Potential log injection detected. Ensure all untrusted input is properly sanitized before logging. Use parameterized logging or validate input against an allow list to prevent log injection vulnerabilities. Consider using a dedicated logging library's built-in sanitization features when available. Learn more - https://cwe.mitre.org/data/definitions/117.html
There was a problem hiding this comment.
False positive. The error message includes new_size and cur_size (both long long) — plain integers that cannot contain log control characters.
- Revert single quotes around CEPH_USER/KEY/CONF in ceph/resize to match the unquoted pattern used by all other ceph datastore scripts - Simplify dead branch 'else if (!success)' to plain 'else' since success is always false at that point - Make TestResize fixture-independent by reading initial image size and computing target size dynamically Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…fety - Add comment to proto ResizeRequest.size: integer string in MiB - Clarify XML-RPC method description with size format - Document write lock guarantees and quota safety in resize_image Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Change callback size validation from >= to > (strictly greater than current), preventing quota leak when driver returns unchanged size - Use case-insensitive SPARSE comparison in fs/resize, matching the pattern used by fs/cp (tr A-Z a-z) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Use std::ws to skip trailing whitespace in _resize callback payload parsing, preventing false errors from driver output formatting - Prepare driver message strings before quota reservation so that all allocations happen before quota is charged - Add SIZE emptiness check in ceph/resize and fs/resize scripts - Fix Ruby OCA doc: MB -> MiB for consistency with C++ and proto - Add test cases for zero and negative size values Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The driver scripts read SIZE via XPath /DS_DRIVER_ACTION_DATA/EXTRA_DATA/SIZE but the extra_data string was missing the <EXTRA_DATA> wrapper element. Without it, SIZE would be at /DS_DRIVER_ACTION_DATA/SIZE and the xpath lookup would return empty, causing all resize operations to fail at the driver level. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
2 participants
Description
Implement the
one.image.resizeXML-RPC/gRPC API method to resize persistent imagesin the datastore. This has been a long-requested feature (#5251).
The implementation follows the standard async datastore driver pattern used by
snap_flattenand other image operations:Core:
ImageManager::resize_image()validates the request (image type OS/DATABLOCK,state READY, new size > current size), reserves datastore quota for the size delta,
locks the image, and sends RESIZE to the datastore driver. The
_resizecallbackupdates the image size on success or rolls back quota on failure.
API:
ImageAPI::resize()withbasic_authorization, exposed as XML-RPC methodone.image.resize(signatureA:sis) and gRPCImageService.Resize.Drivers: Backend-specific resize scripts for fs (
qemu-img resize),ceph (
rbd resize), dummy (echo size), and dev (not supported).Clients: Go goca (
ImageController.Resize), Ruby OCA (Image#resize),and CLI (
oneimage resize <id> <size>).Only upsizing is supported. The image must be in READY state and of type OS or DATABLOCK.
Branches to which this PR applies