Skip to content

Missing Unit Test Suite & Safety Validations for Destructive CLI Commands (velero uninstall in pkg/cmd/cli/uninstall) #10442

Description

@opbot-xd

Describe the problem/challenge you have
velero uninstall (pkg/cmd/cli/uninstall/uninstall.go) is the most destructive CLI command in Velero.

Current Limitations & Hidden Bugs:

  1. Zero Test Coverage: pkg/cmd/cli/uninstall currently has 0 unit test files (uninstall_test.go does not exist).
  2. Global Mutable State Bug: Line 56 defines a package-level global slice var resToDelete = []kbclient.ObjectList{}. During checkResources(), items are appended directly to this global variable without reset, causing slice accumulation, potential data races, and state leakage across multiple calls.
  3. Inverted Namespace/Name Bug in Forced Deletion: In forcedlyDeleteResources() (uninstall.go:L371-376), the deployment object metadata is defined as:
    deploy := &appsv1api.Deployment{
        ObjectMeta: metav1.ObjectMeta{
            Namespace: "velero",
            Name:      namespace,
        },
    }
    This inverts Namespace and Name (hardcoding namespace to "velero" and deployment name to namespace), conflicting with line 388 (key := kbclient.ObjectKey{Namespace: namespace, Name: "velero"}). This bug remained undetected due to the absence of unit tests.

Describe the solution you'd like

  1. Implement pkg/cmd/cli/uninstall/uninstall_test.go:
    • Add unit tests using controller-runtime fake client (fake.NewClientBuilder()) and mock client.Factory.
    • Test --force flag execution versus interactive confirmation prompt cancellation and confirmation.
    • Test CRD deletion handling (when CRDs exist vs when they are already missing).
    • Test graceful deletion timeout transitioning into forced finalizer removal.
    • Test custom namespace flags (e.g. --namespace custom-velero).
  2. Refactor and Fix Identified Bugs in uninstall.go:
    • Eliminate the package-level global variable var resToDelete and pass the target object lists through function parameters or an uninstaller context struct.
    • Fix the inverted Namespace/Name assignment in forcedlyDeleteResources.

Anything else you would like to add:

Environment:

  • Velero version: main (v1.16+)
  • Kubernetes version: v1.28+
  • OS: Any

Vote on this issue!

  • 👍 for "The project would be better with this feature added"
  • 👎 for "This feature will not enhance the project in a meaningful way"

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions