Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Fix: Prevent nil errors in setupLog.Error to ensure proper logging and add sanity check #1599

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+30 −4
Open

🐛 Fix: Prevent nil errors in setupLog.Error to ensure proper logging and add sanity check #1599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f56eba
Fix: Prevent nil errors in setupLog.Error to ensure proper logging
camilamacedo86 Jan 10, 2025
c597f22
Add Sanity Check To prevent the scenario
camilamacedo86 Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ generate: $(CONTROLLER_GEN) #EXHELP Generate code containing DeepCopy, DeepCopyI
.PHONY: verify
verify: tidy fmt vet generate manifests crd-ref-docs #HELP Verify all generated code is up-to-date.
git diff --exit-code
./hack/ci/sanity.sh

.PHONY: fix-lint
fix-lint: $(GOLANGCI_LINT) #EXHELP Fix lint issues
Expand Down
4 changes: 2 additions & 2 deletions catalogd/cmd/catalogd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,12 @@ func main() {
}

if (certFile != "" && keyFile == "") || (certFile == "" && keyFile != "") {
setupLog.Error(nil, "unable to configure TLS certificates: tls-cert and tls-key flags must be used together")
setupLog.Error(fmt.Errorf("unable to configure TLS certificates"), "tls-cert and tls-key flags must be used together")
os.Exit(1)
}

if metricsAddr != "" && certFile == "" && keyFile == "" {
setupLog.Error(nil, "metrics-bind-address requires tls-cert and tls-key flags to be set")
setupLog.Error(fmt.Errorf("unable to configure metrics-bind-address"), "metrics-bind-address requires tls-cert and tls-key flags to be set")
os.Exit(1)
}

Expand Down
4 changes: 2 additions & 2 deletions cmd/operator-controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,12 +128,12 @@ func main() {
}

if (certFile != "" && keyFile == "") || (certFile == "" && keyFile != "") {
setupLog.Error(nil, "unable to configure TLS certificates: tls-cert and tls-key flags must be used together")
setupLog.Error(fmt.Errorf("unable to configure TLS certificates"), "tls-cert and tls-key flags must be used together")
os.Exit(1)
}

if metricsAddr != "" && certFile == "" && keyFile == "" {
setupLog.Error(nil, "metrics-bind-address requires tls-cert and tls-key flags to be set")
setupLog.Error(fmt.Errorf("unable to configure metrics-bind-address"), "metrics-bind-address requires tls-cert and tls-key flags to be set")
os.Exit(1)
}

Expand Down
25 changes: 25 additions & 0 deletions hack/ci/sanity.sh
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I like this!

Nit: I'd probably name this hack/ci/extra_linters.sh
Also nit: Is there a way to define the rules and messages such that they are setup as a list of pairs, and then we go ahead and write a loop to iterate and run each of them?

Lastly, I kinda wonder if there's already a generic regex-based linter built into golangci-lint that we could add custom rules to? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points, I will try to check them out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may also want to look into writing custom analyzers in Go using golang.org/x/tools/go/analysis. As is, if we have a variable name for a logger that is not setupLog, we'll miss it.

I assume an analyzer could find all logr.Logger.Error() method usage where the first parameter is nil.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But now I'm getting really picky :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea of the custom analyse is nice
I will try it out.

Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
####################################################
# The purpose of this script is allow us to do extra
# sanity checks to ensure the quality.
####################################################

#!/bin/bash

set -e

echo_error() {
echo "[Sanity Check Fail]: $1"
}

# Criteria 1: Do not use setupLog.Error(nil, ...)
CRITERIA1_RULE="setupLog\.Error\(nil,"
CRITERIA1_MSG="Avoid using 'setupLog.Error(nil, ...)'. Instead, use 'errors.New()' or 'fmt.Errorf()' \
to ensure logs are created. Using 'nil' for errors can result in silent failures, making bugs harder to detect."

if grep -r --include="*.go" "$CRITERIA1_RULE" .; then
echo_error "$CRITERIA1_MSG"
exit 1
fi

echo "Sanity checks passed with success."
exit 0
Loading