Add labels as a criteria for volume policy

[shubham-pampattiwar]

Thank you for contributing to Velero!

Please add a summary of your change

Implementation for design #8503

Does your change fix a particular issue?

Fixes #8256

Please indicate you've done the following:

• Accepted the DCO. Commits without the DCO will delay acceptance.
• Created a changelog file (make new-changelog) or comment /kind changelog-not-required on this PR.
• Updated the corresponding documentation in site/content/docs/main.

[codecov]

Codecov Report

Attention: Patch coverage is 73.77049% with 16 lines in your changes missing coverage. Please review.

Project coverage is 59.51%. Comparing base (9235fe1) to head (1801a42).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/backup/item_backupper.go	23.07%	7 Missing and 3 partials ⚠️
internal/resourcepolicies/resource_policies.go	78.94%	4 Missing ⚠️
pkg/podvolume/backupper.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8713   +/-   ##
=======================================
  Coverage   59.51%   59.51%           
=======================================
  Files         371      371           
  Lines       40179    40241   +62     
=======================================
+ Hits        23911    23949   +38     
- Misses      14775    14798   +23     
- Partials     1493     1494    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kaovilai]

lgtm. This matches the current design.

[Lyndon-Li]

We had an internal discussion. We feel it is not a good idea to include a dynamic retrieve into the resource policy filtering module, original it only does static match comparisons --- from a static object data against the static policy conditions. Reasons:

• The resource policy filtering is a very fundermental module, it may be used by any part, even 3rd plugins. And the API server connection may not be possible
• We cannot always assume that the object we are checking is from the current API server, e.g., if we use the filter in restore, the PVC should be from the backed up data, instead of the cluster

[Lyndon-Li]

Therefore, we suggest to check the PVC labels from the PVC object, instead of PV object. The existing checks on PV object stays as is.
Let us know if that works.

[sseago]

@Lyndon-Li Everything else is checked on PV (size, nfs, etc). This was added to look up the PVC for the PV. The problem is that we expect that users will generally have labels on PVCs but not on PVs since the PVCs are created explicitly, while the PVs are provisioned for them. Not only that, non-privileged users aren't even allowed to edit PVs -- so if the label selector is applied to the PVs instead of the PVCs, then the usefulness of this feature is significantly reduced.

Also, these policies don't really make sense on restore -- we're deciding how to back up the volumes (fs-backup vs snapshot vs. no data backup) -- at restore, we're stuck with the backup-time decisions and can only restore via the backup method.

[sseago]

@shubham-pampattiwar Unless there's a way to do this from the PVC object -- but that may not be possible, since we need to find the first matching policy, so we need to go through all the policies in order for each volume.

[sseago]

@Lyndon-Li Talking to @shubham-pampattiwar just now, I think we found another way to do this that doesn't require passing in a client. In fact, in most cases we already have a PVC obj in memory that we can use.

[shubham-pampattiwar]

@Lyndon-Li Discussed this with @sseago what if just introduce a new struct and pass that on the GetMatchAction function ?

type VolumeFilterData struct {
   pv PersistentVolume
   vol Volume
   pvc PersistentVolumeClaim
}

[Lyndon-Li]

First, we agree on introducing a new struct for GetMatchAction.
Secondly, in order to make it flexible, it's better to pick the required fields from Volume, PersistentVolume and PersistentVolumeClaim and add them to this new struct. It is not necessary to couple with the aforementioned k8s objects, we don't need all the fields from them.

Sorry, if we didn't make the suggestions clear.

[shubham-pampattiwar]

it's better to pick the required fields from Volume, PersistentVolume and PersistentVolumeClaim and add them to this new struct

@Lyndon-Li We already extract the necessary information for filtering and store it in the structuredVolume struct. Given that, I’m unsure about the need to extract the same information again only to reassign it to structuredVolume. Could you clarify the intent behind this change?

If the goal is to refactor how conditions are passed to the volume policy engine, I believe that would be outside the scope of this PR. Let’s align on whether this change is essential here or if it should be handled separately.

[Lyndon-Li]

I get point that the upper level of the engine is to filter k8s resources, so it is reasonable to pass k8s resources. I agree on that. I also agree that based on the current code, it will be a duplication of we add another struct very similar to structuredVolume.
But I also feel that we should not have expected that the engine needs to filter multiple objects and filter them separately may not be reasonable. So how to create a logical struct to connect/represent these objects (not only for volume, but for all objects) may be something we can do better.

Right now, let's go with your proposal #8713 (comment).

[sseago]

@Lyndon-Li The other advantage of passing in the whole k8s resource is that if we later decide to allow filtering on PVC name, for example, we won't need to change the struct we pass in -- only the internals of the filtering, so this will be a more stable API.

[sseago]

I think this is going in the right direction, but a couple of suggestions in my review comments.