Skip to content

Commit

Permalink
fix: Improve error handling logic in API server (higress-group#92)
Browse files Browse the repository at this point in the history 
1. Wrap all the errors returned by REST implementations with apierrors package
2. Improve the error messages returned by fileREST
  • Loading branch information
@CH3CHO
CH3CHO authored Jun 22, 2024
1 parent 8161bc8 commit 4353591
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 46 deletions.
56 changes: 28 additions & 28 deletions src/apiserver/pkg/registry/file_rest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"context"
"errors"
"fmt"
"k8s.io/klog/v2"
"os"
"path/filepath"
"reflect"
Expand All @@ -26,14 +25,12 @@ import (
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
"k8s.io/apiserver/pkg/registry/rest"
"k8s.io/apiserver/pkg/storage"
"k8s.io/klog/v2"

"github.com/alibaba/higress/api-server/pkg/utils"
"github.com/fsnotify/fsnotify"
)

// ErrFileNotExists means the file doesn't actually exist.
var ErrFileNotExists = fmt.Errorf("file doesn't exist")

const fileChangeProcessInterval = 100 * time.Millisecond
const fileChangeProcessDelay = 250 * time.Millisecond
const defaultNamespace = "higress-system"
Expand Down Expand Up @@ -260,7 +257,10 @@ func (f *fileREST) Get(
return nil, apierrors.NewNotFound(groupResource, name)
}
klog.Infof("[%s] %s got", f.groupResource, name)
return obj, err
if err == nil {
return obj, nil
}
return obj, apierrors.NewInternalError(err)
}

func (f *fileREST) normalizeObject() {
Expand All @@ -285,7 +285,7 @@ func (f *fileREST) List(
newListObj := f.NewList()
v, err := getListPrt(newListObj)
if err != nil {
return nil, err
return nil, apierrors.NewInternalError(err)
}

count := 0
Expand All @@ -295,7 +295,7 @@ func (f *fileREST) List(
appendItem(v, obj)
}
}); err != nil {
return newListObj, nil
return nil, apierrors.NewInternalError(err)
}

klog.Infof("[%s] list count=%d", f.groupResource, count)
Expand All @@ -311,7 +311,7 @@ func (f *fileREST) Create(
) (runtime.Object, error) {
if createValidation != nil {
if err := createValidation(ctx, obj); err != nil {
return nil, err
return nil, apierrors.NewInternalError(err)
}
}

Expand All @@ -328,7 +328,7 @@ func (f *fileREST) Create(

accessor, err := meta.Accessor(obj)
if err != nil {
return nil, err
return nil, apierrors.NewInternalError(err)
}

name := accessor.GetName()
Expand All @@ -346,7 +346,7 @@ func (f *fileREST) Create(
panic(fmt.Sprintf("unable to create data dir: %s", err))
}
if err := f.write(f.codec, filename, obj); err != nil {
return nil, err
return nil, apierrors.NewInternalError(err)
}

f.notifyWatchers(watch.Event{
Expand Down Expand Up @@ -377,32 +377,32 @@ func (f *fileREST) Update(

updatedObj, err := objInfo.UpdatedObject(ctx, oldObj)
if err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
filename := f.objectFileName(ctx, name)

oldAccessor, err := meta.Accessor(oldObj)
if err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}

updatedAccessor, err := meta.Accessor(updatedObj)
if err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}

if isCreate {
if createValidation != nil {
if err := createValidation(ctx, updatedObj); err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
}

updatedAccessor.SetCreationTimestamp(metav1.NewTime(time.Now()))
updatedAccessor.SetResourceVersion("1")

if err := f.write(f.codec, filename, updatedObj); err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
f.notifyWatchers(watch.Event{
Type: watch.Added,
Expand All @@ -413,7 +413,7 @@ func (f *fileREST) Update(

if updateValidation != nil {
if err := updateValidation(ctx, updatedObj, oldObj); err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
}

Expand All @@ -434,14 +434,14 @@ func (f *fileREST) Update(
} else {
newResourceVersion, err = strconv.ParseUint(currentResourceVersion, 10, 64)
if err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
newResourceVersion++
}
updatedAccessor.SetResourceVersion(strconv.FormatUint(newResourceVersion, 10))

if err := f.write(f.codec, filename, updatedObj); err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}

f.notifyWatchers(watch.Event{
Expand All @@ -458,21 +458,21 @@ func (f *fileREST) Delete(
options *metav1.DeleteOptions) (runtime.Object, bool, error) {
filename := f.objectFileName(ctx, name)
if !utils.Exists(filename) {
return nil, false, ErrFileNotExists
return nil, false, apierrors.NewNotFound(f.groupResource, name)
}

oldObj, err := f.Get(ctx, name, nil)
if err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
if deleteValidation != nil {
if err := deleteValidation(ctx, oldObj); err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
}

if err := os.Remove(filename); err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
f.notifyWatchers(watch.Event{
Type: watch.Deleted,
Expand All @@ -490,13 +490,13 @@ func (f *fileREST) DeleteCollection(
newListObj := f.NewList()
v, err := getListPrt(newListObj)
if err != nil {
return nil, err
return nil, apierrors.NewInternalError(err)
}
if err := f.visitDir(f.objRootPath, f.objExtension, f.newFunc, f.codec, func(path string, obj runtime.Object) {
_ = os.Remove(path)
appendItem(v, obj)
}); err != nil {
return nil, fmt.Errorf("failed walking filepath %v", f.objRootPath)
return nil, apierrors.NewInternalError(fmt.Errorf("failed walking filepath %v: %v", f.objRootPath, err))
}
return newListObj, nil
}
Expand Down Expand Up @@ -526,17 +526,17 @@ func (f *fileREST) read(decoder runtime.Decoder, path string, newFunc func() run
if errors.Is(err, os.ErrNotExist) {
return nil, nil
} else {
return nil, err
return nil, fmt.Errorf("failed to stat file [%s]: %v", path, err)
}
}
content, err := os.ReadFile(cleanedPath)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to read file [%s]: %v", path, err)
}
newObj := newFunc()
decodedObj, _, err := decoder.Decode(content, nil, newObj)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to decode data read from file [%s]: %v\n%s", path, err, content)
}
f.normalizeObjectMeta(decodedObj, cleanedPath)
return decodedObj, nil
Expand Down Expand Up @@ -568,7 +568,7 @@ func (f *fileREST) visitDir(dirname string, extension string, newFunc func() run
}
newObj, err := f.read(codec, path, newFunc)
if err != nil {
return err
return fmt.Errorf("failed to load data from file [%s]: %v", path, err)
}
visitFunc(path, newObj)
return nil
Expand Down
39 changes: 21 additions & 18 deletions src/apiserver/pkg/registry/nacos_rest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,10 @@ func (n *nacosREST) Get(
return nil, apierrors.NewNotFound(groupResource, name)
}
klog.Infof("[%s] %s/%s got", n.groupResource, ns, name)
return obj, err
if err == nil {
return obj, nil
}
return obj, apierrors.NewInternalError(err)
}

func (n *nacosREST) List(
Expand Down Expand Up @@ -246,7 +249,7 @@ func (n *nacosREST) List(
}
})
if err != nil {
return nil, err
return nil, apierrors.NewInternalError(err)
}

klog.Infof("[%s] %s list count=%d", n.groupResource, ns, count)
Expand All @@ -261,13 +264,13 @@ func (n *nacosREST) Create(
) (runtime.Object, error) {
if createValidation != nil {
if err := createValidation(ctx, obj); err != nil {
return nil, err
return nil, apierrors.NewInternalError(err)
}
}

accessor, err := meta.Accessor(obj)
if err != nil {
return nil, err
return nil, apierrors.NewInternalError(err)
}

ns, _ := genericapirequest.NamespaceFrom(ctx)
Expand All @@ -283,7 +286,7 @@ func (n *nacosREST) Create(

accessor.SetCreationTimestamp(metav1.NewTime(time.Now()))
if err := n.write(n.codec, ns, dataId, "", obj); err != nil {
return nil, err
return nil, apierrors.NewInternalError(err)
}

nameKey := ns + "/" + name
Expand Down Expand Up @@ -316,38 +319,38 @@ func (n *nacosREST) Update(
isCreate := false
oldObj, oldConfig, err := n.read(n.codec, ns, dataId, n.newFunc)
if err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
if oldConfig == "" && err == nil {
if oldConfig == "" {
if !forceAllowCreate {
return nil, false, err
return nil, false, apierrors.NewNotFound(n.groupResource, name)
}
isCreate = true
}

updatedObj, err := objInfo.UpdatedObject(ctx, oldObj)
if err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}

oldAccessor, err := meta.Accessor(oldObj)
if err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}

updatedAccessor, err := meta.Accessor(updatedObj)
if err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}

if isCreate {
obj, err := n.Create(ctx, updatedObj, createValidation, nil)
return obj, err == nil, err
return obj, err == nil, apierrors.NewInternalError(err)
}

if updateValidation != nil {
if err := updateValidation(ctx, updatedObj, oldObj); err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
}

Expand All @@ -356,7 +359,7 @@ func (n *nacosREST) Update(
}

if err := n.write(n.codec, ns, dataId, oldAccessor.GetResourceVersion(), updatedObj); err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}

return updatedObj, false, nil
Expand All @@ -371,11 +374,11 @@ func (n *nacosREST) Delete(

oldObj, err := n.Get(ctx, name, nil)
if err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
if deleteValidation != nil {
if err := deleteValidation(ctx, oldObj); err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
}

Expand All @@ -385,7 +388,7 @@ func (n *nacosREST) Delete(
Group: ns,
})
if err != nil {
return nil, false, err
return nil, false, apierrors.NewInternalError(err)
}
if !deleted {
return nil, false, errors.New("delete config failed: " + dataId)
Expand Down Expand Up @@ -424,7 +427,7 @@ func (n *nacosREST) DeleteCollection(
deletedItems := n.NewList()
v, err := getListPrt(deletedItems)
if err != nil {
return nil, err
return nil, apierrors.NewInternalError(err)
}

for _, obj := range list.(*unstructured.UnstructuredList).Items {
Expand Down

0 comments on commit 4353591

Please sign in to comment.