Check the NaBSL status and conditions before creating Velero Backup

[mpryc]

Fixes #246

Ensure the state of the NaBSL is satisfying the Backup operation, in a case the admin redefined enforcement the NaBSL state was correctly synced back from the enforcement, however we disallowed real updated of the Velero BSL object causing NaBSL being in not allowed state, however previous Velero BSL fully functional.

This logic fixes that issue.

Why the changes were made

To fix #246

How to test the changes made

1. As described in the Able to perform Backup successsfully using NABSL in phase: BackingOff #246 issue:
2. Create enforcements for the NaBSL in the DPA
3. Create NaBSL to satisfy enforcements
4. Change the NaBSL enforcements in the DPA
5. Try to perform Backup operation using the NaBSL

Expected result: Backup can not be created.

[mateusoliveira43]

I do not like this approach. If a new condition is added, this can break us

If there is a certain condition that is important to check, we can check it, but checking all seems the wrong approach

[shubham-pampattiwar]

@mpryc the bug should be fixed via only checking the phase, right ?

[kaovilai]

// Condition contains details for one aspect of the current state of this API Resource.
// ---
// This struct is intended for direct use as an array at the field path .status.conditions.  For example,
//
//	type FooStatus struct{
//	    // Represents the observations of a foo's current state.
//	    // Known .status.conditions.type are: "Available", "Progressing", and "Degraded"
//	    // +patchMergeKey=type
//	    // +patchStrategy=merge
//	    // +listType=map
//	    // +listMapKey=type
//	    Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
//
//	    // other fields
//	}

condition is not always something that needs to be true to be satisfied.

We are establishing that all conditions in this repo if true is considered an unsatisfied condition. Is that what we want?

[mpryc]

I understand your concern about checking all conditions, but in our specific case, I believe it's a safer approach. I can rework this logic to check for 4 conditions individually:

oadp-non-admin/api/v1alpha1/nonadminbackupstoragelocation_types.go

Lines 30 to 33 in af1bbcd

	NonAdminBSLConditionSecretSynced NonAdminBSLCondition = "SecretSynced"
	NonAdminBSLConditionBSLSynced NonAdminBSLCondition = "BackupStorageLocationSynced"
	NonAdminBSLConditionApproved NonAdminBSLCondition = "ClusterAdminApproved"
	NonAdminBSLConditionSpecUpdateApproved NonAdminBSLCondition = "SpecUpdateApproved"

Each of these conditions must be satisfied to ensure that a NaBSL is actually usable. Relying solely on the status field could lead to situations where an NaBSL is marked as "OK" but is still not in a functional state.

By explicitly checking these conditions, we reduce potential issues that could arise if a new phase or status logic is introduced in the future. I believe this approach is a safeguard rather than an unnecessary restriction, but as I wrote I can change this to explicitly check for the above 4 conditions instead of iterating through all of them.

[mpryc]

Added only 3 conditions which in my opinion are a must to be met here before continuing backup and none of those 3 conditions can be false.

[mateusoliveira43]

I think logic is not correct

current we iterate through NABSL status conditions, if that condition is one of the 3 and is false, we error, right?

but if for example NABSL status conditions has no conditions, no problem happens, but it should report a problem, no?

would change logic to iterate through required conditions types, check if that condition is present in NABSL status (error if not) and if it true (error if not). Example implementation

		requiredConditions := []string{
			string(nacv1alpha1.NonAdminBSLConditionSecretSynced),
			string(nacv1alpha1.NonAdminBSLConditionBSLSynced),
			string(nacv1alpha1.NonAdminBSLConditionApproved),
		}
		for _, condition := range requiredConditions {
			conditionInNABSL := meta.FindStatusCondition(nonAdminBsl.Status.Conditions, condition)
			if conditionInNABSL == nil {
				return fmt.Errorf("TODO")
			}
			if conditionInNABSL.Status != metav1.ConditionTrue {
				return fmt.Errorf("TODO")
			}
		}

[shubham-pampattiwar]

#249 (comment) Something wrong with only checking phase ?

[mpryc]

@shubham-pampattiwar updated the implementation to relay only on phase, which should be sufficient.

[shubham-pampattiwar]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mateusoliveira43, mpryc, shubham-pampattiwar

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [mateusoliveira43,mpryc,shubham-pampattiwar]

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

[mpryc]

/cherry-pick oadp-1.5

[openshift-cherrypick-robot]

@mpryc: new pull request created: #284

Details

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.