iscsi: add support for serving files via CLI#661
iscsi: add support for serving files via CLI#661mangelajo merged 4 commits intojumpstarter-dev:mainfrom
Conversation
WalkthroughAdds a Click-based CLI to ISCSIServerClient for serving/uploading images (supports HTTP(S) with headers, local files, optional compression, and cleanup). Introduces driver routines for clearing LUNs, cleaning orphan storage, safe path joining, and a public decompress method. Updates runtime dependencies and trims whitespace in an example YAML. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as ISCSIServerClient.cli(serve)
participant Src as Source Resolver
participant Upl as Uploader
participant Driver as ISCSI Driver
participant Target as iSCSI Target
User->>CLI: serve --file [path|http(s)] [--name] [--size] [--header]* [--force-upload] [--algo]
CLI->>Src: _get_src_and_operator(file, headers)
alt http(s) with headers
Src->>Src: download to temp file
Src-->>CLI: (temp_path, None, cleanup_path)
else http(s) without headers
Src-->>CLI: (url, operator, None)
else local path
Src-->>CLI: (abs_path, operator, None)
end
CLI->>CLI: derive name if none (_normalized_name_from_file)
CLI->>Driver: clear_all_luns()
CLI->>Upl: _should_skip_upload(...)
alt skip upload
Upl-->>CLI: true
else upload needed
alt with compression algo
Upl->>Upl: compress to temp
Upl->>Driver: upload compressed
Upl->>Driver: decompress(src, dst, algo)
Upl->>Driver: cleanup temp on exporter
else raw upload
Upl->>Driver: upload file
end
end
CLI->>Driver: add_lun(name, size, path)
Driver->>Target: configure target/TPG/LUN
CLI-->>User: print IQN and host:port
CLI->>CLI: cleanup temp if any
sequenceDiagram
autonumber
participant API as Driver.decompress()
participant FS as Storage Root
participant Algo as gzip/xz/bz2
API->>API: _safe_join_under_root(src_path/dst_path)
API->>FS: open src (read)
API->>FS: open dst (write)
API->>Algo: stream decompress src -> dst
API-->>API: close files, handle errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter-driver-iscsi/pyproject.toml (1)
8-15: Add the newrequestsruntime dependency
ISCSIServerClientnow importsrequests, but we don’t declare it in[project].dependencies. A fresh install of this package will therefore crash withModuleNotFoundError: No module named 'requests'when the CLI path executes. Please addrequestsas an explicit dependency so the runtime environment is complete.dependencies = [ "anyio>=4.10.0", "jumpstarter", "jumpstarter-driver-composite", "jumpstarter-driver-opendal", "click>=8.1.8", "rtslib-fb", + "requests>=2.32.3", ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-driver-iscsi/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py(6 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(5 hunks)packages/jumpstarter-driver-iscsi/pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (1)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (1)
remove_lun(139-149)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (2)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (6)
client(86-87)add_lun(372-413)get_target_iqn(254-260)start(201-213)get_host(236-242)get_port(245-251)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
operator_for_path(46-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: e2e
|
See the comment from the bot about now depending on requests: dependencies = [
We are probably depending on it indirectly, but I'd add it just in case |
yeah opendal uses it for the workaround, i'll add |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (1)
271-291: Block symlink escape in _get_full_path (use realpath and return the resolved path).abspath checks don’t catch symlink traversal. A file inside root that’s a symlink to /etc will pass your current check and be written to. Resolve symlinks and validate the resolved path stays under root; also create the parent of the resolved path.
def _get_full_path(self, file_path: str, is_block: bool) -> str: """Get the full path for the LUN file or block device""" if is_block: if not os.path.isabs(file_path): raise ISCSIError("For block devices, file_path must be an absolute path") return file_path else: - normalized_path = os.path.normpath(file_path) - - if normalized_path.startswith("..") or os.path.isabs(normalized_path): + normalized_path = os.path.normpath(file_path) + if normalized_path.startswith("..") or os.path.isabs(normalized_path): raise ISCSIError(f"Invalid file path: {file_path}") - - full_path = os.path.join(self.root_dir, normalized_path) - resolved_path = os.path.abspath(full_path) - root_path = os.path.abspath(self.root_dir) - - if not resolved_path.startswith(root_path + os.sep) and resolved_path != root_path: + full_path = os.path.join(self.root_dir, normalized_path) + # Resolve symlinks to prevent escape from root_dir + resolved_path = os.path.realpath(full_path) + root_path = os.path.realpath(self.root_dir) + if not (resolved_path == root_path or resolved_path.startswith(root_path + os.sep)): raise ISCSIError(f"Path traversal attempt detected: {file_path}") - os.makedirs(os.path.dirname(full_path), exist_ok=True) - return full_path + os.makedirs(os.path.dirname(resolved_path), exist_ok=True) + # Optional: forbid final component being a symlink if it already exists + if os.path.lexists(resolved_path) and os.path.islink(resolved_path): + raise ISCSIError(f"Refusing to use symlinked path: {file_path}") + return resolved_path
🧹 Nitpick comments (4)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (2)
173-188: Prefer realpath when matching orphan backstores under root.Using abspath may misclassify symlinked paths. realpath is safer.
- root_abs = os.path.abspath(self.root_dir) + root_abs = os.path.realpath(self.root_dir) @@ - udev_path = os.path.abspath(getattr(so, "udev_path", "")) + udev_path = os.path.realpath(getattr(so, "udev_path", ""))
371-397: Defensive guard: ensure target configured before add_lun.add_lun assumes _tpg is ready. Consider configuring if None to make the API harder to misuse outside the CLI path.
def add_lun(self, name: str, file_path: str, size_mb: int = 0, is_block: bool = False) -> str: @@ + if self._tpg is None: + self._configure_target()packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (2)
227-243: Minor: simplify algo→ext mapping and guard unexpected values.next(...) raises if algo is unexpected. Consider a dict reverse-lookup with a default and explicit error to make this robust.
- ext_to_algo = {".gz": "gz", ".xz": "xz", ".bz2": "bz2"} - ext = next(k for k, v in ext_to_algo.items() if v == algo) + algo_to_ext = {"gz": ".gz", "xz": ".xz", "bz2": ".bz2"} + try: + ext = algo_to_ext[algo] + except KeyError: + raise click.ClickException(f"Unsupported compression algo: {algo}")
291-337: CLI UX looks solid; consider optional safeguards.Nice flow: start → clear LUNs → derive name → upload → print target. Consider adding:
- A --max-size-mb guard for HTTP downloads to avoid filling disk.
- Downgrade verbose “checking hashes” logs to debug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-driver-iscsi/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py(6 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(5 hunks)packages/jumpstarter-driver-iscsi/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-iscsi/examples/exporter.yaml
- packages/jumpstarter-driver-iscsi/pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (1)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (1)
remove_lun(139-149)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (2)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (5)
client(86-87)add_lun(372-413)start(201-213)get_host(236-242)get_port(245-251)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
operator_for_path(46-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (4)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (2)
155-172: Confirm storage object deletion semantics when clearing LUNs.If multiple LUNs could reference the same backstore, deleting the storage object on the first LUN removal will break subsequent deletions. If that cannot happen, add a short comment stating the assumption.
189-199: Nice: explicit, idempotent clear_all_luns.Configures target if needed, clears TPG LUNs, internal maps, and orphans. Looks good.
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (2)
72-81: Good default LUN name derivation.Strips known compression suffixes and .img; concise and readable.
213-226: Skip logic reads clean and avoids unnecessary uploads.Short-circuiting on force/algo/existence is good; hash only when needed.
| from urllib.parse import urlparse | ||
|
|
||
| import click | ||
| import requests |
There was a problem hiding this comment.
🧩 Analysis chain
Bump Requests minimum to 2.32.5 for security fixes (CVE-2024-47081).
You import requests here; please ensure packaging pins to >=2.32.5, not 2.32.3, to pick up recent fixes.
Based on learnings
🏁 Script executed:
#!/bin/bash
# Find and show all declared requests dependencies
fd -a -t f '(^|/)requirements.*\.txt$|setup\.(cfg|py)$|pyproject\.toml|Pipfile|poetry\.lock' \
| xargs -I {} sh -c 'echo "== {} =="; rg -n "^requests" {} || rg -n "requests[<>=]" {} || true'Length of output: 3162
Bump Requests minimum to 2.32.5 for security fixes (CVE-2024-47081).
Update requests requirement in packages/jumpstarter-driver-iscsi/pyproject.toml from >=2.32.3 to >=2.32.5 in both tool.poetry.dependencies and tool.poetry.dev-dependencies.
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-iscsi/pyproject.toml (update the dependency
entries where requests is specified), bump the requests requirement from
">=2.32.3" to ">=2.32.5" in both tool.poetry.dependencies and
tool.poetry.dev-dependencies; then run your dependency tooling (poetry lock or
poetry update) to regenerate the lockfile/constraints so the new minimum is
applied across builds.
| if isinstance(file_path, str) and file_path.startswith(("http://", "https://")): | ||
| src_path = urlparse(file_path).path | ||
| else: | ||
| src_path = str(file_path) | ||
| with operator.open(str(src_path), "rb") as f: | ||
| while chunk := f.read(8192): | ||
| hash_obj.update(chunk) | ||
| return hash_obj.hexdigest() |
There was a problem hiding this comment.
HTTP equality check drops query string; may hash/size the wrong resource.
Both _calculate_file_hash and _files_are_identical ignore URL query components, leading to false matches or misses (e.g., signed URLs). Preserve the query when using Operator("http").
# in _calculate_file_hash
- if isinstance(file_path, str) and file_path.startswith(("http://", "https://")):
- src_path = urlparse(file_path).path
+ if isinstance(file_path, str) and file_path.startswith(("http://", "https://")):
+ parsed = urlparse(file_path)
+ src_path = parsed.path + (f"?{parsed.query}" if parsed.query else "")
@@
# in _files_are_identical
- if isinstance(src, str) and src.startswith(("http://", "https://")):
- src_path = urlparse(src).path
+ if isinstance(src, str) and src.startswith(("http://", "https://")):
+ parsed = urlparse(src)
+ src_path = parsed.path + (f"?{parsed.query}" if parsed.query else "")Also applies to: 192-197
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py around
lines 170-177 (and similarly 192-197), URL handling strips the query string
which can change the referenced resource (e.g., signed URLs); when the input is
an http/https URL keep the full URL (including the query and fragment as
appropriate) when opening with operator.open and when computing size/hash.
Replace urlparse(...).path usage with the full original URL (or reconstruct via
urlunparse) so operator.open is given the complete URL; ensure any comparisons
that determine identity also include the query component.
| def _safe_join_under_root(self, rel_path: str) -> str: | ||
| normalized_path = os.path.normpath(rel_path) | ||
| if normalized_path.startswith("..") or os.path.isabs(normalized_path): | ||
| raise ISCSIError(f"Invalid path: {rel_path}") | ||
| full_path = os.path.join(self.root_dir, normalized_path) | ||
| resolved_path = os.path.abspath(full_path) | ||
| root_path = os.path.abspath(self.root_dir) | ||
| if not resolved_path.startswith(root_path + os.sep) and resolved_path != root_path: | ||
| raise ISCSIError(f"Path traversal attempt detected: {rel_path}") | ||
| os.makedirs(os.path.dirname(full_path), exist_ok=True) | ||
| return full_path | ||
|
|
There was a problem hiding this comment.
Harden _safe_join_under_root against symlink traversal.
Same issue: abspath doesn’t resolve symlinks. Use realpath for both target and root, validate, and operate on the resolved path.
def _safe_join_under_root(self, rel_path: str) -> str:
- normalized_path = os.path.normpath(rel_path)
- if normalized_path.startswith("..") or os.path.isabs(normalized_path):
+ normalized_path = os.path.normpath(rel_path)
+ if normalized_path.startswith("..") or os.path.isabs(normalized_path):
raise ISCSIError(f"Invalid path: {rel_path}")
- full_path = os.path.join(self.root_dir, normalized_path)
- resolved_path = os.path.abspath(full_path)
- root_path = os.path.abspath(self.root_dir)
- if not resolved_path.startswith(root_path + os.sep) and resolved_path != root_path:
+ full_path = os.path.join(self.root_dir, normalized_path)
+ resolved_path = os.path.realpath(full_path)
+ root_path = os.path.realpath(self.root_dir)
+ if not (resolved_path == root_path or resolved_path.startswith(root_path + os.sep)):
raise ISCSIError(f"Path traversal attempt detected: {rel_path}")
- os.makedirs(os.path.dirname(full_path), exist_ok=True)
- return full_path
+ os.makedirs(os.path.dirname(resolved_path), exist_ok=True)
+ if os.path.lexists(resolved_path) and os.path.islink(resolved_path):
+ raise ISCSIError(f"Refusing to use symlinked path: {rel_path}")
+ return resolved_path📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _safe_join_under_root(self, rel_path: str) -> str: | |
| normalized_path = os.path.normpath(rel_path) | |
| if normalized_path.startswith("..") or os.path.isabs(normalized_path): | |
| raise ISCSIError(f"Invalid path: {rel_path}") | |
| full_path = os.path.join(self.root_dir, normalized_path) | |
| resolved_path = os.path.abspath(full_path) | |
| root_path = os.path.abspath(self.root_dir) | |
| if not resolved_path.startswith(root_path + os.sep) and resolved_path != root_path: | |
| raise ISCSIError(f"Path traversal attempt detected: {rel_path}") | |
| os.makedirs(os.path.dirname(full_path), exist_ok=True) | |
| return full_path | |
| def _safe_join_under_root(self, rel_path: str) -> str: | |
| normalized_path = os.path.normpath(rel_path) | |
| if normalized_path.startswith("..") or os.path.isabs(normalized_path): | |
| raise ISCSIError(f"Invalid path: {rel_path}") | |
| full_path = os.path.join(self.root_dir, normalized_path) | |
| resolved_path = os.path.realpath(full_path) | |
| root_path = os.path.realpath(self.root_dir) | |
| if not (resolved_path == root_path or resolved_path.startswith(root_path + os.sep)): | |
| raise ISCSIError(f"Path traversal attempt detected: {rel_path}") | |
| os.makedirs(os.path.dirname(resolved_path), exist_ok=True) | |
| if os.path.lexists(resolved_path) and os.path.islink(resolved_path): | |
| raise ISCSIError(f"Refusing to use symlinked path: {rel_path}") | |
| return resolved_path |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py around
lines 292-303, _safe_join_under_root currently uses abspath which does not
resolve symlinks and is vulnerable to symlink traversal; replace abspath with
os.path.realpath for both the computed target path and self.root_dir, validate
that the realpath target either equals or startswith the realpath root (with
os.sep guard), and perform os.makedirs and return the resolved path (or the
original joined path only after confirming the resolved target is inside the
resolved root) so operations occur on the real, validated filesystem location.
* Also compression support Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
* check symlinks * remove possible connection leaks Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (1)
296-306: Harden _safe_join_under_root against symlink traversal.The method uses
abspathwhich doesn't resolve symlinks, making it vulnerable to symlink traversal attacks. A symlink withinroot_dirpointing outside would bypass this check.Replace
abspathwithrealpathfor both the target path and root directory:def _safe_join_under_root(self, rel_path: str) -> str: normalized_path = os.path.normpath(rel_path) if normalized_path.startswith("..") or os.path.isabs(normalized_path): raise ISCSIError(f"Invalid path: {rel_path}") full_path = os.path.join(self.root_dir, normalized_path) - resolved_path = os.path.abspath(full_path) - root_path = os.path.abspath(self.root_dir) + resolved_path = os.path.realpath(full_path) + root_path = os.path.realpath(self.root_dir) - if not resolved_path.startswith(root_path + os.sep) and resolved_path != root_path: + if not (resolved_path == root_path or resolved_path.startswith(root_path + os.sep)): raise ISCSIError(f"Path traversal attempt detected: {rel_path}") os.makedirs(os.path.dirname(full_path), exist_ok=True) - return full_path + return resolved_pathpackages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (2)
177-184: HTTP URLs lose query strings in hash calculation.The code strips query parameters from HTTP URLs using
urlparse(file_path).path, which is problematic for signed URLs (e.g., S3 presigned URLs) or URLs where query parameters affect the resource identity. This can cause incorrect hash calculations for different resources.Pass the complete URL to
operator.open:else: hash_obj = hashlib.sha256() - if isinstance(file_path, str) and file_path.startswith(("http://", "https://")): - src_path = urlparse(file_path).path - else: - src_path = str(file_path) - with operator.open(str(src_path), "rb") as f: + with operator.open(str(file_path), "rb") as f: while chunk := f.read(8192): hash_obj.update(chunk) return hash_obj.hexdigest()
199-203: HTTP URLs lose query strings in size check.Same issue as in
_calculate_file_hash: stripping query parameters can cause incorrect size comparisons for URLs where the query string affects the resource.Pass the complete URL to
operator.stat:if operator is None: src_size = os.path.getsize(str(src)) else: - if isinstance(src, str) and src.startswith(("http://", "https://")): - src_path = urlparse(src).path - else: - src_path = str(src) - src_size = operator.stat(str(src_path)).content_length + src_size = operator.stat(str(src)).content_length
🧹 Nitpick comments (1)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (1)
298-343: LGTM: Well-structured CLI with proper cleanup.The Click-based CLI command provides a clean interface for serving images via iSCSI. The implementation correctly:
- Handles both local and HTTP(S) sources
- Supports custom headers for authenticated HTTP requests
- Manages temporary file cleanup via try/finally
- Provides meaningful output with IQN and connection details
One minor consideration: Line 327 uses
urlparse(file).pathwhich strips query strings from HTTP URLs for LUN naming. While this doesn't affect functionality (only the default name), it could be confusing if users have URLs with meaningful query parameters.Consider using the full URL basename or a hash suffix for HTTP URLs with query parameters:
if not name: - candidate = urlparse(file).path if file.startswith(("http://", "https://")) else file + if file.startswith(("http://", "https://")): + parsed = urlparse(file) + candidate = parsed.path + if parsed.query: + # Include a short hash of query params to distinguish resources + import hashlib + query_hash = hashlib.sha256(parsed.query.encode()).hexdigest()[:8] + candidate = f"{candidate}_{query_hash}" + else: + candidate = file name = self._normalized_name_from_file(candidate)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-driver-iscsi/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py(6 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(6 hunks)packages/jumpstarter-driver-iscsi/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-iscsi/examples/exporter.yaml
- packages/jumpstarter-driver-iscsi/pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T07:18:21.332Z
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#661
File: packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py:391-397
Timestamp: 2025-10-05T07:18:21.332Z
Learning: The iSCSI driver in `packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py` is designed to support only one LUN at a time, so hardcoding LUN ID 0 in the `add_lun` method is acceptable.
Applied to files:
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py
🧬 Code graph analysis (2)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (3)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (6)
client(90-91)add_lun(391-432)get_target_iqn(258-264)start(205-217)get_host(240-246)get_port(249-255)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
operator_for_path(46-58)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (1)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (1)
remove_lun(146-156)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: build
🔇 Additional comments (10)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (5)
1-8: LGTM: Imports support new decompression and cleanup features.The new imports (bz2, gzip, lzma, suppress, NamedTemporaryFile) are appropriate for the decompression API and safe temporary file handling introduced in this PR.
159-202: LGTM: LUN cleanup methods with appropriate error handling.The new methods
_clear_tpg_luns,_cleanup_orphan_storage_objects, andclear_all_lunsprovide robust cleanup with proper error suppression during teardown operations. The logic correctly handles both LUN and storage object deletion.
308-315: LGTM: Defense-in-depth against symlink traversal.This helper adds an extra layer of protection by verifying no path component is a symlink. While this is good defense-in-depth, the primary protection should come from fixing
_safe_join_under_rootto userealpath.
317-358: LGTM: Atomic decompression with proper error handling.The implementation correctly uses a temporary file with atomic
os.replace, includes symlink checking via_check_no_symlinks_in_path, and has proper cleanup on error. The chunk-based streaming (1MB buffer) is efficient for large files.However, the security of this method depends on fixing
_safe_join_under_root(lines 296-306) to userealpath.
410-412: LGTM: LUN replacement supports single-LUN design.Removing an existing LUN with the same name before adding a new one aligns with the single-LUN design pattern for this driver.
Based on learnings
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/client.py (5)
1-10: LGTM: Imports support new CLI and HTTP functionality.The new imports (contextlib, click, requests, NamedTemporaryFile, urlparse) are appropriate for the CLI command and HTTP-based source handling.
72-80: LGTM: Filename normalization for LUN naming.The method correctly strips compression extensions (
.gz,.xz,.bz2) and.imgsuffix to derive a clean LUN name, with a sensible fallback to"image"if the basename becomes empty.
82-126: LGTM: Robust source resolution with proper HTTP handling.The method correctly handles three scenarios:
- HTTP(S) with custom headers: downloads to a temporary file with proper cleanup
- HTTP(S) without headers: uses OpenDAL operator for streaming
- Local paths: resolves absolute path
The HTTP download implementation properly uses split timeouts
(10, 60)and ensures temp file cleanup on error.
220-249: LGTM: Clean upload workflow with compression support.The helper methods
_should_skip_uploadand_upload_fileprovide a clean separation of concerns:
- Skip logic correctly accounts for force upload, compression, and file identity
- Upload logic handles both direct and compressed paths
- Proper cleanup of compressed intermediates after decompression
278-296: LGTM: Streamlined upload logic using helpers.The refactored
upload_imagemethod now delegates to_should_skip_uploadand_upload_filehelpers, making the logic cleaner and more maintainable. The compression algorithm detection by extension is straightforward.
2 participants
Summary by CodeRabbit
New Features
Improvements
Chores