refactor(quota): new error types for quota checking (#8726)

Signed-off-by: He Weiwei <hweiwei@vmware.com>

src/common/quota/errors.go

@@ -0,0 +1,111 @@
+// Copyright Project Harbor Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package quota
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/goharbor/harbor/src/pkg/types"
+)
+
+// Errors contains all happened errors
+type Errors []error
+
+// GetErrors gets all errors that have occurred and returns a slice of errors (Error type)
+func (errs Errors) GetErrors() []error {
+	return errs
+}
+
+// Add adds an error to a given slice of errors
+func (errs Errors) Add(newErrors ...error) Errors {
+	for _, err := range newErrors {
+		if err == nil {
+			continue
+		}
+
+		if errors, ok := err.(Errors); ok {
+			errs = errs.Add(errors...)
+		} else {
+			ok = true
+			for _, e := range errs {
+				if err == e {
+					ok = false
+				}
+			}
+			if ok {
+				errs = append(errs, err)
+			}
+		}
+	}
+
+	return errs
+}
+
+// Error takes a slice of all errors that have occurred and returns it as a formatted string
+func (errs Errors) Error() string {
+	var errors = []string{}
+	for _, e := range errs {
+		errors = append(errors, e.Error())
+	}
+	return strings.Join(errors, "; ")
+}
+
+// ResourceOverflow ...
+type ResourceOverflow struct {
+	Resource    types.ResourceName
+	HardLimit   int64
+	CurrentUsed int64
+	NewUsed     int64
+}
+
+func (e *ResourceOverflow) Error() string {
+	resource := e.Resource
+	var (
+		op    string
+		delta int64
+	)
+
+	if e.NewUsed > e.CurrentUsed {
+		op = "add"
+		delta = e.NewUsed - e.CurrentUsed
+	} else {
+		op = "subtract"
+		delta = e.CurrentUsed - e.NewUsed
+	}
+
+	return fmt.Sprintf("%s %s of %s resource overflow the hard limit, current usage is %s and hard limit is %s",
+		op, resource.FormatValue(delta), resource,
+		resource.FormatValue(e.CurrentUsed), resource.FormatValue(e.HardLimit))
+}
+
+// NewResourceOverflowError ...
+func NewResourceOverflowError(resource types.ResourceName, hardLimit, currentUsed, newUsed int64) error {
+	return &ResourceOverflow{Resource: resource, HardLimit: hardLimit, CurrentUsed: currentUsed, NewUsed: newUsed}
+}
+
+// ResourceNotFound ...
+type ResourceNotFound struct {
+	Resource types.ResourceName
+}
+
+func (e *ResourceNotFound) Error() string {
+	return fmt.Sprintf("resource %s not found", e.Resource)
+}
+
+// NewResourceNotFoundError ...
+func NewResourceNotFoundError(resource types.ResourceName) error {
+	return &ResourceNotFound{Resource: resource}
+}

src/common/quota/manager.go

@@ -131,7 +131,13 @@ func (m *Manager) updateUsage(o orm.Ormer, resources types.ResourceList,
 	}
 
 	newUsed := calculate(used, resources)
-	if err := isSafe(hardLimits, newUsed); err != nil {
+
+	// ensure that new used is never negative
+	if negativeUsed := types.IsNegative(newUsed); len(negativeUsed) > 0 {
+		return fmt.Errorf("quota usage is negative for resource(s): %s", prettyPrintResourceNames(negativeUsed))
+	}
+
+	if err := isSafe(hardLimits, used, newUsed); err != nil {
 		return err
 	}
 
@@ -213,6 +219,9 @@ func (m *Manager) EnsureQuota(usages types.ResourceList) error {
 	// existent
 	used := usages
 	quotaUsed, err := types.NewResourceList(quotas[0].Used)
+	if err != nil {
+		return err
+	}
 	if types.Equals(quotaUsed, used) {
 		return nil
 	}

src/common/quota/manager_test.go

@@ -200,7 +200,11 @@ func (suite *ManagerSuite) TestAddResources() {
 	}
 
 	if err := mgr.AddResources(types.ResourceList{types.ResourceStorage: 10000}); suite.Error(err) {
-		suite.True(IsUnsafeError(err))
+		if errs, ok := err.(Errors); suite.True(ok) {
+			for _, err := range errs {
+				suite.IsType(&ResourceOverflow{}, err)
+			}
+		}
 	}
 }
 

src/common/quota/util.go

@@ -15,48 +15,43 @@
 package quota
 
 import (
-	"fmt"
+	"sort"
+	"strings"
 
 	"github.com/goharbor/harbor/src/pkg/types"
 )
 
-type unsafe struct {
+func isSafe(hardLimits types.ResourceList, currentUsed types.ResourceList, newUsed types.ResourceList) error {
-	message string
+	var errs Errors
-}
 
-func (err *unsafe) Error() string {
+	for resource, value := range newUsed {
-	return err.message
+		hardLimit, found := hardLimits[resource]
-}
+		if !found {
-
+			errs = errs.Add(NewResourceNotFoundError(resource))
-func newUnsafe(message string) error {
-	return &unsafe{message: message}
-}
-
-// IsUnsafeError returns true when the err is unsafe error
-func IsUnsafeError(err error) bool {
-	_, ok := err.(*unsafe)
-	return ok
-}
-
-func isSafe(hardLimits types.ResourceList, used types.ResourceList) error {
-	for key, value := range used {
-		if value < 0 {
-			return newUnsafe(fmt.Sprintf("bad used value: %d", value))
-		}
-
-		if hard, found := hardLimits[key]; found {
-			if hard == types.UNLIMITED {
 			continue
 		}
 
-			if value > hard {
+		if hardLimit == types.UNLIMITED || value == currentUsed[resource] {
-				return newUnsafe(fmt.Sprintf("over the quota: used %d but only hard %d", value, hard))
+			continue
-			}
-		} else {
-			return newUnsafe(fmt.Sprintf("hard limit not found: %s", key))
 		}
 
+		if value > hardLimit {
+			errs = errs.Add(NewResourceOverflowError(resource, hardLimit, currentUsed[resource], value))
+		}
+	}
+
+	if len(errs) > 0 {
+		return errs
 	}
 
 	return nil
 }
+
+func prettyPrintResourceNames(a []types.ResourceName) string {
+	values := []string{}
+	for _, value := range a {
+		values = append(values, string(value))
+	}
+	sort.Strings(values)
+	return strings.Join(values, ",")
+}

src/common/quota/util_test.go

@@ -15,45 +15,16 @@
 package quota
 
 import (
-	"errors"
 	"testing"
 
 	"github.com/goharbor/harbor/src/pkg/types"
 )
 
-func TestIsUnsafeError(t *testing.T) {
+func Test_isSafe(t *testing.T) {
-	type args struct {
-		err error
-	}
-	tests := []struct {
-		name string
-		args args
-		want bool
-	}{
-		{
-			"is unsafe error",
-			args{err: newUnsafe("unsafe")},
-			true,
-		},
-		{
-			"is not unsafe error",
-			args{err: errors.New("unsafe")},
-			false,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if got := IsUnsafeError(tt.args.err); got != tt.want {
-				t.Errorf("IsUnsafeError() = %v, want %v", got, tt.want)
-			}
-		})
-	}
-}
-
-func Test_checkQuotas(t *testing.T) {
 	type args struct {
 		hardLimits  types.ResourceList
-		used       types.ResourceList
+		currentUsed types.ResourceList
+		newUsed     types.ResourceList
 	}
 	tests := []struct {
 		name    string
@@ -62,33 +33,44 @@ func Test_checkQuotas(t *testing.T) {
 	}{
 		{
 			"unlimited",
-			args{hardLimits: types.ResourceList{types.ResourceStorage: types.UNLIMITED}, used: types.ResourceList{types.ResourceStorage: 1000}},
+			args{
+				types.ResourceList{types.ResourceStorage: types.UNLIMITED},
+				types.ResourceList{types.ResourceStorage: 1000},
+				types.ResourceList{types.ResourceStorage: 1000},
+			},
 			false,
 		},
 		{
 			"ok",
-			args{hardLimits: types.ResourceList{types.ResourceStorage: 100}, used: types.ResourceList{types.ResourceStorage: 1}},
+			args{
+				types.ResourceList{types.ResourceStorage: 100},
+				types.ResourceList{types.ResourceStorage: 10},
+				types.ResourceList{types.ResourceStorage: 1},
+			},
 			false,
 		},
-		{
-			"bad used value",
-			args{hardLimits: types.ResourceList{types.ResourceStorage: 100}, used: types.ResourceList{types.ResourceStorage: -1}},
-			true,
-		},
 		{
 			"over the hard limit",
-			args{hardLimits: types.ResourceList{types.ResourceStorage: 100}, used: types.ResourceList{types.ResourceStorage: 200}},
+			args{
+				types.ResourceList{types.ResourceStorage: 100},
+				types.ResourceList{types.ResourceStorage: 0},
+				types.ResourceList{types.ResourceStorage: 200},
+			},
 			true,
 		},
 		{
 			"hard limit not found",
-			args{hardLimits: types.ResourceList{types.ResourceStorage: 100}, used: types.ResourceList{types.ResourceCount: 1}},
+			args{
+				types.ResourceList{types.ResourceStorage: 100},
+				types.ResourceList{types.ResourceCount: 0},
+				types.ResourceList{types.ResourceCount: 1},
+			},
 			true,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if err := isSafe(tt.args.hardLimits, tt.args.used); (err != nil) != tt.wantErr {
+			if err := isSafe(tt.args.hardLimits, tt.args.currentUsed, tt.args.newUsed); (err != nil) != tt.wantErr {
 				t.Errorf("isSafe() error = %v, wantErr %v", err, tt.wantErr)
 			}
 		})

src/pkg/types/format.go

@@ -0,0 +1,40 @@
+// Copyright Project Harbor Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package types
+
+import (
+	"fmt"
+)
+
+var (
+	resourceValueFormats = map[ResourceName]func(int64) string{
+		ResourceStorage: byteCountToDisplaySize,
+	}
+)
+
+func byteCountToDisplaySize(value int64) string {
+	const unit = 1024
+	if value < unit {
+		return fmt.Sprintf("%d B", value)
+	}
+
+	div, exp := int64(unit), 0
+	for n := value / unit; n >= unit; n /= unit {
+		div *= unit
+		exp++
+	}
+
+	return fmt.Sprintf("%.1f %ciB", float64(value)/float64(div), "KMGTPE"[exp])
+}

src/pkg/types/format_test.go

@@ -0,0 +1,45 @@
+// Copyright Project Harbor Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package types
+
+import "testing"
+
+func Test_byteCountToDisplaySize(t *testing.T) {
+	type args struct {
+		value int64
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{
+		{"100 B", args{100}, "100 B"},
+		{"1.0 KiB", args{1024}, "1.0 KiB"},
+		{"1.5 KiB", args{1024 * 3 / 2}, "1.5 KiB"},
+		{"1.0 MiB", args{1024 * 1024}, "1.0 MiB"},
+		{"1.5 MiB", args{1024 * 1024 * 3 / 2}, "1.5 MiB"},
+		{"1.0 GiB", args{1024 * 1024 * 1024}, "1.0 GiB"},
+		{"1.5 GiB", args{1024 * 1024 * 1024 * 3 / 2}, "1.5 GiB"},
+		{"1.0 TiB", args{1024 * 1024 * 1024 * 1024}, "1.0 TiB"},
+		{"1.5 TiB", args{1024 * 1024 * 1024 * 1024 * 3 / 2}, "1.5 TiB"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := byteCountToDisplaySize(tt.args.value); got != tt.want {
+				t.Errorf("byteCountToDisplaySize() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

src/pkg/types/resources.go

@@ -16,6 +16,7 @@ package types
 
 import (
 	"encoding/json"
+	"strconv"
 )
 
 const (
@@ -31,6 +32,16 @@ const (
 // ResourceName is the name identifying various resources in a ResourceList.
 type ResourceName string
 
+// FormatValue returns string for the resource value
+func (resource ResourceName) FormatValue(value int64) string {
+	format, ok := resourceValueFormats[resource]
+	if ok {
+		return format(value)
+	}
+
+	return strconv.FormatInt(value, 10)
+}
+
 // ResourceList is a set of (resource name, value) pairs.
 type ResourceList map[ResourceName]int64
 
@@ -113,3 +124,14 @@ func Zero(a ResourceList) ResourceList {
 	}
 	return result
 }
+
+// IsNegative returns the set of resource names that have a negative value.
+func IsNegative(a ResourceList) []ResourceName {
+	results := []ResourceName{}
+	for k, v := range a {
+		if v < 0 {
+			results = append(results, k)
+		}
+	}
+	return results
+}

src/pkg/types/resources_test.go

@@ -76,6 +76,11 @@ func (suite *ResourcesSuite) TestZero() {
 	suite.Equal(ResourceList{ResourceStorage: 0, ResourceCount: 0}, Zero(res2))
 }
 
+func (suite *ResourcesSuite) TestIsNegative() {
+	suite.EqualValues([]ResourceName{ResourceStorage}, IsNegative(ResourceList{ResourceStorage: -100, ResourceCount: 100}))
+	suite.EqualValues([]ResourceName{ResourceStorage, ResourceCount}, IsNegative(ResourceList{ResourceStorage: -100, ResourceCount: -100}))
+}
+
 func TestRunResourcesSuite(t *testing.T) {
 	suite.Run(t, new(ResourcesSuite))
 }