diff --git a/pkg/project/apiserver/registry/project/proxy/proxy.go b/pkg/project/apiserver/registry/project/proxy/proxy.go index 4486c2d8f6..3437dac22a 100644 --- a/pkg/project/apiserver/registry/project/proxy/proxy.go +++ b/pkg/project/apiserver/registry/project/proxy/proxy.go @@ -3,11 +3,13 @@ package proxy import ( "context" "fmt" + "time" kerrors "k8s.io/apimachinery/pkg/api/errors" metainternal "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/watch" apirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" @@ -208,11 +210,84 @@ func (s *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObje var _ = rest.GracefulDeleter(&REST{}) +// maxRetriesOnConflict is the maximum retry count for Delete calls which +// result in resource conflicts. +const maxRetriesOnConflict = 10 + +// maxDuration set max duration of delete retries. Deleting a project affects apiserver latency, +// so this should be kept as small as possible +const maxDuration = time.Second + // Delete deletes a Project specified by its name func (s *REST) Delete(ctx context.Context, name string, objectFunc rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) { var opts metav1.DeleteOptions if options != nil { opts = *options } - return &metav1.Status{Status: metav1.StatusSuccess}, false, s.client.Delete(ctx, name, opts) + var lastErr error + err := wait.ExponentialBackoffWithContext(ctx, wait.Backoff{Steps: maxRetriesOnConflict, Duration: maxDuration}, func(ctx context.Context) (bool, error) { + var err error + if objectFunc != nil { + var obj runtime.Object + getOpts := metav1.GetOptions{} + if opts.Preconditions != nil && opts.Preconditions.ResourceVersion != nil { + getOpts.ResourceVersion = *opts.Preconditions.ResourceVersion + } + obj, err = s.Get(ctx, name, &getOpts) + if err != nil { + lastErr = fmt.Errorf("unable to get project: %w", err) + return false, nil + } + projectObj, ok := obj.(*projectapi.Project) + if !ok || projectObj == nil { + lastErr = fmt.Errorf("not a project: %#v", obj) + return false, nil + } + if opts.Preconditions == nil { + opts.Preconditions = &metav1.Preconditions{} + } + if options.Preconditions != nil { + // Throw an error if the UID or ResourceVersion preconditions do not match fetched object already + // This would avoid extra retries when user has provided invalid preconditions + if opts.Preconditions.UID != nil && projectObj.UID != *options.Preconditions.UID { + lastErr = fmt.Errorf("precondition UID %s does not match project UID %s", *opts.Preconditions.UID, projectObj.UID) + return false, nil + } + if opts.Preconditions.ResourceVersion != nil && projectObj.ResourceVersion != *options.Preconditions.ResourceVersion { + lastErr = fmt.Errorf("precondition RV %s does not match project RV %s", *opts.Preconditions.ResourceVersion, projectObj.ResourceVersion) + return false, nil + } + } + // Make sure the object hasn't changed between Get and Delete - pass UID and RV to delete options + // unless Precondition is already set + if opts.Preconditions.UID == nil { + opts.Preconditions.UID = &projectObj.UID + } + if opts.Preconditions.ResourceVersion == nil { + opts.Preconditions.ResourceVersion = &projectObj.ResourceVersion + } + + if err := objectFunc(ctx, obj); err != nil { + lastErr = fmt.Errorf("validation func failed: %w", err) + return false, nil + } + } + err = s.client.Delete(ctx, name, opts) + switch { + case err == nil: + return true, nil + case kerrors.IsConflict(err): + lastErr = err + return false, nil + default: + return false, err + } + }) + if err != nil { + if wait.Interrupted(err) { + err = lastErr + } + return &metav1.Status{Status: metav1.StatusFailure}, false, err + } + return &metav1.Status{Status: metav1.StatusSuccess}, false, nil } diff --git a/pkg/project/apiserver/registry/project/proxy/proxy_test.go b/pkg/project/apiserver/registry/project/proxy/proxy_test.go index de14cba209..0b9cde0604 100644 --- a/pkg/project/apiserver/registry/project/proxy/proxy_test.go +++ b/pkg/project/apiserver/registry/project/proxy/proxy_test.go @@ -1,17 +1,23 @@ package proxy import ( + "context" + "errors" + "fmt" "strings" "testing" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" + kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime" + ktypes "k8s.io/apimachinery/pkg/types" "k8s.io/apiserver/pkg/authentication/user" apirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" "k8s.io/client-go/kubernetes/fake" + ktesting "k8s.io/client-go/testing" oapi "github.com/openshift/openshift-apiserver/pkg/api" projectapi "github.com/openshift/openshift-apiserver/pkg/project/apis/project" @@ -79,7 +85,7 @@ func TestCreateInvalidProject(t *testing.T) { Annotations: map[string]string{oapi.OpenShiftDisplayName: "h\t\ni"}, }, }, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) - if !errors.IsInvalid(err) { + if !kerrors.IsInvalid(err) { t.Errorf("Expected 'invalid' error, got %v", err) } } @@ -101,6 +107,27 @@ func TestCreateProjectOK(t *testing.T) { } } +func TestCreateProjectValidation(t *testing.T) { + mockClient := &fake.Clientset{} + storage := NewREST(mockClient.CoreV1().Namespaces(), &mockLister{}, nil, nil) + + validationCalled := false + validationFunc := func(ctx context.Context, obj runtime.Object) error { + validationCalled = true + return nil + } + + _, err := storage.Create(apirequest.NewContext(), &projectapi.Project{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + }, validationFunc, &metav1.CreateOptions{}) + if err != nil { + t.Errorf("Unexpected non-nil error: %#v", err) + } + if !validationCalled { + t.Errorf("Expected validation function to be called") + } +} + func TestGetProjectOK(t *testing.T) { mockClient := fake.NewSimpleClientset(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "foo"}}) storage := NewREST(mockClient.CoreV1().Namespaces(), &mockLister{}, nil, nil) @@ -136,9 +163,192 @@ func TestDeleteProject(t *testing.T) { t.Errorf("Expected status=success, got: %#v", status) } if len(mockClient.Actions()) != 1 { - t.Errorf("Expected client action for delete") + t.Errorf("Expected client action for delete, got %v", mockClient.Actions()) } if !mockClient.Actions()[0].Matches("delete", "namespaces") { - t.Errorf("Expected call to delete-namespace") + t.Errorf("Expected call to delete-namespace, got %#v", mockClient.Actions()[0]) + } +} + +func TestDeleteProjectValidation(t *testing.T) { + mockClient := &fake.Clientset{} + storage := REST{ + client: mockClient.CoreV1().Namespaces(), + } + validationCalled := false + validationFunc := func(ctx context.Context, obj runtime.Object) error { + validationCalled = true + return nil + } + + storage.Delete(apirequest.NewContext(), "foo", validationFunc, &metav1.DeleteOptions{}) + if !validationCalled { + t.Errorf("Expected validation function to be called") + } +} + +func TestDeleteProjectValidationRetries(t *testing.T) { + mockClient := &fake.Clientset{} + storage := REST{ + client: mockClient.CoreV1().Namespaces(), + } + maxRetries := 3 + validationRetries := 0 + validationFunc := func(ctx context.Context, obj runtime.Object) error { + if validationRetries < maxRetries { + validationRetries++ + return fmt.Errorf("not there yet") + } + return nil + } + obj, _, err := storage.Delete(apirequest.NewContext(), "foo", validationFunc, &metav1.DeleteOptions{}) + if obj == nil { + t.Error("Unexpected nil obj") + } + if err != nil { + t.Errorf("Unexpected non-nil error: %#v", err) + } + status, ok := obj.(*metav1.Status) + if !ok { + t.Errorf("Expected status type, got: %#v", obj) + } + if status.Status != metav1.StatusSuccess { + t.Errorf("Expected status=success, got: %#v", status) + } + if len(mockClient.Actions()) != maxRetries+2 { + t.Errorf("Expected client action for get, got %v", mockClient.Actions()) + } + for i := range len(mockClient.Actions()) - 1 { + if !mockClient.Actions()[i].Matches("get", "namespaces") { + t.Errorf("Expected call #%d to get-namespace, got %#v", i, mockClient.Actions()[i]) + } + } + if !mockClient.Actions()[len(mockClient.Actions())-1].Matches("delete", "namespaces") { + t.Errorf("Expected call #%d to delete-namespace, got %#v", len(mockClient.Actions())-1, mockClient.Actions()[len(mockClient.Actions())-1]) + } + if validationRetries != maxRetries { + t.Errorf("Expected validation function to be retried %d times, got %d", maxRetries, validationRetries) + } +} + +func TestDeleteProjectValidationError(t *testing.T) { + mockClient := &fake.Clientset{} + storage := REST{ + client: mockClient.CoreV1().Namespaces(), + } + validationError := fmt.Errorf("faulty function") + + validationFunc := func(ctx context.Context, obj runtime.Object) error { + return validationError + } + obj, _, err := storage.Delete(apirequest.NewContext(), "foo", validationFunc, &metav1.DeleteOptions{}) + if obj == nil { + t.Error("Unexpected nil obj") + } + status, ok := obj.(*metav1.Status) + if !ok { + t.Errorf("Expected status type, got: %#v", obj) + } + if status.Status != metav1.StatusFailure { + t.Errorf("Expected status=failure, got: %#v", status) + } + if err == nil { + t.Errorf("Expected error but got nil") + } + if errors.Unwrap(err) != validationError { + t.Errorf("Unexpected error: %#v", errors.Unwrap(err)) + } +} + +func TestDeleteProjectValidationPreconditionUID(t *testing.T) { + uid := ktypes.UID("first-uid") + resourceVersion := "10" + meta := metav1.ObjectMeta{Name: "foo", UID: uid, ResourceVersion: resourceVersion} + namespaceList := corev1.NamespaceList{ + Items: []corev1.Namespace{ + { + ObjectMeta: meta, + }, + }, + } + + tests := []struct { + testName string + uid, resourceVersion string + }{ + { + testName: "all unset", + }, + { + testName: "uid set", + uid: "first-uid", + }, + { + testName: "resourceVersion set", + resourceVersion: "10", + }, + { + testName: "both set", + uid: "first-uid", + resourceVersion: "10", + }, + } + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + mockClient := fake.NewSimpleClientset(&namespaceList) + storage := REST{ + client: mockClient.CoreV1().Namespaces(), + } + validationFunc := func(ctx context.Context, obj runtime.Object) error { + return nil + } + + expectedUID := uid + expectedRV := resourceVersion + deleteOpts := &metav1.DeleteOptions{} + if len(test.uid) > 0 { + expectedUID = ktypes.UID(test.uid) + if deleteOpts.Preconditions == nil { + deleteOpts.Preconditions = &metav1.Preconditions{} + } + deleteOpts.Preconditions.UID = &expectedUID + } + if len(test.resourceVersion) > 0 { + expectedRV = test.resourceVersion + if deleteOpts.Preconditions == nil { + deleteOpts.Preconditions = &metav1.Preconditions{} + } + deleteOpts.Preconditions.ResourceVersion = &expectedRV + } + + storage.Delete(apirequest.NewContext(), "foo", validationFunc, deleteOpts) + if len(mockClient.Actions()) != 2 { + t.Errorf("Expected client action for get and delete, got %v", mockClient.Actions()) + } + if !mockClient.Actions()[0].Matches("get", "namespaces") { + t.Errorf("Expected call to get-namespace, got %#v", mockClient.Actions()[0]) + } + lastAction := mockClient.Actions()[1] + if !lastAction.Matches("delete", "namespaces") { + t.Errorf("Expected call to delete-namespace, got %#v", mockClient.Actions()[1]) + } + deleteAction := lastAction.(ktesting.DeleteActionImpl) + preconditions := deleteAction.DeleteOptions.Preconditions + if preconditions == nil { + t.Fatalf("Expected DeleteOptions precondition to be non-nil") + } + if preconditions.UID == nil { + t.Fatalf("Expected DeleteOptions precondition UID to be non-nil") + } + if *preconditions.UID != expectedUID { + t.Errorf("Expected DeleteOptions precondition UID to %#v, got %#v", expectedUID, *preconditions.UID) + } + if preconditions.ResourceVersion == nil { + t.Fatalf("Expected DeleteOptions precondition ResourceVersion to be non-nil") + } + if *preconditions.ResourceVersion != expectedRV { + t.Errorf("Expected DeleteOptions precondition ResourceVersion to %#v, got %#v", expectedRV, *preconditions.ResourceVersion) + } + }) } }