ssa: add DriftIgnoreRules to ApplyOptions for field-level ignore on apply#1157
ssa: add DriftIgnoreRules to ApplyOptions for field-level ignore on apply#1157dipti-pai wants to merge 1 commit intofluxcd:mainfrom
Conversation
dipti-pai
force-pushed
the
ssa-ignore-rules
branch
from
877f705 to
4849129
Compare
matheuscscp
left a comment
matheuscscp
left a comment
There was a problem hiding this comment.
The code is looking great to me, but I'd like to better understand how applying the ignore rules after dry-run works. I'm not saying it's not correct at all, just saying that it's probably correct and I need to catch up and understand the whole flow. It would be great if we could talk about this in the next dev meeting 🙏
|
Thanks @matheuscscp for reviewing. Adding a few notes below regarding dry run behavior with ssa and potential improvements in the current version of the code to make it easier to discuss during the dev meeting tomorrow. Dry-run behavior with DriftIgnoreRules dryRunApply always sends the complete desired object (including fields targeted by DriftIgnoreRules) to the API server with ForceOwnership. This is intentional - The API server validates the full payload — missing required fields like The ignored fields are only stripped later — from the apply payload (not the dry-run), and only for existing objects (not creates). Summarizing the full behavior for different scenarios and couple of improvements -
My opinion is to take the improvements in 5. Improvement 6 is debatable - if a field is in |
|
I think we should strive to avoid applying objects if only the ignored fields have drifted, and always return |
If I understand this correctly, this is what @stefanprodan commented about above. I agree, we should make this improvement and do not apply if only ignored fields have drifted.
This improvement also seems quite important. |
dipti-pai
force-pushed
the
ssa-ignore-rules
branch
from
4849129 to
43c3b28
Compare
|
With the latest iteration, the improvements 5. and 6. in this comment are implemented. Summarizing the new behavior
Before comparing the existing object with the dry-run result,
When an apply does happen (because non-ignored fields changed), we compute which ignored paths have actually drifted between live object and dry-run. Only those drifted ignored paths are removed from the apply payload via JSON patch. This means --
Because of these changes, a new scenario arises which I want to call out - Flux does not update an ignored field even when its own source changes. For example,
With the current changes, if |
Small but important correction here (if I understood the current iteration correctly; please correct me if I'm wrong, @dipti-pai): when values match, Flux does reclaim ownership even if another manager already owns the field. Stripping only on real drift keeps things simple (no need to parse |
@matheuscscp, yes this is true because we only strip the fields when they drift. So, if they converge, Flux reclaims ownership. Note this happens only when |
LGTM 👍 Are there any outstanding items left for us to address from your initial list? |
dipti-pai
force-pushed
the
ssa-ignore-rules
branch
from
43c3b28 to
2088409
Compare
The things we discussed last dev meeting are addressed in the latest iteration. Made the changes for the AI-generated review comments, they look solid and rebased on main. Thanks so much for reviewing! |
dipti-pai
force-pushed
the
ssa-ignore-rules
branch
from
2088409 to
e0775d7
Compare
stefanprodan
added
enhancement
stefanprodan
left a comment
stefanprodan
left a comment
There was a problem hiding this comment.
LGTM
Thanks @dipti-pai 🥇
|
@dipti-pai please create an issue in flux2 for taking into account the ignore rules in |
|
@dipti-pai Found something: I was looking at how we deal with rules containing an empty selector i.e. the "target-all" rules, and I noticed that we silently use the last one if there are multiple. This is only a thing for target-all rules because the map is by pointer, and the pointers are always unique except for But I also noticed that this is already what we do in helm-controller, so it's too late to change. In that case, I noticed we are duplicating this logic in diff --git a/ssa/jsondiff/unstructured.go b/ssa/jsondiff/unstructured.go
index 400c15e..7d7f458 100644
--- a/ssa/jsondiff/unstructured.go
+++ b/ssa/jsondiff/unstructured.go
@@ -44,6 +44,23 @@ type IgnoreRule struct {
Selector *Selector
}
+// CompiledIgnoreRules is a set of IgnoreRule with compiled selectors.
+type CompiledIgnoreRules map[*SelectorRegex][]string
+
+// CompileIgnoreRules compiles the selectors in the given IgnoreRule slice
+// and returns a CompiledIgnoreRules.
+func CompileIgnoreRules(rules []IgnoreRule) (CompiledIgnoreRules, error) {
+ compiled := make(CompiledIgnoreRules, len(rules))
+ for _, rule := range rules {
+ sr, err := NewSelectorRegex(rule.Selector)
+ if err != nil {
+ return nil, fmt.Errorf("failed to create ignore rule selector: %w", err)
+ }
+ compiled[sr] = rule.Paths
+ }
+ return compiled, nil
+}
+
// UnstructuredList runs a dry-run patch for a list of Kubernetes resources
// against a Kubernetes cluster and compares the result against the original
// objects. It returns a DiffSet, which contains differences between the
@@ -59,13 +76,9 @@ func UnstructuredList(ctx context.Context, c client.Client, objs []*unstructured
o := &ListOptions{}
o.ApplyOptions(opts)
- var sm = make(map[*SelectorRegex][]string, len(o.IgnoreRules))
- for _, ips := range o.IgnoreRules {
- sr, err := NewSelectorRegex(ips.Selector)
- if err != nil {
- return nil, fmt.Errorf("failed to create ignore rule selector: %w", err)
- }
- sm[sr] = ips.Paths
+ sm, err := CompileIgnoreRules(o.IgnoreRules)
+ if err != nil {
+ return nil, err
}
var resOpts []ResourceOption
diff --git a/ssa/manager_apply.go b/ssa/manager_apply.go
index 88bac2e..a6e111d 100644
--- a/ssa/manager_apply.go
+++ b/ssa/manager_apply.go
@@ -105,10 +105,6 @@ type ApplyCleanupOptions struct {
Exclusions map[string]string `json:"exclusions"`
}
-// compiledIgnoreRules is a map of pre-compiled selectors to their associated
-// JSON pointer paths, used to avoid recompiling selectors for each object.
-type compiledIgnoreRules map[*jsondiff.SelectorRegex][]string
-
// DefaultApplyOptions returns the default apply options where force apply is disabled.
func DefaultApplyOptions() ApplyOptions {
return ApplyOptions{
@@ -161,9 +157,9 @@ func (m *ResourceManager) Apply(ctx context.Context, object *unstructured.Unstru
patched = patched || patchedCleanupMetadata
// Compile ignore rules once for both drift detection and conditional field stripping.
- var compiled compiledIgnoreRules
+ var compiled jsondiff.CompiledIgnoreRules
if existingObject.GetResourceVersion() != "" && len(opts.DriftIgnoreRules) > 0 {
- compiled, err = compileIgnoreRules(opts.DriftIgnoreRules)
+ compiled, err = jsondiff.CompileIgnoreRules(opts.DriftIgnoreRules)
if err != nil {
return nil, err
}
@@ -219,10 +215,10 @@ func (m *ResourceManager) ApplyAll(ctx context.Context, objects []*unstructured.
driftedIgnorePaths := make([]jsondiff.IgnorePaths, len(objects))
// Compile ignore rules once for drift detection and conditional field stripping.
- var compiled compiledIgnoreRules
+ var compiled jsondiff.CompiledIgnoreRules
if len(opts.DriftIgnoreRules) > 0 {
var err error
- compiled, err = compileIgnoreRules(opts.DriftIgnoreRules)
+ compiled, err = jsondiff.CompileIgnoreRules(opts.DriftIgnoreRules)
if err != nil {
return nil, err
}
@@ -565,24 +561,10 @@ func (m *ResourceManager) shouldSkipApply(desiredObject *unstructured.Unstructur
return false, nil
}
-// compileIgnoreRules compiles the selectors from the given ignore rules into
-// regular expressions. The compiled rules can be reused across multiple objects.
-func compileIgnoreRules(rules []jsondiff.IgnoreRule) (compiledIgnoreRules, error) {
- sm := make(compiledIgnoreRules, len(rules))
- for _, rule := range rules {
- sr, err := jsondiff.NewSelectorRegex(rule.Selector)
- if err != nil {
- return nil, fmt.Errorf("failed to create ignore rule selector: %w", err)
- }
- sm[sr] = rule.Paths
- }
- return sm, nil
-}
-
// removeIgnoredFields removes the fields matched by the given pre-compiled
// ignore rules from obj. Selectors are evaluated against matchObj so that
// existing and dry-run copies are stripped based on the same decision.
-func removeIgnoredFields(matchObj, obj *unstructured.Unstructured, rules compiledIgnoreRules) error {
+func removeIgnoredFields(matchObj, obj *unstructured.Unstructured, rules jsondiff.CompiledIgnoreRules) error {
var ignorePaths jsondiff.IgnorePaths
for sr, paths := range rules {
if sr.MatchUnstructured(matchObj) {
@@ -620,7 +602,7 @@ func lookupJSONPointer(obj *unstructured.Unstructured, pointer string) (any, boo
// between existingObject and dryRunObject.
func computeDriftedPaths(
existingObject, dryRunObject *unstructured.Unstructured,
- rules compiledIgnoreRules,
+ rules jsondiff.CompiledIgnoreRules,
) jsondiff.IgnorePaths {
var drifted jsondiff.IgnorePaths
for sr, paths := range rules {
@@ -644,7 +626,7 @@ func computeDriftedPaths(
// on the same decision, matching computeDriftedPaths.
func (m *ResourceManager) hasDriftedWithIgnore(
existingObject, dryRunObject *unstructured.Unstructured,
- compiled compiledIgnoreRules,
+ compiled jsondiff.CompiledIgnoreRules,
) (bool, error) {
if compiled == nil {
return m.hasDrifted(existingObject, dryRunObject), nil |
What if I use
Click to see tests// TestApply_DriftIgnoreRules_SameFieldManagerDrift covers the scenario
// where a user runs `kubectl apply --server-side
// --field-manager=<flux-field-manager> --force-conflicts` to drift an
// ignored field. Because the same field manager is reused, Flux remains
// the sole owner — there is no "other manager" claiming the field.
//
// Existing ignore-rule tests only cover the case where a different manager
// owns the drifted field (so releasing ownership hands it over). The sole-
// owner variant is uncovered, and the three subtests below capture the
// expected behavior:
//
// - When the only drift is on the ignored field, Apply is a no-op
// (UnchangedAction) and the drifted value stays in place.
// - When a non-ignored field also drifts and the ignored field is
// required, Flux must correct the drift to its desired value rather
// than strip the field — stripping makes the API server reject the
// apply and the reconciliation gets stuck.
// - When a non-ignored field also drifts and the ignored field has an
// API-server default, Flux must correct the drift to its desired value
// rather than strip the field — stripping lets the API server fill the
// default, silently "correcting" the drift to the wrong value.
func TestApply_DriftIgnoreRules_SameFieldManagerDrift(t *testing.T) {
timeout := 30 * time.Second
ctx, cancel := context.WithTimeout(context.Background(), timeout)
defer cancel()
id := generateName("drift-same-fm")
objects, err := readManifest("testdata/test2.yaml", id)
if err != nil {
t.Fatal(err)
}
manager.SetOwnerLabels(objects, "app1", "default")
if err := normalize.UnstructuredList(objects); err != nil {
t.Fatal(err)
}
_, deployObject := getFirstObject(objects, "Deployment", id)
// Ignored path covers a required + mutable field of Deployment.
opts := DefaultApplyOptions()
opts.DriftIgnoreRules = []jsondiff.IgnoreRule{
{
Paths: []string{"/spec/template/spec/containers"},
Selector: &jsondiff.Selector{
Kind: "Deployment",
},
},
}
// Initial create.
if _, err := manager.ApplyAllStaged(ctx, objects, opts); err != nil {
t.Fatal(err)
}
// Drift the ignored field via SSA reusing Flux's field manager. Reuse
// the manifest copy and mutate only the image so all other container
// subfields stay claimed by the same manager (avoiding incidental drift
// from released subfields). Equivalent to:
// kubectl apply --server-side \
// --field-manager=resource-manager --force-conflicts -f drifted.yaml
drifted := deployObject.DeepCopy()
driftedContainers, _, _ := unstructured.NestedSlice(drifted.Object, "spec", "template", "spec", "containers")
driftedContainers[0].(map[string]interface{})["image"] = "ghcr.io/stefanprodan/podinfo:6.1.0"
if err := unstructured.SetNestedSlice(drifted.Object, driftedContainers, "spec", "template", "spec", "containers"); err != nil {
t.Fatal(err)
}
if err := manager.client.Patch(ctx, drifted, client.Apply,
client.FieldOwner(manager.owner.Field), client.ForceOwnership); err != nil {
t.Fatalf("same-field-manager SSA failed: %v", err)
}
t.Run("no drift on non-ignored fields", func(t *testing.T) {
entry, err := manager.Apply(ctx, deployObject, opts)
if err != nil {
t.Fatalf("expected no-op apply, got error: %v", err)
}
if entry.Action != UnchangedAction {
t.Errorf("expected UnchangedAction (no-op), got %s", entry.Action)
}
// Drifted value is preserved in-cluster.
existing := deployObject.DeepCopy()
if err := manager.client.Get(ctx, client.ObjectKeyFromObject(existing), existing); err != nil {
t.Fatal(err)
}
containers, _, _ := unstructured.NestedSlice(existing.Object, "spec", "template", "spec", "containers")
if len(containers) == 0 {
t.Fatal("expected containers to still exist")
}
if got := containers[0].(map[string]interface{})["image"]; got != "ghcr.io/stefanprodan/podinfo:6.1.0" {
t.Errorf("expected drifted image to be preserved, got %v", got)
}
})
t.Run("drift on non-ignored fields + the drifted ignored field is required", func(t *testing.T) {
// Expected behavior: when a non-ignored field also drifts and Flux
// is the sole owner of the ignored field, the apply should keep the
// ignored field in the payload (i.e. correct the drift to Flux's
// desired value) instead of stripping ownership. Stripping leaves
// the kustomization stuck on a required field, which is worse than
// reverting the manual change.
if err := unstructured.SetNestedField(deployObject.Object, int64(9), "spec", "minReadySeconds"); err != nil {
t.Fatal(err)
}
entry, err := manager.Apply(ctx, deployObject, opts)
if err != nil {
t.Fatalf("expected apply to succeed by correcting the drift, got error: %v", err)
}
if entry.Action != ConfiguredAction {
t.Errorf("expected ConfiguredAction, got %s", entry.Action)
}
existing := deployObject.DeepCopy()
if err := manager.client.Get(ctx, client.ObjectKeyFromObject(existing), existing); err != nil {
t.Fatal(err)
}
containers, _, _ := unstructured.NestedSlice(existing.Object, "spec", "template", "spec", "containers")
if len(containers) == 0 {
t.Fatal("expected containers to still exist")
}
if got := containers[0].(map[string]interface{})["image"]; got != "ghcr.io/stefanprodan/podinfo:6.0.0" {
t.Errorf("expected drift to be corrected to Flux's desired image 6.0.0, got %v", got)
}
})
t.Run("drift on non-ignored fields + the drifted ignored field has a default", func(t *testing.T) {
// Expected behavior: ignored path /spec/replicas has an API-server
// default of 1. With same-field-manager drift on replicas, Flux
// remains sole owner. When a non-ignored field also drifts, the
// apply should keep replicas in the payload and restore Flux's
// desired value (2). Stripping replicas would let the API server
// fill the default (1) — silently "correcting" the drift to the
// wrong value, with no error and no audit trail.
id := generateName("drift-default")
objects, err := readManifest("testdata/test2.yaml", id)
if err != nil {
t.Fatal(err)
}
manager.SetOwnerLabels(objects, "app1", "default")
if err := normalize.UnstructuredList(objects); err != nil {
t.Fatal(err)
}
_, deploy := getFirstObject(objects, "Deployment", id)
replicasOpts := DefaultApplyOptions()
replicasOpts.DriftIgnoreRules = []jsondiff.IgnoreRule{{
Paths: []string{"/spec/replicas"},
Selector: &jsondiff.Selector{Kind: "Deployment"},
}}
// Create with replicas explicitly set so Flux owns it.
if err := unstructured.SetNestedField(deploy.Object, int64(2), "spec", "replicas"); err != nil {
t.Fatal(err)
}
if _, err := manager.ApplyAllStaged(ctx, objects, replicasOpts); err != nil {
t.Fatal(err)
}
// Drift replicas via same field manager — Flux stays the sole owner.
drifted := deploy.DeepCopy()
if err := unstructured.SetNestedField(drifted.Object, int64(5), "spec", "replicas"); err != nil {
t.Fatal(err)
}
if err := manager.client.Patch(ctx, drifted, client.Apply,
client.FieldOwner(manager.owner.Field), client.ForceOwnership); err != nil {
t.Fatalf("same-field-manager SSA failed: %v", err)
}
// Force a non-ignored change to trigger an apply.
if err := unstructured.SetNestedField(deploy.Object, int64(9), "spec", "minReadySeconds"); err != nil {
t.Fatal(err)
}
entry, err := manager.Apply(ctx, deploy, replicasOpts)
if err != nil {
t.Fatalf("expected apply to succeed by correcting the drift, got error: %v", err)
}
if entry.Action != ConfiguredAction {
t.Errorf("expected ConfiguredAction, got %s", entry.Action)
}
existing := deploy.DeepCopy()
if err := manager.client.Get(ctx, client.ObjectKeyFromObject(existing), existing); err != nil {
t.Fatal(err)
}
replicas, found, _ := unstructured.NestedInt64(existing.Object, "spec", "replicas")
if !found {
t.Fatal("expected spec.replicas to exist")
}
if replicas != 2 {
t.Errorf("expected spec.replicas to be corrected to Flux's desired value 2, got %d", replicas)
}
})
}The result is:
I'm wondering if we should leave it like that or if we should correct the drift on the ignored fields in this case. |
Interesting test case :) One could argue that this is a misconfiguration -- two distinct actors should never share the same field-manager name, it defeats the purpose of SSA ownership tracking. Today, the drift logic doesn't check who owns the field, if there is drift, it strips Flux's ownership preserving the existing value. We could fix the above use case by checking if Flux is the sole manager and not dropping ownership if so. To address the case described in 4 (client side edit/edits without a distinct field manager). we have two options - Option A: Always correct sole-owner drift
Option B: Current behavior — accept the inconsistency and document it
Option A seems cleaner, consistent behavior, the feature is intended to work with server side apply. Ignore rules cannot be used to manually override fields via kubectl edit. Thoughts ? |
Thanks @matheuscscp, Ok, no change in behavior, just refactor to remove 5 lines of duplicate code. The actual scenario described should be extremely rare and ok to live with given helm-controller has the same behavior. Will take this change.. |
|
Actually, correcting the drift was a bad suggestion from me, sorry. By correcting the drift I meant correcting it on the desired object to be sent for apply, and not letting the new desired value override the one currently in the cluster (what we normally call drift correction). In other words, correct the drift on the desired state to make it match the actual state, and not the other way around like we normally do. And yes, I think it's worth going down this route and looking at the By the way, here is another observation Stefan made earlier today:
The amount of entropy in Flux user setups is unbelievable... One could argue that this is a long-time bug in helm-controller, if I set multiple ignore rules with an empty selector to target everything, then only the last one will be used. But we can definitely leave this for another day, I'm planning to open an issue in this repo to track it separately. |
|
Please add this test for VPA, I think it's a very important one because:
This would make sure all the path/pointer-processing code is well exercised 🙏 Click to see tests// TestApply_DriftIgnoreRules_ContainerResources exercises the path-handling
// code with an ignore rule whose JSON pointer points to a non-leaf object
// inside a keyed list element — /spec/template/spec/containers/0/resources.
// This combination (array index + sub-object) is not covered elsewhere:
// existing tests use either a top-level leaf (/spec/replicas) or a leaf
// inside an array (/spec/template/spec/containers/0/image). The behavioral
// scenarios (release on other-manager claim, etc.) are already covered for
// those simpler paths; this test only adds the path shape that exercises
// lookupJSONPointer, GenerateRemovePatch + ApplyPatchToUnstructured, and
// hasOtherManagerForPath against an indexed sub-object claim under
// `k:{"name":...}` associative-list keys.
func TestApply_DriftIgnoreRules_ContainerResources(t *testing.T) {
timeout := 30 * time.Second
ctx, cancel := context.WithTimeout(context.Background(), timeout)
defer cancel()
id := generateName("drift-resources")
objects, err := readManifest("testdata/test2.yaml", id)
if err != nil {
t.Fatal(err)
}
manager.SetOwnerLabels(objects, "app1", "default")
if err := normalize.UnstructuredList(objects); err != nil {
t.Fatal(err)
}
_, deployObject := getFirstObject(objects, "Deployment", id)
opts := DefaultApplyOptions()
opts.DriftIgnoreRules = []jsondiff.IgnoreRule{
{
Paths: []string{"/spec/template/spec/containers/0/resources"},
Selector: &jsondiff.Selector{Kind: "Deployment"},
},
}
// vpaPayload returns a minimal SSA payload whose podinfod container has
// the given resources block. Sending only `name` + `resources` for the
// container limits the manager's claim to those fields under
// containers[k:name=podinfod] in managedFields.
vpaPayload := func(resources map[string]interface{}) *unstructured.Unstructured {
container := map[string]interface{}{"name": "podinfod"}
if resources != nil {
container["resources"] = resources
}
return &unstructured.Unstructured{Object: map[string]interface{}{
"apiVersion": "apps/v1",
"kind": "Deployment",
"metadata": map[string]interface{}{
"name": id,
"namespace": id,
},
"spec": map[string]interface{}{
"selector": map[string]interface{}{
"matchLabels": map[string]interface{}{"app": id},
},
"template": map[string]interface{}{
"metadata": map[string]interface{}{
"labels": map[string]interface{}{"app": id},
},
"spec": map[string]interface{}{
"containers": []interface{}{container},
},
},
},
}}
}
containerResources := func(obj *unstructured.Unstructured) map[string]interface{} {
t.Helper()
containers, found, err := unstructured.NestedSlice(obj.Object, "spec", "template", "spec", "containers")
if err != nil || !found || len(containers) == 0 {
t.Fatalf("expected at least one container, got found=%v err=%v", found, err)
}
c0 := containers[0].(map[string]interface{})
res, _ := c0["resources"].(map[string]interface{})
return res
}
// Initial create with Flux owning containers[0].resources from test2.yaml.
if _, err := manager.ApplyAllStaged(ctx, objects, opts); err != nil {
t.Fatal(err)
}
// VPA force-claims containers[0].resources with different values. After
// this, Flux co-owns or has been displaced; either way Flux must
// release on the next reconcile because the path is in the ignore set.
vpaResources := map[string]interface{}{
"limits": map[string]interface{}{"cpu": "4", "memory": "1Gi"},
"requests": map[string]interface{}{"cpu": "2", "memory": "256Mi"},
}
if err := manager.client.Patch(ctx, vpaPayload(vpaResources), client.Apply,
client.FieldOwner("vpa-controller"), client.ForceOwnership); err != nil {
t.Fatalf("VPA force-claim failed: %v", err)
}
// Trigger a non-ignored drift so the apply path runs (and the strip
// logic exercises GenerateRemovePatch + ApplyPatchToUnstructured for
// the indexed sub-object pointer). hasOtherManagerForPath must walk
// vpa-controller's managedFields entry across the
// `k:{"name":"podinfod"}` associative key to detect the claim.
if err := unstructured.SetNestedField(deployObject.Object, int64(7), "spec", "minReadySeconds"); err != nil {
t.Fatal(err)
}
entry, err := manager.Apply(ctx, deployObject, opts)
if err != nil {
t.Fatalf("apply failed: %v", err)
}
if entry.Action != ConfiguredAction {
t.Errorf("expected ConfiguredAction, got %s", entry.Action)
}
existing := deployObject.DeepCopy()
if err := manager.client.Get(ctx, client.ObjectKeyFromObject(existing), existing); err != nil {
t.Fatal(err)
}
// VPA's resource sub-object must be preserved end-to-end.
res := containerResources(existing)
if res == nil {
t.Fatal("expected containers[0].resources to be set")
}
limits, _ := res["limits"].(map[string]interface{})
if limits["cpu"] != "4" || limits["memory"] != "1Gi" {
t.Errorf("expected VPA limits cpu=4 mem=1Gi to be preserved, got %v", limits)
}
requests, _ := res["requests"].(map[string]interface{})
if requests["cpu"] != "2" || requests["memory"] != "256Mi" {
t.Errorf("expected VPA requests cpu=2 mem=256Mi to be preserved, got %v", requests)
}
// Flux must no longer own resources under containers[k:name=podinfod].
for _, mf := range existing.GetManagedFields() {
if mf.Manager != manager.owner.Field || mf.Operation != metav1.ManagedFieldsOperationApply || mf.FieldsV1 == nil {
continue
}
if strings.Contains(string(mf.FieldsV1.Raw), "\"f:resources\"") {
t.Errorf("expected Flux to release containers[0].resources to vpa-controller")
}
}
} |
@matheuscscp, Parsing the managed field is introducing fair bit of complexity, example -- fieldsV1 uses k:{"name":"value"} while JSON pointers use numeric indices. The merge key varies by list type and isn't available without the OpenAPI schema. We could go with Option B. from this comment for 2.9 release, addressing the primary use-case for server-side apply based controllers (VPA, HPA, cert-manager). I am not opposed to making the change, but we could do it based on user interest/feedback in 2.10 rather than introducing additional complexity and error paths. Thoughts ? |
|
I think modifying the behavior once the feature is released is one of:
I think we should aim for the right thing from the start, this is an opensource project with 3 releases per year and we have enough time until 2.9 |
dipti-pai
force-pushed
the
ssa-ignore-rules
branch
from
e0775d7 to
68384c6
Compare
|
Thanks @matheuscscp @stefanprodan for the reviews!
Added logic to parse managed fields and adopt the value when Flux is sole owner OR when there is no owner. Added a real-world test-case for VPA to cover JSON pointer parsing.
Added this scenario as a test case. Flux adopts the in-cluster value because after cleanup, no other manager owned the field. |
…pply Signed-off-by: Dipti Pai <diptipai89@outlook.com>
dipti-pai
force-pushed
the
ssa-ignore-rules
branch
from
68384c6 to
bdf2d87
Compare
3 participants
Changes include
[]jsondiff.IgnoreRuletossaApplyOptionscreatecode paths. InApplyandApplyAllcode paths that correspond toupdate, remove ignored fields matching the ignored rules from the object.Includes tests for various scenarios for optional and mandatory fields, claiming shared/forceful ownership and orphaning owned fields without a manager.
Associated kustomize controller draft PR testing the changes from controller perspective - fluxcd/kustomize-controller#1627
Fixes: #696