OCPBUGS-38120: Ensure Project delete requests are passed through admission

[everettraven]

This PR picks up the work from #520 with some adjustments.

[coderabbitai]

Walkthrough

Replaces single-shot Project Delete with an exponential-backoff retry loop that fetches and validates the Project before deletion, applies inferred preconditions when none provided, surfaces last non-conflict error, distinguishes terminal timeout errors, and expands tests into a table-driven suite with reactor/validator helpers.

Changes

Cohort / File(s)	Summary
Proxy delete implementation pkg/project/apiserver/registry/project/proxy/proxy.go	Replaces single-shot Delete with retry-enabled exponential backoff. Adds pre-deletion fetch/validation, applies inferred delete preconditions when absent, retries on conflicts, distinguishes terminal timeout errors (including ResourceVersionTooLarge), and adds helpers: getProjectForDeletion, validateDeletePreconditionsForProject, isTerminalTimeoutError. Consolidates exported interface assertions.
Deletion & create tests pkg/project/apiserver/registry/project/proxy/proxy_test.go	Refactors and expands tests into table-driven scenarios for Delete (success, transient errors, timeouts, conflicts, resource-version issues). Adds a Create validation test. Introduces test helpers/types (objectValidator, reactor, reactionFunc, newReactor), adjusts imports, and updates assertions to match new control flow.
Manifest / test support ...	Test scaffolding and imports expanded to support new reactor/validation patterns and error handling in tests (reflected in diff).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci-robot]

@everettraven: This pull request references Jira Issue OCPBUGS-38120, which is invalid:

• expected the bug to target either version "4.22." or "openshift-4.22.", but it targets "4.21.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

This PR picks up the work from #520 with some adjustments.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: everettraven
Once this PR has been reviewed and has the lgtm label, please assign derekwaynecarr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• pkg/project/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@pkg/project/apiserver/registry/project/proxy/proxy_test.go`:
- Around line 257-273: The test's reactor for "get" currently returns (nil, nil)
on success which lets s.client.Get yield a nil namespace and can mask real
issues in getProjectForDeletion or projectutil.ConvertNamespaceFromExternal;
update the reactor (used by newReactor in this test case) to return a concrete
*corev1.Namespace (or appropriate runtime.Object) on the successful call instead
of nil so that s.client.Get returns a valid namespace object and the validation
flow exercised by objectValidator and getProjectForDeletion behaves
realistically.

In `@pkg/project/apiserver/registry/project/proxy/proxy.go`:
- Around line 253-264: The auto-generated delete preconditions
(opts.Preconditions.UID and ResourceVersion) must be refreshed on every retry
instead of only set when nil; modify the logic in proxy.go so you first capture
caller-provided preconditions from the incoming options parameter, then for each
fresh project fetched on retry ensure opts.Preconditions exists and
unconditionally set the auto-generated fields to the current project.UID and
project.ResourceVersion only when the caller did not provide those specific
fields (i.e., preserve any non-nil UID or ResourceVersion from the original
options), but overwrite the auto-generated ones with the fresh values from
project on every retry so stale values are not reused.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@pkg/project/apiserver/registry/project/proxy/proxy.go`:
- Around line 258-263: The code dereferences options.Preconditions inside the
objectFunc block which can panic if options is nil; change the logic in the
objectFunc branch (around objectFunc, opts, and project usage) to first test
whether options is nil and handle both cases: if options == nil set
opts.Preconditions = &metav1.Preconditions{UID: &project.UID, ResourceVersion:
&project.ResourceVersion}, else check options.Preconditions and only copy it
into opts when non-nil (or set opts.Preconditions when options.Preconditions ==
nil), ensuring you reference the existing opts variable rather than
dereferencing options unguarded.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@pkg/project/apiserver/registry/project/proxy/proxy.go`:
- Around line 229-264: The retry loop is reusing auto-generated preconditions
across iterations causing getProjectForDeletion to use a stale ResourceVersion;
inside the wait.ExponentialBackoffWithContext callback ensure that when the
caller did not supply preconditions (options == nil || options.Preconditions ==
nil) you either (A) clear opts.Preconditions before calling
getProjectForDeletion so each iteration fetches without the previously auto-set
RV, or (B) call getProjectForDeletion with the original options (pass options
instead of &opts) so only user-supplied Preconditions influence the Get; update
the logic around opts, options and opts.Preconditions in the callback
(referencing getProjectForDeletion, opts, options, and opts.Preconditions)
accordingly.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@pkg/project/apiserver/registry/project/proxy/proxy.go`:
- Around line 301-305: The code currently replaces an interruption error by
doing "if wait.Interrupted(err) { err = lastErr }" which can set err to nil if
lastErr was never set; change the logic in the error handling block so that you
only overwrite err with lastErr when lastErr != nil (i.e., if
wait.Interrupted(err) && lastErr != nil { err = lastErr }), otherwise preserve
the original interruption error from wait.Interrupted(err); update the block
around wait.Interrupted(err), lastErr, and the return of &metav1.Status{Status:
metav1.StatusFailure} to implement this conditional assignment so callers
receive the real context cancellation/timeout error.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find wait.Interrupted implementation and understand the code context
# First, let's find the implementation of wait.Interrupted
rg -n "func Interrupted" --type=go | head -20

Repository: openshift/openshift-apiserver

Length of output: 160

---

🏁 Script executed:

#!/bin/bash
# Read the actual file to see the full context around lines 301-305
file="pkg/project/apiserver/registry/project/proxy/proxy.go"
if [ -f "$file" ]; then
  echo "=== Lines 280-320 of $file ==="
  sed -n '280,320p' "$file"
fi

Repository: openshift/openshift-apiserver

Length of output: 1325

---

🏁 Script executed:

#!/bin/bash
# Find where lastErr is declared and used in the file
file="pkg/project/apiserver/registry/project/proxy/proxy.go"
if [ -f "$file" ]; then
  echo "=== lastErr references in $file ==="
  rg -n "lastErr" "$file" -B 2 -A 2
fi

Repository: openshift/openshift-apiserver

Length of output: 946

---

🏁 Script executed:

#!/bin/bash
# Get the implementation of wait.Interrupted to understand its behavior
cat -n vendor/k8s.io/apimachinery/pkg/util/wait/error.go | sed -n '40,70p'

Repository: openshift/openshift-apiserver

Length of output: 1027

---

Don't drop interruption errors when no lastErr is available.

The backoff loop initializes lastErr to nil and only sets it under specific conditions (errors during Delete, project fetch, or validation). If the loop is interrupted by context cancellation or timeout before any of these conditions occur, wait.Interrupted(err) returns true but lastErr remains nil. Assigning err = lastErr then overwrites the interruption error with nil, causing the function to return a failure status without indicating why (context cancellation or timeout). Preserve the original interruption error unless lastErr is non‑nil.

💡 Proposed fix

-	if wait.Interrupted(err) {
-		err = lastErr
-	}
+	if wait.Interrupted(err) && lastErr != nil {
+		err = lastErr
+	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if err != nil {
	if wait.Interrupted(err) {
	err = lastErr
	}
	return &metav1.Status{Status: metav1.StatusFailure}, false, err
	if err != nil {
	if wait.Interrupted(err) && lastErr != nil {
	err = lastErr
	}
	return &metav1.Status{Status: metav1.StatusFailure}, false, err

🤖 Prompt for AI Agents

In `@pkg/project/apiserver/registry/project/proxy/proxy.go` around lines 301 -
305, The code currently replaces an interruption error by doing "if
wait.Interrupted(err) { err = lastErr }" which can set err to nil if lastErr was
never set; change the logic in the error handling block so that you only
overwrite err with lastErr when lastErr != nil (i.e., if wait.Interrupted(err)
&& lastErr != nil { err = lastErr }), otherwise preserve the original
interruption error from wait.Interrupted(err); update the block around
wait.Interrupted(err), lastErr, and the return of &metav1.Status{Status:
metav1.StatusFailure} to implement this conditional assignment so callers
receive the real context cancellation/timeout error.

[openshift-ci]

@everettraven: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.