Add support for ResourceModifier (AKA Json Substitutions) in restore flow

[anshulahuja98]

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #5809
Design: #5880

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.

[codecov-commenter]

Codecov Report

Merging #6452 (c8f970a) into main (c4286d7) will increase coverage by 0.15%.
The diff coverage is 79.55%.

@@            Coverage Diff             @@
##             main    #6452      +/-   ##
==========================================
+ Coverage   60.13%   60.28%   +0.15%     
==========================================
  Files         236      238       +2     
  Lines       25090    25256     +166     
==========================================
+ Hits        15088    15226     +138     
- Misses       8957     8976      +19     
- Partials     1045     1054       +9     

Impacted Files	Coverage Δ
pkg/restore/request.go	100.00% <ø> (ø)
pkg/restore/restore.go	64.44% <16.66%> (+0.33%)	⬆️
pkg/cmd/cli/restore/create.go	61.42% <45.45%> (-0.66%)	⬇️
internal/resourcemodifiers/resource_modifiers.go	73.40% <73.40%> (ø)
pkg/controller/restore_controller.go	66.17% <96.96%> (+1.33%)	⬆️
.../resourcemodifiers/resource_modifiers_validator.go	100.00% <100.00%> (ø)

[blackpiglet]

First, the Github action checks cannot pass. I think this is related to the restore format change.
Maybe the ResourceModifier should not be a required field.

Second, please correct me if I made misunderstood, this is the generic resource policy field, https://github.com/vmware-tanzu/velero/blob/main/pkg/apis/velero/v1/backup_types.go#L167
we should use this name in the restore spec too.

[anshulahuja98]

First, the Github action checks cannot pass. I think this is related to the restore format change. Maybe the ResourceModifier should not be a required field.

Second, please correct me if I made misunderstood, this is the generic resource policy field, https://github.com/vmware-tanzu/velero/blob/main/pkg/apis/velero/v1/backup_types.go#L167 we should use this name in the restore spec too.

1. Pushed changes to fix the "optional" behaviour in CLI, hopefully the tests should pass this time, otherwise will check again.
2. I think there is some confusion, the resource policies is for filtering - what to backup / restore. My PR is for modifying the resources which are being restored. The current field is not modeled in a generic way and is specific to resource policy functionality. It does not accrue / align with the functionality I am adding.

[anshulahuja98]

Would be adding E2E tests as part of separate PR.
Will try to add documentation update as part of this same PR.

[ywk253100]

Handle error

[anshulahuja98]

when match comes false, an error will come
I don't see additional use in handling this error
since we just need to rely on the "match" value

Logging it on the other hand might just bloat logs since match will be called for each resource which matches NS/ GroupKind filters.

[ywk253100]

Could you explain more about when match comes false, an error will come? When the match is false, the current code returns nil directly.

There is a case that the regex provided may contain syntax errors, if we don't handle such kinds of errors, users don't know about that and the restore may completed but with unexpected resources restored.

[anshulahuja98]

I understand your concern, validated what you explained with a POC.
Have updated the PR with the fix for handling and also added test case for it.

[ywk253100]

Seems an error is returned if no path matches the test operation. Correct me if I'm wrong.
So that means if users set the test patch, they must make sure it matches, or the restore will fail.

[anshulahuja98]

the restore will not fail
just that particular patch would fail and restore will go on as it is.

For example there are 5 PVCs which match the "conditions"

but only 1 PVC which satisfies the "test" operator. Only that PVC will be patched and all the others will get skipped in some sense.
The logs will show that, but there won't be any hard failure.

[ywk253100]

Yes, the error is shown in the logs, but won't the restore be marked as partially failed? Is this the expected behavior?

[anshulahuja98]

Handled the "test operator related errors separately to avoid partial failure states based on your feedback. Updated code with test case.

[ywk253100]

@anshulahuja98 Please also add some more unit test cases. We have one task in the 1.12 timeframe to improve the unit test coverage to be >60%.

[anshulahuja98]

Sure will do.

[anshulahuja98]

@sseago / @blackpiglet can you please review this PR.

[sseago]

@anshulahuja98 looks like there's a conflict to resolve in this now

[anshulahuja98]

Fixed the merge conflict @sseago
@blackpiglet / @ywk253100 you folks would have to approve again post merge conflict resolution.