Skip to content

replaced panic() calls with proper error handling in production code #6658

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

replaced panic() calls with proper error handling in production code #6658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion cmd/controller-manager/app/controllermanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
"github.com/karmada-io/karmada/pkg/controllers/execution"
"github.com/karmada-io/karmada/pkg/controllers/federatedhpa"
metricsclient "github.com/karmada-io/karmada/pkg/controllers/federatedhpa/metrics"
"github.com/karmada-io/karmada/pkg/controllers/federatedresourcequota"

Check failure on line 65 in cmd/controller-manager/app/controllermanager.go

View workflow job for this annotation

GitHub Actions / lint 

could not import github.com/karmada-io/karmada/pkg/controllers/federatedresourcequota (-: # github.com/karmada-io/karmada/pkg/controllers/federatedresourcequota
"github.com/karmada-io/karmada/pkg/controllers/gracefuleviction"
"github.com/karmada-io/karmada/pkg/controllers/hpascaletargetmarker"
"github.com/karmada-io/karmada/pkg/controllers/mcs"
Expand Down Expand Up @@ -163,7 +163,8 @@

controlPlaneRestConfig, err := controllerruntime.GetConfig()
if err != nil {
panic(err)
klog.Errorf("Failed to get kubernetes config: %v", err)
return fmt.Errorf("failed to get kubernetes config: %w", err)

Check failure on line 167 in cmd/controller-manager/app/controllermanager.go

View workflow job for this annotation

GitHub Actions / lint 

undefined: fmt (typecheck)
}
controlPlaneRestConfig.RateLimiter = flowcontrol.NewTokenBucketRateLimiter(opts.KubeAPIQPS, opts.KubeAPIBurst)
controllerManager, err := controllerruntime.NewManager(controlPlaneRestConfig, controllerruntime.Options{
Expand Down
4 changes: 3 additions & 1 deletion cmd/service-name-resolution-detector-example/app/service-name-resolution-detector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,9 @@ func NewDetectorInitializers() map[string]InitFunc {
if _, ok := detectors[name]; !ok {
detectors[name] = fn
} else {
panic(fmt.Sprintf("detector name %q was registered twice", name))
klog.Errorf("Detector name %q was registered twice", name)
// Return error instead of panic for better error handling
return
Comment on lines +203 to +205
Copy link

Choose a reason for hiding this comment

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

high

The return statement here only exits the anonymous register function, not NewDetectorInitializers. This means a duplicate detector registration would be silently ignored (apart from the log message), which is likely not the intended behavior. The original panic indicated a fatal error. To maintain this behavior while improving logging, consider using klog.Fatalf which logs the error and exits the program.

Suggested change
klog.Errorf("Detector name %q was registered twice", name)
// Return error instead of panic for better error handling
return
klog.Fatalf("Detector name %q was registered twice", name)

}
}

Expand Down
3 changes: 2 additions & 1 deletion cmd/webhook/app/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ func Run(ctx context.Context, opts *options.Options) error {

config, err := controllerruntime.GetConfig()
if err != nil {
panic(err)
klog.Errorf("Failed to get kubernetes config: %v", err)
return fmt.Errorf("failed to get kubernetes config: %w", err)
}
config.RateLimiter = flowcontrol.NewTokenBucketRateLimiter(opts.KubeAPIQPS, opts.KubeAPIBurst)

Expand Down
3 changes: 2 additions & 1 deletion examples/customresourceinterpreter/webhook/app/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ func NewWebhookCommand(ctx context.Context) *cobra.Command {
func Run(ctx context.Context, opts *options.Options) error {
config, err := controllerruntime.GetConfig()
if err != nil {
panic(err)
klog.Errorf("Failed to get kubernetes config: %v", err)
return fmt.Errorf("failed to get kubernetes config: %w", err)
}

hookManager, err := controllerruntime.NewManager(config, controllerruntime.Options{
Expand Down
9 changes: 6 additions & 3 deletions hack/tools/preferredimports/preferredimports.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,17 +124,20 @@ func (a *analyzer) collect(dir string) {
ast.SortImports(a.fset, file)
var buffer bytes.Buffer
if err = format.Node(&buffer, a.fset, file); err != nil {
panic(fmt.Sprintf("Error formatting ast node after rewriting import.\n%s\n", err.Error()))
fmt.Fprintf(os.Stderr, "Error formatting ast node after rewriting import: %v\n", err)
continue
}

fileInfo, err := os.Stat(pathToFile)
if err != nil {
panic(fmt.Sprintf("Error stat'ing file: %s\n%s\n", pathToFile, err.Error()))
fmt.Fprintf(os.Stderr, "Error stat'ing file %s: %v\n", pathToFile, err)
continue
}

err = os.WriteFile(pathToFile, buffer.Bytes(), fileInfo.Mode())
if err != nil {
panic(fmt.Sprintf("Error writing file: %s\n%s\n", pathToFile, err.Error()))
fmt.Fprintf(os.Stderr, "Error writing file %s: %v\n", pathToFile, err)
continue
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,8 @@
case metav1.ConditionFalse:
applied = false
default: // should not happen unless the condition api changed.
panic("unexpected status")
klog.Errorf("Unexpected condition status: %v", cond.Status)
return nil, fmt.Errorf("unexpected condition status: %v", cond.Status)

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / Test on Kubernetes (v1.33.0) 

undefined: fmt

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / Test on Kubernetes (v1.32.0) 

undefined: fmt

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / lint 

undefined: fmt (typecheck)

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / lint 

undefined: fmt) (typecheck)

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / Test on Kubernetes (v1.31.0) 

undefined: fmt

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / Test on Kubernetes (v1.32.0) 

undefined: fmt

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / init with config file (v1.31.0) 

undefined: fmt

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / init with config file (v1.33.0) 

undefined: fmt

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / init with config file (v1.32.0) 

undefined: fmt

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / Test on Kubernetes (v1.32.0) 

undefined: fmt

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / Test on Kubernetes (v1.31.0) 

undefined: fmt

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / Test on Kubernetes (v1.33.0) 

undefined: fmt

Check failure on line 203 in pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go

View workflow job for this annotation

GitHub Actions / compile 

undefined: fmt
}
}
if !applied {
Expand Down
9 changes: 6 additions & 3 deletions pkg/karmadactl/get/get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,8 @@ func (g *CommandGetOptions) watchMultiClusterObj(watchObjs []WatchObj, mapping *
go func(watchObj WatchObj) {
obj, err := watchObj.r.Object()
if err != nil {
panic(err)
klog.Errorf("Failed to get object: %v", err)
return
}

rv := "0"
Expand All @@ -761,7 +762,8 @@ func (g *CommandGetOptions) watchMultiClusterObj(watchObjs []WatchObj, mapping *
// an initial watch event
rv, err = meta.NewAccessor().ResourceVersion(obj)
if err != nil {
panic(err)
klog.Errorf("Failed to get resource version: %v", err)
return
}
// we can start outputting objects now, watches started from lists don't emit synthetic added events
*outputObjects = true
Expand All @@ -773,7 +775,8 @@ func (g *CommandGetOptions) watchMultiClusterObj(watchObjs []WatchObj, mapping *
// print watched changes
w, err := watchObj.r.Watch(rv)
if err != nil {
panic(err)
klog.Errorf("Failed to start watch: %v", err)
return
}

ctx, cancel := context.WithCancel(context.Background())
Expand Down
15 changes: 8 additions & 7 deletions pkg/search/proxy/store/resource_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package store

import (
"context"
"fmt"
"sync"

"k8s.io/apimachinery/pkg/api/meta"
Expand Down Expand Up @@ -163,31 +164,31 @@ func (s *simpleRESTCreateStrategy) NamespaceScoped() bool {
}

func (s *simpleRESTCreateStrategy) ObjectKinds(runtime.Object) ([]schema.GroupVersionKind, bool, error) {
panic("simpleRESTCreateStrategy.ObjectKinds is not supported")
return nil, false, fmt.Errorf("simpleRESTCreateStrategy.ObjectKinds is not supported")
}

func (s *simpleRESTCreateStrategy) Recognizes(schema.GroupVersionKind) bool {
panic("simpleRESTCreateStrategy.Recognizes is not supported")
return false
}

func (s *simpleRESTCreateStrategy) GenerateName(string) string {
panic("simpleRESTCreateStrategy.GenerateName is not supported")
return ""
}

func (s *simpleRESTCreateStrategy) PrepareForCreate(context.Context, runtime.Object) {
panic("simpleRESTCreateStrategy.PrepareForCreate is not supported")
// No-op implementation
}

func (s *simpleRESTCreateStrategy) Validate(context.Context, runtime.Object) field.ErrorList {
panic("simpleRESTCreateStrategy.Validate is not supported")
return field.ErrorList{}
}

func (s *simpleRESTCreateStrategy) WarningsOnCreate(context.Context, runtime.Object) []string {
panic("simpleRESTCreateStrategy.WarningsOnCreate is not supported")
return nil
}

func (s *simpleRESTCreateStrategy) Canonicalize(runtime.Object) {
panic("simpleRESTCreateStrategy.Canonicalize is not supported")
// No-op implementation
}

func restCreateStrategy(namespaced bool) rest.RESTCreateStrategy {
Expand Down
6 changes: 5 additions & 1 deletion pkg/util/gclient/gclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package gclient

import (
"fmt"
"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/client-go/kubernetes/scheme"
Expand Down Expand Up @@ -70,10 +71,13 @@ func NewForConfig(config *rest.Config) (client.Client, error) {

// NewForConfigOrDie creates a new client for the given config and
// panics if there is an error in the config.
// Note: This function is kept for backward compatibility but should be avoided in new code.
// Use NewForConfig instead for proper error handling.
func NewForConfigOrDie(config *rest.Config) client.Client {
c, err := NewForConfig(config)
if err != nil {
panic(err)
// Log the error before panicking for better debugging
panic(fmt.Sprintf("failed to create client: %v", err))
Copy link

Choose a reason for hiding this comment

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

medium

Using fmt.Sprintf here converts the error to a string, which can cause the loss of the original error's type and stack trace. It's better to use fmt.Errorf with the %w verb to wrap the original error. This preserves the error context, which is valuable for debugging when a panic is recovered.

Suggested change
panic(fmt.Sprintf("failed to create client: %v", err))
panic(fmt.Errorf("failed to create client: %w", err))

}
return c
}
3 changes: 2 additions & 1 deletion pkg/util/helper/workstatus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,8 @@ func assembleWorkStatus(works []workv1alpha1.Work, objRef workv1alpha2.ObjectRef
applied = false
appliedMsg = cond.Message
default: // should not happen unless the condition api changed.
panic("unexpected status")
klog.Errorf("Unexpected condition status: %v", cond.Status)
return nil, fmt.Errorf("unexpected condition status: %v", cond.Status)
}
}
if !applied {
Expand Down
Loading