Remove ModelMesh and Serverless from get_ca_bundle#1173
Merged
threcc merged 9 commits intoopendatahub-io:mainfrom Mar 6, 2026
Merged
Remove ModelMesh and Serverless from get_ca_bundle#1173threcc merged 9 commits intoopendatahub-io:mainfrom
threcc merged 9 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/cherry-pick', '/lgtm', '/verified', '/hold', '/wip'} |
mwaykole
previously approved these changes
Mar 5, 2026
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCA bundle handling simplified: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/llmd_utils.py (1)
586-592:⚠️ Potential issue | 🔴 CriticalTLS verification is silently disabled even when secure mode is requested.
Lines 586-592 force
ca_bundle = None, soinsecure=Falsestill results in--insecure. This defeats certificate validation and can hide regressions in LLMD auth/TLS flows.🔒 Proposed fix (restore CA path and fail closed)
@@ - # ca_bundle = get_ca_bundle(client=client) - ca_bundle = None + ca_bundle = get_ca_bundle(client=client) if ca_bundle: cmd += f" --cacert {ca_bundle}" else: - cmd += " --insecure" - except Exception: # noqa: BLE001 - cmd += " --insecure" + raise InferenceResponseError( + "CA bundle was not found while insecure=False; refusing insecure TLS downgrade." + ) + except Exception as e: # noqa: BLE001 + raise InferenceResponseError( + "Failed to resolve CA bundle while insecure=False." + ) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/llmd_utils.py` around lines 586 - 592, The code force-sets ca_bundle = None which causes TLS verification to be disabled; restore the call to get_ca_bundle(client=client) and use its result to decide whether to append " --cacert {ca_bundle}" or fail closed instead of appending " --insecure". In the block around ca_bundle, replace the hardcoded None with ca_bundle = get_ca_bundle(client=client), and update the exception handler in the same scope to surface/log the error and abort (or raise) when no CA bundle is available and secure mode is requested so you never silently add "--insecure" to cmd.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utilities/certificates_utils.py`:
- Around line 67-84: get_ca_bundle always returns the knative CA for any
self-managed cluster, which is wrong for RAW routes and causes callers to fall
back to insecure mode; update get_ca_bundle to accept a deployment_mode (or an
enum/string from utilities/inference_utils) and use that to select the correct
secret/type when calling create_ca_bundle_file instead of always passing
ca_type="knative". Locate get_ca_bundle, is_managed_cluster, and
create_ca_bundle_file and change the signature/use so callers (e.g., the caller
in utilities/inference_utils.py around the deployment_mode handling) pass
deployment_mode through to get_ca_bundle, and map deployment_mode to the
appropriate ca_type when invoking create_ca_bundle_file.
---
Outside diff comments:
In `@utilities/llmd_utils.py`:
- Around line 586-592: The code force-sets ca_bundle = None which causes TLS
verification to be disabled; restore the call to get_ca_bundle(client=client)
and use its result to decide whether to append " --cacert {ca_bundle}" or fail
closed instead of appending " --insecure". In the block around ca_bundle,
replace the hardcoded None with ca_bundle = get_ca_bundle(client=client), and
update the exception handler in the same scope to surface/log the error and
abort (or raise) when no CA bundle is available and secure mode is requested so
you never silently add "--insecure" to cmd.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c7f0373e-2b33-4cd7-a887-8d4f6cf9170b
📒 Files selected for processing (3)
utilities/certificates_utils.pyutilities/inference_utils.pyutilities/llmd_utils.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/llmd_utils.py (1)
585-590:⚠️ Potential issue | 🔴 CriticalRestore CA bundle lookup; this change currently forces insecure TLS for all requests.
Because
ca_bundleis hardcoded toNone, Line 590 always appends--insecurein the non-insecurepath, which disables cert verification by default. That’s a security regression from the prior behavior and fromutilities/inference_utils.py.🔧 Proposed fix
+from utilities.certificates_utils import get_ca_bundle ... else: try: from ocp_resources.resource import get_client client = get_client() - # ca_bundle = get_ca_bundle(client=client) - ca_bundle = None + ca_bundle = get_ca_bundle(client=client) if ca_bundle: cmd += f" --cacert {ca_bundle}" else: + LOGGER.warning("No CA bundle found, using insecure access") cmd += " --insecure" except Exception: # noqa: BLE001 cmd += " --insecure"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/llmd_utils.py` around lines 585 - 590, The code hardcodes ca_bundle = None which forces appending "--insecure" to cmd; replace that line to call get_ca_bundle(client=client) and use its return value (ca_bundle) so the existing conditional will add "--cacert {ca_bundle}" when available and only add "--insecure" when no bundle is found, matching the behavior in utilities/inference_utils.py; ensure get_ca_bundle is in scope/imported and use the same fallback semantics as the other module so TLS verification is not disabled by default (references: ca_bundle, get_ca_bundle, cmd).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@utilities/llmd_utils.py`:
- Around line 585-590: The code hardcodes ca_bundle = None which forces
appending "--insecure" to cmd; replace that line to call
get_ca_bundle(client=client) and use its return value (ca_bundle) so the
existing conditional will add "--cacert {ca_bundle}" when available and only add
"--insecure" when no bundle is found, matching the behavior in
utilities/inference_utils.py; ensure get_ca_bundle is in scope/imported and use
the same fallback semantics as the other module so TLS verification is not
disabled by default (references: ca_bundle, get_ca_bundle, cmd).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e54ce8a-b753-48c8-aad4-40512a2ad0f2
📒 Files selected for processing (1)
utilities/llmd_utils.py
threcc
force-pushed
the
fix-get-ca-bundle
branch
from
d0c30ec to
0b6e8cf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utilities/certificates_utils.py`:
- Around line 29-43: The code directly indexes
certs_secret.instance.data["tls.crt"] which can raise if the secret or key is
missing; update the logic around certs_secret (and the block that builds
bundle/filepath using OPENSHIFT_CA_BUNDLE_FILENAME and
py_config["tmp_base_dir"]) to first verify certs_secret.instance and
certs_secret.instance.data exist and that "tls.crt" is present, and if any of
these are missing return the documented empty string; additionally guard the
base64.b64decode/decode step in case of invalid data by catching exceptions and
returning an empty string to preserve fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3ab4f54-7b37-41b8-a2fb-bc824caa9511
📒 Files selected for processing (6)
tests/conftest.pytests/model_explainability/evalhub/conftest.pytests/model_explainability/guardrails/conftest.pytests/model_explainability/trustyai_service/trustyai_service_utils.pyutilities/certificates_utils.pyutilities/llmd_utils.py
threcc
force-pushed
the
fix-get-ca-bundle
branch
from
4fe4171 to
2a8499a
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
utilities/certificates_utils.py (1)
32-43:⚠️ Potential issue | 🟠 MajorGuard missing/invalid router cert data so fallback behavior is preserved.
At Line 39, direct access to
certs_secret.instance.data["tls.crt"]can raise (AttributeError/KeyError/decode errors) and bypass the empty-string contract described at Line 60, which downstream callers rely on.Proposed fix
def create_ca_bundle_file(client: DynamicClient) -> str: @@ - certs_secret = Secret( + certs_secret = Secret( client=client, name="router-certs-default", namespace="openshift-ingress", + ensure_exists=True, ) filename = OPENSHIFT_CA_BUNDLE_FILENAME - bundle = base64.b64decode(certs_secret.instance.data["tls.crt"]).decode() + tls_crt_b64 = (getattr(certs_secret, "instance", None) and certs_secret.instance.data or {}).get("tls.crt") + if not tls_crt_b64: + LOGGER.warning("router-certs-default secret missing tls.crt; skipping CA bundle creation") + return "" + try: + bundle = base64.b64decode(tls_crt_b64).decode("utf-8") + except Exception as exc: # noqa: BLE001 + LOGGER.warning(f"Failed to decode router certificate: {exc}") + return "" filepath = os.path.join(py_config["tmp_base_dir"], filename) - with open(filepath, "w") as fd: + with open(filepath, "w", encoding="utf-8") as fd: fd.write(bundle)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/certificates_utils.py` around lines 32 - 43, Accessing certs_secret.instance.data["tls.crt"] can raise AttributeError/KeyError or decoding errors and must be guarded so downstream code relying on an empty-string fallback still works; update the block around certs_secret, OPENSHIFT_CA_BUNDLE_FILENAME, bundle and filepath to first check that certs_secret.instance and certs_secret.instance.data exist and use .get("tls.crt") safely, then wrap base64.b64decode(...) and .decode() in a try/except that on any exception sets bundle = "" (or leaves it as an empty string), and finally write that bundle (possibly empty) to the filepath so the file is always created with a safe fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@utilities/certificates_utils.py`:
- Around line 32-43: Accessing certs_secret.instance.data["tls.crt"] can raise
AttributeError/KeyError or decoding errors and must be guarded so downstream
code relying on an empty-string fallback still works; update the block around
certs_secret, OPENSHIFT_CA_BUNDLE_FILENAME, bundle and filepath to first check
that certs_secret.instance and certs_secret.instance.data exist and use
.get("tls.crt") safely, then wrap base64.b64decode(...) and .decode() in a
try/except that on any exception sets bundle = "" (or leaves it as an empty
string), and finally write that bundle (possibly empty) to the filepath so the
file is always created with a safe fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95e5abe0-7c05-4611-b9ec-2015dca1c50a
📒 Files selected for processing (7)
tests/conftest.pytests/model_explainability/evalhub/conftest.pytests/model_explainability/guardrails/conftest.pytests/model_explainability/trustyai_service/trustyai_service_utils.pyutilities/certificates_utils.pyutilities/inference_utils.pyutilities/llmd_utils.py
🚧 Files skipped from review as they are similar to previous changes (4)
- utilities/llmd_utils.py
- utilities/inference_utils.py
- tests/model_explainability/trustyai_service/trustyai_service_utils.py
- tests/conftest.py
This comment was marked as spam.
This comment was marked as spam.
fege
reviewed
Mar 5, 2026
for more information, see https://pre-commit.ci
threcc
force-pushed
the
fix-get-ca-bundle
branch
from
4b06ee2 to
8152230
Compare
for more information, see https://pre-commit.ci
dbasunag
approved these changes
Mar 5, 2026
mwaykole
approved these changes
Mar 6, 2026
|
Status of building tag latest: success. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
This PR simplifies
certificates_utils.pyby removing deployment-mode branching that is no longer needed since the test suite only targetsRawDeploymentmode.Changes:
create_ca_bundle_file: Removedca_typeparameter and the knative/istio code path — the function now only handles the OpenShift router cert (router-certs-default).get_ca_bundle: Removed deployment_mode parameter and all Serverless/ModelMesh branching. Returns empty string on managed clusters, creates the OpenShift router CA bundle on self-managed clusters.Removed unused
ISTIO_CA_BUNDLE_FILENAME,KServeDeploymentType, andis_self_managed_operatorimports.Updated all callers (
tests/conftest.py,evalhub,guardrails,trustyai,llmd_utils.py) to use the simplified signatures.How it has been tested
Summary by CodeRabbit