Skip to content

Conversation

@openshift-cherrypick-robot
Copy link
Contributor

This is an automated cherry-pick of #4103

/assign zdrapela

@sonarqubecloud
Copy link

@zdrapela
Copy link
Member

@github-actions
Copy link
Contributor

@zdrapela
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 29, 2026
@openshift-ci
Copy link

openshift-ci bot commented Jan 29, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zdrapela

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@zdrapela
Copy link
Member

/hold

@zdrapela
Copy link
Member

/unhold

@openshift-ci
Copy link

openshift-ci bot commented Jan 29, 2026

@openshift-cherrypick-robot: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ocp-helm f7c5b3d link unknown /test e2e-ocp-helm

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.

@zdrapela
Copy link
Member

/review
-i

@qodo-code-review
Copy link

qodo-code-review bot commented Jan 29, 2026

PR Reviewer Guide 🔍

(Review updated until commit f7c5b3d)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🔒 Security concerns

Potential sensitive information exposure:
This change is intended to prevent Kubernetes bearer tokens from being logged by avoiding printing full @kubernetes/client-node HttpError objects (which can include request headers). However, getKubeApiErrorMessage() may still return err.message (or body.message) verbatim, and if those strings ever contain an Authorization: Bearer ... value (directly or embedded), it could still leak. Consider adding a conservative redaction on returned strings (e.g., replace /Bearer\s+[A-Za-z0-9\-._~+/]+=*/g with Bearer [REDACTED]) before logging/throwing.

⚡ Recommended focus areas for review

Type Guard

isKubeApiError() currently returns true for any non-null object, so getKubeApiErrorMessage() will treat many arbitrary thrown values as a KubeApiError. Consider strengthening the guard (e.g., check for presence of body, response, statusCode, or message keys) to avoid misleading “safe” formatting and to reduce the chance of unexpected shapes causing confusing output.

function isKubeApiError(error: unknown): error is KubeApiError {
  return error !== null && typeof error === "object";
}

/**
 * Safely extracts error information from Kubernetes API errors without leaking sensitive data.
 * The @kubernetes/client-node HttpError contains the full HTTP request/response which includes
 * the Authorization header with the bearer token. This function extracts only safe information.
 */
function getKubeApiErrorMessage(error: unknown): string {
  if (isKubeApiError(error)) {
    const err = error;

    // Kubernetes API errors have a body with message, reason, and code
    if (err.body?.message) {
      const parts = [err.body.message];
      if (err.body.reason) parts.push(`reason: ${err.body.reason}`);
      if (err.body.code) parts.push(`code: ${err.body.code}`);
      return parts.join(", ");
    }

    // Fallback to statusCode and statusMessage from response
    if (err.response?.statusCode) {
      return `HTTP ${err.response.statusCode}: ${err.response.statusMessage || "Unknown error"}`;
    }

    // Fallback to statusCode on error object
    if (err.statusCode) {
      return `HTTP ${err.statusCode}`;
    }

    // Fallback to error message (safe as it doesn't contain request details)
    if (err.message) {
      return err.message;
    }
  }

  return "Unknown Kubernetes API error";
}
Error Context

Some catch blocks replace the original error with new Error(...), which can lose stack/context and makes debugging harder. Consider preserving the original error via throw new Error(msg, { cause: error as Error }) (Node/TS support permitting) or rethrowing the original after logging a sanitized message. Also consider a final string-level redaction step in getKubeApiErrorMessage() (e.g., redact Bearer <token> patterns) in case a token appears in err.message unexpectedly.

  console.log(
    `ConfigMap '${actualConfigMapName}' updated successfully with new title: '${newTitle}'`,
  );
} catch (error) {
  console.error(
    `Error updating ConfigMap: ${getKubeApiErrorMessage(error)}`,
  );
  throw new Error(
    `Failed to update ConfigMap: ${getKubeApiErrorMessage(error)}`,
  );
}
📄 References
  1. redhat-developer/rhdh/e2e-tests/playwright/e2e/plugins/keycloak/catalog-users.spec.ts [65-128]
  2. redhat-developer/rhdh/e2e-tests/playwright/utils/authentication-providers/rhdh-deployment.ts [1-18]
  3. redhat-developer/rhdh/e2e-tests/playwright/e2e/plugins/kubernetes/kubernetes-rbac.spec.ts [8-98]
  4. redhat-developer/rhdh/e2e-tests/playwright/support/pages/kubernetes.ts [1-55]
  5. redhat-developer/rhdh/e2e-tests/playwright/e2e/plugins/topology/topology-rbac.spec.ts [1-86]
  6. redhat-developer/rhdh/e2e-tests/playwright/e2e/plugins/kubernetes/kubernetes-rbac.spec.ts [1-6]
  7. redhat-developer/rhdh/e2e-tests/playwright/e2e/auth-providers/gitlab.spec.ts [1-252]
  8. redhat-developer/rhdh/e2e-tests/playwright/e2e/auth-providers/oidc.spec.ts [21-461]

@rhdh-qodo-merge
Copy link

Persistent review updated to latest commit f7c5b3d

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants