Skip to content

feat: new jqPaths ignore differences field for dynamic field selection (#24599) #24658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
puretension wants to merge 6 commits into
base: master
Choose a base branch
from
Open

feat: new jqPaths ignore differences field for dynamic field selection (#24599) #24658

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b5b6a0d
fix: support jqPathExpressions that return multiple fields (#24599)
puretension Sep 19, 2025
0711ef1
fix: replace interface{} with any and fix formatting
puretension Sep 19, 2025
60472ed
fix: apply gofumpt formatting
puretension Sep 19, 2025
acc92fc
chore: trigger CI rebuild for formatting check
puretension Sep 19, 2025
7798860
fix: resolve gofumpt formatting issues
puretension Sep 19, 2025
310bf14
Implement jqPaths field using delpaths() instead of del()
puretension Sep 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix: support jqPathExpressions that return multiple fields (#24599) 
- Add detection for multi-value jq expressions in ignoreApplicationDifferences
- Implement jqMultiPathNormalizerPatch for handling annotation key selections
- Fix issue where expressions like '.metadata.annotations | keys[] | select(startswith(...))' failed
- Add comprehensive test coverage for multiple field scenarios

Fixes #24599

Signed-off-by: puretension <[email protected]>
  • Loading branch information
@puretension
puretension committed Sep 19, 2025
commit b5b6a0d8d93f0386e2cae590905907f9cd178bf5
151 changes: 135 additions & 16 deletions util/argo/normalizers/diff_normalizer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@
jqExecutionTimeout time.Duration
}

type jqMultiPathNormalizerPatch struct {
baseNormalizerPatch
pathExpression string
jqExecutionTimeout time.Duration
}

func (np *jqNormalizerPatch) Apply(data []byte) ([]byte, error) {
dataJSON := make(map[string]any)
err := json.Unmarshal(data, &dataJSON)
Expand Down Expand Up @@ -102,6 +108,102 @@
return patchedData, err
}

func (np *jqMultiPathNormalizerPatch) Apply(data []byte) ([]byte, error) {
dataJSON := make(map[string]any)
err := json.Unmarshal(data, &dataJSON)
if err != nil {
return nil, err
}

ctx, cancel := context.WithTimeout(context.Background(), np.jqExecutionTimeout)
defer cancel()

// First, evaluate the path expression to get the paths to delete
pathQuery, err := gojq.Parse(np.pathExpression)
if err != nil {
return nil, fmt.Errorf("failed to parse path expression: %w", err)
}
pathCode, err := gojq.Compile(pathQuery)
if err != nil {
return nil, fmt.Errorf("failed to compile path expression: %w", err)
}

// Collect all paths that match the expression
var pathsToDelete []string
iter := pathCode.RunWithContext(ctx, dataJSON)
for {
v, ok := iter.Next()
if !ok {
break
}
if err, ok := v.(error); ok {
if errors.Is(err, context.DeadlineExceeded) {
return nil, fmt.Errorf("JQ path evaluation timed out (%v)", np.jqExecutionTimeout.String())
}
// If the path expression fails (e.g., field doesn't exist), just continue
continue
}
if pathStr, ok := v.(string); ok {
pathsToDelete = append(pathsToDelete, pathStr)
}
Comment on lines +152 to +154
Copy link
Member

@crenshaw-dev crenshaw-dev Sep 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think trying to infer the user's intent from the output of their expression is going to be too error-prone.

Consider this input:

{"a": 1, "b": "a", "c": [["a"]]}

Suppose we decide that, if the user's query evaluates to a string, we're going to treat it as a path.

If the user passes this query:

.b

then we'll receive the value "a", assume it's a path, and delete the "a" key from the input. But that's unintuitive. The user obviously wanted to delete the "b" key.

We could be more strict and say that the user must produce a valid delpaths() input, i.e. a [][]string.

But if the input object actually contains a [][]string, we have no way of knowing whether the user is trying to pass us paths or has directly targeted something they want to delete:

.c

Output:

[["a"]]

Did the user mean to delete the .c from the object, or .a?

tl;dr, I don't think we should reuse the jqPathExpressions field. I think we should create a new jqPaths field. It should be an array of queries that produce jq paths:

each path is an array of strings and numbers

So the solution to the user's problem would be something like:

jqPaths:
- '.metadata?.annotations // {} | [keys | map(select(startswith("a")))]'

Then we should use delpaths() instead of del() to complete the mutation.

Copy link
Contributor Author

@puretension puretension Sep 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crenshaw-dev Thanks for the sharp analysis!
the examples with .b and .c really highlight how ambiguous and error-prone the current approach would be.
I completely agree with your reasoning. I’ll follow your suggestion and introduce a new jqPaths field (array of jq path queries), and update the implementation to use delpaths() instead of del().

I’ll update the schema, implementation, tests, and docs accordingly. 🙏

Copy link
Member

@crenshaw-dev crenshaw-dev Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added an item to the next Contributors' meeting to discuss w/ other maintainers. I'm always hesitant to add new features, so I want to make sure everyone feels its worth it.

Copy link
Contributor Author

@puretension puretension Sep 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bringing this to the Contributors' meeting!
I've implemented the jqPaths approach you suggested.
Happy to push it for reference during the discussion! we can always adjust based on the team's feedback.

Looking forward to hearing everyone's thoughts! 🙏

}

// If no paths to delete, return original data
if len(pathsToDelete) == 0 {
return data, nil
}

// For annotation-based expressions, we need to handle them specially
// Check if this is an annotation key selection expression
if strings.Contains(np.pathExpression, ".metadata.annotations") && strings.Contains(np.pathExpression, "keys[]") {
// This is selecting annotation keys, so we need to delete those specific annotations
result := dataJSON
if metadata, ok := result["metadata"].(map[string]interface{}); ok {

Check failure on line 161 in util/argo/normalizers/diff_normalizer.go

View workflow job for this annotation

GitHub Actions / Lint Go code 

use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)
if annotations, ok := metadata["annotations"].(map[string]interface{}); ok {

Check failure on line 162 in util/argo/normalizers/diff_normalizer.go

View workflow job for this annotation

GitHub Actions / Lint Go code 

use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)
for _, key := range pathsToDelete {
delete(annotations, key)
}
// If annotations is now empty, we can remove it entirely
if len(annotations) == 0 {
delete(metadata, "annotations")
}
}
}

Check failure on line 172 in util/argo/normalizers/diff_normalizer.go

View workflow job for this annotation

GitHub Actions / Lint Go code 

File is not properly formatted (gofumpt)
patchedData, err := json.Marshal(result)
if err != nil {
return nil, err
}
return patchedData, nil
}

// For other types of expressions, try to delete each path individually
result := dataJSON
for _, path := range pathsToDelete {
deletionQuery, parseErr := gojq.Parse(fmt.Sprintf("del(.%s)", path))
if parseErr != nil {
continue // Skip invalid paths
}
deletionCode, compileErr := gojq.Compile(deletionQuery)
if compileErr != nil {
continue // Skip invalid paths
}

iter := deletionCode.RunWithContext(ctx, result)
if v, ok := iter.Next(); ok {
if _, isErr := v.(error); !isErr {
result = v.(map[string]interface{})

Check failure on line 195 in util/argo/normalizers/diff_normalizer.go

View workflow job for this annotation

GitHub Actions / Lint Go code 

use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)
}
}
}

patchedData, err := json.Marshal(result)
if err != nil {
return nil, err
}
return patchedData, nil
}

type ignoreNormalizer struct {
patches []normalizerPatch
}
Expand Down Expand Up @@ -159,23 +261,40 @@
})
}
for _, pathExpression := range ignore[i].JQPathExpressions {
jqDeletionQuery, err := gojq.Parse(fmt.Sprintf("del(%s)", pathExpression))
if err != nil {
return nil, err
}
jqDeletionCode, err := gojq.Compile(jqDeletionQuery)
if err != nil {
return nil, err
// For expressions that select multiple annotation keys, we need special handling
if strings.Contains(pathExpression, ".metadata.annotations") &&
strings.Contains(pathExpression, "keys[]") &&
strings.Contains(pathExpression, "select") {
// This is likely selecting multiple annotation keys
patches = append(patches, &jqMultiPathNormalizerPatch{
baseNormalizerPatch: baseNormalizerPatch{
groupKind: schema.GroupKind{Group: ignore[i].Group, Kind: ignore[i].Kind},
name: ignore[i].Name,
namespace: ignore[i].Namespace,
},
pathExpression: pathExpression,
jqExecutionTimeout: opts.getJQExecutionTimeout(),
})
} else {
// Standard single-path deletion
jqDeletionQuery, err := gojq.Parse(fmt.Sprintf("del(%s)", pathExpression))
if err != nil {
return nil, err
}
jqDeletionCode, err := gojq.Compile(jqDeletionQuery)
if err != nil {
return nil, err
}
patches = append(patches, &jqNormalizerPatch{
baseNormalizerPatch: baseNormalizerPatch{
groupKind: schema.GroupKind{Group: ignore[i].Group, Kind: ignore[i].Kind},
name: ignore[i].Name,
namespace: ignore[i].Namespace,
},
code: jqDeletionCode,
jqExecutionTimeout: opts.getJQExecutionTimeout(),
})
}
patches = append(patches, &jqNormalizerPatch{
baseNormalizerPatch: baseNormalizerPatch{
groupKind: schema.GroupKind{Group: ignore[i].Group, Kind: ignore[i].Kind},
name: ignore[i].Name,
namespace: ignore[i].Namespace,
},
code: jqDeletionCode,
jqExecutionTimeout: opts.getJQExecutionTimeout(),
})
}
}
return &ignoreNormalizer{patches: patches}, nil
Expand Down
101 changes: 101 additions & 0 deletions util/argo/normalizers/diff_normalizer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,3 +293,104 @@
})
assert.Contains(t, out, "fromjson cannot be applied")
}

func TestNormalizeJQPathExpressionMultipleFields(t *testing.T) {
// Test case for issue #24599: jqPathExpressions should work for matching multiple fields
normalizer, err := NewIgnoreNormalizer([]v1alpha1.ResourceIgnoreDifferences{{
Group: "apps",
Kind: "Deployment",
JQPathExpressions: []string{".metadata.annotations? // {} | keys[] | select ( . | startswith(\"customprefix.\"))"},
}}, make(map[string]v1alpha1.ResourceOverride), IgnoreNormalizerOpts{})

require.NoError(t, err)

deployment := test.NewDeployment()

// Add annotations with custom prefix and other annotations
annotations := map[string]interface{}{

Check failure on line 310 in util/argo/normalizers/diff_normalizer_test.go

View workflow job for this annotation

GitHub Actions / Lint Go code 

use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)
"customprefix.foo": "value1",
"customprefix.bar": "value2",

Check failure on line 312 in util/argo/normalizers/diff_normalizer_test.go

View workflow job for this annotation

GitHub Actions / Lint Go code 

File is not properly formatted (gofumpt)
"other.annotation": "value3",
"another.annotation": "value4",
}
err = unstructured.SetNestedMap(deployment.Object, annotations, "metadata", "annotations")
require.NoError(t, err)

// Verify annotations exist before normalization
actualAnnotations, has, err := unstructured.NestedMap(deployment.Object, "metadata", "annotations")
require.NoError(t, err)
assert.True(t, has)
assert.Len(t, actualAnnotations, 4)
assert.Contains(t, actualAnnotations, "customprefix.foo")
assert.Contains(t, actualAnnotations, "customprefix.bar")
assert.Contains(t, actualAnnotations, "other.annotation")

// Apply normalization - should now work with the fix
err = normalizer.Normalize(deployment)
require.NoError(t, err)

// Verify that only customprefix annotations are removed
actualAnnotations, has, err = unstructured.NestedMap(deployment.Object, "metadata", "annotations")
require.NoError(t, err)
assert.True(t, has)
assert.Len(t, actualAnnotations, 2) // Only non-customprefix annotations should remain
assert.NotContains(t, actualAnnotations, "customprefix.foo")
assert.NotContains(t, actualAnnotations, "customprefix.bar")
assert.Contains(t, actualAnnotations, "other.annotation")
assert.Contains(t, actualAnnotations, "another.annotation")
}

func TestNormalizeJQPathExpressionMultipleFieldsNoMatch(t *testing.T) {
// Test case where no annotations match the prefix
normalizer, err := NewIgnoreNormalizer([]v1alpha1.ResourceIgnoreDifferences{{
Group: "apps",
Kind: "Deployment",
JQPathExpressions: []string{".metadata.annotations? // {} | keys[] | select ( . | startswith(\"nonexistent.\"))"},
}}, make(map[string]v1alpha1.ResourceOverride), IgnoreNormalizerOpts{})

require.NoError(t, err)

deployment := test.NewDeployment()

// Add annotations without the target prefix
annotations := map[string]interface{}{

Check failure on line 356 in util/argo/normalizers/diff_normalizer_test.go

View workflow job for this annotation

GitHub Actions / Lint Go code 

use-any: since Go 1.18 'interface{}' can be replaced by 'any' (revive)
"customprefix.foo": "value1",
"other.annotation": "value3",
}
err = unstructured.SetNestedMap(deployment.Object, annotations, "metadata", "annotations")
require.NoError(t, err)

// Apply normalization
err = normalizer.Normalize(deployment)
require.NoError(t, err)

// Verify that no annotations are removed since none match
actualAnnotations, has, err := unstructured.NestedMap(deployment.Object, "metadata", "annotations")
require.NoError(t, err)
assert.True(t, has)
assert.Len(t, actualAnnotations, 2) // All annotations should remain
assert.Contains(t, actualAnnotations, "customprefix.foo")
assert.Contains(t, actualAnnotations, "other.annotation")
}

func TestNormalizeJQPathExpressionMultipleFieldsNoAnnotations(t *testing.T) {
// Test case where there are no annotations at all
normalizer, err := NewIgnoreNormalizer([]v1alpha1.ResourceIgnoreDifferences{{
Group: "apps",
Kind: "Deployment",
JQPathExpressions: []string{".metadata.annotations? // {} | keys[] | select ( . | startswith(\"customprefix.\"))"},
}}, make(map[string]v1alpha1.ResourceOverride), IgnoreNormalizerOpts{})

require.NoError(t, err)

deployment := test.NewDeployment()

// Apply normalization without any annotations
err = normalizer.Normalize(deployment)
require.NoError(t, err)

// Verify that no annotations field is created
_, has, err := unstructured.NestedMap(deployment.Object, "metadata", "annotations")
require.NoError(t, err)
assert.False(t, has) // No annotations should exist
}
Loading