Proposal to add support for Resource Modifier (AKA JSON Substitutions) in Restore workflow

[anshulahuja98]

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Design for #5809

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.

[reasonerjt]

TBH I still prefer the RIA approach for its flexibility.

This can be a good utility, but not quite sure if really needed to be part of the upstream

[anshulahuja98]

This is core functionality and not anything which is platform specific. Similar to the Storage Class plugin, I hope it will be okay to push to upstream?
Or do you think this can add any additional bulk to existing code, it is mostly and opt in feature.?

[sseago]

@reasonerjt "TBH I still prefer the RIA approach for its flexibility."

The proposal is to write a RIA plugin to do this. A separate question is whether this is a RIA that should be part of core velero (alongside existing core RIAs such as storageclass change, etc.), or whether this makes more sense as a separate plugin, installed as a separate image. One advantage of the plugin approach is that if implemented as a RIA, it could first be written as a completely external RIA plugin, at least as a proof of concept, and then if there is general interest in including this in velero core, it could be moved. The challenge for making something non-core that's generally useful is that there isn't really an existing upstream repo that is appropriate for "supported plugins that aren't in the core repo", and it's probably too small to warrant its own independent repo (as we have for the storage plugins, etc.), and we don't really have a mechanism for sharing/supporting plugin content outside of creating new repos or putting it in the core repo.

[codecov-commenter]

Codecov Report

Merging #5880 (242025c) into main (c5efb54) will increase coverage by 19.28%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main    #5880       +/-   ##
===========================================
+ Coverage   40.90%   60.18%   +19.28%     
===========================================
  Files         248      229       -19     
  Lines       21554    24215     +2661     
===========================================
+ Hits         8817    14575     +5758     
+ Misses      12100     8632     -3468     
- Partials      637     1008      +371     

see 178 files with indirect coverage changes

[reasonerjt]

Do you have a concrete use case for "current value regex"? In your sample they are all ".*", if it's not necessary we may consider add it in future?
In addition, I'm relatively newbie to jsonpath but it seems to support filter feature, so maybe such filter can be used to matching current values when it's used with @

[anshulahuja98]

This has been moved to future scope and not intended for current proposal since it will require forking the jsonpatch library.

[reasonerjt]

@anshulahuja98 thanks for the update, I've added a few comments in details but generally I'm OK we introduce such capability in v1.12

@sseago @shubham-pampattiwar let us know your thoughts.

[ywk253100]

Does this support replace part of the value? e.g. I want to replace the registry part of all the images: reg1/xxx/xxx -> reg2/xxx/xxx

[anshulahuja98]

that is not part of intended design.
Let me think through of the incremental effort to add that capability - assuming it does not affect the contracts too much.

[anshulahuja98]

updating thread after the proposal update from jsonpath to jsonpatch.

Updating partial values is still not a scenario which is inteneded to supported.

However in the future scope - I have called out regex support on the value. Where you can use the "test" operator to see if the image is of the pattern reg.* and only update in those cases.

[ywk253100]

The JSON path only locates the target value, but how can we substitute it? Are there any functions provided by k8s.io/client-go/util/jsonpath for this?

[ywk253100]

Does this support substitute complicated structures rather than simple values? e.g.

a:
  b:
    c

->

a:
  d

[anshulahuja98]

there is not edit function/ substitute function inbuilt in the jsonpath library.
The intention is to make a fork of sorts in the velero code base and just use the relevant functions from the json path library as utils

I don't expect any significant changes to come o the library over time, so I am not worried about keeping the fork in sync with upstream.

[anshulahuja98]

Searching for complex structures using jsonpath is possible. But providing a substitute like this will complicate the config map design.
The user scenarios I have mentioned can mostly be covered with just substituting values of leaf nodes(string/ int)
So it is not my intention to support such complex substitutions atm. We will mostly skip such scenarios in case attempted by user.

[sseago]

@anshulahuja98 as we were discussing in the community meeting today, I looked into the question of an internal RIA -- but it looks like that won't avoid the GRPC call, so I think we probably need to stick with the current approach of doing it directly in the restore workflow.

[anshulahuja98]

@anshulahuja98 as we were discussing in the community meeting today, I looked into the question of an internal RIA -- but it looks like that won't avoid the GRPC call, so I think we probably need to stick with the current approach of doing it directly in the restore workflow.

Thank you for confirming this!

[Lyndon-Li]

@anshulahuja98
There is a case in Data Mover scenarios -- backup from one provider(e.g., EKS) and restore to a different provider(e.g., AKS).
Then if the source PVC is with StorageClass explicitly specified, even though the SC is backed up by Velero, users cannot directly restore the SC into the target since the SC's content are almost 100% different.

Do you see any problem for the current PR and its implementation to cover this case?
Moreover, since the two SCs are very different, do you think it is better to give users an option to overwrite the entire content of the yaml?

[anshulahuja98]

@Lyndon-Li
So firstly to call out there is a design change which I called out in the previous community meeting where we are switching the approach to JsonPatch based impl as compared to JsonPath. I haven't yet pushed the design changes to this PR.

With Json Patch, it will be even possible to change the entire storage class. Since you can kind of provide a JSON Patch at a Spec level and it will end up modifying the entire storage class.

Quoting a Kubectl patch example which is possible
"https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/"
kubectl patch deployment patch-demo --patch 'spec:\n template:\n spec:\n containers:\n - name: patch-demo-ctr-2\n image: redis'

So using a similar patch properly escaped should work. I don't see any issue.

[anshulahuja98]

/kind changelog-not-required

[sseago]

@anshulahuja98 Velero already supports storageclass mapping on restore so if a backup has volumes in storageclass "foo", we can map all foo volumes to storageclass "bar" on restore. As long as both of these are CSI-backed storage classes, this should be consistent with datamover functionality as well. I don't think we'd need anything in this proposal to support that.

[draghuram]

To answer @anshulahuja98's comment about replacing storage class resources, I agree with @sseago that existing storage class mapping support is sufficient. Users cannot simply restore storage classes in one way or another because configuring storage on a cluster is more than just creating storage class.

[ywk253100]

Should we put the test patch in the conditions section as it is the condition actually?

[ywk253100]

Shall we allow multiple test patches? What is the relationship of them? and or or?

[anshulahuja98]

I would prefer to keep it in the patches section to make the overall configmap easier to understand to end user and keep it closer to the JsonPatch RFC.

The test patches are and in some sense, basically even if 1 test patch fails, the overall patch will fail.
We can allow multiple test patches since it is a supported scenario.