Skip to content

Commit 4304961

Browse files
camilamacedo86camilamacedo86
authored
Add custom Linter to not allow improper usage of logger.Error(nil, ...) calls and fix found linter issues. (#1599)
Fix: Prevent nil errors in setupLog.Error to ensure proper logging Closes; #1566 Closes: #1556 Furthermore, it solves a similar scenario in the EventHandler code implementation found with the new custom linter check.
1 parent c451127 commit 4304961

File tree

11 files changed

+239
-18
lines changed

11 files changed

+239
-18
lines changed

‎Makefile

+9-1
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,17 @@ help-extended: #HELP Display extended help.
9999
#SECTION Development
100100

101101
.PHONY: lint
102-
lint: $(GOLANGCI_LINT) #HELP Run golangci linter.
102+
lint: lint-custom $(GOLANGCI_LINT) #HELP Run golangci linter.
103103
$(GOLANGCI_LINT) run --build-tags $(GO_BUILD_TAGS) $(GOLANGCI_LINT_ARGS)
104104

105+
.PHONY: custom-linter-build
106+
custom-linter-build: #HELP Build custom linter
107+
go build -tags $(GO_BUILD_TAGS) -o ./bin/custom-linter ./hack/ci/custom-linters/cmd
108+
109+
.PHONY: lint-custom
110+
lint-custom: custom-linter-build #HELP Call custom linter for the project
111+
go vet -tags=$(GO_BUILD_TAGS) -vettool=./bin/custom-linter ./...
112+
105113
.PHONY: tidy
106114
tidy: #HELP Update dependencies.
107115
# Force tidy to use the version already in go.mod

‎catalogd/cmd/catalogd/main.go

+7-2
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package main
1818

1919
import (
2020
"crypto/tls"
21+
"errors"
2122
"flag"
2223
"fmt"
2324
"log"
@@ -149,12 +150,16 @@ func main() {
149150
}
150151

151152
if (certFile != "" && keyFile == "") || (certFile == "" && keyFile != "") {
152-
setupLog.Error(nil, "unable to configure TLS certificates: tls-cert and tls-key flags must be used together")
153+
setupLog.Error(errors.New("missing TLS configuration"),
154+
"tls-cert and tls-key flags must be used together",
155+
"certFile", certFile, "keyFile", keyFile)
153156
os.Exit(1)
154157
}
155158

156159
if metricsAddr != "" && certFile == "" && keyFile == "" {
157-
setupLog.Error(nil, "metrics-bind-address requires tls-cert and tls-key flags to be set")
160+
setupLog.Error(errors.New("invalid metrics configuration"),
161+
"metrics-bind-address requires tls-cert and tls-key flags to be set",
162+
"metricsAddr", metricsAddr, "certFile", certFile, "keyFile", keyFile)
158163
os.Exit(1)
159164
}
160165

‎cmd/operator-controller/main.go

+7-2
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package main
1919
import (
2020
"context"
2121
"crypto/tls"
22+
"errors"
2223
"flag"
2324
"fmt"
2425
"net/http"
@@ -136,12 +137,16 @@ func main() {
136137
}
137138

138139
if (certFile != "" && keyFile == "") || (certFile == "" && keyFile != "") {
139-
setupLog.Error(nil, "unable to configure TLS certificates: tls-cert and tls-key flags must be used together")
140+
setupLog.Error(errors.New("missing TLS configuration"),
141+
"tls-cert and tls-key flags must be used together",
142+
"certFile", certFile, "keyFile", keyFile)
140143
os.Exit(1)
141144
}
142145

143146
if metricsAddr != "" && certFile == "" && keyFile == "" {
144-
setupLog.Error(nil, "metrics-bind-address requires tls-cert and tls-key flags to be set")
147+
setupLog.Error(errors.New("invalid metrics configuration"),
148+
"metrics-bind-address requires tls-cert and tls-key flags to be set",
149+
"metricsAddr", metricsAddr, "certFile", certFile, "keyFile", keyFile)
145150
os.Exit(1)
146151
}
147152

‎go.mod

+2-1
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ require (
2727
github.com/stretchr/testify v1.10.0
2828
golang.org/x/exp v0.0.0-20241009180824-f66d83c29e7c
2929
golang.org/x/sync v0.11.0
30+
golang.org/x/tools v0.29.0
3031
gopkg.in/yaml.v2 v2.4.0
3132
helm.sh/helm/v3 v3.17.0
3233
k8s.io/api v0.32.0
@@ -226,13 +227,13 @@ require (
226227
go.opentelemetry.io/otel/trace v1.33.0 // indirect
227228
go.opentelemetry.io/proto/otlp v1.3.1 // indirect
228229
golang.org/x/crypto v0.32.0 // indirect
230+
golang.org/x/mod v0.22.0 // indirect
229231
golang.org/x/net v0.34.0 // indirect
230232
golang.org/x/oauth2 v0.25.0 // indirect
231233
golang.org/x/sys v0.29.0 // indirect
232234
golang.org/x/term v0.28.0 // indirect
233235
golang.org/x/text v0.21.0 // indirect
234236
golang.org/x/time v0.7.0 // indirect
235-
golang.org/x/tools v0.29.0 // indirect
236237
gomodules.xyz/jsonpatch/v2 v2.4.0 // indirect
237238
google.golang.org/genproto v0.0.0-20240903143218-8af14fe29dc1 // indirect
238239
google.golang.org/genproto/googleapis/api v0.0.0-20240903143218-8af14fe29dc1 // indirect

‎hack/ci/custom-linters/analyzers/analyzers_test.go

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
package analyzers
2+
3+
import (
4+
"testing"
5+
6+
"golang.org/x/tools/go/analysis/analysistest"
7+
)
8+
9+
func TestSetupLogErrorCheck(t *testing.T) {
10+
testdata := analysistest.TestData()
11+
analysistest.Run(t, testdata, SetupLogErrorCheck)
12+
}

‎hack/ci/custom-linters/analyzers/setuplognilerrorcheck.go

+119
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
package analyzers
2+
3+
import (
4+
"bytes"
5+
"go/ast"
6+
"go/format"
7+
"go/token"
8+
"go/types"
9+
10+
"golang.org/x/tools/go/analysis"
11+
)
12+
13+
var SetupLogErrorCheck = &analysis.Analyzer{
14+
Name: "setuplogerrorcheck",
15+
Doc: "Detects improper usage of logger.Error() calls, ensuring the first argument is a non-nil error.",
16+
Run: runSetupLogErrorCheck,
17+
}
18+
19+
func runSetupLogErrorCheck(pass *analysis.Pass) (interface{}, error) {
20+
for _, f := range pass.Files {
21+
ast.Inspect(f, func(n ast.Node) bool {
22+
callExpr, ok := n.(*ast.CallExpr)
23+
if !ok {
24+
return true
25+
}
26+
27+
// Ensure function being called is logger.Error
28+
selectorExpr, ok := callExpr.Fun.(*ast.SelectorExpr)
29+
if !ok || selectorExpr.Sel.Name != "Error" {
30+
return true
31+
}
32+
33+
// Ensure receiver (logger) is identified
34+
ident, ok := selectorExpr.X.(*ast.Ident)
35+
if !ok {
36+
return true
37+
}
38+
39+
// Verify if the receiver is logr.Logger
40+
obj := pass.TypesInfo.ObjectOf(ident)
41+
if obj == nil {
42+
return true
43+
}
44+
45+
named, ok := obj.Type().(*types.Named)
46+
if !ok || named.Obj().Pkg() == nil || named.Obj().Pkg().Path() != "github.com/go-logr/logr" || named.Obj().Name() != "Logger" {
47+
return true
48+
}
49+
50+
if len(callExpr.Args) == 0 {
51+
return true
52+
}
53+
54+
// Get the actual source code line where the issue occurs
55+
var srcBuffer bytes.Buffer
56+
if err := format.Node(&srcBuffer, pass.Fset, callExpr); err != nil {
57+
return true
58+
}
59+
sourceLine := srcBuffer.String()
60+
61+
// Check if the first argument of the error log is nil
62+
firstArg, ok := callExpr.Args[0].(*ast.Ident)
63+
if ok && firstArg.Name == "nil" {
64+
suggestedError := "errors.New(\"kind error (i.e. configuration error)\")"
65+
suggestedMessage := "\"error message describing the failed operation\""
66+
67+
if len(callExpr.Args) > 1 {
68+
if msgArg, ok := callExpr.Args[1].(*ast.BasicLit); ok && msgArg.Kind == token.STRING {
69+
suggestedMessage = msgArg.Value
70+
}
71+
}
72+
73+
pass.Reportf(callExpr.Pos(),
74+
"Incorrect usage of 'logger.Error(nil, ...)'. The first argument must be a non-nil 'error'. "+
75+
"Passing 'nil' results in silent failures, making debugging harder.\n\n"+
76+
"\U0001F41B **What is wrong?**\n %s\n\n"+
77+
"\U0001F4A1 **How to solve? Return the error, i.e.:**\n logger.Error(%s, %s, \"key\", value)\n\n",
78+
sourceLine, suggestedError, suggestedMessage)
79+
return true
80+
}
81+
82+
// Ensure at least two arguments exist (error + message)
83+
if len(callExpr.Args) < 2 {
84+
pass.Reportf(callExpr.Pos(),
85+
"Incorrect usage of 'logger.Error(error, ...)'. Expected at least an error and a message string.\n\n"+
86+
"\U0001F41B **What is wrong?**\n %s\n\n"+
87+
"\U0001F4A1 **How to solve?**\n Provide a message, e.g. logger.Error(err, \"descriptive message\")\n\n",
88+
sourceLine)
89+
return true
90+
}
91+
92+
// Ensure key-value pairs (if any) are valid
93+
if (len(callExpr.Args)-2)%2 != 0 {
94+
pass.Reportf(callExpr.Pos(),
95+
"Incorrect usage of 'logger.Error(error, \"msg\", ...)'. Key-value pairs must be provided after the message, but an odd number of arguments was found.\n\n"+
96+
"\U0001F41B **What is wrong?**\n %s\n\n"+
97+
"\U0001F4A1 **How to solve?**\n Ensure all key-value pairs are complete, e.g. logger.Error(err, \"msg\", \"key\", value, \"key2\", value2)\n\n",
98+
sourceLine)
99+
return true
100+
}
101+
102+
for i := 2; i < len(callExpr.Args); i += 2 {
103+
keyArg := callExpr.Args[i]
104+
keyType := pass.TypesInfo.TypeOf(keyArg)
105+
if keyType == nil || keyType.String() != "string" {
106+
pass.Reportf(callExpr.Pos(),
107+
"Incorrect usage of 'logger.Error(error, \"msg\", key, value)'. Keys in key-value pairs must be strings, but got: %s.\n\n"+
108+
"\U0001F41B **What is wrong?**\n %s\n\n"+
109+
"\U0001F4A1 **How to solve?**\n Ensure keys are strings, e.g. logger.Error(err, \"msg\", \"key\", value)\n\n",
110+
keyType, sourceLine)
111+
return true
112+
}
113+
}
114+
115+
return true
116+
})
117+
}
118+
return nil, nil
119+
}

‎hack/ci/custom-linters/analyzers/testdata/go.mod

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
module testdata
2+
3+
go 1.23.4
4+
5+
require github.com/go-logr/logr v1.4.2

‎hack/ci/custom-linters/analyzers/testdata/go.sum

+2
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
github.com/go-logr/logr v1.4.2 h1:6pFjapn8bFcIbiKo3XT4j/BhANplGihG6tvd+8rYgrY=
2+
github.com/go-logr/logr v1.4.2/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=

‎hack/ci/custom-linters/analyzers/testdata/main.go

+26
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package main
2+
3+
import (
4+
"github.com/go-logr/logr"
5+
)
6+
7+
func testLogger() {
8+
var logger logr.Logger
9+
var err error
10+
var value int
11+
12+
// Case 1: Nil error - Ensures the first argument cannot be nil.
13+
logger.Error(nil, "message") // want ".*results in silent failures, making debugging harder.*"
14+
15+
// Case 2: Odd number of key-value arguments - Ensures key-value pairs are complete.
16+
logger.Error(err, "message", "key1") // want ".*Key-value pairs must be provided after the message, but an odd number of arguments was found.*"
17+
18+
// Case 3: Key in key-value pair is not a string - Ensures keys in key-value pairs are strings.
19+
logger.Error(err, "message", 123, value) // want ".*Ensure keys are strings.*"
20+
21+
// Case 4: Values are passed without corresponding keys - Ensures key-value arguments are structured properly.
22+
logger.Error(err, "message", value, "key2", value) // want ".*Key-value pairs must be provided after the message, but an odd number of arguments was found.*"
23+
24+
// Case 5: Correct Usage - Should not trigger any warnings.
25+
logger.Error(err, "message", "key1", value, "key2", "value")
26+
}

‎hack/ci/custom-linters/cmd/main.go

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
package main
2+
3+
import (
4+
"golang.org/x/tools/go/analysis"
5+
"golang.org/x/tools/go/analysis/unitchecker"
6+
7+
"github.com/operator-framework/operator-controller/hack/ci/custom-linters/analyzers"
8+
)
9+
10+
// Define the custom Linters implemented in the project
11+
var customLinters = []*analysis.Analyzer{
12+
analyzers.SetupLogErrorCheck,
13+
}
14+
15+
func main() {
16+
unitchecker.Main(customLinters...)
17+
}

‎internal/contentmanager/source/internal/eventhandler.go

+33-12
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ limitations under the License.
3838

3939
import (
4040
"context"
41+
"errors"
4142
"fmt"
4243

4344
cgocache "k8s.io/client-go/tools/cache"
@@ -94,8 +95,11 @@ func (e *EventHandler[object, request]) OnAdd(obj interface{}) {
9495
if o, ok := obj.(object); ok {
9596
c.Object = o
9697
} else {
97-
log.Error(nil, "OnAdd missing Object",
98-
"object", obj, "type", fmt.Sprintf("%T", obj))
98+
log.Error(errors.New("failed to cast object"),
99+
"OnAdd missing Object",
100+
"expected_type", fmt.Sprintf("%T", c.Object),
101+
"received_type", fmt.Sprintf("%T", obj),
102+
"object", obj)
99103
return
100104
}
101105

@@ -118,20 +122,27 @@ func (e *EventHandler[object, request]) OnUpdate(oldObj, newObj interface{}) {
118122
if o, ok := oldObj.(object); ok {
119123
u.ObjectOld = o
120124
} else {
121-
log.Error(nil, "OnUpdate missing ObjectOld",
122-
"object", oldObj, "type", fmt.Sprintf("%T", oldObj))
125+
log.Error(errors.New("failed to cast old object"),
126+
"OnUpdate missing ObjectOld",
127+
"object", oldObj,
128+
"expected_type", fmt.Sprintf("%T", u.ObjectOld),
129+
"received_type", fmt.Sprintf("%T", oldObj))
123130
return
124131
}
125132

126133
// Pull Object out of the object
127134
if o, ok := newObj.(object); ok {
128135
u.ObjectNew = o
129136
} else {
130-
log.Error(nil, "OnUpdate missing ObjectNew",
131-
"object", newObj, "type", fmt.Sprintf("%T", newObj))
137+
log.Error(errors.New("failed to cast new object"),
138+
"OnUpdate missing ObjectNew",
139+
"object", newObj,
140+
"expected_type", fmt.Sprintf("%T", u.ObjectNew),
141+
"received_type", fmt.Sprintf("%T", newObj))
132142
return
133143
}
134144

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

162+
// Handle tombstone events (cache.DeletedFinalStateUnknown)
163+
if obj == nil {
164+
log.Error(errors.New("received nil object"),
165+
"OnDelete received a nil object, ignoring event")
166+
return
167+
}
168+
151169
// Deal with tombstone events by pulling the object out. Tombstone events wrap the object in a
152170
// DeleteFinalStateUnknown struct, so the object needs to be pulled out.
153171
// Copied from sample-controller
154172
// This should never happen if we aren't missing events, which we have concluded that we are not
155173
// and made decisions off of this belief. Maybe this shouldn't be here?
156-
var ok bool
157-
if _, ok = obj.(client.Object); !ok {
174+
if _, ok := obj.(client.Object); !ok {
158175
// If the object doesn't have Metadata, assume it is a tombstone object of type DeletedFinalStateUnknown
159176
tombstone, ok := obj.(cgocache.DeletedFinalStateUnknown)
160177
if !ok {
161-
log.Error(nil, "Error decoding objects. Expected cache.DeletedFinalStateUnknown",
162-
"type", fmt.Sprintf("%T", obj),
178+
log.Error(errors.New("unexpected object type"),
179+
"Error decoding objects, expected cache.DeletedFinalStateUnknown",
180+
"received_type", fmt.Sprintf("%T", obj),
163181
"object", obj)
164182
return
165183
}
@@ -175,8 +193,11 @@ func (e *EventHandler[object, request]) OnDelete(obj interface{}) {
175193
if o, ok := obj.(object); ok {
176194
d.Object = o
177195
} else {
178-
log.Error(nil, "OnDelete missing Object",
179-
"object", obj, "type", fmt.Sprintf("%T", obj))
196+
log.Error(errors.New("failed to cast object"),
197+
"OnDelete missing Object",
198+
"expected_type", fmt.Sprintf("%T", d.Object),
199+
"received_type", fmt.Sprintf("%T", obj),
200+
"object", obj)
180201
return
181202
}
182203

0 commit comments

Comments
 (0)