Skip to content

TRT-2580: Revert "OCPBUGS-78557: Fix admin context race condition in parallel tests" #1258

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

Merged
openshift-merge-bot merged 1 commit into from
Mar 17, 2026
Merged

TRT-2580: Revert "OCPBUGS-78557: Fix admin context race condition in parallel tests" #1258

Show file tree
Hide file tree
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
113 changes: 3 additions & 110 deletions tests-extension/test/qe/util/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"path/filepath"
"runtime/debug"
"strings"
"sync"
"time"

g "github.com/onsi/ginkgo/v2"
Expand Down Expand Up @@ -55,12 +54,6 @@ import (
e2edebug "k8s.io/kubernetes/test/e2e/framework/debug"
)

var (
// Global cache for admin context name to avoid repeated parsing across all test suites
globalAdmContextName string
globalAdmContextOnce sync.Once
)

// CLI provides function to call the OpenShift CLI and Kubernetes and OpenShift
// clients.
type CLI struct {
Expand All @@ -69,9 +62,6 @@ type CLI struct {
configPath string
guestConfigPath string
adminConfigPath string
tempAdmConfPath string // Temporary admin config for this test to avoid concurrent access
admContextName string // Cached admin context name (parsed once, lazy-loaded in AsAdmin)
isUseAdminCxt bool
username string
globalArgs []string
commandArgs []string
Expand Down Expand Up @@ -115,8 +105,6 @@ func NewCLI(project, adminConfigPath string) *CLI {
client.execPath = "oc"
client.showInfo = true
client.adminConfigPath = adminConfigPath
// Get cached admin context name (parsed only once globally for efficiency)
client.admContextName = getAdminContextNameCached(client.adminConfigPath)

g.BeforeEach(client.SetupProject)

Expand All @@ -140,10 +128,6 @@ func NewCLIWithoutNamespace(project string) *CLI {
client.execPath = "oc"
client.adminConfigPath = KubeConfigPath()
client.showInfo = true
// Get cached admin context name (parsed only once globally for efficiency)
client.admContextName = getAdminContextNameCached(client.adminConfigPath)
client.tempAdmConfPath = duplicateFileToTempOrEmpty(client.adminConfigPath, "admin-config-")

return client
}

Expand Down Expand Up @@ -187,20 +171,9 @@ func (c *CLI) Username() string {
}

// AsAdmin changes current config file path to the admin config.
// If a temporary admin config was created for this test (to avoid concurrent access),
// it will be used instead of the shared admin config.
// Lazy-loads admin context name on first call for automatic --context specification.
func (c *CLI) AsAdmin() *CLI {
nc := *c
// Use temporary admin config if available (created in SetupProject for concurrent safety)
if c.tempAdmConfPath != "" {
nc.configPath = c.tempAdmConfPath
} else {
nc.configPath = c.adminConfigPath
}

nc.isUseAdminCxt = true

nc.configPath = c.adminConfigPath
Copy link

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

This reintroduces shared admin-kubeconfig race risk in parallel specs.

These lines route admin and run-time kubeconfig selection back to shared adminConfigPath/configPath semantics. That restores the original shared mutable kubeconfig surface (the race from OCPBUGS-78557), so context-mutating oc calls can interfere across parallel tests again.

Please keep the revert if needed for the current blocker, but preserve per-test/admin-isolated kubeconfig files (or equivalent immutable context handling) in the re-landing fix so both issues are addressed together.

Also applies to: 618-619, 674-674

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests-extension/test/qe/util/client.go` at line 176, The assignment
nc.configPath = c.adminConfigPath reintroduces a shared, mutable admin
kubeconfig and must be changed to preserve per-test/admin isolation; stop
routing runtime client config to the shared c.adminConfigPath and instead ensure
each test gets its own immutable config (e.g., create and assign a per-test temp
kubeconfig or a separate field like nc.adminConfigPath) or use the intended
runtime config variable (not c.adminConfigPath) when initializing nc.configPath;
update the analogous occurrences referenced (around the other spots noted) so no
code assigns nc.configPath from the shared c.adminConfigPath and instead uses
isolated per-test copies or the proper runtime config variable.

return &nc
}

Expand Down Expand Up @@ -313,9 +286,6 @@ func (c *CLI) GetGuestKubeconf() string {
// SetAdminKubeconf instructs the admin cluster kubeconf file is set
func (c *CLI) SetAdminKubeconf(adminKubeconf string) *CLI {
c.adminConfigPath = adminKubeconf
// Refresh derived fields to match new admin kubeconfig
c.admContextName = getAdminContextNameCached(adminKubeconf)
c.tempAdmConfPath = duplicateFileToTempOrEmpty(adminKubeconf, "admin-config-")
return c
}

Expand Down Expand Up @@ -361,10 +331,6 @@ func (c *CLI) SetupProject() {
newNamespace := names.SimpleNameGenerator.GenerateName(fmt.Sprintf("e2e-test-%s-", c.kubeFramework.BaseName))
c.SetNamespace(newNamespace)

if c.tempAdmConfPath == "" {
c.tempAdmConfPath = duplicateFileToTempOrEmpty(c.adminConfigPath, "admin-config-")
}

// The user.openshift.io and oauth.openshift.io APIs are unavailable on external OIDC clusters by design.
// We will create and switch to a temporary user for non-external-oidc clusters only.
//
Expand Down Expand Up @@ -552,20 +518,11 @@ func (c *CLI) TeardownProject() {
}
}

// Use AdminDynamicClient to delete resources (needs tempAdmConfPath)
dynamicClient := c.AdminDynamicClient()
for _, resource := range c.resourcesToDelete {
err := dynamicClient.Resource(resource.Resource).Namespace(resource.Namespace).Delete(context.Background(), resource.Name, metav1.DeleteOptions{})
e2e.Logf("Deleted %v, err: %v", resource, err)
}

// Clean up temporary admin config after all admin operations complete
if len(c.tempAdmConfPath) > 0 {
if err := os.Remove(c.tempAdmConfPath); err != nil {
e2e.Logf("Failed to remove temporary admin config file %s: %v", c.tempAdmConfPath, err)
}
c.tempAdmConfPath = "" // Clear the path after removal
}
}

// CreateNamespace creates and returns a test namespace, automatically torn down after the test.
Expand Down Expand Up @@ -658,12 +615,7 @@ func (c *CLI) UserConfig() *rest.Config {

// AdminConfig method
func (c *CLI) AdminConfig() *rest.Config {
// Use temporary admin config if available (for concurrent safety)
configPath := c.adminConfigPath
if c.tempAdmConfPath != "" {
configPath = c.tempAdmConfPath
}
clientConfig, err := getClientConfig(configPath)
clientConfig, err := getClientConfig(c.adminConfigPath)
if err != nil {
FatalErr(err)
}
Expand Down Expand Up @@ -709,9 +661,6 @@ func (c *CLI) Run(commands ...string) *CLI {
verb: commands[0],
kubeFramework: c.KubeFramework(),
adminConfigPath: c.adminConfigPath,
tempAdmConfPath: c.tempAdmConfPath,
admContextName: c.admContextName,
isUseAdminCxt: c.isUseAdminCxt,
configPath: c.configPath,
showInfo: c.showInfo,
guestConfigPath: c.guestConfigPath,
Expand All @@ -722,16 +671,7 @@ func (c *CLI) Run(commands ...string) *CLI {
if c.asGuestKubeconf {
nc.globalArgs = c.appendGuestKubeconfig(nc.globalArgs)
} else {
// Use configPath if available, fallback to tempAdmConfPath if configPath is empty
kubeConfigPath := c.configPath
if kubeConfigPath == "" && c.tempAdmConfPath != "" {
kubeConfigPath = c.tempAdmConfPath
}
nc.globalArgs = append([]string{fmt.Sprintf("--kubeconfig=%s", kubeConfigPath)}, nc.globalArgs...)
// If admin context is cached (from AsAdmin call), use it explicitly (double insurance)
if c.admContextName != "" && c.isUseAdminCxt {
nc.globalArgs = append([]string{fmt.Sprintf("--context=%s", c.admContextName)}, nc.globalArgs...)
}
nc.globalArgs = append([]string{fmt.Sprintf("--kubeconfig=%s", c.configPath)}, nc.globalArgs...)
}
}
if c.asGuestKubeconf && !c.withoutNamespace {
Expand Down Expand Up @@ -1183,50 +1123,3 @@ func (c *CLI) SilentOutput() (string, error) {
FatalErr(fmt.Errorf("unable to execute %q: %v", c.execPath, err))
return "", nil
}

// getAdminContextName reads the kubeconfig file and searches for a context with "admin" in its name.
// This is used to explicitly specify the admin context in oc commands for concurrent safety.
// Returns empty string if no admin context is found.
func getAdminContextName(kubeConfigFile string) string {
if kubeConfigFile == "" {
return ""
}

kubeConfigBytes, err := os.ReadFile(kubeConfigFile)
if err != nil {
e2e.Logf("Failed to read kubeconfig for admin context search: %v", err)
return ""
}

kubeConfig, err := clientcmd.Load(kubeConfigBytes)
if err != nil {
e2e.Logf("Failed to parse kubeconfig for admin context search: %v", err)
return ""
}

// Search for admin context with exact matching
// Common patterns: "admin", "system:admin", "<cluster>/admin"
for contextName := range kubeConfig.Contexts {
lowerName := strings.ToLower(contextName)
// Match exact "admin", "system:admin", or contexts ending with "/admin"
if lowerName == "admin" || lowerName == "system:admin" || strings.HasSuffix(lowerName, "/admin") {
e2e.Logf("Found admin context: %s", contextName)
return contextName
}
}

e2e.Logf("No admin context found in kubeconfig")
return ""
}

// getAdminContextNameCached returns the cached admin context name, parsing only once globally.
// This ensures that across all test suites, the kubeconfig is parsed only once for efficiency.
func getAdminContextNameCached(kubeConfigFile string) string {
globalAdmContextOnce.Do(func() {
globalAdmContextName = getAdminContextName(kubeConfigFile)
if globalAdmContextName != "" {
e2e.Logf("Cached admin context for all test suites: %s", globalAdmContextName)
}
})
return globalAdmContextName
}
93 changes: 29 additions & 64 deletions tests-extension/test/qe/util/tools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,73 +20,36 @@ import (
e2e "k8s.io/kubernetes/test/e2e/framework"
)

// duplicateFileToPathSafe copies a file without using Gomega (safe for initialization phase).
// Returns error instead of using Expect, suitable for calling in var initialization.
func duplicateFileToPathSafe(srcPath, destPath string) error {
// DuplicateFileToPath copies a file from source path to destination path with error handling
// Parameters:
// - srcPath: path to the source file to copy
// - destPath: path where the file will be copied (created with mode 0666 if not exists, truncated if exists)
func DuplicateFileToPath(srcPath string, destPath string) {
// Validate and clean paths to prevent path traversal attacks
srcPath = filepath.Clean(srcPath)
destPath = filepath.Clean(destPath)

// Ensure source file exists and is readable
srcInfo, err := os.Stat(srcPath)
o.Expect(err).NotTo(o.HaveOccurred(), "Source file does not exist or is not accessible")
o.Expect(srcInfo.Mode().IsRegular()).To(o.BeTrue(), "Source path is not a regular file")

srcFile, err := os.Open(srcPath)
if err != nil {
return fmt.Errorf("failed to open source file: %w", err)
}
defer func() { _ = srcFile.Close() }()
o.Expect(err).NotTo(o.HaveOccurred())
defer func() {
o.Expect(srcFile.Close()).NotTo(o.HaveOccurred())
}()

// Create destination file with restrictive permissions (0644)
destFile, err := os.OpenFile(destPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0644)
if err != nil {
return fmt.Errorf("failed to create destination file: %w", err)
}
defer func() { _ = destFile.Close() }()

if _, err := io.Copy(destFile, srcFile); err != nil {
return fmt.Errorf("failed to copy file: %w", err)
}

return destFile.Sync()
}

// DuplicateFileToTemp creates a temporary duplicate of a file with a specified prefix
// Safe version for initialization phase (returns error instead of using Gomega)
func duplicateFileToTempSafe(srcPath, destPrefix string) (string, error) {
if srcPath == "" {
return "", nil
}

destFile, err := os.CreateTemp(os.TempDir(), destPrefix)
if err != nil {
return "", fmt.Errorf("failed to create temporary file: %w", err)
}
destPath := destFile.Name()
_ = destFile.Close()

if err := duplicateFileToPathSafe(srcPath, destPath); err != nil {
_ = os.Remove(destPath) // best-effort cleanup on error
return "", err
}

return destPath, nil
}

// duplicateFileToTempOrEmpty creates a temporary duplicate of a file, returns empty string on error.
// Logs warning on failure. Safe for initialization phase (doesn't use Gomega).
func duplicateFileToTempOrEmpty(srcPath, destPrefix string) string {
tempPath, err := duplicateFileToTempSafe(srcPath, destPrefix)
if err != nil {
e2e.Logf("Warning: failed to create temporary admin config: %v", err)
return ""
}
return tempPath
}
o.Expect(err).NotTo(o.HaveOccurred())
defer func() {
o.Expect(destFile.Close()).NotTo(o.HaveOccurred())
}()

// DuplicateFileToPath copies a file from source path to destination path with error handling
// Parameters:
// - srcPath: path to the source file to copy
// - destPath: path where the file will be copied (created with mode 0666 if not exists, truncated if exists)
//
// Uses Gomega assertions - only call in test execution phase (It, BeforeEach, etc.)
func DuplicateFileToPath(srcPath string, destPath string) {
err := duplicateFileToPathSafe(srcPath, destPath)
o.Expect(err).NotTo(o.HaveOccurred(), "Failed to duplicate file from %s to %s", srcPath, destPath)
_, err = io.Copy(destFile, srcFile)
o.Expect(err).NotTo(o.HaveOccurred())
o.Expect(destFile.Sync()).NotTo(o.HaveOccurred())
}

// DuplicateFileToTemp creates a temporary duplicate of a file with a specified prefix
Expand All @@ -96,11 +59,13 @@ func DuplicateFileToPath(srcPath string, destPath string) {
//
// Returns:
// - string: path to the created temporary file
//
// Uses Gomega assertions - only call in test execution phase (It, BeforeEach, etc.)
func DuplicateFileToTemp(srcPath string, destPrefix string) string {
destPath, err := duplicateFileToTempSafe(srcPath, destPrefix)
o.Expect(err).NotTo(o.HaveOccurred(), "Failed to duplicate file %s to temp", srcPath)
destFile, err := os.CreateTemp(os.TempDir(), destPrefix)
o.Expect(err).NotTo(o.HaveOccurred(), "Failed to create temporary file")
o.Expect(destFile.Close()).NotTo(o.HaveOccurred(), "Failed to close temporary file")

destPath := destFile.Name()
DuplicateFileToPath(srcPath, destPath)
Comment on lines +63 to +68
Copy link

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Avoid close-then-reopen on temp files (TOCTOU window).

Line 65 closes the temp file, and Line 68 reopens by path via DuplicateFileToPath. That introduces a race window where the path can be swapped before copy. Copy into the already-open temp fd instead.

Suggested fix 
 func DuplicateFileToTemp(srcPath string, destPrefix string) string {
-	destFile, err := os.CreateTemp(os.TempDir(), destPrefix)
+	destFile, err := os.CreateTemp(os.TempDir(), destPrefix)
 	o.Expect(err).NotTo(o.HaveOccurred(), "Failed to create temporary file")
-	o.Expect(destFile.Close()).NotTo(o.HaveOccurred(), "Failed to close temporary file")
-
-	destPath := destFile.Name()
-	DuplicateFileToPath(srcPath, destPath)
+	srcFile, err := os.Open(filepath.Clean(srcPath))
+	o.Expect(err).NotTo(o.HaveOccurred(), "Failed to open source file")
+	defer func() { o.Expect(srcFile.Close()).NotTo(o.HaveOccurred()) }()
+
+	_, err = io.Copy(destFile, srcFile)
+	o.Expect(err).NotTo(o.HaveOccurred(), "Failed to copy source file to temporary file")
+	o.Expect(destFile.Sync()).NotTo(o.HaveOccurred(), "Failed to sync temporary file")
+	o.Expect(destFile.Close()).NotTo(o.HaveOccurred(), "Failed to close temporary file")
+
+	destPath := destFile.Name()
 	return destPath
 }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests-extension/test/qe/util/tools.go` around lines 63 - 68, The code closes
the temp file (destFile.Close) and then reopens it by path via
DuplicateFileToPath, creating a TOCTOU race; instead keep the temp file open and
copy into its descriptor to avoid the window. Modify the call site in
tests-extension/test/qe/util/tools.go to not close destFile before copying and
either (a) add a new helper DuplicateFileToFile/DuplicateFileToWriter that
accepts *os.File or io.Writer and copies srcPath contents into destFile using
io.Copy, or (b) change DuplicateFileToPath to accept an io.Writer and use
destFile directly; after the copy, close destFile. Ensure you reference
destFile, DuplicateFileToPath (or the new DuplicateFileToFile helper), and
remove the destFile.Close() before the copy.

return destPath
}

Expand Down