-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal to add support for Resource Modifier (AKA JSON Substitutions) in Restore workflow #5880
Conversation
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 |
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? |
@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 Report
@@ 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 |
- ![image](https://user-images.githubusercontent.com/36476228/219611516-7fa9527d-c592-4471-8e3e-39ad5e7606db.png) | ||
- This filter can be used through golang Package: "k8s.io/client-go/util/jsonpath" | ||
|
||
- Last the user will provide the current value regex, basically at the path specified above, we check if the provided regex matches the value at that path. If it does, we substitute the value with the new value provided by the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 @
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been moved to future scope and not intended for current proposal since it will require forking the jsonpatch library.
@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. |
- ![image](https://user-images.githubusercontent.com/36476228/219611516-7fa9527d-c592-4471-8e3e-39ad5e7606db.png) | ||
- This filter can be used through golang Package: "k8s.io/client-go/util/jsonpath" | ||
|
||
- Last the user will provide the current value regex, basically at the path specified above, we check if the provided regex matches the value at that path. If it does, we substitute the value with the new value provided by the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
- ![image](https://user-images.githubusercontent.com/36476228/219611516-7fa9527d-c592-4471-8e3e-39ad5e7606db.png) | ||
- This filter can be used through golang Package: "k8s.io/client-go/util/jsonpath" | ||
|
||
- Last the user will provide the current value regex, basically at the path specified above, we check if the provided regex matches the value at that path. If it does, we substitute the value with the new value provided by the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this support substitute complicated structures rather than simple values? e.g.
a:
b:
c
->
a:
d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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! |
@anshulahuja98 Do you see any problem for the current PR and its implementation to cover this case? |
@Lyndon-Li 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 So using a similar patch properly escaped should work. I don't see any issue. |
Signed-off-by: Anshul Ahuja <[email protected]>
7b2b842
to
2b387f4
Compare
Signed-off-by: Anshul Ahuja <[email protected]>
/kind changelog-not-required |
Signed-off-by: Anshul Ahuja <[email protected]>
@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. |
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. |
- bar | ||
- foo | ||
patches: | ||
- operation: test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we put the test
patch in the conditions
section as it is the condition actually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we allow multiple test
patches? What is the relationship of them? and
or or
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Signed-off-by: Anshul Ahuja <[email protected]>
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:
/kind changelog-not-required
as a comment on this pull request.site/content/docs/main
.