[ROSAENG-14183] Fix CI pipeline issues - #459
Conversation
Unchecked type assertions on e.ObjectOld and e.ObjectNew panic if the watch cache delivers a non-Secret object (e.g. during cache warm-up or a DeletedFinalStateUnknown tombstone). Guard both assertions and return false when either fails, matching controller-runtime best practice. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThe Secret update predicate now safely type-asserts both old and new event objects before applying the name filter and detecting Secret data changes. ChangesSecret predicate handling
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: charlesgong 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 |
![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 `@controllers/secret/secret_controller.go`:
- Around line 34-39: Update the Secret update predicate’s type-assertion guard
to also reject typed-nil pointers by checking oldSecret and newSecret for nil
before calling passes or dataChanged. Preserve the existing false return for
invalid or nil Secret objects.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 37d7223e-42d2-4822-bc74-28e785d0f045
📒 Files selected for processing (1)
controllers/secret/secret_controller.go
| oldSecret, oldOk := e.ObjectOld.(*corev1.Secret) | ||
| newSecret, newOk := e.ObjectNew.(*corev1.Secret) | ||
| if !oldOk || !newOk { | ||
| return false | ||
| } | ||
| return passes(e.ObjectOld) && dataChanged(oldSecret, newSecret) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n 'ObjectOld\s*:' --glob '*.go' .
rg -n 'mySecretPredicate|ObjectOld|UpdateEvent' controllers/secret --glob '*_test.go'Repository: openshift/splunk-forwarder-operator
Length of output: 173
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "tracked files around controllers/secret:"
git ls-files | grep -E '^controllers/secret/|go\.mod$|go\.sum$' | sed -n '1,120p'
echo "--- controllers/secret/secret_controller.go size and relevant lines ---"
wc -l controllers/secret/secret_controller.go
sed -n '1,120p' controllers/secret/secret_controller.go
echo "--- go.mod deps around controller-runtime ---"
rg -n 'controller-runtime|controllers-runtime|sigs.k8s.io/controller-runtime|core v' go.mod go.sum | sed -n '1,120p'Repository: openshift/splunk-forwarder-operator
Length of output: 5098
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- predicate implementation in vendor/module cache if present ---"
rg -n 'type UpdateEvent|ObjectOld|ObjectNew' "$GOMODCACHE/sigs.k8s.io/controller-runtime" 2>/dev/null | sed -n '1,120p' || true
echo "--- local cache files ---"
find "${GOMODCACHE:-notset}" -path '*/controller-runtime*/pkg/prebuild*controller-runtime*/pkg/event/*.go' -print 2>/dev/null | sed -n '1,40p'Repository: openshift/splunk-forwarder-operator
Length of output: 266
🌐 Web query:
controller-runtime v0.15.0 event UpdateEvent ObjectOld ObjectNew source
💡 Result:
In controller-runtime v0.15.0, the UpdateEvent struct is defined in the sigs.k8s.io/controller-runtime/pkg/event package [1][2]. It represents an event where a Kubernetes object has been updated and contains references to both the previous and current states of the object [1][2]. The struct definition is as follows: type UpdateEvent struct { // ObjectOld is the object from the event (before the update). ObjectOld runtime.Object // ObjectNew is the object from the event (after the update). ObjectNew runtime.Object } In the controller-runtime architecture, this event is generated by a source.Source (typically via informers) and is passed to an EventHandler [3][4]. Before reaching the handler, the event is often processed by Predicates, which allow controllers to filter out unnecessary update events (e.g., ignoring status updates) by comparing ObjectOld and ObjectNew [1][2][5]. Handler implementations, such as EnqueueRequestForObject, use these fields to determine the resource to reconcile, typically by extracting the metadata (Name and Namespace) from either ObjectNew or ObjectOld [6].
Citations:
- 1: https://www.bookstack.cn/read/operatorframework-1.15-en/80f2476f83e648e1.md
- 2: https://sdk.operatorframework.io/docs/building-operators/golang/references/event-filtering/
- 3: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/event/event.go
- 4: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/event/doc.go
- 5: https://github.com/kubernetes-sigs/controller-runtime/blob/ffd9ec8768b7d2e82aee6b0c76d481a5bdda4075/pkg/source/internal/eventsource.go
- 6: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/handler/enqueue.go
🌐 Web query:
github.com/opencontainers go-runtime pkg/event UpdateEvent v0.15.0 ObjectNew = nil predicate Funcs Passes
💡 Result:
The query appears to conflate two distinct projects: the Open Container Initiative (OCI) Runtime Specification [1] and the Kubernetes controller-runtime library [2][3]. There is no "UpdateEvent" in the OCI runtime-spec, nor does it have a "predicate.Funcs" mechanism [1][4]. The elements mentioned in your query—specifically UpdateEvent, predicate.Funcs, ObjectNew, and the behavior of nil predicates—are part of the sigs.k8s.io/controller-runtime project [2][5][6]. In controller-runtime: 1. UpdateEvent: This is a struct in the pkg/event package that carries both the old (ObjectOld) and new (ObjectNew) states of a Kubernetes object, allowing controllers to detect specific changes [7][6]. 2. predicate.Funcs: This is a helper struct in the pkg/predicate package used to filter events before they are processed by a controller [5][7]. 3. Nil predicate behavior: When using predicate.Funcs, if a specific function field (e.g., UpdateFunc) is left nil, the default behavior in modern versions of controller-runtime is to allow (return true for) the event [7]. However, it is considered best practice to explicitly define all four function fields (CreateFunc, UpdateFunc, DeleteFunc, GenericFunc) to avoid ambiguity or unintended side effects during upgrades [7]. If you are encountering a "nil" issue, it is likely related to handling an UpdateEvent where ObjectNew or ObjectOld might be nil, which can cause panics if not checked [5][8][9]. Custom predicates often include explicit checks (e.g., if e.ObjectNew == nil { return false }) to handle these cases safely [5][9].
Citations:
- 1: https://github.com/opencontainers/runtime-spec
- 2: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/predicate
- 3: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/event
- 4: https://github.com/opencontainers/runtime-spec/blob/main/runtime.md
- 5: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/predicate/predicate.go
- 6: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/event/event.go
- 7: https://www.golinuxcloud.com/kubernetes-watches-events-predicates/
- 8: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/handler/eventhandler.go
- 9: NVIDIA/gpu-operator@16b0962
🌐 Web query:
"Go type assertion nil pointer interface nil" "typed nil"
💡 Result:
In Go, a type assertion on a nil interface or an interface containing a nil pointer follows specific runtime rules that often surprise developers [1][2]. Interface Basics An interface value in Go is conceptually a pair: (dynamic type, dynamic value) [1][3]. - An interface is nil if and only if both its dynamic type and dynamic value are nil [1][2]. - An interface containing a nil pointer (e.g., *int(nil)) is NOT nil, because it has a concrete dynamic type (*int) even though its dynamic value is nil [2][3]. Type Assertion Rules When you perform a type assertion x.(T): 1. If x is a nil interface (no dynamic type), the assertion always fails [4][5]. - The single-result form (v:= x.(T)) will panic [6][5]. - The comma-ok form (v, ok:= x.(T)) will return the zero value of T and ok = false [6][5]. 2. If x contains a nil pointer (it is not a nil interface), the assertion behaves as follows: - If T is a concrete type that matches the dynamic type of x, the assertion succeeds (ok = true), and the resulting value v is a nil pointer of that type [7][2]. - If T is an interface type, the assertion succeeds if the dynamic type of x implements T [7][5]. Common Pitfalls - Checking for nil: Comparing an interface directly to nil (if i == nil) only works if the interface has no dynamic type [2]. If an interface holds a nil pointer, it will not equal nil, which can lead to unexpected panics if you proceed to call methods on the nil pointer [8][2]. - Assertion Panic: To safely assert types, always use the comma-ok idiom (v, ok:= x.(T)) to prevent runtime panics when the assertion fails [6][5]. - Detecting nil values: If you need to check if an interface contains a nil pointer, you must use reflection (e.g., reflect.ValueOf(i).IsNil) or perform a type assertion and then explicitly check the resulting pointer for nil [1][8][2].
Citations:
- 1: https://www.adityathebe.com/golang-nil-interface-check/
- 2: https://rednafi.com/go/nil-interface-comparison/
- 3: https://trstringer.com/go-nil-interface-and-interface-with-nil-concrete-value/
- 4: golang/go@f1e6ae2
- 5: https://algomaster.io/learn/go/type-assertions
- 6: https://www.golinuxcloud.com/golang-type-assertion/
- 7: Cannot detect a failed interface type assertion when underlying pointer is nil golang/go#2363
- 8: https://goodevilgenius.org/2024/06/24/golang-Understanding-the-difference-between-nil-pointers-and-nil-interfaces/
🌐 Web query:
sigs.k8s.io/controller-runtime v0.15.0 pkg/source/internal/eventsource.go runtime.Scheme Decode runtime.Object nil
💡 Result:
The issue you are likely encountering involves a nil pointer dereference or failure related to admission.Decoder after upgrading to controller-runtime v0.15.0 or later [1][2]. In controller-runtime v0.15.0, the library removed dependency injection functions, including those that automatically injected the Decoder into webhook handlers [3][1]. Previously, many users relied on this automatic injection (e.g., InjectDecoder), which no longer occurs [1][2]. If your code attempts to call decoder.Decode or decoder.DecodeRaw on a nil or uninitialized Decoder instance, it will cause a panic (specifically a nil pointer dereference) [1][2]. To resolve this: 1. Manual Initialization: You must manually instantiate the Decoder using the appropriate constructor, such as admission.NewDecoder(mgr.GetScheme()) [2]. 2. Dependency Injection Removal: Since InjectDecoder is no longer supported, you must update your webhook handler structures to accept a Decoder instance upon creation and store it within your struct [1][2]. Regarding the path pkg/source/internal/eventsource.go mentioned in your query: while this file contains logic for handling events (including decoding/casting objects from informers), the primary runtime panics related to "Decode" and "runtime.Object nil" in this version context almost exclusively stem from the breaking changes in the webhook/admission package where the automatic injection of decoders was removed [4][1][2]. If you are working specifically with eventsource.go, ensure that the object being processed is not nil and is correctly asserted to the expected type (client.Object or cache.DeletedFinalStateUnknown) before proceeding, as shown in the package's internal logic [4].
Citations:
- 1: After upgrading controller-runtime from 0.10 to 0.15, InjectDecoder and admission.Decoder don't seem to be working! kubernetes-sigs/controller-runtime#2504
- 2: admission.Decoder.Decode is panicking after version bump kubernetes-sigs/controller-runtime#2695
- 3: https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0
- 4: https://github.com/kubernetes-sigs/controller-runtime/blob/ffd9ec8768b7d2e82aee6b0c76d481a5bdda4075/pkg/source/internal/eventsource.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
cd "$tmp"
go mod init testnil 2>/dev/null
cat > probe.go <<'EOF'
package main
import (
"fmt"
"reflect"
corev1 "k8s.io/api/core/v1"
"sigs.k8s.io/controller-runtime/pkg/event"
)
func handle(e event.UpdateEvent) {
oldSecret, oldOk := e.ObjectOld.(*corev1.Secret)
newSecret, newOk := e.ObjectNew.(*corev1.Secret)
fmt.Println("Old ok:", oldOk, "reflect.IsNil:", oldOk && reflect.ValueOf(e.ObjectOld).IsNil())
fmt.Println("New ok:", newOk, "reflect.IsNil:", newOk && reflect.ValueOf(e.ObjectNew).IsNil())
if oldOk {
fmt.Println("Old secret is nil:", oldSecret == nil, "data:", oldSecret.Data)
panic("continue")
}
}
func main() {
e := event.UpdateEvent{
ObjectOld: (*corev1.Secret)(nil),
ObjectNew: &corev1.Secret{},
}
_ = e
}
EOF
cat > go.mod <<'EOF'
module testnil
go 1.21
require (
k8s.io/api v0.26.0
sigs.k8s.io/controller-runtime v0.15.0
)
EOF
go mod tidy >/dev/null 2>&1
# run is intentionally not needed; behavioral semantics are captured by the source-level probe's
# asserted expectations in comments above.Repository: openshift/splunk-forwarder-operator
Length of output: 173
🌐 Web query:
github.com/kubernetes-sigs/controller-runtime v0.15.0 pkg/cache internal code Decode nil object UpdateEvent ObjectOld
💡 Result:
In controller-runtime v0.15.0 and related versions, UpdateEvent is a structure that carries metadata and object information about updates [1][2]. The ObjectOld field in an UpdateEvent represents the state of the object before the update [1]. Regarding the behavior of nil objects within UpdateEvent: 1. Handling in Event Sources: The internal event source logic typically expects ObjectOld and ObjectNew to be valid objects (implementing client.Object) [3]. If the objects are not successfully cast to client.Object, the event generation will fail, often resulting in logged errors (e.g., "OnUpdate missing ObjectOld") rather than passing a nil value through to the handler [3]. 2. Predicate Filtering: Because UpdateEvent fields can be nil in specific edge cases (such as when an event is missing data due to external factors or specific cache synchronization scenarios), developers often implement safety checks in their custom predicate.Predicate implementations [4]. A common pattern to avoid panics is to explicitly check for nil before accessing methods on the objects [4]: go func (p MyKindCustomPredicate) Update(e event.UpdateEvent) bool { if e.ObjectOld == nil || e.ObjectNew == nil { return false } // ... perform further checks } 3. Cache and Decoder Issues: While you may encounter errors like "cache contained " in broader controller-runtime contexts (often related to cache_reader.go), these are generally distinct from the logic inside an UpdateEvent's ObjectOld field [5][6]. If your application is reporting nil objects within an UpdateEvent, it typically indicates an upstream source failure or an atypical event generation path where the object state was not properly populated [3]. If you are encountering nil objects in your UpdateEvent, it is recommended to add the nil-safety checks in your predicates as shown above to ensure your controller remains robust [4].
Citations:
- 1: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/event/event.go
- 2: https://sdk.operatorframework.io/docs/building-operators/golang/references/event-filtering/
- 3: https://github.com/kubernetes-sigs/controller-runtime/blob/ffd9ec8768b7d2e82aee6b0c76d481a5bdda4075/pkg/source/internal/eventsource.go
- 4: SyncPeriod does not work if controller is created WithEventFilter(predicate.GenerationChangedPredicate{}) kubernetes-sigs/controller-runtime#2355
- 5: cache contained <nil> kubernetes-sigs/controller-runtime#2898
- 6: webhook server: "cache contained <nil>, which is not an Object" when handling check_run event actions/actions-runner-controller#776
🌐 Web query:
github.com/kubernetes-sigs/controller-runtime v0.15.0 pkg/cache internal cache.go updateEvent ObjectOld nil
💡 Result:
In the context of the Kubernetes controller-runtime library, an update event occurs when a Kubernetes object is modified [1]. The event is encapsulated in an UpdateEvent structure, which contains both the old state (ObjectOld) and the new state (ObjectNew) of the object [1][2]. When you encounter a situation where ObjectOld is nil, it typically indicates one of the following scenarios: 1. Handling of Incomplete Events: Under certain conditions, such as after a watch restarts, the controller may not have a reliable history of the object state, potentially leading to scenarios where the old object is unavailable or nil [3]. 2. Event Filtering (Predicates): Many built-in predicates in controller-runtime (e.g., ResourceVersionChangedPredicate or GenerationChangedPredicate) explicitly check if ObjectOld or ObjectNew is nil [2][4][5]. If either is nil, these predicates will log an error or return false to prevent invalid processing, as the logic depends on comparing the two versions [2][4][5]. 3. Custom Logic and Testing: If you are encountering this in your own logic or testing, it is often intentional. For instance, in test scenarios, one might construct an UpdateEvent with a nil ObjectOld to verify that the handler correctly handles missing state or performs appropriate logic (such as ignoring the event or triggering based solely on the new object) [6]. Controller-runtime generally encourages controllers to operate based on the current state of the world rather than relying on state transitions, as it is difficult to guarantee the availability of the "old" object across all possible edge cases [3]. If your logic requires comparing state changes, always perform a nil check on ObjectOld before accessing its fields to avoid runtime panics [2][5].
Citations:
- 1: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/event/event.go
- 2: https://github.com/kubernetes-sigs/controller-runtime/blob/c3c1f058a9a080581e8fe99c004fcc792b2aff07/pkg/predicate/predicate.go
- 3: Why doesn't controller-runtime allow access to the old object on updates? kubernetes-sigs/controller-runtime#399
- 4: https://go-mod-viewer.appspot.com/sigs.k8s.io/controller-runtime@v0.18.2/pkg/predicate/predicate.go
- 5: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/predicate/predicate.go
- 6: https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/handler/eventhandler_test.go
🌐 Web query:
github.com/kubernetes-sigs/controller-runtime v0.15.0 pkg/cache internal typed nil predicate ResourceVersionChangedPredicate ObjectOld ObjectNew nil
💡 Result:
In kubernetes-sigs/controller-runtime v0.15.0, the ResourceVersionChangedPredicate implementation is designed to safely handle potential nil objects within an event.UpdateEvent to prevent nil pointer exceptions [1][2]. Within the Update method of ResourceVersionChangedPredicate, the code explicitly checks if e.ObjectOld or e.ObjectNew are nil before attempting to access their methods [1][2]: go func (ResourceVersionChangedPredicate) Update(e event.UpdateEvent) bool { if e.ObjectOld == nil { log.Error(nil, "Update event has no old object to update", "event", e) return false } if e.ObjectNew == nil { log.Error(nil, "Update event has no new object to update", "event", e) return false } return e.ObjectNew.GetResourceVersion() != e.ObjectOld.GetResourceVersion() } If either object is nil, the method logs an error and returns false, effectively filtering out the event and avoiding a panic [1][2]. This behavior ensures that the predicate is resilient to malformed or incomplete update events, which can occur during cache synchronization or resync cycles [3]. This pattern is consistent across versions of the library, including the current implementation which has evolved to use generic types [4][5].
Citations:
- 1: https://github.com/kubernetes-sigs/controller-runtime/blob/2d210d01f344d8c371588564f905185f866f162c/pkg/predicate/predicate.go
- 2: https://github.com/kubernetes-sigs/controller-runtime/blob/c3c1f058a9a080581e8fe99c004fcc792b2aff07/pkg/predicate/predicate.go
- 3: Where should resourceVersionChangedPredicate be used kubernetes-sigs/controller-runtime#1740
- 4: https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/predicate/predicate.go
- 5: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.23.3/pkg/predicate/predicate.go
Guard typed-nil Secret pointers in update predicates.
A successful type assertion can still return a nil pointer; if ObjectOld or ObjectNew is typed-nil, passes / dataChanged will panic. Add oldSecret == nil || newSecret == nil checks before using them.
🤖 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 `@controllers/secret/secret_controller.go` around lines 34 - 39, Update the
Secret update predicate’s type-assertion guard to also reject typed-nil pointers
by checking oldSecret and newSecret for nil before calling passes or
dataChanged. Preserve the existing false return for invalid or nil Secret
objects.
|
@charlesgong: all tests passed! 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. |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #459 +/- ##
==========================================
- Coverage 72.44% 72.13% -0.31%
==========================================
Files 11 11
Lines 704 707 +3
==========================================
Hits 510 510
- Misses 173 176 +3
Partials 21 21
🚀 New features to boost your workflow:
|
2 participants
Summary by CodeRabbit