s3: Filter S3 profiles to only those used by the application - #503
Draft
parikshithb wants to merge 1 commit into
Draft
s3: Filter S3 profiles to only those used by the application#503parikshithb wants to merge 1 commit into
parikshithb wants to merge 1 commit into
Conversation
When a hub is shared by multiple managed clusters, the ramen configmap contains S3 profiles for all DR pairs. Previously, validate, gather, and test commands attempted to access all profiles, causing failures for unrelated or stale profiles. Read the primary VRG's spec.s3Profiles to determine which profiles belong to the application, and skip the rest. This avoids failures caused by inaccessible S3 profiles that are unrelated to the application being validated or gathered. Changes: - Add ApplicationS3Profiles() helper that reads a VRG and returns its S3 profile names. - Add ApplicationS3Prefix() helper that computes the S3 object prefix. - Filter store profiles using slices.Contains in validate, gather, and test commands. - Merge s3PrefixesToGather and profile filtering into a single s3InfoToGather method in the test command to avoid reading the DRPC twice per failed test. Assisted-by: Opus 4.6 Signed-off-by: Parikshith <parikshithb@gmail.com>
parikshithb
marked this pull request as draft
August 11, 2026 18:55
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughS3 discovery now reads application profile references from the primary VRG. It derives prefixes from the VRG namespace and DRPC name. Gathering, validation, and failed-test reporting filter S3 secrets to application-associated profiles. ChangesApplication-specific S3 discovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ApplicationCommand
participant DRPC
participant PrimaryVRG
participant S3Profiles
participant S3Secrets
ApplicationCommand->>DRPC: Read application DRPC
ApplicationCommand->>PrimaryVRG: Resolve VRG namespace and primary cluster
ApplicationCommand->>S3Profiles: Read referenced profile names
ApplicationCommand->>S3Secrets: Retrieve matching S3 secrets
ApplicationCommand->>ApplicationCommand: Derive prefix from VRG namespace and DRPC name
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/gather/command.go`:
- Around line 406-408: In pkg/gather/command.go lines 406-408, validate that
every appProfiles entry has a matching hub profile before filtering and fail the
inspection when any are missing. Apply the same pre-gather validation in
pkg/validate/application/command.go lines 312-314. In pkg/test/command.go lines
267-269, log the missing profile names and skip or mark the affected
application’s S3 gathering as incomplete so gathering cannot proceed with an
empty profile list.
In `@pkg/test/command.go`:
- Around line 490-491: Update the profile and prefix collection around
appProfiles and prefixes so each application’s profiles remain associated with
its own ramen.ApplicationS3Prefix value instead of building independent unions.
Adjust the data passed to s3.Gather to preserve these per-application
associations and prevent profiles from receiving unrelated prefixes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e658c3db-a917-4316-b891-2283456f9c32
📒 Files selected for processing (4)
pkg/gather/command.gopkg/ramen/ramen.gopkg/test/command.gopkg/validate/application/command.go
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: Build (linux, arm64)
- GitHub Check: Build (windows, amd64)
- GitHub Check: Test (ubuntu-24.04)
- GitHub Check: Build (darwin, amd64)
- GitHub Check: Build (linux, amd64)
- GitHub Check: Test (macos-15)
- GitHub Check: Go compatibility
- GitHub Check: Build (darwin, arm64)
- GitHub Check: Test (ubuntu-24.04-arm)
- GitHub Check: Test (macos-15-intel)
- GitHub Check: Lint
- GitHub Check: Test (windows-latest)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
**/*.go: Use proper punctuation in comments - end sentences with periods
All Go files need SPDX license headers - check existing files for the format
Check existing code for error formatting conventions in Go
Files:
pkg/test/command.gopkg/ramen/ramen.gopkg/validate/application/command.gopkg/gather/command.go
🔇 Additional comments (4)
pkg/ramen/ramen.go (1)
375-391: LGTM!pkg/gather/command.go (1)
364-396: LGTM!Also applies to: 420-420
pkg/validate/application/command.go (1)
270-302: LGTM!Also applies to: 326-326
pkg/test/command.go (1)
254-256: LGTM!Also applies to: 448-488, 492-494
Comment on lines
+406
to
+408
| if !slices.Contains(appProfiles, sp.S3ProfileName) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Detect VRG profiles that are missing from the hub profile store.
Each filter drops an S3 profile referenced by the primary VRG when no storeProfiles entry has the same name. The command then has no credential for that application profile. In pkg/test/command.go, a retained prefix can still cause S3 gathering with an empty profile list.
pkg/gather/command.go#L406-L408: compare allappProfilesagainst matched hub profiles and fail the inspection when a referenced profile is absent.pkg/validate/application/command.go#L312-L314: apply the same validation before S3 gathering.pkg/test/command.go#L267-L269: log the missing profile names and skip or mark the affected application S3 gathering as incomplete.
📍 Affects 3 files
pkg/gather/command.go#L406-L408(this comment)pkg/validate/application/command.go#L312-L314pkg/test/command.go#L267-L269
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/gather/command.go` around lines 406 - 408, In pkg/gather/command.go lines
406-408, validate that every appProfiles entry has a matching hub profile before
filtering and fail the inspection when any are missing. Apply the same
pre-gather validation in pkg/validate/application/command.go lines 312-314. In
pkg/test/command.go lines 267-269, log the missing profile names and skip or
mark the affected application’s S3 gathering as incomplete so gathering cannot
proceed with an empty profile list.
Comment on lines
+490
to
+491
| appProfiles = append(appProfiles, names...) | ||
| prefixes = append(prefixes, ramen.ApplicationS3Prefix(vrgNamespace, drpc.Name)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the GatherS3 declaration before inspecting its implementation.
fd -e go | while IFS= read -r file; do
if rg -q 'func .*GatherS3|GatherS3\s*=' "$file"; then
ast-grep outline "$file" --items all
rg -n -C 12 'func .*GatherS3|GatherS3\s*\(' "$file"
fi
doneRepository: RamenDR/ramenctl
Length of output: 12709
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the production S3 gather implementation and the call-site data flow.
fd -e go | while IFS= read -r file; do
if rg -q 'func Gather\s*\(|appProfiles|prefixes\s*=\s*append' "$file"; then
echo "=== $file ==="
rg -n -C 18 'func Gather\s*\(|appProfiles|prefixes\s*=\s*append' "$file"
fi
doneRepository: RamenDR/ramenctl
Length of output: 9988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the complete production gather path and its key construction.
sed -n '1,220p' pkg/s3/s3.go
printf '\n=== test call site ===\n'
sed -n '230,300p' pkg/test/command.go
printf '\n=== test data aggregation ===\n'
sed -n '448,498p' pkg/test/command.goRepository: RamenDR/ramenctl
Length of output: 9565
Preserve application-specific profile-to-prefix associations.
s3.Gather passes every prefix to every profile. Independent unions therefore create a profile-prefix cross-product and can download unrelated application data.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/test/command.go` around lines 490 - 491, Update the profile and prefix
collection around appProfiles and prefixes so each application’s profiles remain
associated with its own ramen.ApplicationS3Prefix value instead of building
independent unions. Adjust the data passed to s3.Gather to preserve these
per-application associations and prevent profiles from receiving unrelated
prefixes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a hub is shared by multiple managed clusters, the ramen configmap contains S3 profiles for all DR pairs. Previously, validate, gather, and test commands attempted to access all profiles, causing failures for unrelated or stale profiles.
Read the primary VRG's spec.s3Profiles to determine which profiles belong to the application, and skip the rest. This avoids failures caused by inaccessible S3 profiles that are unrelated to the application being validated or gathered.
Changes: