Support variables in patchesJson6902 #1774
Conversation
Signed-off-by: Max Goncharenko <kacejot@fex.net>
There was a problem hiding this comment.
We also need to handle auto-gen for mutate.patchesJson6902 :
https://github.com/kyverno/kyverno/blob/main/pkg/policymutation/policymutation.go#L384
| return ruleResponse, h.patchedResource | ||
| } | ||
|
|
||
| patchedPatch, err := variables.SubstituteAll(h.logger, h.evalCtx, patch) |
There was a problem hiding this comment.
As Jim mentioned here, do we want to allow variables to be specified in path? What if it resolves to a list?
There was a problem hiding this comment.
I think this situation is OK, because in this case path won't apply. The more places we can use to substitute vars, the more flexible we are. Invalid path with an list inside must be validated further when patch is applied.
There was a problem hiding this comment.
Yes I agree we want to make it as flexible as possible, but supporting variable substitution in path could potentially bring unexpected results for different types, thus lead to confusing situations. Do you see any use case there?
There was a problem hiding this comment.
I see no use case for more complex types than string in path, but I also think that this is not the actual place where this check should be applied. We could check here that we have unmarshall error and this will bring user the understanding of what he did wrong with path.
It is not clear for me. I see that we have no resource context here, so variables cannot be substituted. |
|
This PR is not actual anymore, because we substitute variables substitution for entire rule. kyverno/pkg/engine/mutation.go Line 93 in 64f49ca |
|
Closed due to already implemented logic in #1785 |
Signed-off-by: Max Goncharenko kacejot@fex.net
Related issue
Fixes #1521
What type of PR is this
/kind bug
Proposed changes
Added variables resolution logic in mutation patchesJson6902 handler
Checklist