OCPBUGS-62730 - Fixed typos and truncated field values in JSON audit logs generated by the JSON enricher#3071
Conversation
k8s-ci-robot
added
release-note
|
Hi @BhargaviGudi. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BhargaviGudi The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/S
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3071 +/- ##
===========================================
- Coverage 45.50% 24.69% -20.82%
===========================================
Files 79 126 +47
Lines 7782 18002 +10220
===========================================
+ Hits 3541 4445 +904
- Misses 4099 13265 +9166
- Partials 142 292 +150 🚀 New features to boost your workflow:
|
BhargaviGudi
force-pushed
the
OCPBUGS-62730-fix
branch
from
81e7e67 to
224b145
Compare
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
BhargaviGudi
force-pushed
the
OCPBUGS-62730-fix
branch
2 times, most recently
from
006a556 to
9d57d22
Compare
|
@ngopalak-redhat Could you please help me review the PR? Thanks |
|
/retitle OCPBUGS-62730: Fixed typos and truncated field values in JSON audit logs generated by the JSON enricher |
|
@BhargaviGudi: Re-titling can only be requested by trusted users, like repository collaborators. DetailsIn response to this:
Instructions 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. |
BhargaviGudi
force-pushed
the
OCPBUGS-62730-fix
branch
from
9d57d22 to
43a5e54
Compare
|
/verified |
|
/payload-job periodic-ci-openshift-openshift-tests-private-release-4.22-amd64-nightly-baremetalds-ipi-ovn-lvms-f14-security-profiles |
saschagrunert
left a comment
saschagrunert
left a comment
There was a problem hiding this comment.
Thanks for fixing this data race. The mutex approach is correct. A few suggestions below.
| logBucket.Mu.Lock() | ||
|
|
||
| if logBucket.ContainerInfo == nil { | ||
| logBucket.ContainerInfo = e.fetchContainerInfo(ctx, auditLine.ProcessID, nodeName) | ||
| } | ||
|
|
||
| logBucket.Mu.Unlock() |
There was a problem hiding this comment.
fetchContainerInfo calls into the Kubernetes API. Holding the write lock during a network call blocks dispatchSeccompLine from dispatching any logs for this bucket until the API responds.
Consider narrowing the lock to just the pointer assignment:
| logBucket.Mu.Lock() | |
| if logBucket.ContainerInfo == nil { | |
| logBucket.ContainerInfo = e.fetchContainerInfo(ctx, auditLine.ProcessID, nodeName) | |
| } | |
| logBucket.Mu.Unlock() | |
| if logBucket.ContainerInfo == nil { | |
| ci := e.fetchContainerInfo(ctx, auditLine.ProcessID, nodeName) | |
| logBucket.Mu.Lock() | |
| logBucket.ContainerInfo = ci | |
| logBucket.Mu.Unlock() | |
| } | |
| podName := logBucket.ContainerInfo.PodName | ||
| namespace := logBucket.ContainerInfo.Namespace | ||
| containerName := logBucket.ContainerInfo.ContainerName | ||
|
|
||
| resource = map[string]string{ | ||
| "pod": logBucket.ContainerInfo.PodName, | ||
| "namespace": logBucket.ContainerInfo.Namespace, | ||
| "container": logBucket.ContainerInfo.ContainerName, | ||
| "pod": podName, | ||
| "namespace": namespace, | ||
| "container": containerName, | ||
| } |
There was a problem hiding this comment.
These local variables add no safety since the RLock is already held. They make it look like the copies are the fix, when the lock is. The original direct access is clearer:
| podName := logBucket.ContainerInfo.PodName | |
| namespace := logBucket.ContainerInfo.Namespace | |
| containerName := logBucket.ContainerInfo.ContainerName | |
| resource = map[string]string{ | |
| "pod": logBucket.ContainerInfo.PodName, | |
| "namespace": logBucket.ContainerInfo.Namespace, | |
| "container": logBucket.ContainerInfo.ContainerName, | |
| "pod": podName, | |
| "namespace": namespace, | |
| "container": containerName, | |
| } | |
| resource = map[string]string{ | |
| "pod": logBucket.ContainerInfo.PodName, | |
| "namespace": logBucket.ContainerInfo.Namespace, | |
| "container": logBucket.ContainerInfo.ContainerName, | |
| } |
| return true | ||
| }) | ||
|
|
||
| // Acquire read lock to safely access ProcessInfo and ContainerInfo |
There was a problem hiding this comment.
The comment restates what the code does. logBucket.Mu.RLock() is self-explanatory.
| // Acquire read lock to safely access ProcessInfo and ContainerInfo |
| @@ -324,12 +326,18 @@ func (e *JsonEnricher) Run(ctx context.Context, runErr chan<- error) { | |||
| auditLine.Executable, uid, gid) | |||
| } | |||
|
|
|||
| logBucket.Mu.Unlock() | |||
There was a problem hiding this comment.
Same concern as the fetchContainerInfo block below: the lock wraps both the nil check and fetchProcessInfo, which reads /proc files. Consider fetching outside the lock and only locking for the pointer assignment:
| if logBucket.ProcessInfo == nil { | |
| uid, gid, err := auditsource.GetUidGid(line) | |
| if err != nil { | |
| e.logger.V(config.VerboseLevel).Info( | |
| "unable to get uid and gid", "line", line) | |
| } | |
| pi := e.fetchProcessInfo(auditLine.ProcessID, | |
| auditLine.Executable, uid, gid) | |
| logBucket.Mu.Lock() | |
| logBucket.ProcessInfo = pi | |
| logBucket.Mu.Unlock() | |
| } | |
This eliminates the lock-unlock-lock-unlock pattern in Run(), keeping locks to short pointer assignments only.
resolved format issue Updated proper synchronization for the LogBucket Resolved go-lint issues revert nodeNameCopy := nodeName
BhargaviGudi
force-pushed
the
OCPBUGS-62730-fix
branch
from
43a5e54 to
61e4e69
Compare
|
@BhargaviGudi: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
|
@BhargaviGudi do you mind fixing the CI? |
5 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes typos and truncated field values in the JSON audit logs generated by the Security Profiles Operator's JSON enricher. The issue manifested as:
Changes:
podName,namespace, andcontainerNamefromContainerInfointo local variables before assigning to the resource mapnodeNamebefore assigning to the node mapThis ensures all JSON keys and values in audit logs are correctly spelled and complete.
Which issue(s) this PR fixes:
Fixes OCPBUGS-62730
Does this PR have test?
Tested manually to verify the values
test result:
With this fix, all field names and values are now complete and correctly spelled.
Special notes for your reviewer:
The fix is minimal and surgical - by copying string values to local variables before map assignment, we prevent the memory corruption that was causing character truncation and field name typos. This is a critical fix for audit log reliability and automated log processing.
Does this PR introduce a user-facing change?