use old namespace in resource modifier

[27149chen]

Please add a summary of your change

Use old(origin) namespace in resource modifier conditions in case namespace may change during restore

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

• Accepted the DCO. Commits without the DCO will delay acceptance.
• Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
• Updated the corresponding documentation in site/content/docs/main.

[27149chen]

@anshulahuja98 PTAL

[codecov]

Codecov Report

Merging #6724 (e2cd5f6) into main (2348099) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main    #6724   +/-   ##
=======================================
  Coverage   60.45%   60.45%           
=======================================
  Files         242      242           
  Lines       26029    26031    +2     
=======================================
+ Hits        15736    15738    +2     
  Misses       9187     9187           
  Partials     1106     1106           

Files Changed	Coverage Δ
pkg/restore/restore.go	64.39% <0.00%> (+0.04%)	⬆️

📢 Have feedback on the report? Share it here.

[anshulahuja98]

I personally feel it's better if we do changes with the new namespace, (resource modifier filter having new namespace). Since this will lead to less confusion IMO. And also easier for the end user to debug any issues in resourcemodifier setup.

Otherwise think of the scenario where the original namespace is already there in the cluster and wishes to restore to a diff namespace, IMO the filters should target the new namespace for clarity.

[anshulahuja98]

We can also document this behaviour in docs.

[27149chen]

But I think the goal of resource modifier is to modify an object from old to new, so there is a mapping in resource modifier config(an old part and a new part). To me, name, namespace, label and old value in given path belong to the old part, and new value is the new part. If I want to restore an object with resource modifier, I will first write rules according the origin object and pass it to velero to perform the restore. I might later change my mind and want to change the namespace or not, the rules should remain unchanged.

[anshulahuja98]

so namespace is just part of the change which happens if you see.

There are the other RestoreItemAction plugins which can change the YAMLs based on various rules. Even there could be RIA to change the namespace based on certain rules.

In the design doc the approach we took was to modify just before creation and after all RIA / modifications are applied.

Our docs line could indicate that both namespace mapping and RIA would have run before Resourcemodifier is applied.

I think of RIA and namespace mapping together, either we do resourcemodifier before both or after both

In between ismore confusing IMO.

[27149chen]

I prefer to do resourcemodifier before both

[27149chen]

@sseago I agree with you that a RIA should never change namespace, here is a bad example: vmware-tanzu/velero-plugin-for-csi#82

[27149chen]

Some reasons for using old namespace:

1. resource modifier is to modify an object from old to new, old object is more clear since new object can be changed.
2. resource modifier is a config (ConfigMap), and may be a large one, it should be reusable when some restore options are changed, like namespace mapping

[sseago]

@27149chen I think your second reason is the biggest reason to stick with the old namespace. Logically, namespace mapping is one of the last things to do -- first we change what we restore, then we change where we restore it to. And yes, you might have one backup that you're using as a template for restoring to different namespaces. Basically for any given backup, the old namespace will be valid for every restore that's made from it, but the new namespace may not.

[sseago]

@27149chen So the CSI example seems like a bit of a special case. It's not arbitrarily changing namespace outside of the NS mapping -- if it were to do that, I suspect that it would break things. Instead, it's actually using the NS mapping and changing it here (same change as velero would eventually change without it), but the reason for it is that the namespace is then used to find the matching VolumeSnapshot resource, so it needs to use the new namespace here.

[anshulahuja98]

@27149chen can you update the website docs and resource modifier design docs with this callout for changes done before NS mapping?
Post that we can get this checked in. Since overall folks are inline with your proposal of using old namespace, I agree with further comments and the majority.

[27149chen]

@anshulahuja98 docs updated