Skip to content

Conversation

shubham-pampattiwar
Copy link
Member

@shubham-pampattiwar shubham-pampattiwar commented Aug 4, 2025

Why the changes were made

The ValidateBackupStorageLocations() function was missing validation to detect duplicate backup location names before BSL object creation.

How to test the changes made

Added early validation in the BSL validation process to detect and reject duplicate backup location names, preventing the problematic state from occurring.

  • Create a DPA with duplicate backup location names to verify validation:
  apiVersion: oadp.openshift.io/v1alpha1
  kind: DataProtectionApplication
  metadata:
    name: test-dpa
    namespace: openshift-adp
  spec:
    configuration:
      velero:
        defaultPlugins:
          - aws
          - azure
    backupLocations:
      - name: "duplicate-name"  # First location
        velero:
          provider: aws
          default: true
          objectStorage:
            bucket: test-aws-bucket
            prefix: velero/backups
          config:
            region: us-east-1
          credential:
            name: cloud-credentials
            key: cloud
      - name: "duplicate-name"  # Duplicate name - should fail
        velero:
          provider: azure
          default: false
          objectStorage:
            bucket: test-azure-bucket
            prefix: velero/backups
          config:
            resourceGroup: test-rg
            storageAccount: test-sa
          credential:
            name: azure-credentials
            key: cloud

Expected behavior:

  • DPA creation/update should be rejected with validation error
  • Error message: backup location name 'duplicate-name' is duplicated. Backup location names must be unique
  • No BackupStorageLocation objects should be created

🤖 Generated with https://claude.ai/code

Co-Authored-By: Claude [email protected]

@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 4, 2025

@shubham-pampattiwar: This pull request references OADP-6459 which is a valid jira issue.

In response to this:

Why the changes were made

The ValidateBackupStorageLocations() function was missing validation to detect duplicate backup location names before BSL object creation.

How to test the changes made

Added early validation in the BSL validation process to detect and reject duplicate backup location names, preventing the problematic state from occurring.

  • Create a DPA with duplicate backup location names to verify validation:
 apiVersion: oadp.openshift.io/v1alpha1
 kind: DataProtectionApplication
 metadata:
   name: test-dpa
   namespace: openshift-adp
 spec:
   configuration:
     velero:
       defaultPlugins:
         - aws
         - azure
   backupLocations:
     - name: "duplicate-name"  # First location
       velero:
         provider: aws
         default: true
         objectStorage:
           bucket: test-aws-bucket
           prefix: velero/backups
         config:
           region: us-east-1
         credential:
           name: cloud-credentials
           key: cloud
     - name: "duplicate-name"  # Duplicate name - should fail
       velero:
         provider: azure
         default: false
         objectStorage:
           bucket: test-azure-bucket
           prefix: velero/backups
         config:
           resourceGroup: test-rg
           storageAccount: test-sa
         credential:
           name: azure-credentials
           key: cloud

Expected behavior:

  • DPA creation/update should be rejected with validation error
  • Error message: backup location name 'duplicate-name' is duplicated. Backup location names must be unique
  • No BackupStorageLocation objects should be created

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.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 4, 2025
@openshift-ci openshift-ci bot requested review from kaovilai and mpryc August 4, 2025 21:31
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2025
@shubham-pampattiwar shubham-pampattiwar self-assigned this Aug 4, 2025
Copy link
Member

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

  1. Consider extracting BSL name generation logic: The BSL naming logic is
    duplicated between ValidateBackupStorageLocations and
    ReconcileBackupStorageLocations. Consider extracting this into a helper
    function:
  func (r *DataProtectionApplicationReconciler) getBSLName(bslSpec 
  *BackupLocation, index int) string {
      if bslSpec.Name != "" {
          return bslSpec.Name
      }
      return fmt.Sprintf("%s-%d", r.NamespacedName.Name, index+1)
  }
  1. Add validation for empty names: While the current logic handles empty
    names by generating them, consider validating that user-provided names
    are not empty strings to avoid confusion.

@shubham-pampattiwar
Copy link
Member Author

/cherry-pick oadp-1.5

@openshift-cherrypick-robot
Copy link
Contributor

@shubham-pampattiwar: once the present PR merges, I will cherry-pick it on top of oadp-1.5 in a new PR and assign it to you.

In response to this:

/cherry-pick oadp-1.5

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 kubernetes-sigs/prow repository.

Copy link

openshift-ci bot commented Aug 4, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai, shubham-pampattiwar, weshayutin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [kaovilai,shubham-pampattiwar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@shubham-pampattiwar
Copy link
Member Author

/retest

@kaovilai
Copy link
Member

kaovilai commented Aug 5, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2025
@shubham-pampattiwar
Copy link
Member Author

/retest

@openshift openshift deleted a comment from openshift-ci bot Aug 5, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 5, 2025

@shubham-pampattiwar: This pull request references OADP-6459 which is a valid jira issue.

In response to this:

Why the changes were made

The ValidateBackupStorageLocations() function was missing validation to detect duplicate backup location names before BSL object creation.

How to test the changes made

Added early validation in the BSL validation process to detect and reject duplicate backup location names, preventing the problematic state from occurring.

  • Create a DPA with duplicate backup location names to verify validation:
 apiVersion: oadp.openshift.io/v1alpha1
 kind: DataProtectionApplication
 metadata:
   name: test-dpa
   namespace: openshift-adp
 spec:
   configuration:
     velero:
       defaultPlugins:
         - aws
         - azure
   backupLocations:
     - name: "duplicate-name"  # First location
       velero:
         provider: aws
         default: true
         objectStorage:
           bucket: test-aws-bucket
           prefix: velero/backups
         config:
           region: us-east-1
         credential:
           name: cloud-credentials
           key: cloud
     - name: "duplicate-name"  # Duplicate name - should fail
       velero:
         provider: azure
         default: false
         objectStorage:
           bucket: test-azure-bucket
           prefix: velero/backups
         config:
           resourceGroup: test-rg
           storageAccount: test-sa
         credential:
           name: azure-credentials
           key: cloud

Expected behavior:

  • DPA creation/update should be rejected with validation error
  • Error message: backup location name 'duplicate-name' is duplicated. Backup location names must be unique
  • No BackupStorageLocation objects should be created

🤖 Generated with https://claude.ai/code

Co-Authored-By: Claude [email protected]

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.

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 08b9b61 and 2 for PR HEAD a6e29c9 in total

Copy link

openshift-ci bot commented Aug 6, 2025

@shubham-pampattiwar: all tests passed!

Full PR test history. Your PR dashboard.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit 76cb430 into openshift:oadp-dev Aug 6, 2025
9 checks passed
@openshift-cherrypick-robot
Copy link
Contributor

@shubham-pampattiwar: new pull request created: #1887

In response to this:

/cherry-pick oadp-1.5

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 kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants