-
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
Only one change image rule can match when restore change image #6344
Comments
I'm not sure about this proposal. |
I think it is really a bug that the rule is only matched with the first one that is found. |
My understanding of the ConfigMap rules is
I'm not sure about allowing multiple rules to apply to the same image. Of course, that can make the rules more flexible, but the rules are less readable to users. Users cannot tell the result until they read through all the existing rules. @sseago @qiuming-best What's your opinion? |
I think this is a bug, the rules here should be more flexible
This is not realistic in real scenarios. For example, the user's image may come from multiple different registries, different projects, and different tags. It is very difficult to complete accurate matching, and there will also be conflicts in the order of configuration rules. such as
|
There is a proposed design to add a generic substitution in the Restore workflow. Could this requested function be integrated into the design workflow? |
I think we will support whatever the resource modifier can handle. @anshulahuja98 In addition, any idea on supporting the use case proposed by op? |
Logically just the act of changing images is fully supported by resourcemodifer design. practically the only issue might be that the user will have to define multiple substitutions whereas in case of "image-change" they could provide prefix. To enhance the end user experience, we can add regex support to check on the prefix of the image instead of the whole image and then substitute it. This was already called as future scope in design. It won't be the exact same functionality as "image-change" / or the request in the issue, since they are focusing on specific rule-based sub, but with explicit substitution it is still be supported. We can think through other approaches apart from regex match on value to see if we can add more value here using resourcemodifer, or if regex might be enough here. |
version: v1
resourceModifierRules:
- conditions:
groupKind: deployments.apps
resourceNameRegex: "resource-modifiers-.*"
patches:
- operation: replace
path: "/spec/template/spec/containers/0"
value: "{\"name\": \"nginx\", \"image\": \"nginx:1.14.2\"}" @anshulahuja98 Take this YAML for example, it will replace the deployments images with nginx:1.14.2, as it will replace the images directly. In this issue, @wawa0210 wants to replace container images that contain the keywords Currently, we can replace images with one specific value without further matching and replacing target JSON values. below is the shell command that replaces the image: kubectl get pod my-pod -o json | \
jq '.spec.containers[0].image |= (sub("docker.m.daocloud.io"; "docker.m1.daocloud.io") | \
sub("test"; "v1.2.0"))' | \
kubectl patch pod my-pod -p "$(cat -)" For this image matching and replacement requirement, what about enhancing the handling for the target replacement JSON values? we could support version: v1
resourceModifierRules:
- conditions:
groupKind: deployments.apps
resourceNameRegex: "resource-modifiers-.*"
patches:
- operation: replace
path: "/spec/template/spec/containers/0"
valueReplaceRegex: 'sub("docker.m.daocloud.io"; "docker.m1.daocloud.io") | sub("test"; "v1.2.0")' Also version: v1
resourceModifierRules:
- conditions:
groupKind: deployments.apps
resourceNameRegex: "resource-modifiers-.*"
patches:
- operation: replace
path: "/spec/template/spec/containers/0"
valueReplaceRegex: 'sub("([^/]+)/daocloud"; "\\1/abccloud")' |
Thanks for this proposal @qiuming-best, I think what you shared above makes sense and I agree to it in principle. Initially I was also evaluating using libraries which could give good regex support for value checks, and hence started with jsonpath. But couldn't get the required functionality through it. Jq was not something I was able to evaluate as an alternative then. 1 way is that we deprecate current jsonpatch based impl in favour of a pure jq based approach Or we extend the current patch contract to have another field "jqQuery" and let jsonpatch and jqQuery both stay supported scenarios. Although before we jump into velero side changes to support / extend resource modifiers. We should perhaps look at the feature completeness / gaps in go impl of jq libraries. And also be wary of the maintenance of such libraries. |
@anshulahuja98 Do you think it's necessary to include the enhancement that supports |
Hi @qiuming-best |
I won't be able to commit to implementing this. |
@ wawa0210 The flexible condition to match the specific fields and do the replacement is a good requirement. However, many users have not raised this requirement, and we will consider adding it later. |
I wonder whether there are edge cases here that we're not handling. Is the assumption, for example, that rules are applied sequentially to the results of the last rule, or do we find all matches to the original value? Starting with the case defined in the original issue, what about the following more complicated scenarios: scenario 1:
In this case there are two rules that match the image from backup. I imagine here we'd want just the first match to be applied. scenario 2:
In this case, case2 matches the image after case1 rule is applied, but if we're determining matches before making any changes we won't find a match in case2. The proposed PR applies the cases on top of prior subsitutions, which would mean that scenario1 would just make the first replace, and scenario 2 would make both replaces, resulting in a final value of "docker.m2.daocloud.io" from the initial "docker.m.daocloud.io". One use case that the proposed change would break, though is this: Say you have pods with two images: "docker.io/image1' and "docker.io/image2" and you want to swap these values on restore. Current code would accomplish that with the following rule:
Current code would result in swapping these values on restore -- i.e. image1 pods get image2, and image2 pod get image1. However, with the proposed change, all pods will have image1 -- i.e. image2 pods get image1, and image1 pods are unchanged, since the processing will change image1 to image2 and back to image1 again. This may not be a valid or useful use case at all -- but I have a feeling there are edge cases here that we need to be careful about before we start making changes that changes behavior of existing code. |
What steps did you take and what happened:
When I try to restore with the velero change image rule, multiple rules only hit one
What did you expect to happen:
It is hoped that the image can be hit by all rules and change completed
The following information will help us better understand what's going on:
this is origin pod yaml
Below is my change rule configmap
I hope the effect after the restore is
but actual is
After checking the code, it is found that once a certain rule is matched, it will return directly, ignoring subsequent matches
velero/pkg/restore/change_image_name_action.go
Lines 196 to 206 in 8427a9f
Anything else you would like to add:
Environment:
velero v1.11.0
Vote on this issue!
This is an invitation to the Velero community to vote on issues, you can see the project's top voted issues listed here.
Use the "reaction smiley face" up to the right of this comment to vote.
The text was updated successfully, but these errors were encountered: