Ignore missing path error in conditional match

[seanblong]

Thank you for contributing to Velero!

Please add a summary of your change

For Resource Modifiers, don't treat unmatched paths as an error (ErrMissing) instead treat them the same way as an ErrTestFailed response.

This is particularly useful when we want to match on paths should they exist, such as the following example where I'm looking to remove a particular affinity rule.

resourceModifierRules:
- conditions:
    groupResource: statefulsets.apps
    matches:
    - path: "/spec/template/spec/affinity/nodeAffinity/requiredDuringSchedulingIgnoredDuringExecution/nodeSelectorTerms/0/matchExpressions/0/key"
      value: cloud.google.com/gke-preemptible
  patches:
  - operation: remove
    path: "/spec/template/spec/affinity/nodeAffinity/requiredDuringSchedulingIgnoredDuringExecution/nodeSelectorTerms/0/matchExpressions/0"

I've tested this locally and with a deployed container image. All statefulsets containing this particular path have it removed and all statefulset without it are filtered out, without creating an error message and allowing the restore to finish with a Completed status.

Does your change fix a particular issue?

Fixes #(issue)

N/A

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.

[anshulahuja98]

Valid suggestion
@seanblong - can you look into adding a unit test for this case?

[seanblong]

Valid suggestion @seanblong - can you look into adding a unit test for this case?

You bet! The test I added is a little nonsensical as It's checking for a subattribute of a label, but testing before/after change shows that it accurately reports on ErrMissing. I could replace with a more practical example, maybe using podYAMLWithPVCVolume and checking for a container index or mount out of range, but preferred keeping it concise (fitting with other tests).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3b8370e) 61.64% compared to head (017b9a4) 61.64%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7410   +/-   ##
=======================================
  Coverage   61.64%   61.64%           
=======================================
  Files         262      262           
  Lines       28477    28477           
=======================================
  Hits        17556    17556           
  Misses       9689     9689           
  Partials     1232     1232           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[anshulahuja98]

Hi @seanblong, sorry for the late reply.
I was rethinking on the impact of current behaviour of this PR.
I might have missed few aspects when I first reviewed.

Basically with your change - all path missing errors are swallowed and not logged.
This might be convenient for your use case, but I am worried, this would lead incorrect surfacing of error to users who might actually make mistakes in path.

An alternative to achieve what you wish is by using a test patch.
You first do a test patch to see if the path exists and then only do the chagnes -> this would lead to completed as the end status.

Let me know if that sounds reasonable?

[seanblong]

👋 @anshulahuja98,

No problem at all. I actually went the route of test originally, but found some limitations, especially when we have lists (of lists) of potentially complex types that we're looking to represent. Currently matches operates very similarly to test, but provides the ability/opportunity to ignore the ErrMissing error (in addition to ignoring ErrTestFailed), which I think is closer to the intention when conditionally matching on paths.

Let me better illustrate this using two of our test variables. The first having a list of volumeMounts and the second being a more vanilla Nginx image. This acts as a good example where we may want a generic resourceModifier, but don't want to incur a lot of errors or a failed restoration.

velero/internal/resourcemodifiers/resource_modifiers_test.go

Lines 1062 to 1083 in d45b313

	var podYAMLWithNFSVolume = `
	apiVersion: v1
	kind: Pod
	metadata:
	name: pod1
	namespace: fake
	spec:
	containers:
	- image: fake
	name: fake
	volumeMounts:
	- mountPath: /fake1
	name: vol1
	- mountPath: /fake2
	name: vol2
	volumes:
	- name: vol1
	nfs:
	path: /fake2
	- name: vol2
	emptyDir: {}
	`

velero/internal/resourcemodifiers/resource_modifiers_test.go

Lines 1050 to 1060 in d45b313

	var podYAMLWithNginx1Image = `
	apiVersion: v1
	kind: Pod
	metadata:
	name: pod1
	namespace: fake
	spec:
	containers:
	- image: nginx1
	name: nginx
	`

Today the following will work on the former, but not the latter. In the case of the latter it will throw an error:

resourceModifierRules:
- conditions:
    groupResource: pods
    matches:
    - path: "/spec/containers/0/volumeMounts/0/name"
      value: "vol2"
  patches:
  - operation: replace
    path: "/spec/containers/0/volumeMounts/0/name"
    value: "vol1"

In order to get this rule to work with test, without generating errors, we'd need to write something like this:

resourceModifierRules:
- conditions:
    groupResource: pods
  patches:
  - operation: test
    path: "/spec/containers/0/volumeMounts"
    value: "[{\"mountPath\": \"/fake1\", \"name\": \"vol1\"},{\"mountPath\": \"/fake2\", \"name\": \"vol2\"}]"
  - operation: replace
    path: "/spec/containers/0/volumeMounts/0/name"
    value: "vol1"

This requires a complicated value that requires some sense of omniscience about the whole object. Keep in mind that if we tried to use the following snippet in the test condition instead, the former would once again generate an error while the latter would succeed.

  - operation: test
    path: "/spec/containers/0/volumeMounts/0/name"
    value: "name"

Alternative workarounds would be to make much more specific resourceModifiers for each workload being migrated, but I believe the strength of Velero, and more specifically, the resourceModifier, is to be fairly agnostic. I'm hoping that in this PR we can leave test as is while leveraging some of the additional logic provided by matchConditions.

[anshulahuja98]

@seanblong I think I understand now.
The mistake I might have made is getting confused between the impl of actual test patch and the matches part.

This looks good to me.
Approving.