fix(kubeapifilter): add missing logic for omitManagedFields#3283
Open
jcantrill wants to merge 1 commit into
Open
fix(kubeapifilter): add missing logic for omitManagedFields#3283jcantrill wants to merge 1 commit into
jcantrill wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds OmitManagedFields support to the KubeAPIAudit VRL template and tests. The template emits an ChangesOmitManagedFields filtering support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcantrill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/generator/vector/filter/apiaudit/policy.vrl.tmpl`:
- Around line 33-35: The template is emitting an internal flag into the event
payload via `.omit_managed_fields = {{.OmitManagedFields}}`; instead, capture
the input flag into a local template variable (e.g. `{{- $omitManaged :=
.OmitManagedFields}}`) and use that local variable for any conditional logic,
then remove the `.omit_managed_fields = ...` assignment so the internal control
flag is not written to the output event; update any references in this template
that used `.OmitManagedFields` to use the new local `$omitManaged` variable
instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 25a4ed72-e58e-4f7e-b665-7241771baf5d
📒 Files selected for processing (2)
internal/generator/vector/filter/apiaudit/filter_test.gointernal/generator/vector/filter/apiaudit/policy.vrl.tmpl
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
jcantrill
force-pushed
the
log9388_managed_fields
branch
from
aec2599 to
4d7f502
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/filters/apiaudit/vrl/filter_test.go`:
- Around line 240-350: The test assigns v1.PartialObjectMetadata directly to
Event.RequestObject/ResponseObject but those fields are *runtime.Unknown after
Filtered() marshaling; change createEventWithManagedFields to set RequestObject
and ResponseObject to &runtime.Unknown containing the JSON-marshaled
PartialObjectMetadata (set ContentType to "application/json"), and update
assertions in the three its to first type-assert to *runtime.Unknown (from
filtered.RequestObject/ResponseObject), then json.Unmarshal respUnknown.Raw into
a v1.PartialObjectMetadata to inspect ObjectMeta.ManagedFields; reference
symbols: createEventWithManagedFields, Event.RequestObject,
Event.ResponseObject, Filtered(), and v1.PartialObjectMetadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2f37ae64-c6f7-4925-bfcd-edf16ad45c4c
📒 Files selected for processing (3)
internal/generator/vector/filter/apiaudit/filter_test.gointernal/generator/vector/filter/apiaudit/policy.vrl.tmpltest/functional/filters/apiaudit/vrl/filter_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/generator/vector/filter/apiaudit/policy.vrl.tmpl
jcantrill
force-pushed
the
log9388_managed_fields
branch
from
4d7f502 to
2ab0334
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/filters/apiaudit/vrl/filter_test.go`:
- Around line 268-269: The test currently ignores errors from json.Marshal
(e.g., the reqJSON, respJSON assignments), which can mask setup failures; update
each json.Marshal call in filter_test.go to check the returned error and fail
the test immediately (use t.Fatalf or require.NoError) when marshal
fails—specifically handle the errors for reqJSON and respJSON and the other
marshal/unmarshal occurrences noted (around the other
json.Marshal/json.Unmarshal calls) so failures surface clearly during test
setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a4b7c3e3-682e-4fb5-bd4a-4a274e9cc8fc
📒 Files selected for processing (4)
internal/generator/vector/api/sinks/prometheus_export.gointernal/generator/vector/filter/apiaudit/filter_test.gointernal/generator/vector/filter/apiaudit/policy.vrl.tmpltest/functional/filters/apiaudit/vrl/filter_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/generator/vector/api/sinks/prometheus_export.go
Comment on lines
+268
to
+269
| reqJSON, _ := json.Marshal(reqObj) | ||
| respJSON, _ := json.Marshal(respObj) |
There was a problem hiding this comment.
Handle JSON errors explicitly in test helpers.
Ignoring marshal/unmarshal errors here can hide setup failures and produce less actionable test failures.
Suggested patch
- reqJSON, _ := json.Marshal(reqObj)
- respJSON, _ := json.Marshal(respObj)
+ reqJSON, err := json.Marshal(reqObj)
+ Expect(err).NotTo(HaveOccurred())
+ respJSON, err := json.Marshal(respObj)
+ Expect(err).NotTo(HaveOccurred())
@@
- eventJSON, _ := json.Marshal(event)
+ eventJSON, err := json.Marshal(event)
+ Expect(err).NotTo(HaveOccurred())
@@
- _ = json.Unmarshal(eventJSON, &eventMap)
+ err = json.Unmarshal(eventJSON, &eventMap)
+ Expect(err).NotTo(HaveOccurred())
@@
- modifiedJSON, _ := json.Marshal(eventMap)
+ modifiedJSON, err := json.Marshal(eventMap)
+ Expect(err).NotTo(HaveOccurred())
return modifiedJSONAlso applies to: 370-370, 374-374, 383-383
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/functional/filters/apiaudit/vrl/filter_test.go` around lines 268 - 269,
The test currently ignores errors from json.Marshal (e.g., the reqJSON, respJSON
assignments), which can mask setup failures; update each json.Marshal call in
filter_test.go to check the returned error and fail the test immediately (use
t.Fatalf or require.NoError) when marshal fails—specifically handle the errors
for reqJSON and respJSON and the other marshal/unmarshal occurrences noted
(around the other json.Marshal/json.Unmarshal calls) so failures surface clearly
during test setup.
Contributor
Author
|
/retest |
Contributor
|
@jcantrill: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
LOG-9388
Description
This PR:
Links
cc @vparfonov @Clee2691
Summary by CodeRabbit
New Features
OmitManagedFieldsoption for audit policies (defaults false); when enabled, managed fields are removed from root metadata as well as request and response object metadata in audit events.Tests
OmitManagedFieldsset true, false, and unset, ensuring correct removal or preservation of managed fields.