Storage: Use Error property for error handling rather than Status (#90416)

replace status with error

pkg/storage/unified/apistore/storage.go

@@ -20,14 +20,14 @@ import (
 	"k8s.io/apimachinery/pkg/conversion"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/apiserver/pkg/storage"
 	"k8s.io/apiserver/pkg/storage/storagebackend"
 	"k8s.io/apiserver/pkg/storage/storagebackend/factory"
 
-	grafanaregistry "github.com/grafana/grafana/pkg/apiserver/registry/generic"
-
 	"github.com/grafana/grafana/pkg/apimachinery/utils"
+	grafanaregistry "github.com/grafana/grafana/pkg/apiserver/registry/generic"
 	"github.com/grafana/grafana/pkg/storage/unified/resource"
 )
 
@@ -69,14 +69,31 @@ func NewStorage(
 	}, nil, nil
 }
 
-func errorWrap(status *resource.StatusResult) error {
+func errorWrap(status *resource.ErrorResult) error {
 	if status != nil {
-		return &apierrors.StatusError{ErrStatus: metav1.Status{
+		err := &apierrors.StatusError{ErrStatus: metav1.Status{
 			Status:  metav1.StatusFailure,
 			Code:    status.Code,
 			Reason:  metav1.StatusReason(status.Reason),
 			Message: status.Message,
 		}}
+		if status.Details != nil {
+			err.ErrStatus.Details = &metav1.StatusDetails{
+				Group:             status.Details.Group,
+				Kind:              status.Details.Kind,
+				Name:              status.Details.Name,
+				UID:               types.UID(status.Details.Uid),
+				RetryAfterSeconds: status.Details.RetryAfterSeconds,
+			}
+			for _, c := range status.Details.Causes {
+				err.ErrStatus.Details.Causes = append(err.ErrStatus.Details.Causes, metav1.StatusCause{
+					Type:    metav1.CauseType(c.Reason),
+					Message: c.Message,
+					Field:   c.Field,
+				})
+			}
+		}
+		return err
 	}
 	return nil
 }
@@ -131,13 +148,13 @@ func (s *Storage) Create(ctx context.Context, key string, obj runtime.Object, ou
 	if err != nil {
 		return err
 	}
-	err = errorWrap(rsp.Status)
+	err = errorWrap(rsp.Error)
 	if err != nil {
 		return err
 	}
 
-	if rsp.Status != nil {
-		return fmt.Errorf("error in status %+v", rsp.Status)
+	if rsp.Error != nil {
+		return fmt.Errorf("error in status %+v", rsp.Error)
 	}
 
 	// Create into the out value
@@ -185,7 +202,7 @@ func (s *Storage) Delete(ctx context.Context, key string, out runtime.Object, pr
 	if err != nil {
 		return err
 	}
-	err = errorWrap(rsp.Status)
+	err = errorWrap(rsp.Error)
 	if err != nil {
 		return err
 	}
@@ -262,7 +279,7 @@ func (s *Storage) Get(ctx context.Context, key string, opts storage.GetOptions,
 	if err != nil {
 		return err
 	}
-	err = errorWrap(rsp.Status)
+	err = errorWrap(rsp.Error)
 	if err != nil {
 		return err
 	}
@@ -499,7 +516,7 @@ func (s *Storage) GuaranteedUpdate(
 	if err != nil {
 		return err
 	}
-	err = errorWrap(rsp.Status)
+	err = errorWrap(rsp.Error)
 	if err != nil {
 		return err
 	}

pkg/storage/unified/resource/cdk_backend.go

@@ -5,6 +5,7 @@ import (
 	context "context"
 	"fmt"
 	"io"
+	"net/http"
 	"sort"
 	"strconv"
 	"strings"
@@ -18,6 +19,7 @@ import (
 	_ "gocloud.dev/blob/fileblob"
 	_ "gocloud.dev/blob/memblob"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
@@ -136,7 +138,7 @@ func (s *cdkBackend) WriteEvent(ctx context.Context, event WriteEvent) (rv int64
 func (s *cdkBackend) Read(ctx context.Context, req *ReadRequest) (*ReadResponse, error) {
 	rv := req.ResourceVersion
 
-	path := s.getPath(req.Key, req.ResourceVersion)
+	path := s.getPath(req.Key, rv)
 	if rv < 1 {
 		iter := s.bucket.List(&blob.ListOptions{Prefix: path + "/", Delimiter: "/"})
 		for {
@@ -159,13 +161,42 @@ func (s *cdkBackend) Read(ctx context.Context, req *ReadRequest) (*ReadResponse,
 	}
 
 	raw, err := s.bucket.ReadAll(ctx, path)
-	if raw == nil || (err == nil && isDeletedMarker(raw)) {
+	if raw == nil && req.ResourceVersion > 0 {
+		if req.ResourceVersion > s.rv.Load() {
+			return nil, &apierrors.StatusError{
+				ErrStatus: metav1.Status{
+					Reason:  metav1.StatusReasonTimeout, // match etcd behavior
+					Code:    http.StatusGatewayTimeout,
+					Message: "ResourceVersion is larger than max",
+					Details: &metav1.StatusDetails{
+						Causes: []metav1.StatusCause{
+							{
+								Type:    metav1.CauseTypeResourceVersionTooLarge,
+								Message: fmt.Sprintf("requested: %d, current %d", req.ResourceVersion, s.rv.Load()),
+							},
+						},
+					},
+				},
+			}
+		}
+
+		// If the there was an explicit request, get the latest
+		rsp, _ := s.Read(ctx, &ReadRequest{Key: req.Key})
+		if rsp != nil && len(rsp.Value) > 0 {
+			raw = rsp.Value
+			rv = rsp.ResourceVersion
+			err = nil
+		}
+	}
+	if err == nil && isDeletedMarker(raw) {
+		raw = nil
+	}
+	if raw == nil {
 		return nil, apierrors.NewNotFound(schema.GroupResource{
 			Group:    req.Key.Group,
 			Resource: req.Key.Resource,
 		}, req.Key.Name)
 	}
-
 	return &ReadResponse{
 		ResourceVersion: rv,
 		Value:           raw,

pkg/storage/unified/resource/resource.proto

@@ -40,26 +40,94 @@ message ResourceMeta {
 
 // Status structure is copied from:
 // https://github.com/kubernetes/apimachinery/blob/v0.30.1/pkg/apis/meta/v1/generated.proto#L979
-message StatusResult {
-  // Status of the operation.
-  // One of: "Success" or "Failure".
-  // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#spec-and-status
-  // +optional
-  string status = 1;
+// However, this is only used for error handling, never for succesful results
+message ErrorResult {
   // A human-readable description of the status of this operation.
   // +optional
-  string message = 2;
+  string message = 1;
+
   // A machine-readable description of why this operation is in the
   // "Failure" status. If this value is empty there
   // is no information available. A Reason clarifies an HTTP status
   // code but does not override it.
   // +optional
-  string reason = 3;
+  string reason = 2;
+
+  // Extended data associated with the reason.  Each reason may define its
+  // own extended details. This field is optional and the data returned
+  // is not guaranteed to conform to any schema except that defined by
+  // the reason type.
+  // +optional
+  // +listType=atomic
+  ErrorDetails details = 3;
+
   // Suggested HTTP return code for this status, 0 if not set.
   // +optional
   int32 code = 4;
 }
 
+// ErrorDetails is a set of additional properties that MAY be set by the
+// server to provide additional information about a response. The Reason
+// field of a Status object defines what attributes will be set. Clients
+// must ignore fields that do not match the defined type of each attribute,
+// and should assume that any attribute may be empty, invalid, or under
+// defined.
+message ErrorDetails {
+  // The name attribute of the resource associated with the status StatusReason
+  // (when there is a single name which can be described).
+  // +optional
+  string name = 1;
+
+  // The group attribute of the resource associated with the status StatusReason.
+  // +optional
+  string group = 2;
+
+  // The kind attribute of the resource associated with the status StatusReason.
+  // On some operations may differ from the requested resource Kind.
+  // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
+  // +optional
+  string kind = 3;
+
+  // UID of the resource.
+  // (when there is a single resource which can be described).
+  // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names#uids
+  // +optional
+  string uid = 6;
+
+  // The Causes array includes more details associated with the StatusReason
+  // failure. Not all StatusReasons may provide detailed causes.
+  // +optional
+  // +listType=atomic
+  repeated ErrorCause causes = 4;
+
+  // If specified, the time in seconds before the operation should be retried. Some errors may indicate
+  // the client must take an alternate action - for those errors this field may indicate how long to wait
+  // before taking the alternate action.
+  // +optional
+  int32 retryAfterSeconds = 5;
+}
+
+message ErrorCause {
+	// A machine-readable description of the cause of the error. If this value is
+	// empty there is no information available.
+	string reason = 1;
+	// A human-readable description of the cause of the error.  This field may be
+	// presented as-is to a reader.
+	// +optional
+	string message = 2;
+	// The field of the resource that has caused this error, as named by its JSON
+	// serialization. May include dot and postfix notation for nested attributes.
+	// Arrays are zero-indexed.  Fields may appear more than once in an array of
+	// causes due to fields having multiple errors.
+	// Optional.
+	//
+	// Examples:
+	//   "name" - the field "name" on the current resource
+	//   "items[0].name" - the field "name" on the first array entry in "items"
+	// +optional
+	string field = 3;
+}
+
 // ----------------------------------
 // CRUD Objects
 // ----------------------------------
@@ -75,8 +143,8 @@ message CreateRequest {
 }
 
 message CreateResponse {
-  // Status code
-  StatusResult status = 1;
+  // Error details
+  ErrorResult error = 1;
 
   // The updated resource version
   int64 resource_version = 2;
@@ -97,8 +165,8 @@ message UpdateRequest {
 }
 
 message UpdateResponse {
-  // Status code
-  StatusResult status = 1;
+  // Error details
+  ErrorResult error = 1;
 
   // The updated resource version
   int64 resource_version = 2;
@@ -119,8 +187,8 @@ message DeleteRequest {
 }
 
 message DeleteResponse {
-  // Status code
-  StatusResult status = 1;
+  // Error details
+  ErrorResult error = 1;
 
   // The resource version for the deletion marker
   int64 resource_version = 2;
@@ -134,8 +202,8 @@ message ReadRequest {
 }
 
 message ReadResponse {
-  // Status code
-  StatusResult status = 1;
+  // Error details
+  ErrorResult error = 1;
 
   // The new resource version
   int64 resource_version = 2;

pkg/storage/unified/resource/server.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"net/http"
 	"sync"
 	"time"
 
@@ -262,9 +263,18 @@ func (s *server) Create(ctx context.Context, req *CreateRequest) (*CreateRespons
 	}
 
 	rsp := &CreateResponse{}
+	found, _ := s.backend.Read(ctx, &ReadRequest{Key: req.Key})
+	if found != nil && len(found.Value) > 0 {
+		rsp.Error = &ErrorResult{
+			Code:    http.StatusConflict,
+			Message: "key already exists",
+		}
+		return rsp, nil
+	}
+
 	builder, err := s.newEventBuilder(ctx, req.Key, req.Value, nil)
 	if err != nil {
-		rsp.Status, err = errToStatus(err)
+		rsp.Error, err = errToStatus(err)
 		return rsp, err
 	}
 
@@ -277,7 +287,7 @@ func (s *server) Create(ctx context.Context, req *CreateRequest) (*CreateRespons
 
 	event, err := builder.toEvent()
 	if err != nil {
-		rsp.Status, err = errToStatus(err)
+		rsp.Error, err = errToStatus(err)
 		return rsp, err
 	}
 
@@ -285,28 +295,43 @@ func (s *server) Create(ctx context.Context, req *CreateRequest) (*CreateRespons
 	if err == nil {
 		rsp.Value = event.Value // with mutated fields
 	} else {
-		rsp.Status, err = errToStatus(err)
+		rsp.Error, err = errToStatus(err)
 	}
 	return rsp, err
 }
 
 // Convert golang errors to status result errors that can be returned to a client
-func errToStatus(err error) (*StatusResult, error) {
+func errToStatus(err error) (*ErrorResult, error) {
 	if err != nil {
 		apistatus, ok := err.(apierrors.APIStatus)
 		if ok {
 			s := apistatus.Status()
-			return &StatusResult{
-				Status:  s.Status,
+			res := &ErrorResult{
 				Message: s.Message,
 				Reason:  string(s.Reason),
 				Code:    s.Code,
-			}, nil
+			}
+			if s.Details != nil {
+				res.Details = &ErrorDetails{
+					Group:             s.Details.Group,
+					Kind:              s.Details.Kind,
+					Name:              s.Details.Name,
+					Uid:               string(s.Details.UID),
+					RetryAfterSeconds: s.Details.RetryAfterSeconds,
+				}
+				for _, c := range s.Details.Causes {
+					res.Details.Causes = append(res.Details.Causes, &ErrorCause{
+						Reason:  string(c.Type),
+						Message: c.Message,
+						Field:   c.Field,
+					})
+				}
+			}
+			return res, nil
 		}
 
-		// TODO... better conversion!!!
-		return &StatusResult{
-			Status:  "Failure",
+		// TODO... better conversion??
+		return &ErrorResult{
 			Message: err.Error(),
 			Code:    500,
 		}, nil
@@ -324,7 +349,7 @@ func (s *server) Update(ctx context.Context, req *UpdateRequest) (*UpdateRespons
 
 	rsp := &UpdateResponse{}
 	if req.ResourceVersion < 0 {
-		rsp.Status, _ = errToStatus(apierrors.NewBadRequest("update must include the previous version"))
+		rsp.Error, _ = errToStatus(apierrors.NewBadRequest("update must include the previous version"))
 		return rsp, nil
 	}
 
@@ -344,7 +369,7 @@ func (s *server) Update(ctx context.Context, req *UpdateRequest) (*UpdateRespons
 
 	builder, err := s.newEventBuilder(ctx, req.Key, req.Value, latest.Value)
 	if err != nil {
-		rsp.Status, err = errToStatus(err)
+		rsp.Error, err = errToStatus(err)
 		return rsp, err
 	}
 
@@ -354,7 +379,7 @@ func (s *server) Update(ctx context.Context, req *UpdateRequest) (*UpdateRespons
 
 	event, err := builder.toEvent()
 	if err != nil {
-		rsp.Status, err = errToStatus(err)
+		rsp.Error, err = errToStatus(err)
 		return rsp, err
 	}
 
@@ -362,11 +387,11 @@ func (s *server) Update(ctx context.Context, req *UpdateRequest) (*UpdateRespons
 	event.PreviousRV = latest.ResourceVersion
 
 	rsp.ResourceVersion, err = s.backend.WriteEvent(ctx, event)
-	rsp.Status, err = errToStatus(err)
+	rsp.Error, err = errToStatus(err)
 	if err == nil {
 		rsp.Value = event.Value // with mutated fields
 	} else {
-		rsp.Status, err = errToStatus(err)
+		rsp.Error, err = errToStatus(err)
 	}
 	return rsp, err
 }
@@ -431,7 +456,7 @@ func (s *server) Delete(ctx context.Context, req *DeleteRequest) (*DeleteRespons
 	}
 
 	rsp.ResourceVersion, err = s.backend.WriteEvent(ctx, event)
-	rsp.Status, err = errToStatus(err)
+	rsp.Error, err = errToStatus(err)
 	return rsp, err
 }
 
@@ -446,7 +471,7 @@ func (s *server) Read(ctx context.Context, req *ReadRequest) (*ReadResponse, err
 	// }
 	if req.Key.Resource == "" {
 		status, _ := errToStatus(apierrors.NewBadRequest("missing resource"))
-		return &ReadResponse{Status: status}, nil
+		return &ReadResponse{Error: status}, nil
 	}
 
 	rsp, err := s.backend.Read(ctx, req)
@@ -454,7 +479,7 @@ func (s *server) Read(ctx context.Context, req *ReadRequest) (*ReadResponse, err
 		if rsp == nil {
 			rsp = &ReadResponse{}
 		}
-		rsp.Status, err = errToStatus(err)
+		rsp.Error, err = errToStatus(err)
 	}
 	return rsp, err
 }

pkg/storage/unified/resource/server_test.go

@@ -97,13 +97,13 @@ func TestSimpleServer(t *testing.T) {
 			Key:   key,
 		})
 		require.NoError(t, err)
-		require.Nil(t, created.Status)
+		require.Nil(t, created.Error)
 		require.True(t, created.ResourceVersion > 0)
 
 		// The key does not include resource version
 		found, err := server.Read(ctx, &ReadRequest{Key: key})
 		require.NoError(t, err)
-		require.Nil(t, found.Status)
+		require.Nil(t, found.Error)
 		require.Equal(t, created.ResourceVersion, found.ResourceVersion)
 
 		// Now update the value
@@ -125,13 +125,13 @@ func TestSimpleServer(t *testing.T) {
 			Value:           raw,
 			ResourceVersion: created.ResourceVersion})
 		require.NoError(t, err)
-		require.Nil(t, updated.Status)
+		require.Nil(t, updated.Error)
 		require.True(t, updated.ResourceVersion > created.ResourceVersion)
 
 		// We should still get the latest
 		found, err = server.Read(ctx, &ReadRequest{Key: key})
 		require.NoError(t, err)
-		require.Nil(t, found.Status)
+		require.Nil(t, found.Error)
 		require.Equal(t, updated.ResourceVersion, found.ResourceVersion)
 
 		all, err = server.List(ctx, &ListRequest{Options: &ListOptions{
@@ -151,8 +151,8 @@ func TestSimpleServer(t *testing.T) {
 		// We should get not found status when trying to read the latest value
 		found, err = server.Read(ctx, &ReadRequest{Key: key})
 		require.NoError(t, err)
-		require.NotNil(t, found.Status)
-		require.Equal(t, int32(404), found.Status.Code)
+		require.NotNil(t, found.Error)
+		require.Equal(t, int32(404), found.Error.Code)
 
 		// And the deleted value should not be in the results
 		all, err = server.List(ctx, &ListRequest{Options: &ListOptions{