feat: add FieldIgnore functionality#1708
Conversation
|
/retest |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1708 +/- ##
===========================================
+ Coverage 69.52% 80.81% +11.29%
===========================================
Files 46 52 +6
Lines 2838 2732 -106
===========================================
+ Hits 1973 2208 +235
+ Misses 697 394 -303
+ Partials 168 130 -38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a generalized “field ignore” mechanism used both by the Helm post-renderer and controller predicates to avoid reconcile/install/upgrade loops caused by external controllers mutating specific fields.
Changes:
- Introduces
pkg/fieldignorewith rules, manifest field stripping, and an update predicate. - Replaces bespoke webhook ignore logic in the IstioRevision controller with
fieldignore.NewPredicateand centralizes default rules. - Wires Helm chart install/upgrade post-rendering to apply ignore rules via
ChartManager+ updatedNewHelmPostRendererAPI.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/helm/postrenderer_test.go | Updates Helm post-renderer tests to pass field ignore rules and adds new coverage for caBundle stripping. |
| pkg/helm/postrenderer.go | Replaces hard-coded webhook field stripping with rule-driven fieldignore.RemoveFieldsFromManifest. |
| pkg/helm/chartmanager.go | Adds WithFieldIgnoreRules option and passes rules into Helm post-renderer for install/upgrade. |
| pkg/fieldignore/fieldignore.go | New rule engine: rule compilation, manifest matching, field-path removal, and controller-runtime predicate. |
| pkg/fieldignore/fieldignore_test.go | New unit tests for rule compilation/matching, field removal, and predicates. |
| controllers/istiorevision/istiorevision_controller.go | Introduces DefaultFieldIgnoreRules and swaps webhook predicate implementation to fieldignore predicate. |
| cmd/main.go | Configures ChartManager with the controller’s default ignore rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // defaultTestRules replicates the built-in field ignore rules used in production. | ||
| var defaultTestRules = []fieldignore.FieldIgnoreRule{ | ||
| { | ||
| Group: "admissionregistration.k8s.io", | ||
| Version: "v1", | ||
| Kind: "ValidatingWebhookConfiguration", | ||
| Fields: []string{"webhooks[*].failurePolicy"}, | ||
| OnlyOnUpdate: true, | ||
| }, | ||
| { | ||
| Group: "admissionregistration.k8s.io", | ||
| Version: "v1", | ||
| Kind: "ValidatingWebhookConfiguration", | ||
| Fields: []string{"webhooks[*].clientConfig.caBundle"}, | ||
| }, | ||
| { | ||
| Group: "admissionregistration.k8s.io", | ||
| Version: "v1", | ||
| Kind: "MutatingWebhookConfiguration", | ||
| Fields: []string{"webhooks[*].clientConfig.caBundle"}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The comment says these replicate production defaults, but they currently diverge from controllers/istiorevision.DefaultFieldIgnoreRules (notably missing NameRegex on the failurePolicy rule and not being pre-compiled). This can let tests pass even if production name-filtering or regex compilation behavior is broken. Consider either (a) reusing a shared default-rules definition from a package-level location (so tests can import it without depending on controllers/...), or (b) updating defaultTestRules to include the same NameRegex and using fieldignore.MustCompileRules(...) so the test exercises the same matching path as production.
| return nil, fmt.Errorf("error removing ValidatingWebhookConfiguration failurePolicy: %v", err) | ||
| } | ||
| } | ||
| fieldignore.RemoveFieldsFromManifest(manifest, pr.fieldIgnoreRules, pr.isUpdate) |
There was a problem hiding this comment.
RemoveFieldsFromManifest is “best-effort” and silently no-ops on unexpected manifest shapes (e.g., webhooks not being an array, non-map elements, etc.). In this post-renderer context that can reintroduce the exact upgrade/install drift this feature is meant to prevent, without any signal. Recommend changing the fieldignore API to return an error (or at least a boolean “removed something” + error) when a rule matches a manifest but one of the configured paths can’t be traversed due to a type mismatch, and then propagate/log that here so failures are visible.
| func gvkForObject(scheme *runtime.Scheme, obj client.Object) (schema.GroupVersionKind, error) { | ||
| gvks, _, err := scheme.ObjectKinds(obj) | ||
| if err != nil { | ||
| return schema.GroupVersionKind{}, err | ||
| } | ||
| if len(gvks) == 0 { | ||
| return schema.GroupVersionKind{}, fmt.Errorf("no GVK found for object %T", obj) | ||
| } | ||
| return gvks[0], nil | ||
| } |
There was a problem hiding this comment.
Returning gvks[0] can be incorrect when a type is registered under multiple GVKs/versions; picking the first can cause rules not to match and the predicate to incorrectly treat ignored-field-only updates as “changed” (leading to reconcile loops). Prefer a deterministic GVK resolution that matches the object’s actual GVK (e.g., controller-runtime’s apiutil.GVKForObject), or select the GVK that matches the object’s apiVersion/kind when available.
| func WithFieldIgnoreRules(rules []fieldignore.FieldIgnoreRule) ChartManagerOption { | ||
| return func(cm *ChartManager) { | ||
| cm.fieldIgnoreRules = rules |
There was a problem hiding this comment.
This accepts rules in either compiled or uncompiled form, but uncompiled rules with NameRegex will fall back to regexp.MatchString(...) in Matches, incurring regex compilation overhead repeatedly during rendering/predicate evaluation. To make the option safer and more predictable, consider compiling once here (or documenting/enforcing that callers must pass fieldignore.MustCompileRules(...)), and failing fast on invalid regex rather than deferring to runtime behavior.
| func WithFieldIgnoreRules(rules []fieldignore.FieldIgnoreRule) ChartManagerOption { | |
| return func(cm *ChartManager) { | |
| cm.fieldIgnoreRules = rules | |
| // Rules are compiled when the option is applied so invalid regexes fail fast and | |
| // matching does not repeatedly recompile patterns at render time. | |
| func WithFieldIgnoreRules(rules []fieldignore.FieldIgnoreRule) ChartManagerOption { | |
| return func(cm *ChartManager) { | |
| cm.fieldIgnoreRules = fieldignore.MustCompileRules(rules) |
| // NameRegex is an optional regex to match resource names. Empty matches all names. | ||
| NameRegex string `json:"nameRegex,omitempty"` |
There was a problem hiding this comment.
Do we need regex support? This seems like it would make the API harder to use and could lead to false positives. iirc we removed the regex from the current hardcoded rules. Using either Name and (optionally) Namespace or a label selector to match resources would be more reliable. If these aren't specified then matching all GVKs.
There was a problem hiding this comment.
removed it, now we match on exact name or match all objects of a kind
| // NewPredicate returns a predicate that ignores changes to fields specified by | ||
| // the given rules. On update events it converts both old and new objects to | ||
| // unstructured maps, removes the ignored fields (plus standard metadata noise | ||
| // like resourceVersion, generation, managedFields), and only triggers | ||
| // reconciliation when the remaining content differs. | ||
| func NewPredicate(scheme *runtime.Scheme, rules []FieldIgnoreRule) predicate.Funcs { |
There was a problem hiding this comment.
Is it allowed to specify multiple FieldIgnoreRule for the same GVK? e.g.
podGVK :=
NewPredicate(s, []FieldIgnoreRule{
{Group: "core", Version: "v1", Kind: "Pod", Fields: [".spec.imagePullSecrets"]},
{Group: "core", Version: "v1", Kind: "Pod", Fields: [".spec.containers[*]"]},
}
There was a problem hiding this comment.
Predicates should have a type parameter: https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.23.3/pkg/predicate#Predicate
Maybe FieldIgnoreRule should have one as well so that the GVK matching can be enforced by the compiler? I think we only want to pass ignore rules that match the type anyways and not all DefaultIgnoreRules?
dgn
changed the title
this is a cleaner API around ignoring certain fields in watches and when installing charts, through a PostRender func. The goal is to eventually expose this API to the user, but for now it'll just help to keep track of our default ignores that we have implemented to fix specific bugs. Signed-off-by: Daniel Grimm <dgrimm@redhat.com>
|
/retest |
Removes the "Untyped" variants of FieldIgnoreRules in favor of a passing `Unstructured` as the specific type for generic rules coming from either helm manifest or user input. Signed-off-by: Nick Fox <nfox@redhat.com>
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
this is a cleaner API around ignoring certain fields in watches and when installing charts, through a PostRender func. The goal is to eventually expose this API to the user, but for now it'll just help to keep track of our default ignores that we have implemented to fix specific bugs.
Pushing this here so we can discuss in this week's call.
Update: I added a bunch of tests and improved the API a bit. Removed the Regexes, but now you can select where the rules should apply: reconcile, reconcile + upgrade, or reconcile + upgrade + install
Fixes #430, #1148