Fix exec to pod has no workloadUID and update packages #357
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughGetControllerDetails now returns the Pod and workload UID in addition to prior workload metadata; helper signatures for resolving owners were expanded to include UIDs; GetContainerID was added to obtain a container's ID from a Pod. Rules and tests were updated to propagate WorkloadUID and ContainerID. go.mod dependency upgrades included. Changes
Sequence DiagramsequenceDiagram
participant Rule as exec-to-pod Rule
participant Helper as GetControllerDetails / helpers
participant K8sAPI as Kubernetes API
participant Alert as Alert Generator
Rule->>Helper: call GetControllerDetails(event, clientset)
Helper->>K8sAPI: Get Pod by event info
K8sAPI-->>Helper: Pod object
Helper->>Helper: Extract ownerRef → workload kind/name/namespace
Helper->>K8sAPI: Query workload resource (resolveReplicaSet/resolveJob/...)
K8sAPI-->>Helper: Workload resource (includes UID)
Helper-->>Rule: Pod, workloadKind, workloadName, workloadNamespace, workloadUID, nodeName
Rule->>Rule: GetContainerID(pod, containerName) → containerID
Rule->>Alert: Create alert with ContainerID, WorkloadUID, other details
Alert-->>Rule: Enriched RuntimeAlertK8sDetails
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
…extraction Signed-off-by: Yakir Oren <yakiroren@gmail.com>
YakirOren
force-pushed
the
fix/exec-to-pod-no-containerID
branch
from
1b481bd to
896bc13
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the exec-to-pod admission rule to enrich emitted alerts with workload UID and container ID, and performs a broad dependency/toolchain update to align with newer Kubernetes/Go ecosystems.
Changes:
- Add pod-returning controller lookup and new helpers to derive container ID and workload UID.
- Populate
WorkloadUIDandContainerIDin the R2000 “Exec to pod” alert, and update tests/mocks accordingly. - Bump Go version and refresh a large set of Go module dependencies (including Kubernetes libraries).
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
admission/rules/v1/r2000_exec_to_pod.go |
Uses new helper to fetch pod + sets WorkloadUID/ContainerID in alert details. |
admission/rules/v1/helpers.go |
Adds GetControllerDetailsWithPod, GetContainerID, and GetWorkloadUID helpers. |
admission/rules/v1/r2000_exec_to_pod_test.go |
Extends assertions to cover WorkloadUID and ContainerID. |
objectcache/objectcache_mock.go |
Enhances fake objects with ReplicaSet UID + pod container status data for tests. |
go.mod |
Updates Go version, dependencies, and changes the inspektor-gadget replace to a different fork. |
go.sum |
Regenerated dependency checksums to match go.mod updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@admission/rules/v1/r2000_exec_to_pod.go`:
- Around line 88-91: workloadUID is only being set when containerID is
non-empty, but workload UID resolution is independent; remove the containerID
gating and always call GetWorkloadUID so workloadUID is populated even if
containerID/status is empty. Locate the workloadUID variable and the conditional
that checks containerID, then change the flow to call GetWorkloadUID(client,
workloadKind, workloadName, workloadNamespace) unconditionally (retain any error
handling/logging around GetWorkloadUID as needed) so correlation works when
containerID is absent.
In `@go.mod`:
- Around line 41-44: The dependency bump to k8s.io/api, k8s.io/apimachinery,
k8s.io/apiserver, and k8s.io/client-go v0.35.0 requires you to: ensure the
project Go toolchain is >=1.25 (verify go.mod "go" version and CI images), scan
code and dependencies for uses of the removed ProtoMessage() marker and either
update those callsites or add the temporary build tag
kubernetes_protomessage_one_more_release where necessary, review any logic that
compares or assumes opaque resourceVersion (watch/informer/relist-resume code)
and update to the new ordering semantics, and confirm target clusters (EKS/GKE)
meet Kubernetes 1.35 before merging.
🧹 Nitpick comments (3)
admission/rules/v1/helpers.go (2)
15-52:GetControllerDetailsshould delegate toGetControllerDetailsWithPodto avoid duplication.The two functions share identical logic. Refactoring
GetControllerDetailsto wrap the new variant eliminates the duplicated code.♻️ Proposed refactor
func GetControllerDetails(event admission.Attributes, clientset kubernetes.Interface) (string, string, string, string, error) { - podName, namespace := event.GetName(), event.GetNamespace() - - if podName == "" || namespace == "" { - return "", "", "", "", fmt.Errorf("invalid pod details from admission event") - } - - pod, err := GetPodDetails(clientset, podName, namespace) - if err != nil { - return "", "", "", "", fmt.Errorf("failed to get pod details: %w", err) - } - - workloadKind, workloadName, workloadNamespace := ExtractPodOwner(pod, clientset) - nodeName := pod.Spec.NodeName - - return workloadKind, workloadName, workloadNamespace, nodeName, nil + _, workloadKind, workloadName, workloadNamespace, nodeName, err := GetControllerDetailsWithPod(event, clientset) + return workloadKind, workloadName, workloadNamespace, nodeName, err }
156-195: Errors from API calls are silently discarded — consider logging them.When the clientset call fails (e.g., network issue, RBAC), the function returns
""with no indication of failure. A debug/warning log would help troubleshoot cases where the UID is unexpectedly empty.go.mod (1)
358-358: Fork replacement forinspektor-gadget— track upstream merge.This points to a personal fork (
matthyx/inspektor-gadget). Ensure there's a tracking issue to switch back to the upstream module once the required changes are merged.
|
Summary:
|
…llback Signed-off-by: Yakir Oren <yakiroren@gmail.com>
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
YakirOren
force-pushed
the
fix/exec-to-pod-no-containerID
branch
from
e3ecd30 to
8ef9660
Compare
|
Summary:
|
2 participants
Summary by CodeRabbit
New Features
Tests
Dependencies