WRKLDS-1191: oc rsync: Add --last flag #2078
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tchap: This pull request references WRKLDS-1191 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@tchap: This pull request references WRKLDS-1191 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
I guess this can also wait for 4.21 as nobody is going to use it in 4.20 anyway... |
tchap
force-pushed
the
rsync-last-n-include
branch
2 times, most recently
from
9f81cd5 to
b3c84cc
Compare
|
/test okd-scos-e2e-aws-ovn |
|
/retest |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
WalkthroughIntroduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
Comment |
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
The flag can be used to only sync N most recently modified files. The flag is mutually exclusive to: * --include * --exclude * --delete * --watch It is also ignored when the source is a local directory when using tar, because the implementation doesn't allow to select particular files. Generally when there is any problem when using --last, the error is ignored and sync happens as if the flag was not specified. Regarding implementation details, oc rsync performs an extras step when --last is specified, and that is discovering relevant files to select. This is done using manual directory walking when local, for remote the remote executor is used to invoke a shell using find+sort+head. The resulting filenames are then passed to --files-from for rsync, for tar they are simply passed to the command as arguments. Tests were added for testing the discovery mechanism, the rest has been tested manually. oc rsync is poorly unit-tested in general. Assisted-by: Claude Code
tchap
force-pushed
the
rsync-last-n-include
branch
from
b3c84cc to
a587e18
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @pkg/cli/rsync/copy_rsync.go:
- Around line 95-112: When r.Last > 0 make a nil-check for r.fileDiscovery
before calling r.fileDiscovery.DiscoverFiles to avoid a panic in cases where
rsyncStrategy was instantiated directly (e.g., in tests); if r.fileDiscovery is
nil, either initialize a sensible default file discovery implementation or log a
warning and skip the --last branch (leaving in and dst unchanged), otherwise
call r.fileDiscovery.DiscoverFiles as before and proceed; reference r.Last,
r.fileDiscovery.DiscoverFiles, and rsyncStrategy (and Complete() which normally
sets fileDiscovery) to locate the code to change.
In @pkg/cli/rsync/copy_rsyncd.go:
- Around line 238-255: The block that applies --last in copy_rsyncd.go calls
s.fileDiscovery.DiscoverFiles when s.Last > 0 but lacks a nil check and can
panic if s.fileDiscovery is nil; update the conditional to first verify
s.fileDiscovery != nil before calling DiscoverFiles, e.g., if s.Last > 0 &&
s.fileDiscovery != nil { ... } and if fileDiscovery is nil log a warning (using
klog.Infof or klog.Warningf) that --last was ignored; keep the existing behavior
of populating in, adjusting dst, and logging when DiscoverFiles returns an error
or succeeds.
In @pkg/cli/rsync/discovery_remote.go:
- Around line 44-47: The code extracts filename using filepath.Base on a remote
container path (variables fullPath and filename in discovery_remote.go) which
breaks on Windows; replace filepath.Base with path.Base (from the "path"
package) to operate on POSIX-style forward-slash remote paths and update imports
accordingly so the file uses the "path" package instead of "path/filepath".
🧹 Nitpick comments (3)
pkg/cli/rsync/discovery_remote.go (1)
51-52: Checkscanner.Err()after the scan loop.The scanner may encounter errors during iteration. Add a check after the loop to ensure no errors occurred.
🔎 Proposed fix
} + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("error reading remote command output: %w", err) + } return filenames, nilpkg/cli/rsync/copy_tar_test.go (1)
66-74: Type assertion could panic; consider a safer approach.The type assertion
NewTarStrategy(options).(*tarStrategy)will panic ifNewTarStrategyreturns a different concrete type. While this is unlikely in a test, using the two-value form provides clearer test failure output.🔎 Suggested improvement
- strategy := NewTarStrategy(options).(*tarStrategy) + strategy, ok := NewTarStrategy(options).(*tarStrategy) + if !ok { + t.Fatal("NewTarStrategy did not return *tarStrategy") + }pkg/cli/rsync/copy_rsyncd.go (1)
227-274: Consider extracting duplicated file discovery logic.The file discovery block (building stdin buffer, adjusting destination path, logging) is nearly identical to
copy_rsync.go. Extracting this to a helper function would improve maintainability and ensure consistent behavior across strategies.🔎 Example helper signature
// discoverFilesForRsync returns the stdin reader and adjusted destination path // when --last filtering is enabled. Returns nil reader if discovery fails or is disabled. func discoverFilesForRsync(fd fileDiscoverer, last uint, sourcePath, destPath string) (io.Reader, string) { // ... shared logic ... }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (12)
pkg/cli/rsync/copy_rsync.gopkg/cli/rsync/copy_rsyncd.gopkg/cli/rsync/copy_tar.gopkg/cli/rsync/copy_tar_test.gopkg/cli/rsync/discovery.gopkg/cli/rsync/discovery_local.gopkg/cli/rsync/discovery_local_test.gopkg/cli/rsync/discovery_remote.gopkg/cli/rsync/discovery_remote_test.gopkg/cli/rsync/discovery_test.gopkg/cli/rsync/exec_test.gopkg/cli/rsync/rsync.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cli/rsync/discovery_remote_test.gopkg/cli/rsync/discovery_remote.gopkg/cli/rsync/rsync.gopkg/cli/rsync/discovery_test.gopkg/cli/rsync/copy_rsyncd.gopkg/cli/rsync/copy_tar.gopkg/cli/rsync/discovery_local_test.gopkg/cli/rsync/discovery.gopkg/cli/rsync/discovery_local.gopkg/cli/rsync/copy_tar_test.gopkg/cli/rsync/exec_test.gopkg/cli/rsync/copy_rsync.go
🧬 Code graph analysis (2)
pkg/cli/rsync/copy_tar.go (1)
pkg/helpers/source-to-image/tar/tar.go (2)
Tar(32-73)Writer(82-86)
pkg/cli/rsync/copy_rsync.go (1)
pkg/cli/rsync/pathspec.go (1)
PathSpec(17-20)
🔇 Additional comments (14)
pkg/cli/rsync/exec_test.go (1)
1-27: LGTM!Clean mock implementation for testing command execution. The structure properly validates expected commands and returns configurable output/errors.
pkg/cli/rsync/copy_tar.go (2)
48-65: LGTM on the discovery integration.The error handling correctly logs warnings and proceeds without filtering when discovery fails, which aligns with the graceful degradation behavior described in the PR.
222-253: Verify--no-recursionbehavior with trailing-slash sources.The
--no-recursionflag is only applied when the source path does not end with/. When the source ends with/, tar includes specific file patterns without--no-recursion. Please confirm this asymmetry is intentional and that file-only filtering works correctly for both path styles.pkg/cli/rsync/discovery.go (1)
1-7: LGTM!Clean interface definition with appropriate abstraction for file discovery.
pkg/cli/rsync/discovery_local_test.go (1)
12-97: LGTM!Good test coverage with clear setup and multiple scenarios. The test properly validates ordering by modification time and ensures subdirectories are ignored.
pkg/cli/rsync/discovery_local.go (1)
19-65: LGTM!Clean implementation with proper error handling and efficient pre-allocation. The sorting and limiting logic is correct.
pkg/cli/rsync/discovery_test.go (1)
1-14: LGTM!Simple and effective mock for testing discovery behavior.
pkg/cli/rsync/copy_tar_test.go (1)
12-77: Well-structured test with good coverage of edge cases.The test cases comprehensively cover the file discovery logic: successful discovery replacing originals, fallback when discovery returns empty or errors, and handling of nil/empty states. The use of table-driven tests and
cmp.Difffor clear failure output follows Go testing best practices.pkg/cli/rsync/rsync.go (2)
237-245: File discovery initialization is correctly ordered.The file discovery setup happens before strategy initialization, ensuring strategies can use the discovered files during construction. The conditional logic properly distinguishes between local and remote sources.
276-287: Validation for mutually exclusive flags is comprehensive.The switch-case structure cleanly validates that
--lastcannot be combined with--watch,--include,--exclude, or--delete. Error messages clearly identify which flags conflict.pkg/cli/rsync/discovery_remote_test.go (2)
12-87: Good test coverage for remote file discovery.The test suite covers the expected scenarios: successful discovery with exact and partial results, empty directories, and error propagation. The mock executor approach cleanly isolates the discovery logic from actual command execution.
64-72: Fix shell injection vulnerability in command construction.Line 28 directly embeds
basePathin single quotes without escaping:fmt.Sprintf("find '%s' ...", basePath). IfbasePathcontains a single quote, it breaks the shell command and enables injection. Use proper shell escaping or an escaping library before embedding user-controlled paths into shell strings.pkg/cli/rsync/copy_rsync.go (1)
87-133: Clean integration of--lastfiltering with rsync strategy.The implementation properly builds the file list for
--files-from, adjusts the destination path to maintain rsync semantics, and gracefully falls back on discovery failure. The warning log ensures users are informed when filtering cannot be applied.pkg/cli/rsync/copy_rsyncd.go (1)
324-341: Strategy initialization correctly propagates--lastoptions.The
NewRsyncDaemonStrategyfunction properly initializesLastandfileDiscoveryfrom the options, maintaining consistency with other strategy constructors.
| cmd := []string{"sh", "-c", fmt.Sprintf("find '%s' -maxdepth 1 -type f -printf '%%T@ %%p\\n' | sort -rn | head -n %d", basePath, lastN)} | ||
| if err := discoverer.exec.Execute(cmd, nil, &output, &errOutput); err != nil { | ||
| return nil, fmt.Errorf("failed to execute remote find+sort+head command: %w, stderr: %s", err, errOutput.String()) | ||
| } |
There was a problem hiding this comment.
Potential command injection if basePath contains shell metacharacters.
The basePath is embedded in a shell command with single quotes. If the path contains a single quote ('), it could break out of the quoting and allow command injection. Consider sanitizing or escaping the path.
🔎 Proposed fix
- cmd := []string{"sh", "-c", fmt.Sprintf("find '%s' -maxdepth 1 -type f -printf '%%T@ %%p\\n' | sort -rn | head -n %d", basePath, lastN)}
+ // Escape single quotes in basePath to prevent command injection.
+ escapedPath := strings.ReplaceAll(basePath, "'", "'\\''")
+ cmd := []string{"sh", "-c", fmt.Sprintf("find '%s' -maxdepth 1 -type f -printf '%%T@ %%p\\n' | sort -rn | head -n %d", escapedPath, lastN)}📝 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.
| cmd := []string{"sh", "-c", fmt.Sprintf("find '%s' -maxdepth 1 -type f -printf '%%T@ %%p\\n' | sort -rn | head -n %d", basePath, lastN)} | |
| if err := discoverer.exec.Execute(cmd, nil, &output, &errOutput); err != nil { | |
| return nil, fmt.Errorf("failed to execute remote find+sort+head command: %w, stderr: %s", err, errOutput.String()) | |
| } | |
| // Escape single quotes in basePath to prevent command injection. | |
| escapedPath := strings.ReplaceAll(basePath, "'", "'\\''") | |
| cmd := []string{"sh", "-c", fmt.Sprintf("find '%s' -maxdepth 1 -type f -printf '%%T@ %%p\\n' | sort -rn | head -n %d", escapedPath, lastN)} | |
| if err := discoverer.exec.Execute(cmd, nil, &output, &errOutput); err != nil { | |
| return nil, fmt.Errorf("failed to execute remote find+sort+head command: %w, stderr: %s", err, errOutput.String()) | |
| } |
|
@tchap: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
The flag can be used to only sync N most recently modified files.
The flag is mutually exclusive to:
--include--exclude--delete--watchIt is also ignored when the source is a local directory when using tar, because the implementation doesn't allow to select particular files. We would need to exclude all other files, which can explode easily. Simply use a different strategy in that case.
Generally when there is any problem when using
--last, the error is ignored and sync happens as if the flag was not specified.Regarding implementation details,
oc rsyncperforms an extras step when--lastis specified, and that is discovering relevant files to select. This is done using manual directory walking when local, for remote the remote executor is used to invoke a shell using find+sort+head.The resulting filenames are then passed to
--files-fromfor rsync, for tar they are simply passed to the command as arguments.Tests were added for testing the discovery mechanism, the rest has been tested manually.
oc rsyncis rather poorly unit-tested in general.