LCORE-1027: Unit tests for RH identity authentication plugin

[tisnik]

Description

LCORE-1027: Unit tests for RH identity authentication plugin

Type of change

• Refactor
• New feature
• Bug fix
• CVE fix
• Optimization
• Documentation Update
• Configuration Update
• Bump-up service version
• Bump-up dependent library
• Bump-up library or tool used for development (does not change the final image)
• CI configuration change
• Konflux configuration change
• Unit tests improvement
• Integration tests improvement
• End to end tests improvement

Related Tickets & Documents

• Related Issue #LCORE-1027

Summary by CodeRabbit

Release Notes

• New Features

  • Red Hat Identity authentication support is now available with configurable entitlements settings.

• Tests

  • Enhanced test coverage for Red Hat Identity authentication scenarios, including validation and edge cases.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR introduces RH Identity authentication support by adding RHIdentityConfiguration as a publicly exported configuration type and AUTH_MOD_RH_IDENTITY as a new authentication module constant. Test coverage is extended to validate RH Identity scenarios including default configuration, entitlements, and validation error cases.

Changes

Cohort / File(s)	Change Summary
RH Identity Authentication Support tests/unit/models/config/test_authentication_configuration.py	Extended test suite to cover RH Identity authentication scenarios (default, with entitlements, multiple entitlements) and validation error handling. Updated existing tests to account for new rh_identity_config field on AuthenticationConfiguration. Renamed/expanded tests for authentication modules (noop, k8s, jwk, rh_identity).
Configuration & Constants models/config (RHIdentityConfiguration), constants (AUTH_MOD_RH_IDENTITY)	Added RHIdentityConfiguration as publicly exported configuration type and AUTH_MOD_RH_IDENTITY constant to support RH Identity as a new authentication module option.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Review test coverage for RH Identity scenarios and validation logic
• Verify rh_identity_config field integration with existing AuthenticationConfiguration model
• Confirm new constants and configuration types follow existing naming and export patterns

Possibly related PRs

• #814 — Adds RH Identity authentication support at the code level with RHIdentityConfiguration and AUTH_MOD_RH_IDENTITY constant introduction in configuration/constants codepaths
• #336 — Adds unit tests for AuthenticationConfiguration model validation across different authentication modules including JWK and RH Identity

Suggested labels

ok-to-test

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title clearly and specifically summarizes the main change: adding unit tests for the RH identity authentication plugin, which aligns with the changeset that introduces RH identity support and extends tests.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

tests/unit/models/config/test_authentication_configuration.py (4)

27-49: Add a negative test for rh_identity_configuration on non-RH modules

The extra assertion that auth_config.rh_identity_config is None for the noop module is good. To fully cover the new rh_identity_configuration property, consider adding a dedicated test that verifies it raises on non-RH modules, similar to how jwk_configuration is validated:

def test_authentication_configuration_rh_identity_property_unavailable_for_non_rh() -> None:
    auth_config = AuthenticationConfiguration(
        module=AUTH_MOD_NOOP,
        skip_tls_verification=False,
        k8s_ca_cert_path=None,
        k8s_cluster_api=None,
    )

    with pytest.raises(
        ValueError,
        match="RH Identity configuration is only available for RH Identity authentication module",
    ):
        _ = auth_config.rh_identity_configuration

This guards the public property contract rather than just the underlying field.

---

51-123: Strengthen RH Identity tests by asserting required_entitlements and exercising the property

These four tests nicely cover the constructor success cases for RH Identity with different entitlement setups, but they currently only assert that rh_identity_config is not None. Given the test names, it would be more robust to also assert the actual required_entitlements values and to exercise the rh_identity_configuration property. For example:

 def test_authentication_configuration_rh_identity() -> None:
@@
-    assert auth_config.rh_identity_config is not None
+    assert auth_config.rh_identity_config is not None
+    assert auth_config.rh_identity_configuration is auth_config.rh_identity_config
+    assert auth_config.rh_identity_configuration.required_entitlements == []

@@
 def test_authentication_configuration_rh_identity_default_value() -> None:
@@
-    assert auth_config.rh_identity_config is not None
+    assert auth_config.rh_identity_config is not None
+    assert auth_config.rh_identity_configuration is auth_config.rh_identity_config
+    assert auth_config.rh_identity_configuration.required_entitlements is None

@@
 def test_authentication_configuration_rh_identity_one_entitlement() -> None:
@@
-    assert auth_config.rh_identity_config is not None
+    assert auth_config.rh_identity_config is not None
+    assert auth_config.rh_identity_configuration is auth_config.rh_identity_config
+    assert auth_config.rh_identity_configuration.required_entitlements == ["foo"]

@@
 def test_authentication_configuration_rh_identity_more_entitlements() -> None:
@@
-    assert auth_config.rh_identity_config is not None
+    assert auth_config.rh_identity_config is not None
+    assert auth_config.rh_identity_configuration is auth_config.rh_identity_config
+    assert auth_config.rh_identity_configuration.required_entitlements == [
+        "foo",
+        "bar",
+        "baz",
+    ]

This both validates the intended semantics of different entitlement configurations and ensures the new property is tested on the happy path.

---

125-135: Tighten the ValidationError assertion for insufficient RH Identity config

match="RH" is very broad and would pass for almost any RH-related ValidationError. To better document and lock in the expected failure reason, consider matching the more specific substring from the validator message:

-def test_authentication_configuration_rh_identity_but_insufficient_config() -> None:
-    """Test the AuthenticationConfiguration with RH identity token."""
-
-    with pytest.raises(ValidationError, match="RH"):
-        AuthenticationConfiguration(
+def test_authentication_configuration_rh_identity_but_insufficient_config() -> None:
+    """Test the AuthenticationConfiguration with RH identity token."""
+
+    with pytest.raises(
+        ValidationError,
+        match="RH Identity configuration must be specified",
+    ):
+        AuthenticationConfiguration(
             module=AUTH_MOD_RH_IDENTITY,
             skip_tls_verification=False,
             k8s_ca_cert_path=None,
             k8s_cluster_api=None,
         )

This makes the test more resilient to unrelated changes while still tolerating any prefixes added by the validation framework.

---

254-337: Consider deduplicating Configuration setup across in-config tests

The three tests test_authentication_configuration_in_config_k8s, _rh_identity, and _jwktoken all construct a Configuration instance with nearly identical arguments, differing only in the authentication block. To reduce duplication and make future changes to the common config cheaper, you could factor out shared kwargs into a helper or fixture, e.g.:

import pytest

from typing import Any

@pytest.fixture
def base_configuration_kwargs() -> dict[str, Any]:
    return {
        "name": "test_name",
        "service": ServiceConfiguration(),
        "llama_stack": LlamaStackConfiguration(
            use_as_library_client=True,
            library_client_config_path="tests/configuration/run.yaml",
        ),
        "user_data_collection": UserDataCollection(
            feedback_enabled=False, feedback_storage=None
        ),
        "mcp_servers": [],
    }

and then:

def test_authentication_configuration_in_config_k8s(base_configuration_kwargs: dict[str, Any]) -> None:
    cfg = Configuration(
        authentication=AuthenticationConfiguration(
            module=AUTH_MOD_K8S,
            skip_tls_verification=True,
            k8s_ca_cert_path="tests/configuration/server.crt",
            k8s_cluster_api=None,
        ),
        **base_configuration_kwargs,
    )
    ...

Apply the same pattern for RH Identity and JWK. The current tests are fine as-is; this is just a maintainability improvement.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 211a3e0 and 048b0c2.

📒 Files selected for processing (1)

• tests/unit/models/config/test_authentication_configuration.py (7 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-09-02T11:09:40.404Z

Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.

Applied to files:

• tests/unit/models/config/test_authentication_configuration.py

🧬 Code graph analysis (1)

tests/unit/models/config/test_authentication_configuration.py (1)

src/models/config.py (7)

• RHIdentityConfiguration (412-415)
• AuthenticationConfiguration (418-473)
• Configuration (623-649)
• ServiceConfiguration (149-166)
• LlamaStackConfiguration (177-220)
• UserDataCollection (223-256)
• JwkConfiguration (405-409)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: build-pr
• GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
• GitHub Check: e2e_tests (ci)
• GitHub Check: e2e_tests (azure)

🔇 Additional comments (2)

tests/unit/models/config/test_authentication_configuration.py (2)

9-24: Imports for RH Identity configuration and constant are consistent

Adding RHIdentityConfiguration and AUTH_MOD_RH_IDENTITY here keeps this test module aligned with the new configuration model and constants; no issues spotted.

---

232-252: Default Configuration.authentication behavior is well covered

This test validates that the default Configuration.authentication module is AUTH_MOD_NOOP and that the associated TLS/K8s fields are unset, which is exactly what we want from the implicit configuration. No further changes needed here.