Skip to content

Commit

Permalink
Fixes improper usage of log.Error(nil,..) scenarios for EventHandler
Browse files Browse the repository at this point in the history
  • Loading branch information
camilamacedo86 committed Feb 8, 2025
1 parent adec9e9 commit 47a2c70
Show file tree
Hide file tree
Showing 6 changed files with 37 additions and 29 deletions.
3 changes: 1 addition & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,8 @@ lint: lint-custom $(GOLANGCI_LINT) #HELP Run golangci linter.
$(GOLANGCI_LINT) run --build-tags $(GO_BUILD_TAGS) $(GOLANGCI_LINT_ARGS)

.PHONY: custom-linter-build
LINTER_DIR := ./hack/ci/custom-linters/cmd
custom-linter-build: # HELP Build custom linter
cd $(LINTER_DIR) && go build -tags '$(GO_BUILD_TAGS)' -o $(ROOT_DIR)/bin/custom-linter
go build -tags $(GO_BUILD_TAGS) -o ./bin/custom-linter ./hack/ci/custom-linters/cmd

.PHONY: lint-custom #HELP Call custom linter for the project
lint-custom: custom-linter-build
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ require (
github.com/stretchr/testify v1.10.0
golang.org/x/exp v0.0.0-20241009180824-f66d83c29e7c
golang.org/x/sync v0.11.0
golang.org/x/tools v0.29.0
gopkg.in/yaml.v2 v2.4.0
helm.sh/helm/v3 v3.17.0
k8s.io/api v0.32.0
Expand Down Expand Up @@ -232,7 +233,6 @@ require (
golang.org/x/term v0.28.0 // indirect
golang.org/x/text v0.21.0 // indirect
golang.org/x/time v0.7.0 // indirect
golang.org/x/tools v0.29.0 // indirect
gomodules.xyz/jsonpatch/v2 v2.4.0 // indirect
google.golang.org/genproto v0.0.0-20240903143218-8af14fe29dc1 // indirect
google.golang.org/genproto/googleapis/api v0.0.0-20240903143218-8af14fe29dc1 // indirect
Expand Down
3 changes: 2 additions & 1 deletion hack/ci/custom-linters/cmd/main.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package main

import (
"github.com/operator-framework/operator-controller/hack/ci/custom-linters/analyzers"
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/unitchecker"

"github.com/operator-framework/operator-controller/hack/ci/custom-linters/analyzers"
)

// Define the custom Linters implemented in the project
Expand Down
5 changes: 0 additions & 5 deletions hack/ci/custom-linters/go.mod

This file was deleted.

8 changes: 0 additions & 8 deletions hack/ci/custom-linters/go.sum

This file was deleted.

45 changes: 33 additions & 12 deletions internal/contentmanager/source/internal/eventhandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ limitations under the License.

import (
"context"
"errors"
"fmt"

cgocache "k8s.io/client-go/tools/cache"
Expand Down Expand Up @@ -94,8 +95,11 @@ func (e *EventHandler[object, request]) OnAdd(obj interface{}) {
if o, ok := obj.(object); ok {
c.Object = o
} else {
log.Error(nil, "OnAdd missing Object",
"object", obj, "type", fmt.Sprintf("%T", obj))
log.Error(errors.New("failed to cast object"),
"OnAdd missing Object",
"expected_type", fmt.Sprintf("%T", c.Object),
"received_type", fmt.Sprintf("%T", obj),
"object", obj)
return
}

Expand All @@ -118,20 +122,27 @@ func (e *EventHandler[object, request]) OnUpdate(oldObj, newObj interface{}) {
if o, ok := oldObj.(object); ok {
u.ObjectOld = o
} else {
log.Error(nil, "OnUpdate missing ObjectOld",
"object", oldObj, "type", fmt.Sprintf("%T", oldObj))
log.Error(errors.New("failed to cast old object"),
"OnUpdate missing ObjectOld",
"object", oldObj,
"expected_type", fmt.Sprintf("%T", u.ObjectOld),
"received_type", fmt.Sprintf("%T", oldObj))
return
}

// Pull Object out of the object
if o, ok := newObj.(object); ok {
u.ObjectNew = o
} else {
log.Error(nil, "OnUpdate missing ObjectNew",
"object", newObj, "type", fmt.Sprintf("%T", newObj))
log.Error(errors.New("failed to cast new object"),
"OnUpdate missing ObjectNew",
"object", newObj,
"expected_type", fmt.Sprintf("%T", u.ObjectNew),
"received_type", fmt.Sprintf("%T", newObj))
return
}

// Run predicates before proceeding
for _, p := range e.predicates {
if !p.Update(u) {
return
Expand All @@ -148,18 +159,25 @@ func (e *EventHandler[object, request]) OnUpdate(oldObj, newObj interface{}) {
func (e *EventHandler[object, request]) OnDelete(obj interface{}) {
d := event.TypedDeleteEvent[object]{}

// Handle tombstone events (cache.DeletedFinalStateUnknown)
if obj == nil {
log.Error(errors.New("received nil object"),
"OnDelete received a nil object, ignoring event")
return
}

// Deal with tombstone events by pulling the object out. Tombstone events wrap the object in a
// DeleteFinalStateUnknown struct, so the object needs to be pulled out.
// Copied from sample-controller
// This should never happen if we aren't missing events, which we have concluded that we are not
// and made decisions off of this belief. Maybe this shouldn't be here?
var ok bool
if _, ok = obj.(client.Object); !ok {
if _, ok := obj.(client.Object); !ok {
// If the object doesn't have Metadata, assume it is a tombstone object of type DeletedFinalStateUnknown
tombstone, ok := obj.(cgocache.DeletedFinalStateUnknown)
if !ok {
log.Error(nil, "Error decoding objects. Expected cache.DeletedFinalStateUnknown",
"type", fmt.Sprintf("%T", obj),
log.Error(errors.New("unexpected object type"),
"Error decoding objects, expected cache.DeletedFinalStateUnknown",
"received_type", fmt.Sprintf("%T", obj),
"object", obj)
return
}
Expand All @@ -175,8 +193,11 @@ func (e *EventHandler[object, request]) OnDelete(obj interface{}) {
if o, ok := obj.(object); ok {
d.Object = o
} else {
log.Error(nil, "OnDelete missing Object",
"object", obj, "type", fmt.Sprintf("%T", obj))
log.Error(errors.New("failed to cast object"),
"OnDelete missing Object",
"expected_type", fmt.Sprintf("%T", d.Object),
"received_type", fmt.Sprintf("%T", obj),
"object", obj)
return
}

Expand Down

0 comments on commit 47a2c70

Please sign in to comment.