HYPERFLEET-1057 - fix: Update idempotent delete test to scale by sele…

[ma-hill]

Summary

Fixes the idempotent delete test to scale down all sentinel cluster instances using label selectors instead of relying on Helm release names. Previously, the test scaled down only the deployment found by Helm release name sentinel-clusters, which failed when sentinel deployments with different release names existed. This change allows the helmfile installation to properly scale down. In the future if testing with multi-instance sentinels, will need to modify this to be more inclusive.

HYPERFLEET-1057

Notes

In conjunction with openshift/release#80366

Changes

• Added ScaleDeploymentBySelector method to pkg/client/kubernetes/client.go that scales all deployments matching a label selector
• Added ScaleDeploymentBySelector wrapper method to pkg/helper/k8s.go with logging
• Updated e2e/cluster/delete_edge_cases.go to scale using selector app.kubernetes.io/instance in (sentinel-clusters,clusters) instead of querying Helm release name
• The test now scales down all matching sentinel deployments and restores them in cleanup, ensuring no sentinel instances are running during the idempotent DELETE test

Test Plan

• Unit tests added/updated
• make test-all passes
• make lint passes
• E2E tests passed (idempotent delete test validated with multiple sentinel deployments)

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tirthct 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:

• OWNERS

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

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: d2611d20-a868-4b23-abf0-5613381b23df

📥 Commits

Reviewing files that changed from the base of the PR and between 98178ae and 8347aef.

📒 Files selected for processing (3)

• e2e/cluster/delete_edge_cases.go
• pkg/client/kubernetes/client.go
• pkg/helper/k8s.go

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• openshift-hyperfleet/architecture (manual)
• openshift-hyperfleet/hyperfleet-api (manual)
• openshift-hyperfleet/hyperfleet-sentinel (manual)
• openshift-hyperfleet/hyperfleet-adapter (manual)
• openshift-hyperfleet/hyperfleet-broker (manual)

🚧 Files skipped from review as they are similar to previous changes (2)

• e2e/cluster/delete_edge_cases.go
• pkg/helper/k8s.go

---

📝 Walkthrough

Summary by CodeRabbit

• Tests

  • Enhanced “Re-DELETE Idempotency and API Boundary Tests” by using label-selector-based deployment scaling to cover all related deployments, with cleanup restoring replicas consistently.

• Internal Improvements

  • Improved Kubernetes scaling support to scale all deployments matching a provided label selector in a namespace, including validation of selector input and aggregated error reporting for more reliable operations.

Walkthrough

ScaleDeploymentBySelector is added to pkg/client/kubernetes/client.go: it validates the selector is not empty, lists deployments in a namespace by label selector, returns an error if none match, and scales each matching deployment via the existing ScaleDeployment helper. Errors from individual scaling calls are accumulated and returned together rather than failing on the first error. pkg/helper/k8s.go wraps this with logging and error context. The e2e delete idempotency test replaces the previous single-deployment lookup (GetDeploymentName + ScaleDeployment targeting only sentinel-clusters) with a single ScaleDeploymentBySelector call using the selector app.kubernetes.io/instance in (sentinel-clusters,clusters), covering both deployments in scale-down and deferred scale-up.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

---

Security & Risk

CWE-284 (Improper Access Control) — selector scope.
ScaleDeploymentBySelector scales all deployments matching the provided selector in the given namespace with no allowlist or count cap. A caller passing an overly broad selector (e.g., an empty string, whitespace, or a too-permissive label expression) would scale every matching deployment in the namespace to the requested replica count, including to zero. Verify that all call sites supply tightly scoped selectors and that no code path permits the selector argument to originate from untrusted or external input.

CWE-754 (Improper Check for Unusual Exceptional Conditions) — partial failure without rollback.
The ScaleDeploymentBySelector loop accumulates errors but continues scaling after a failure. Deployments listed and processed before a failing ScaleDeployment call are already at the new replica count; deployments after it are untouched. There is no rollback. In the e2e test teardown (deferred restore to 1 replica), a mid-loop failure leaves the cluster in a mixed-replica state that may poison subsequent test cases.

CWE-391 (Unchecked Error) — deferred error ignored.
The deferred ScaleDeploymentBySelector(..., 1) call in the e2e test does not inspect or log its error return value. A silent failure during teardown will leave sentinel/clusters deployments at 0 replicas, breaking test isolation and affecting any subsequent test in the same suite that depends on those deployments being ready.

🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
No Pii Or Sensitive Data In Logs	⚠️ Warning	Logging statements in pkg/helper/k8s.go lines 180 and 186 expose namespace names via logger.Info() without redaction; namespaces may contain PII or internal infrastructure information (CWE-532: Ins...	Implement field redaction in logging or remove namespace from INFO-level logs; use error-level logging only for debugging, or redact sensitive namespace components before logging.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly matches the primary changeset objective: adding ScaleDeploymentBySelector functionality to fix the idempotent delete test by scaling using label selectors instead of Helm release names.
Description check	✅ Passed	The description comprehensively relates to the changeset, detailing the problem (Helm release name dependency), the solution (label selector-based scaling), all three code changes, and test validation plans.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Sec-02: Secrets In Log Output	✅ Passed	No log statements (slog, fmt.Errorf) include tokens, passwords, credentials, or secrets. All logged fields are non-sensitive operational data: namespace names, label selectors, replica counts, depl...
No Hardcoded Secrets	✅ Passed	No hardcoded secrets detected. Code contains only error messages, logging, Kubernetes label selectors, and test configuration paths—no API keys, tokens, passwords, private keys, base64 secrets, or...
No Weak Cryptography	✅ Passed	Pull request contains no cryptographic operations, banned primitives, or weak cryptography. Changes add Kubernetes deployment scaling utilities without crypto dependencies.
No Injection Vectors	✅ Passed	Selector parameter is hardcoded in test; passed to Kubernetes API LabelSelector field (standard pattern). No string concatenation, exec.Command, template.HTML, or yaml.Unmarshal patterns detected.
No Privileged Containers	✅ Passed	PR modifies only Go source code (test and library files); no Kubernetes manifests, Helm templates, or Dockerfiles are modified. Check is not applicable.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

✨ Simplify code

• Create PR with simplified code

---

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

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@e2e/cluster/delete_edge_cases.go`:
- Around line 43-45: The error from h.ScaleDeploymentBySelector in this cleanup
step is being logged but not propagated, which violates the coding guideline
that log-and-continue patterns must be intentional degradation with a comment.
Either return the error from this cleanup step to properly fail if sentinel
restoration fails, or if intentional degradation is acceptable, add a detailed
comment with a ticket reference explaining why the failure is being
intentionally swallowed instead of using the bare log-and-continue pattern.

In `@pkg/client/kubernetes/client.go`:
- Around line 309-320: Add input validation at the beginning of the
ScaleDeploymentBySelector method to reject empty or whitespace-only selectors
before attempting to list deployments. Check if the selector parameter is empty
after trimming whitespace using strings.TrimSpace and return a descriptive error
if validation fails. This prevents unintended scaling operations when an invalid
selector is passed to the method.
- Around line 322-326: The loop in the deployment scaling code returns
immediately when ScaleDeployment fails for the first deployment, preventing
subsequent deployments from being processed and leaving the system in a partial
state. Instead of returning on the first error, accumulate all errors that occur
while iterating through the deployments, continue processing each deployment in
the Items slice, and return all collected errors as a single error message after
the loop completes. This ensures all matching deployments are attempted to be
scaled regardless of individual failures.

In `@pkg/helper/k8s.go`:
- Around line 182-183: The error returned from the ScaleDeploymentBySelector
method call is being returned bare without context information. Instead of
returning the error directly, wrap it with contextual details including the
namespace and selector values to make the error diagnosable at the helper
boundary. Use an error wrapping approach (such as fmt.Errorf with the %w verb or
your project's standard error wrapping method) to include these contextual
details in the error message before returning it.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 70592edd-c143-49b7-91ce-75b31dcf4949

📥 Commits

Reviewing files that changed from the base of the PR and between 7839d03 and 98178ae.

📒 Files selected for processing (3)

• e2e/cluster/delete_edge_cases.go
• pkg/client/kubernetes/client.go
• pkg/helper/k8s.go

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• openshift-hyperfleet/architecture (manual)
• openshift-hyperfleet/hyperfleet-api (manual)
• openshift-hyperfleet/hyperfleet-sentinel (manual)
• openshift-hyperfleet/hyperfleet-adapter (manual)
• openshift-hyperfleet/hyperfleet-broker (manual)

[ma-hill]

/test images-images