feat(tests): handle clusters without OSSM Istio installation#1002
Open
silvi-t wants to merge 3 commits into
Open
feat(tests): handle clusters without OSSM Istio installation#1002silvi-t wants to merge 3 commits into
silvi-t wants to merge 3 commits into
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a ChangesUser-Managed Istio Detection and Conditional Test Execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@testsuite/tests/conftest.py`:
- Around line 451-456: The has_ossm fixture's logic is inverted: instead of
returning True when any user-managed Istio CR exists it currently returns True
when none exist and False when both exist; open the has_ossm function and inside
with cluster.context call selector("Istio", all_namespaces=True).objects() and
first check if the returned list is empty—if so return False; otherwise return
True if any istio.name() != "openshift-gateway" (i.e. at least one user-managed
Istio CR exists), otherwise return False. Ensure you reference the
selector("Istio", all_namespaces=True) call and the has_ossm fixture name when
applying the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5605a709-141a-44f0-a45b-b786524d3293
📒 Files selected for processing (7)
pyproject.tomltestsuite/tests/conftest.pytestsuite/tests/singlecluster/authorino/identity/x509/test_xfcc_forwarding.pytestsuite/tests/singlecluster/gateway/mtls/test_mtls_behaviour.pytestsuite/tests/singlecluster/gateway/mtls/test_mtls_configuration.pytestsuite/tests/singlecluster/tracing/data_plane_tracing/test_kuadrant_tracing.pytestsuite/tests/singlecluster/tracing/data_plane_tracing/test_kuadrant_tracing_rate_limit_only.py
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
testsuite/tests/conftest.py (1)
451-456:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
has_ossmmisclassifies mixed Istio environments.At Line 456, the predicate returns
Falsewhen bothopenshift-gatewayand at least one user-managed Istio CR exist. That breaks the fixture contract and can wrongly skiprequired_ossmtests and downgrade tracing expectations. Detect OSSM by checking for any non-openshift-gatewayIstio CR.Suggested fix
`@pytest.fixture`(scope="session") def has_ossm(cluster): """True if a full OSSM3 Istio installation exists (not just Gateway API-managed Istio)""" with cluster.context: istios = selector("Istio", all_namespaces=True).objects() - return bool(istios) and not any(i.name() == "openshift-gateway" for i in istios) + return any(i.name() != "openshift-gateway" for i in istios)🤖 Prompt for 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. In `@testsuite/tests/conftest.py` around lines 451 - 456, The has_ossm fixture currently returns False if any Istio CR named "openshift-gateway" exists, even when other user-managed Istio CRs are present; change the predicate to detect OSSM when any Istio CR has a name different from "openshift-gateway". In the has_ossm function (which calls selector("Istio", all_namespaces=True).objects()), replace the return expression with a check like: return any(i.name() != "openshift-gateway" for i in istios) so that presence of at least one non-"openshift-gateway" Istio CR yields True.
🤖 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.
Duplicate comments:
In `@testsuite/tests/conftest.py`:
- Around line 451-456: The has_ossm fixture currently returns False if any Istio
CR named "openshift-gateway" exists, even when other user-managed Istio CRs are
present; change the predicate to detect OSSM when any Istio CR has a name
different from "openshift-gateway". In the has_ossm function (which calls
selector("Istio", all_namespaces=True).objects()), replace the return expression
with a check like: return any(i.name() != "openshift-gateway" for i in istios)
so that presence of at least one non-"openshift-gateway" Istio CR yields True.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c2ebf78-2af6-456a-a90b-f73e5b1157b3
📒 Files selected for processing (7)
pyproject.tomltestsuite/tests/conftest.pytestsuite/tests/singlecluster/authorino/identity/x509/test_xfcc_forwarding.pytestsuite/tests/singlecluster/gateway/mtls/test_mtls_behaviour.pytestsuite/tests/singlecluster/gateway/mtls/test_mtls_configuration.pytestsuite/tests/singlecluster/tracing/data_plane_tracing/test_kuadrant_tracing.pytestsuite/tests/singlecluster/tracing/data_plane_tracing/test_kuadrant_tracing_rate_limit_only.py
✅ Files skipped from review due to trivial changes (3)
- testsuite/tests/singlecluster/gateway/mtls/test_mtls_behaviour.py
- pyproject.toml
- testsuite/tests/singlecluster/gateway/mtls/test_mtls_configuration.py
🚧 Files skipped from review as they are similar to previous changes (2)
- testsuite/tests/singlecluster/authorino/identity/x509/test_xfcc_forwarding.py
- testsuite/tests/singlecluster/tracing/data_plane_tracing/test_kuadrant_tracing_rate_limit_only.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@testsuite/component_metadata.py`:
- Around line 141-146: The helper currently hardcodes namespaces and treats any
presence of an "openshift-gateway" CR as gateway-managed; change it to derive
the namespace from the discovered Istio CR objects and only classify as
"gateway-api-managed" when the sole Istio CR is named "openshift-gateway".
Concretely, in the block using istios = oc.selector("Istio",
all_namespaces=True).objects(), return "none", None if istios is empty; if
len(istios) == 1 and istios[0].name() == "openshift-gateway" return
"gateway-api-managed" and use istios[0].namespace() for the namespace; otherwise
return "user-managed-ossm" and use the namespace() of a representative Istio CR
(e.g., the first non-openshift-gateway CR if present, or istios[0].namespace())
instead of hardcoding "openshift-ingress" or "istio-system".
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f35dd9dd-8fa2-43e4-9825-f6fdce596262
📒 Files selected for processing (2)
testsuite/component_metadata.pytestsuite/tests/info_collector.py
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
testsuite/component_metadata.py (1)
144-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMixed-install detection is misclassified as gateway-managed.
At Line 145, any presence of
openshift-gatewayimmediately returns"gateway-api-managed", and at Line 147 user-managed OSSM is only recognised when CR name is exactlydefault. This can misreport clusters that have both CRs (or non-defaultuser-managed CR names), causingtest_istio_properties()to collect from the wrong namespace or skip valid metadata paths.Suggested minimal fix
- gateway_istio = next((i for i in istios if i.name() == "openshift-gateway"), None) - if gateway_istio: - return "gateway-api-managed", gateway_istio.model.spec.namespace - ossm_istio = next((i for i in istios if i.name() == "default"), None) - if ossm_istio: - return "user-managed-ossm", ossm_istio.model.spec.namespace + gateway_istio = next((i for i in istios if i.name() == "openshift-gateway"), None) + non_gateway_istio = next((i for i in istios if i.name() != "openshift-gateway"), None) + + if gateway_istio and non_gateway_istio is None: + return "gateway-api-managed", gateway_istio.model.spec.namespace + + if non_gateway_istio: + return "user-managed-ossm", non_gateway_istio.model.spec.namespace + return "none", None🤖 Prompt for 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. In `@testsuite/component_metadata.py` around lines 144 - 150, The current logic checks for gateway_istio first and only recognizes user-managed OSSM when the CR name is exactly "default", which misclassifies mixed installs; update the detection to first look for any non-"openshift-gateway" Istio CR (change ossm_istio search from i.name()=="default" to i.name()!="openshift-gateway") and prefer returning "user-managed-ossm" when such an OSSM CR exists, otherwise return "gateway-api-managed" if gateway_istio exists, else "none"; adjust the code around the gateway_istio and ossm_istio variables in the same block to reflect this new preference and naming.
🤖 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.
Duplicate comments:
In `@testsuite/component_metadata.py`:
- Around line 144-150: The current logic checks for gateway_istio first and only
recognizes user-managed OSSM when the CR name is exactly "default", which
misclassifies mixed installs; update the detection to first look for any
non-"openshift-gateway" Istio CR (change ossm_istio search from
i.name()=="default" to i.name()!="openshift-gateway") and prefer returning
"user-managed-ossm" when such an OSSM CR exists, otherwise return
"gateway-api-managed" if gateway_istio exists, else "none"; adjust the code
around the gateway_istio and ossm_istio variables in the same block to reflect
this new preference and naming.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5966d87-148d-4df6-94ad-3bdfcac3883d
📒 Files selected for processing (2)
testsuite/component_metadata.pytestsuite/tests/info_collector.py
🚧 Files skipped from review as they are similar to previous changes (1)
- testsuite/tests/info_collector.py
averevki
reviewed
Jun 12, 2026
| def has_ossm(cluster): | ||
| """True if a full OSSM3 Istio installation exists (not just Gateway API-managed Istio)""" | ||
| with cluster.context: | ||
| istios = selector("Istio", all_namespaces=True).objects() |
Member
There was a problem hiding this comment.
won't this fail without ignore_not_found: true on .objects() call?
Contributor
Author
There was a problem hiding this comment.
.objects() has ignore_not_found=True by default
| """True if a full OSSM3 Istio installation exists (not just Gateway API-managed Istio)""" | ||
| with cluster.context: | ||
| istios = selector("Istio", all_namespaces=True).objects() | ||
| return bool(istios) and not any(i.name() == "openshift-gateway" for i in istios) |
Member
There was a problem hiding this comment.
Existing Istio resource that is not named openshift-gateway on the cluster doesn't mean that OSSM will be installed there. Could work for now, although would probably pass if Istio is installed through the Sail Operator on kind clusters
| Gateway API-managed Istio does not allow modifications to the Istio CR.""" | ||
| marker = request.node.get_closest_marker("required_ossm") | ||
| if marker and not has_ossm: | ||
| skip_or_fail("Test requires user-managed OSSM3 Istio; Gateway API-managed Istio cannot be modified") |
Member
There was a problem hiding this comment.
Suggested change
| skip_or_fail("Test requires user-managed OSSM3 Istio; Gateway API-managed Istio cannot be modified") | |
| skip_or_fail("Test requires Istio installed through the OpenShift Service Mesh") |
it's OCP-managed, not gateway api-managed
| return "none", None | ||
| gateway_istio = next((i for i in istios if i.name() == "openshift-gateway"), None) | ||
| if gateway_istio: | ||
| return "gateway-api-managed", gateway_istio.model.spec.namespace |
Member
There was a problem hiding this comment.
it's OCP-managed, not gateway api-managed
Comment on lines
+150
to
+153
| return "none", None | ||
| except (oc.OpenShiftPythonException, AttributeError, KeyError) as e: | ||
| logger.warning("Failed to detect Istio type: %s", e) | ||
| return "none", None |
Member
There was a problem hiding this comment.
Suggested change
| return "none", None | |
| except (oc.OpenShiftPythonException, AttributeError, KeyError) as e: | |
| logger.warning("Failed to detect Istio type: %s", e) | |
| return "none", None | |
| except (oc.OpenShiftPythonException, AttributeError, KeyError) as e: | |
| logger.warning("Failed to detect Istio type: %s", e) | |
| return "none", None |
| "egress_gateway: Test is using egress gateway", | ||
| "min_ocp_version: Minimum OpenShift version required for test (e.g., @pytest.mark.min_ocp_version((4, 20)))", | ||
| "gateway_api_version: Gateway API version requirement (e.g., @pytest.mark.gateway_api_version((1, 5, 0)) or @pytest.mark.gateway_api_version((1, 5, 0), operator.eq))", | ||
| "required_ossm: Test requires user-managed OSSM3 Istio that can be modified (skipped with Gateway API-managed Istio)", |
Member
There was a problem hiding this comment.
I think something like service_mesh_istio would be more verbose, wyt?
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
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
3 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.
Important
This PR modifies shared/core testsuite code that could potentially affect multiple test areas. 2 reviewers should review this PR to ensure adequate coverage.
Description
openshift-gatewayCR) that cannot be modified like a user-managed OSSM3 installationmeshConfig.enableTracing,extensionProviders, andTelemetryresources cannot be configured on Gateway API-managed IstioChanges
New pytest marker and fixture
@pytest.mark.required_ossmmarker inpyproject.tomlfor tests requiring user-managed OSSM3 Istiohas_ossmsession fixture intestsuite/tests/conftest.pythat detects whether a full OSSM3 Istio installation exists (returnsFalseif only the auto-createdopenshift-gatewayIstio CR is present)check_required_ossmautouse fixture that skips/fails tests marked with@pytest.mark.required_ossmwhen OSSM3 is not availableTest adjustments
required_ossmmarker totest_mtls_configuration.py,test_mtls_behaviour.py, andtest_xfcc_forwarding.pytest_kuadrant_tracing.pyandtest_kuadrant_tracing_rate_limit_only.pyto make gateway service assertions conditional onhas_ossmand lowermin_processesthresholds when gateway traces are unavailableInfo collector
get_istio_type()toReportPortalMetadataCollectorto detect Istio installation type (gateway-api-managed,user-managed-ossm, ornone) via cluster-wide Istio CR querytest_istio_propertiesto first detect the Istio type and then query the correct namespace for metadata (openshift-ingressfor Gateway API-managed,istio-systemfor user-managed OSSM) instead ofhardcoding
istio-systemKnown failures (unrelated to this PR)
The following data plane tracing tests fail due to a known wasm-shim issue with missing
send_replyspans and span hierarchy:test_send_reply_span_on_request_rejection[429-trace_429]test_send_reply_span_on_request_rejection[401-trace_401]test_span_hierarchyVerification steps
Run on a cluster with Gateway API-managed Istio (auto-provisioned
openshift-gatewayCR, no user-managed OSSM3):Summary by CodeRabbit