Skip to content

s3: Filter S3 profiles to only those used by the application #503

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
parikshithb wants to merge 1 commit into
base: main
Choose a base branch
from
+55 −2
Draft

s3: Filter S3 profiles to only those used by the application #503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion pkg/gather/command.go
View file
Open in desktop
Comment thread
parikshithb marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,9 @@ func (c *Command) applicationS3Info() ([]*s3.Profile, string, error) {
configMapName := ramen.HubOperatorConfigMapName
configMapNamespace := c.config.Namespaces.RamenHubNamespace

storeProfiles, err := ramen.ClusterProfiles(reader, configMapName, configMapNamespace)
storeProfiles, err := ramen.ApplicationProfiles(
c, reader, c.outputReader, configMapName, configMapNamespace,
c.opts.DRPCName, c.opts.DRPCNamespace)

@nirs nirs Aug 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a good start - change the function we call to get the application profiles instead of all profiles!

But we pass 2 many arguments like configMapName and configMapNamespace - this values comes from ramen package, so ramen already know them, so no need to pass them.

Need rethink passing these arguments:

  • c - the command - is it for the logger? we should pass the logger instead.
  • reader
  • c.outputReader - would be better to implement this in the ramen package and not pass functions like this
  • configMapName, configMapNamespace - ramen already know these.

@parikshithb parikshithb Aug 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@nirs

  1. ctx is used not for logging. It's needed currently for PrimaryCluster(ctx, drpc) which calls ctx.Env().GetCluster()
  2. c.outputReader: not sure how, add OutputReader(string) to the ramen.Context interface?
  3. configmap:
    • configmap Name: Yes, but existing configMapName := ramen.HubOperatorConfigMapName is passed from caller, refactor this code to directly use?
    • Configmap ns: this comes from user config config.Namespaces.RamenHubNamespace varies between k8s and ocp

@nirs nirs Aug 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ctx is good - matching how we pass commands everywhere - keep it.

The hub reader is already used in other functions, but current coded needed single cluster, and here we need to read form 2 clusters. Reading clusters requires the command dataDir(), so we added outputDir() since it was used only in the command command. Passing it to the ramen code works but not great.

If we add OutputReader(string) to the context interface, ramen code can create readers from the context so we don't need to pass the hub and the outputReader function. This seems useful since ramen package need to read stuff from the output directory.

The ramen package knows the ramen hub config map name so we don't need to pass it.

The context has the config, so the ramen package can find the config namespace.

With these changes the call will become:

storeProfiles, err := ramen.ApplicationProfiles(c, c.opts.DRPCName, c.opts.DRPCNamespace)

Lets try do this:

  1. commit 1: Add OuptutReader to ramen context
  2. commit 2: Add ApplicationS3Profiles (this commit)

If adding OutputReader() is too big we can pass the function and let ramen.ApplicationS3Profiles create the readers.

We can change ramen.ApplicationS3Prefix() to accept a context later for creating the reader instead of passing the reader. This can be a followup PR in main. release 0.23 will have less consistent code.

if err != nil {
return nil, "", err
}
Expand Down
49 changes: 49 additions & 0 deletions pkg/ramen/ramen.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,55 @@ func ClusterProfiles(
return profiles, nil
}

// ApplicationProfiles returns the S3 store profiles for an application,
// filtered to only those referenced by the primary VRG.
func ApplicationProfiles(
ctx Context,
hubReader gathering.OutputReader,
clusterReader func(string) gathering.OutputReader,
configMapName, configMapNamespace string,
drpcName, drpcNamespace string,
) ([]*ramenapi.S3StoreProfile, error) {
allProfiles, err := ClusterProfiles(hubReader, configMapName, configMapNamespace)
if err != nil {
return nil, err
}

drpc, err := ReadDRPC(hubReader, drpcName, drpcNamespace)
if err != nil {
return nil, fmt.Errorf("failed to read drpc \"%s/%s\": %w",
drpcNamespace, drpcName, err)
}
vrgNamespace := VRGNamespace(drpc)
if vrgNamespace == "" {
return nil, fmt.Errorf("drpc \"%s/%s\" annotation %q not found",
drpc.Namespace, drpc.Name, drpcAppNamespaceAnnotation)
}
primary, err := PrimaryCluster(ctx, drpc)
if err != nil {
return nil, fmt.Errorf("failed to find primary cluster: %w", err)
}

primaryReader := clusterReader(primary.Name)
vrg, err := ReadVRG(primaryReader, drpc.Name, vrgNamespace)
if err != nil {
return nil, fmt.Errorf("failed to read vrg \"%s/%s\": %w",
vrgNamespace, drpc.Name, err)
}
if len(vrg.Spec.S3Profiles) == 0 {
return nil, fmt.Errorf("vrg \"%s/%s\" has no s3Profiles",
vrgNamespace, drpc.Name)
}

var profiles []*ramenapi.S3StoreProfile
for _, p := range allProfiles {
if slices.Contains(vrg.Spec.S3Profiles, p.S3ProfileName) {
profiles = append(profiles, p)
}
}
return profiles, nil
}

// S3ProfileFromStore creates an s3.Profile from a ramen S3StoreProfile and secret.
// If secret is nil, the profile will have empty credentials.
func S3ProfileFromStore(storeProfile *ramenapi.S3StoreProfile, secret *corev1.Secret) *s3.Profile {
Expand Down
4 changes: 3 additions & 1 deletion pkg/validate/application/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,9 @@ func (c *Command) s3Info() ([]*s3.Profile, string, error) {
configMapName := ramen.HubOperatorConfigMapName
configMapNamespace := c.Config().Namespaces.RamenHubNamespace

storeProfiles, err := ramen.ClusterProfiles(reader, configMapName, configMapNamespace)
storeProfiles, err := ramen.ApplicationProfiles(
c, reader, c.OutputReader, configMapName, configMapNamespace,
c.opts.DRPCName, c.opts.DRPCNamespace)
if err != nil {
return nil, "", err
}
Expand Down