Merge pull request #3599 from SimonTheLeg/fix-tests

[release-0.28] Fix object validation

Author: kcp-ci-bot

pkg/virtual/framework/forwardingregistry/store.go

@@ -95,25 +95,39 @@ func DefaultDynamicDelegatedStoreFuncs(
 
 		return delegate.Get(ctx, name, *options, subResources...)
 	}
-	s.CreaterFunc = func(ctx context.Context, obj runtime.Object, _ rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) {
+	s.CreaterFunc = func(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) {
 		unstructuredObj, ok := obj.(*unstructured.Unstructured)
 		if !ok {
 			return nil, fmt.Errorf("not an Unstructured: %T", obj)
 		}
 
+		if err := createValidation(ctx, obj); err != nil {
+			return nil, err
+		}
+
 		delegate, err := client(ctx)
 		if err != nil {
 			return nil, err
 		}
 
 		return delegate.Create(ctx, unstructuredObj, *options, subResources...)
 	}
-	s.GracefulDeleterFunc = func(ctx context.Context, name string, _ rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
+	s.GracefulDeleterFunc = func(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
 		delegate, err := client(ctx)
 		if err != nil {
 			return nil, false, err
 		}
 
+		preDeleteObj, err := delegate.Get(ctx, name, metav1.GetOptions{})
+		if err != nil {
+			return nil, false, err
+		}
+
+		err = deleteValidation(ctx, preDeleteObj)
+		if err != nil {
+			return nil, false, err
+		}
+
 		deleter, err := dynamicextension.NewDeleterWithResults(delegate)
 		if err != nil {
 			return nil, false, err
@@ -171,7 +185,7 @@ func DefaultDynamicDelegatedStoreFuncs(
 
 		return delegate.List(ctx, v1ListOptions)
 	}
-	s.UpdaterFunc = func(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, _ rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) {
+	s.UpdaterFunc = func(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) {
 		delegate, err := client(ctx)
 		if err != nil {
 			return nil, false, err
@@ -228,6 +242,11 @@ func DefaultDynamicDelegatedStoreFuncs(
 
 				return delegate.Create(ctx, unstructuredObj, updateToCreateOptions(options), subResources...)
 			}
+
+			if err := updateValidation(ctx, obj, oldObj); err != nil {
+				return nil, err
+			}
+
 			return delegate.Update(ctx, unstructuredObj, *options, subResources...)
 		}
 

test/e2e/virtual/initializingworkspaces/virtualworkspace_test.go

@@ -19,6 +19,7 @@ package initializingworkspaces
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"math/rand"
 	"sort"
 	"strings"
@@ -27,6 +28,7 @@ import (
 
 	jsonpatch "github.com/evanphx/json-patch"
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	corev1 "k8s.io/api/core/v1"
@@ -356,10 +358,9 @@ func TestInitializingWorkspacesVirtualWorkspaceAccess(t *testing.T) {
 
 	t.Log("Ensure that LIST calls through the virtual workspace eventually show the correct values")
 	for _, wsName := range wsNames {
-		require.Eventually(t, func() bool {
+		require.EventuallyWithT(t, func(c *assert.CollectT) {
 			_, err := sourceKcpClusterClient.CoreV1alpha1().Cluster(wsPath.Join(wsName)).LogicalClusters().Get(ctx, corev1alpha1.LogicalClusterName, metav1.GetOptions{})
-			require.True(t, err == nil || errors.IsForbidden(err), "got %#v error getting logicalcluster %q, expected unauthorized or success", err, wsPath.Join(wsName))
-			return err == nil
+			require.NoError(c, err, "got %#v error getting logicalcluster %q, expected success", err, wsPath.Join(wsName))
 		}, wait.ForeverTestTimeout, 100*time.Millisecond)
 	}
 
@@ -522,12 +523,25 @@ func TestInitializingWorkspacesVirtualWorkspaceAccess(t *testing.T) {
 
 		t.Logf("Attempt to do something more than just removing our initializer %q, get denied", initializer)
 		patchBytes := patchBytesFor(logicalCluster, func(workspace *corev1alpha1.LogicalCluster) {
-			workspace.Status.Initializers = []corev1alpha1.LogicalClusterInitializer{"wrong"}
+			workspace.Status.Initializers = []corev1alpha1.LogicalClusterInitializer{"wrong:wrong"}
 		})
 		_, err = clusterClient.Cluster(wsClusterName.Path()).Patch(ctx, corev1alpha1.LogicalClusterName, types.MergePatchType, patchBytes, metav1.PatchOptions{}, "status")
 		if !errors.IsInvalid(err) {
 			t.Fatalf("got %#v error from patch, expected invalid", err)
 		}
+		// Since Invalid is a generic error, which is not exclusive to an
+		// initializer failing our custom updateValidation, we need to check for it
+		// as well.
+		// Unfortunately, it is not possible to make use of
+		// field.Error.Origin to do so, as we convert our field.ErrorList into an
+		// errors.StatusError, thus loosing this information. As a result, our only
+		// option is to reconstruct the expected error.
+		expErrMsg := fmt.Sprintf("only removing the %q initializer is supported", initialization.InitializerForType(workspacetypes[initializer]))
+		// for now using contains seems to strike the best balance between
+		// identifying the error, while not making the test too brittle as
+		// kubernetes statusError creation use a lot of squashing an string
+		// manipulation to create the final exact message.
+		require.Contains(t, err.Error(), expErrMsg)
 
 		t.Logf("Remove just our initializer %q", initializer)
 		patchBytes = patchBytesFor(logicalCluster, func(workspace *corev1alpha1.LogicalCluster) {