fix: existing user-supplied ns when lacking permissions (#657)

Signed-off-by: Marcin Owsiany <[email protected]>

Author: porridge

internal/testcase/case.go

@@ -192,7 +192,13 @@ func (c *Case) deleteNamespace(cl clientWithKubeConfig) error {
 	return cl.Wrapf(err, "waiting for namespace %q to be deleted timed out", c.ns.name)
 }
 
-func (c *Case) createNamespace(test *testing.T, cl clientWithKubeConfig) error {
+type T interface {
+	Context() context.Context
+	Cleanup(f func())
+	Error(args ...any)
+}
+
+func (c *Case) createNamespace(test T, cl clientWithKubeConfig) error {
 	cl.Logf("Creating namespace %q", c.ns.name)
 
 	ctx := test.Context()
@@ -210,11 +216,15 @@ func (c *Case) createNamespace(test *testing.T, cl clientWithKubeConfig) error {
 			Kind: "Namespace",
 		},
 	})
+	nsExisted, err := c.didNsExist(ctx, err, cl)
+	if err != nil {
+		return fmt.Errorf("failed to create test namespace %q: %w", c.ns.name, err)
+	}
 	if !c.skipDelete {
 		test.Cleanup(func() {
 			// Namespace cleanup is tracked per-client for multi-cluster tests.
 			// See KEP-0008 for details on backward compatibility decisions.
-			if c.ns.userSupplied && k8serrors.IsAlreadyExists(err) {
+			if c.ns.userSupplied && nsExisted {
 				cl.Logf("Skipping deletion of pre-existing user supplied namespace %s", c.ns.name)
 			} else {
 				if err := c.deleteNamespace(cl); err != nil {
@@ -224,14 +234,35 @@ func (c *Case) createNamespace(test *testing.T, cl clientWithKubeConfig) error {
 		})
 	}
 
+	return nil
+}
+
+func (c *Case) didNsExist(ctx context.Context, err error, cl clientWithKubeConfig) (bool, error) {
 	if k8serrors.IsAlreadyExists(err) {
 		cl.Logf("Namespace %q already exists", c.ns.name)
-		return nil
+		return true, nil
 	}
-	if err != nil {
-		return fmt.Errorf("failed to create test namespace %q: %w", c.ns.name, err)
+	if k8serrors.IsForbidden(err) {
+		// If we got a permission error, check separately if the namespace exists
+		var existingNs corev1.Namespace
+		getErr := cl.Get(ctx, client.ObjectKey{Name: c.ns.name}, &existingNs)
+		if getErr == nil {
+			cl.Logf("Namespace %q already exists", c.ns.name)
+			return true, nil
+		}
+		if k8serrors.IsNotFound(getErr) {
+			return false, fmt.Errorf("cannot create namespace %q: %w", c.ns.name, err)
+		}
+		cl.Logf("Namespace %q creation forbidden (%v) and cannot check its existence: %v", c.ns.name, err, getErr)
+		// If the user supplied the namespace, it might be due to restrictive RBAC, so let's trust the user.
+		// If this is an ephemeral namespace, then:
+		// a) either our permissions are very wrong and the test is unlikely to pass anyway, or
+		// b) it's a subtle multi-kubeconfig setup where this client is not meant to manage the namespace, but another is.
+		// Either way, we log the above for visibility and carry on as if a pre-existing namespace is usable.
+		// The worst that could happen is that the test will fail slightly later than during setup.
+		return true, nil
 	}
-	return nil
+	return false, err
 }
 
 func (c *Case) maybeReportEvents() {

internal/testcase/case_test.go

@@ -1,16 +1,21 @@
 package testcase
 
 import (
+	"context"
 	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
+	k8serrors "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/labels"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	"github.com/kudobuilder/kuttl/internal/kubernetes"
 	"github.com/kudobuilder/kuttl/internal/step"
@@ -353,3 +358,262 @@ func TestLoadTestSteps(t *testing.T) {
 		})
 	}
 }
+
+// testMock is an object useful for unit-testing Case.createNamespace().
+type testMock struct {
+	cleanup    func()
+	testErrors [][]any
+}
+
+func (t *testMock) Context() context.Context {
+	return context.Background()
+}
+
+func (t *testMock) Cleanup(f func()) {
+	t.cleanup = f
+}
+
+func (t *testMock) Error(args ...any) {
+	t.testErrors = append(t.testErrors, args)
+}
+
+func TestCase_createNamespace(t *testing.T) {
+	tests := map[string]struct {
+		options              []CaseOption
+		cl                   func(*testing.T, string) client.Client
+		wantErr              error
+		expectedCleanupError func(err error) bool
+		getNsBeforeCleanup   func(*testing.T, error)
+		getNsAfterCleanup    func(*testing.T, error)
+	}{
+		"user-supplied exists": {
+			options: []CaseOption{WithNamespace("foo")},
+			cl:      newClientWithExistingNs,
+			getNsBeforeCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+			getNsAfterCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+		},
+		"user-supplied absent": {
+			options: []CaseOption{WithNamespace("foo")},
+			cl:      newClientWithAbsentNs,
+			getNsBeforeCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+			getNsAfterCleanup: func(t *testing.T, err error) {
+				assert.True(t, k8serrors.IsNotFound(err), "expected namespace to be deleted after cleanup, but client returned %v", err)
+			},
+		},
+		"user-supplied absent and no write permission": {
+			options: []CaseOption{WithNamespace("foo")},
+			cl:      newClientWithAbsentNsNoWritePerm,
+			wantErr: errCreationForbidden,
+			getNsBeforeCleanup: func(t *testing.T, err error) {
+				assert.True(t, k8serrors.IsNotFound(err), "expected namespace to be missing before cleanup, but client returned %v", err)
+			},
+			getNsAfterCleanup: func(t *testing.T, err error) {
+				assert.True(t, k8serrors.IsNotFound(err), "expected namespace to be missing after cleanup, but client returned %v", err)
+			},
+		},
+		"user-supplied exists and no write permission": {
+			options: []CaseOption{WithNamespace("foo")},
+			cl:      newClientWithExistingNsNoWritePerm,
+			getNsBeforeCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+			getNsAfterCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+		},
+		"user-supplied exists and no permissions at all": {
+			options: []CaseOption{WithNamespace("foo")},
+			cl:      newClientWithExistingNsNoPerms,
+			getNsBeforeCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+			getNsAfterCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+		},
+		"ephemeral exists": {
+			cl: newClientWithExistingNs,
+			getNsBeforeCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+			getNsAfterCleanup: func(t *testing.T, err error) {
+				assert.True(t, k8serrors.IsNotFound(err), "expected namespace to be deleted after cleanup, but client returned %v", err)
+			},
+		},
+		"ephemeral absent": {
+			cl: newClientWithAbsentNs,
+			getNsBeforeCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+			getNsAfterCleanup: func(t *testing.T, err error) {
+				assert.True(t, k8serrors.IsNotFound(err), "expected namespace to be deleted after cleanup, but client returned %v", err)
+			},
+		},
+		"ephemeral absent and no write permission": {
+			cl:      newClientWithAbsentNsNoWritePerm,
+			wantErr: errCreationForbidden,
+			getNsBeforeCleanup: func(t *testing.T, err error) {
+				assert.True(t, k8serrors.IsNotFound(err), "expected namespace to be missing before cleanup, but client returned %v", err)
+			},
+			getNsAfterCleanup: func(t *testing.T, err error) {
+				assert.True(t, k8serrors.IsNotFound(err), "expected namespace to be missing after cleanup, but client returned %v", err)
+			},
+		},
+		"ephemeral exists and no write permission": {
+			cl:                   newClientWithExistingNsNoWritePerm,
+			expectedCleanupError: k8serrors.IsForbidden,
+			getNsBeforeCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+			getNsAfterCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+		},
+		"ephemeral exists and no permissions at all": {
+			cl:                   newClientWithExistingNsNoPerms,
+			expectedCleanupError: k8serrors.IsForbidden,
+			getNsBeforeCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+			getNsAfterCleanup: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+		},
+	}
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			c := NewCase(name, "", tt.options...)
+			tm := &testMock{}
+			cl := tt.cl(t, c.ns.name)
+			if npc, ok := cl.(*noPermClient); ok {
+				npc.t = t
+			}
+			clk := clientWithKubeConfig{
+				Client:         cl,
+				kubeConfigPath: "kubeconfig/path",
+				logger:         testutils.NewTestLogger(t, ""),
+			}
+
+			gotErr := c.createNamespace(tm, clk)
+			if tt.wantErr == nil {
+				assert.NoError(t, gotErr)
+			} else {
+				assert.ErrorIs(t, gotErr, tt.wantErr)
+			}
+
+			baseClient := cl
+			if npc, ok := cl.(*noPermClient); ok {
+				baseClient = npc.Client
+			}
+
+			ns := &corev1.Namespace{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: c.ns.name,
+				},
+			}
+			err := baseClient.Get(t.Context(), kubernetes.ObjectKey(ns), ns)
+			tt.getNsBeforeCleanup(t, err)
+
+			if tm.cleanup != nil {
+				tm.cleanup()
+			}
+
+			if tt.expectedCleanupError != nil {
+				// Error should have been called once...
+				require.Len(t, tm.testErrors, 1)
+				// ...with one parameter...
+				require.Len(t, tm.testErrors[0], 1)
+				// ...which is an error.
+				err, ok := tm.testErrors[0][0].(error)
+				require.True(t, ok)
+				assert.True(t, tt.expectedCleanupError(err))
+			} else {
+				assert.Empty(t, tm.testErrors)
+			}
+			ns = &corev1.Namespace{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: c.ns.name,
+				},
+			}
+			err = baseClient.Get(t.Context(), kubernetes.ObjectKey(ns), ns)
+
+			tt.getNsAfterCleanup(t, err)
+		})
+	}
+}
+
+// noPermClient wraps a client and returns forbidden errors for Create/Delete operations.
+// Optionally it also refuses Get operations.
+type noPermClient struct {
+	client.Client
+	forbidGet bool
+	t         *testing.T
+}
+
+var errCreationForbidden = k8serrors.NewForbidden(schema.GroupResource{Group: "", Resource: "namespaces"}, "foo", fmt.Errorf("forbidden: User cannot create resource \"namespaces\""))
+
+func (c *noPermClient) Create(_ context.Context, obj client.Object, _ ...client.CreateOption) error {
+	c.t.Logf("Create object %v refused", obj.GetObjectKind().GroupVersionKind())
+	return errCreationForbidden
+}
+
+func (c *noPermClient) Delete(_ context.Context, obj client.Object, _ ...client.DeleteOption) error {
+	c.t.Logf("Delete object %v refused", obj.GetObjectKind().GroupVersionKind())
+	return k8serrors.NewForbidden(schema.GroupResource{Group: "", Resource: "namespaces"}, obj.GetName(), fmt.Errorf("forbidden: User cannot delete resource \"namespaces\""))
+}
+
+func (c *noPermClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+	if c.forbidGet {
+		c.t.Logf("Get object %v refused", key)
+		return k8serrors.NewForbidden(schema.GroupResource{Group: "", Resource: "namespaces"}, obj.GetName(), fmt.Errorf("forbidden: User cannot get resource \"namespaces\""))
+	}
+	return c.Client.Get(ctx, key, obj, opts...)
+}
+
+func newClientWithExistingNsNoWritePerm(t *testing.T, nsName string) client.Client {
+	return &noPermClient{
+		Client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(&corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: nsName,
+			},
+		}).Build(),
+		forbidGet: false,
+		t:         t,
+	}
+}
+func newClientWithAbsentNsNoWritePerm(t *testing.T, _ string) client.Client {
+	return &noPermClient{
+		Client:    fake.NewClientBuilder().WithScheme(scheme.Scheme).Build(),
+		forbidGet: false,
+		t:         t,
+	}
+}
+func newClientWithExistingNsNoPerms(t *testing.T, nsName string) client.Client {
+	return &noPermClient{
+		Client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(&corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: nsName,
+			},
+		}).Build(),
+		forbidGet: true,
+		t:         t,
+	}
+}
+
+func newClientWithAbsentNs(*testing.T, string) client.Client {
+	return fake.NewClientBuilder().WithScheme(scheme.Scheme).Build()
+}
+
+func newClientWithExistingNs(_ *testing.T, nsName string) client.Client {
+	return fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(&corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: nsName,
+		},
+	}).Build()
+}