Validate diff.comparePatches fields early by 0xavi0 · Pull Request #5669 · rancher/fleet

0xavi0 · 2026-08-27T13:12:23Z

Bad values in diff.comparePatches are currently accepted by fleet apply and then discarded on the agent, where the failure is either silent or only visible at -v=1. This validates four of them up front, so a typo fails at apply time with the offending path and value.

Validated fields:

diff.comparePatches[].name — must be a valid Go regular expression
diff.comparePatches[].operations[].op — one of add, ignore, remove, replace, test (copy/move are unsupported: Operation has no from field to encode them)
diff.comparePatches[].operations[].path — non-empty JSON pointer starting with /; skipped for ignore, which never reads the path
diff.comparePatches[].jsonPointers — same rule

Refers to: #4663

Additional Information

Checklist

~~- [ ] I have updated the documentation via a pull request in the fleet-product-docs repository.~~

Bad values in `diff.comparePatches` are currently accepted by `fleet apply` and then discarded on the agent, where the failure is either silent or only visible at `-v=1`. This validates four of them up front, so a typo fails at apply time with the offending path and value. Validated fields: - `diff.comparePatches[].name` — must be a valid Go regular expression - `diff.comparePatches[].operations[].op` — one of `add`, `ignore`, `remove`, `replace`, `test` (`copy`/`move` are unsupported: `Operation` has no `from` field to encode them) - `diff.comparePatches[].operations[].path` — non-empty JSON pointer starting with `/`; skipped for `ignore`, which never reads the path - `diff.comparePatches[].jsonPointers` — same rule Refers to: rancher#4663 Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

Copilot

Pull request overview

This PR adds upfront validation for diff.comparePatches configuration so invalid regexes, unsupported JSON Patch ops, and malformed JSON pointers fail early (during fleet apply and HelmOp validation) instead of being silently dropped or only logged by the agent.

Changes:

Introduces a shared internal/validation leaf package to validate comparePatch names, operations, and JSON pointer syntax consistently across entry points.
Wires the shared validation into fleet apply (fleet.yaml) and HelmOp reconciliation, and improves agent-side handling/logging for legacy/invalid values.
Updates schemas/CRD descriptions and adds extensive unit + integration test coverage around the new validation behavior.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
schemas/fleet.yaml.json	Updates schema descriptions and constrains `operations[].op` via enum in the JSON schema.
pkg/apis/fleet.cattle.io/v1alpha1/bundledeployment_types.go	Updates API field docs and adds a `jsonschema` enum tag for `Operation.Op`.
internal/validation/patchop.go	Defines the canonical supported operation set and shared error formatting.
internal/validation/comparepatches.go	Implements shared validation for regex names, supported ops, and JSON pointer syntax with path-aware errors.
internal/validation/comparepatches_test.go	Unit tests for the new shared validation helpers and their error messages.
internal/cmd/controller/helmops/reconciler/helmop_controller.go	Runs shared BundleSpec option validation for HelmOps so bad values fail controller-side.
internal/cmd/controller/helmops/reconciler/helmop_controller_test.go	Adds tests to ensure HelmOps reject invalid comparePatches inputs and validate embedded BundleSpec options.
internal/cmd/agent/deployer/desiredset/normalizers_test.go	Pins validator ↔ agent behavior equivalence for supported patch ops and JSON pointers.
internal/cmd/agent/deployer/desiredset/normalizers_internal_test.go	Tests agent behavior to drop invalid ops/pointers without discarding valid sibling operations, and to log errors visibly.
internal/cmd/agent/deployer/desiredset/diff.go	Plumbs logger into normalizer construction and adds agent-side discarding + error logging for legacy invalid config.
internal/bundlereader/validate_fleetyaml.go	Runs shared validation across all BundleDeploymentOptions sites in fleet.yaml (bundle/targets/targetCustomizations).
internal/bundlereader/validate_fleetyaml_test.go	Adds extensive fleet.yaml validation tests for comparePatches name/op/path/jsonPointers across option sites and indices.
integrationtests/cli/apply/apply_test.go	Adds integration coverage ensuring `fleet apply` fails early with useful paths/values for invalid comparePatches.
charts/fleet-crd/templates/crds.yaml	Updates CRD descriptions for comparePatch fields (but does not currently add enums to the CRD schema).

Suppressed comments (6)

pkg/apis/fleet.cattle.io/v1alpha1/bundledeployment_types.go:208

This comment says the operation path must be an “RFC 6901” pointer, but Fleet’s validation is intentionally laxer (it accepts other "~" sequences literally per json-patch). Rewording would prevent users from assuming strict RFC 6901 escaping rules.

	// Path is the JSON pointer the operation applies to, e.g. /spec/replicas.
	// It must be a non-empty RFC 6901 pointer starting with a slash; a
	// Kubernetes-style field path such as spec.replicas addresses nothing and is
	// rejected. Required unless Op is "ignore", which drops the whole resource
	// from the comparison and never reads the path.

charts/fleet-crd/templates/crds.yaml:975

The CRD schema documents a fixed set of supported operations, but the OpenAPI schema here still allows any string. Adding an enum for op would align server-side validation with the fleet.yaml schema and apply-time validation.

                                        the whole patch fail to

                                        apply.'
                                      nullable: true
                                      type: string

charts/fleet-crd/templates/crds.yaml:2080

The CRD schema documents a fixed set of supported operations, but the OpenAPI schema here still allows any string. Adding an enum for op would align server-side validation with the fleet.yaml schema and apply-time validation.

                                    the whole patch fail to

                                    apply.'
                                  nullable: true
                                  type: string

charts/fleet-crd/templates/crds.yaml:3180

The CRD schema documents a fixed set of supported operations, but the OpenAPI schema here still allows any string. Adding an enum for op would align server-side validation with the fleet.yaml schema and apply-time validation.

                                          makes the whole patch fail to

                                          apply.'
                                        nullable: true
                                        type: string

charts/fleet-crd/templates/crds.yaml:8515

The CRD schema documents a fixed set of supported operations, but the OpenAPI schema here still allows any string. Adding an enum for op would align server-side validation with the fleet.yaml schema and apply-time validation.

                                    the whole patch fail to

                                    apply.'
                                  nullable: true
                                  type: string

charts/fleet-crd/templates/crds.yaml:9647

The CRD schema documents a fixed set of supported operations, but the OpenAPI schema here still allows any string. Adding an enum for op would align server-side validation with the fleet.yaml schema and apply-time validation.

                                          makes the whole patch fail to

                                          apply.'
                                        nullable: true
                                        type: string

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

weyfonk

LGTM, with a few nitpicks :)

weyfonk · 2026-08-31T13:01:28Z

+	// Each entry must be a non-empty pointer starting with a slash; a
+	// Kubernetes-style field path such as spec.replicas addresses nothing and is
+	// rejected. Escaping is the one the JSON patch library applies, which is
+	// laxer than RFC 6901: "~0" and "~1" are read as "~" and "/", and any other


nit:

Suggested change

// laxer than RFC 6901: "~0" and "~1" are read as "~" and "/", and any other

// laxer than RFC 6901: "~0" and "~1" are read as "~" and "/", respectively, and any other

weyfonk · 2026-08-31T13:16:27Z

          },
          "type": "array",
-          "description": "JSONPointers ignore diffs at a certain JSON path."
+          "description": "JSONPointers ignore diffs at the given JSON pointers, e.g. /spec/replicas.\nEach entry must be a non-empty pointer starting with a slash; a\nKubernetes-style field path such as spec.replicas addresses nothing and is\nrejected. Escaping is the one the JSON patch library applies, which is\nlaxer than RFC 6901: \"~0\" and \"~1\" are read as \"~\" and \"/\", and any other\n\"~\" sequence is left as written."


nit:

Suggested change

"description": "JSONPointers ignore diffs at the given JSON pointers, e.g. /spec/replicas.\nEach entry must be a non-empty pointer starting with a slash; a\nKubernetes-style field path such as spec.replicas addresses nothing and is\nrejected. Escaping is the one the JSON patch library applies, which is\nlaxer than RFC 6901: \"~0\" and \"~1\" are read as \"~\" and \"/\", and any other\n\"~\" sequence is left as written."

"description": "JSONPointers ignore diffs at the given JSON pointers, e.g. /spec/replicas.\nEach entry must be a non-empty pointer starting with a slash; a\nKubernetes-style field path such as spec.replicas addresses nothing and is\nrejected. Escaping is the one the JSON patch library applies, which is\nlaxer than RFC 6901: \"~0\" and \"~1\" are read as \"~\" and \"/\", respectively, and any other\n\"~\" sequence is left as written."

weyfonk · 2026-08-31T13:49:18Z

+// ValidateComparePatchNames checks that every diff.comparePatches[].name in opts compiles
+// as a regular expression. The agent matches a patch by exact name first and falls
+// back to matching the name as a regex, so a name which does not compile is dead
+// config: it is dropped by the agent instead of being reported to the user.


"dropped by the agent" suggests that it would reach the Fleet agent; isn't the whole point of this PR to prevent this? 🤔
Edit: it seems to me that this phrases in an indicative mood instead of using a conditional tense to denote what would happen on the agent side if this controller-side validation were not in place.

weyfonk · 2026-08-31T15:29:57Z

This does not cover patch names nor operations; is that deliberate?

Covered now, thanks!

weyfonk · 2026-08-31T15:34:28Z

+// TestValidateBundleSpec covers the rules a HelmOp shares with fleet.yaml through
+// its embedded BundleSpec. A HelmOp never passes through
+// bundlereader.validateFleetYAML, so without these the values would only fail on
+// the agent, where the failure is logged and swallowed.


nit: Glp! The following may be clearer:

Suggested change

// the agent, where the failure is logged and swallowed.

// the agent, where the failure is logged and not surfaced to the user.

weyfonk · 2026-09-01T09:39:59Z

+			// An operation with a path but no op reads as a no-op and is not one:
+			// it makes the whole patch fail. Rejecting it is a deliberate
+			// tightening over what fleet apply accepted before.


Great explanation, I think we could surface this in user-facing docs.

weyfonk · 2026-09-01T10:17:48Z

+					// validation.ValidateComparePatchNames), but BundleDeployments
+					// created by an older Fleet version, or from a HelmOp, still reach
+					// this point, hence the check and the error log.
+					logger.Error(


Nice, thanks for making these errors more visible!

Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

0xavi0 added this to the v2.16.0 milestone Aug 27, 2026

0xavi0 self-assigned this Aug 27, 2026

0xavi0 added the area/validation label Aug 27, 2026

0xavi0 added this to Fleet Aug 27, 2026

Change ignroe->bogus as typos was complaining

fee4199

Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

0xavi0 marked this pull request as ready for review August 27, 2026 14:21

Copilot AI lite review requested due to automatic review settings August 27, 2026 14:21

0xavi0 requested a review from a team as a code owner August 27, 2026 14:21

Copilot started reviewing on behalf of 0xavi0 August 27, 2026 14:22 View session

Copilot AI reviewed Aug 27, 2026

View reviewed changes

Comment thread pkg/apis/fleet.cattle.io/v1alpha1/bundledeployment_types.go Outdated

Comment thread charts/fleet-crd/templates/crds.yaml

Update jsonpointers comment

8f01bbf

Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

weyfonk moved this to 👀 In review in Fleet Aug 31, 2026

weyfonk approved these changes Sep 1, 2026

View reviewed changes

Comment changes and extra tests

d144a6c

Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

0xavi0 force-pushed the early-verify-diff-compare-patches branch from b351ad0 to d144a6c Compare September 2, 2026 10:09

0xavi0 requested a review from weyfonk September 2, 2026 10:51

0xavi0 merged commit d563126 into rancher:main Sep 2, 2026
25 of 27 checks passed

github-project-automation Bot moved this from 👀 In review to ✅ Done in Fleet Sep 2, 2026

Navigation Menu

Uh oh!

Validate diff.comparePatches fields early - #5669

Validate diff.comparePatches fields early#5669
0xavi0 merged 4 commits into
rancher:mainfrom
0xavi0:early-verify-diff-compare-patches

0xavi0 commented Aug 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

weyfonk left a comment

Uh oh!

weyfonk Aug 31, 2026

Uh oh!

weyfonk Aug 31, 2026

Uh oh!

weyfonk Aug 31, 2026

Uh oh!

0xavi0 Sep 2, 2026

Uh oh!

weyfonk Aug 31, 2026

Uh oh!

0xavi0 Sep 2, 2026

Uh oh!

weyfonk Aug 31, 2026

Uh oh!

weyfonk Sep 1, 2026

Uh oh!

weyfonk Sep 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// laxer than RFC 6901: "~0" and "~1" are read as "~" and "/", and any other
	// laxer than RFC 6901: "~0" and "~1" are read as "~" and "/", respectively, and any other

	// the agent, where the failure is logged and swallowed.
	// the agent, where the failure is logged and not surfaced to the user.

Uh oh!

Conversation

0xavi0 commented Aug 27, 2026

Additional Information

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

weyfonk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants