Skip to content

investigate: test passes even without the proper SA permissions #1263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MikelAlejoBR wants to merge 1 commit into
base: master
Choose a base branch
from
+14 −0
Open

investigate: test passes even without the proper SA permissions #1263

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions test/e2e/namespaces_reset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package e2e

import (
"context"
"net/http"
"testing"
"time"
Expand All @@ -13,6 +14,8 @@ import (
"github.com/google/uuid"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
rbacv1 "k8s.io/api/rbac/v1"
"k8s.io/apimachinery/pkg/types"
)

type namespaceResetFeatureIntegrationSuite struct {
Expand All @@ -33,6 +36,7 @@ func (s *namespaceResetFeatureIntegrationSuite) SetupSuite() {
// expected.
func (s *namespaceResetFeatureIntegrationSuite) TestResetNamespaces() {
// given
hostAwaitily := s.Host()
memberAwaitily := s.Member1()

// Create a new user signup for the test.
Expand Down Expand Up @@ -107,4 +111,14 @@ func (s *namespaceResetFeatureIntegrationSuite) TestResetNamespaces() {

require.NotEqual(s.T(), timestamp, fetchedNamespace.CreationTimestamp.Time, `the namespace "%s" appears to not have been recreated due to having the same creation timestamp as before`, fetchedNamespace.Name)
}

// DELETEME - printing the member service account's cluster role to check
// its permissions.
clusterRole := rbacv1.ClusterRole{}
err = hostAwaitily.Client.Get(context.TODO(), types.NamespacedName{Name: "toolchaincluster-" + memberAwaitily.Namespace}, &clusterRole)
require.NoError(s.T(), err, "unable to obtain the cluster role")

s.T().Logf("ClusterRole: %+v", clusterRole)

require.FailNow(s.T(), "the test succeeded but we are failing to gather information")
Comment on lines +115 to +123
Copy link

@coderabbitai coderabbitai bot Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Remove unconditional FailNow; this makes the test permanently fail.

Line 123 aborts the test regardless of behavior, so this cannot be merged as-is. Convert this investigation block into a concrete RBAC assertion (delete on namespaces) instead of forced failure.

Proposed change 
-	// DELETEME - printing the member service account's cluster role to check
-	// its permissions.
+	// Verify that the member cluster role can delete namespaces.
 	clusterRole := rbacv1.ClusterRole{}
 	err = hostAwaitily.Client.Get(context.TODO(), types.NamespacedName{Name: "toolchaincluster-" + memberAwaitily.Namespace}, &clusterRole)
 	require.NoError(s.T(), err, "unable to obtain the cluster role")
 
-	s.T().Logf("ClusterRole: %+v", clusterRole)
-
-	require.FailNow(s.T(), "the test succeeded but we are failing to gather information")
+	hasDeleteNamespaces := false
+	for _, rule := range clusterRole.Rules {
+		matchesCoreNamespaces := false
+		for _, apiGroup := range rule.APIGroups {
+			if apiGroup == "" || apiGroup == "*" {
+				for _, resource := range rule.Resources {
+					if resource == "namespaces" || resource == "*" {
+						matchesCoreNamespaces = true
+						break
+					}
+				}
+			}
+			if matchesCoreNamespaces {
+				break
+			}
+		}
+		if !matchesCoreNamespaces {
+			continue
+		}
+		for _, verb := range rule.Verbs {
+			if verb == "delete" || verb == "*" {
+				hasDeleteNamespaces = true
+				break
+			}
+		}
+		if hasDeleteNamespaces {
+			break
+		}
+	}
+	require.True(s.T(), hasDeleteNamespaces, "expected ClusterRole %q to allow deleting namespaces", clusterRole.Name)
As per coding guidelines `**`: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/e2e/namespaces_reset_test.go` around lines 115 - 123, The test contains
an investigation block that unconditionally aborts via require.FailNow (remove
the call to require.FailNow) — instead replace the temporary dump of clusterRole
with a concrete RBAC assertion: fetch the ClusterRole (variable clusterRole via
hostAwaitily.Client.Get with types.NamespacedName{Name:
"toolchaincluster-"+memberAwaitily.Namespace}), then assert that the role grants
namespace delete permission (e.g., check rules in clusterRole.Rules include
verbs ["delete"] for resources ["namespaces"] and appropriate API groups) and
use require.NoError/require.True assertions rather than forcing a failure; keep
the logging line if helpful but do not call require.FailNow.

}
Loading