feat(egress): refactored credential injection test to use Vault#951
feat(egress): refactored credential injection test to use Vault#951averevki wants to merge 1 commit intoKuadrant:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis pull request introduces Vault-based credential injection for egress gateway testing, adding configuration templates, hvac dependencies, a Vault client wrapper class, authorization model extensions to support CEL expressions, and comprehensive test implementations validating Vault-backed credential flows using Kubernetes authentication. Changes
Sequence Diagram(s)sequenceDiagram
participant Workload as Workload<br/>(with SA)
participant Authorino as Authorino<br/>(AuthPolicy)
participant Vault as Vault<br/>(Server)
participant Egress as Egress<br/>Gateway
participant Backend as MockServer<br/>Backend
Workload->>Authorino: 1. Request (with K8s SA token)
Authorino->>Vault: 2. POST /auth/kubernetes/login<br/>(SA token) [metadata.http p0]
Vault-->>Authorino: 3. Client token + auth data
Authorino->>Vault: 4. GET /secret/data/egress/.../<sa-name><br/>(Client token) [metadata.http p1]
Vault-->>Authorino: 5. Secret data {api_key: "..."}
Authorino->>Authorino: 6. Authorize (pattern match success)
Authorino->>Egress: 7. Request + response directives
Egress->>Backend: 8. Outbound request<br/>(Authorization: Bearer <api_key>)
Backend-->>Egress: 9. 200 OK
Egress-->>Workload: 10. Response
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
averevki
added
Test case
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
testsuite/kuadrant/policy/authorization/sections.py (1)
202-234:⚠️ Potential issue | 🟡 MinorConsider validating that at least one of
endpointorurl_expressionis provided.With both
endpointandurl_expressionnow optional and no mutual-exclusivity/presence check, callers can construct anhttpmetadata entry with neitherurlnorurlExpression, or with both set simultaneously. Authorino will reject it server-side, but surfacing the error at construction time makes failures easier to localise.🛡️ Suggested validation
"""Set metadata http external auth feature""" + if not endpoint and not url_expression: + raise ValueError("add_http requires either `endpoint` or `url_expression`") + if endpoint and url_expression: + raise ValueError("add_http accepts only one of `endpoint` or `url_expression`") http_config: dict = {"method": method, "headers": {"Accept": {"value": "application/json"}}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/kuadrant/policy/authorization/sections.py` around lines 202 - 234, In add_http, ensure callers supply exactly one of endpoint or url_expression: add a validation at the start of the add_http method (before building http_config) that raises a ValueError if neither endpoint nor url_expression is provided, and optionally also raise if both are provided (to enforce mutual exclusivity) so you don't construct an invalid "http" metadata object; reference the add_http function and the endpoint and url_expression parameters and perform this check before calling self.add_item.
🧹 Nitpick comments (5)
testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py (4)
115-127: Fragile manual JSON assembly in CEL; consider a readable alternative.The double-escaped JSON literal (
'"{\\"jwt\\": \\"" + ... + "\\", \\"role\\": \\"..."}}"') is hard to read and fragile — any future token whose value contains a double quote or backslash would produce invalid JSON. Since Authorino supports JSON body types viacontent_type="application/json"with a structuredbody, prefer a dict-shaped payload if the API allows it, or at minimum extract the CEL into a named helper/constant for readability.Also,
substring(7)implicitly assumes the incomingAuthorizationheader starts with exactlyBearer(7 chars). That matches the Kubernetes identity credentials convention used here, but a brief comment would save future readers a lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py` around lines 115 - 127, Replace the fragile manually-escaped JSON CEL expression passed to auth.metadata.add_http with a structured body using ValueOrSelector/CelExpression that builds a map/object (e.g., {"jwt": <token>, "role": <role>}) instead of concatenating JSON strings; locate the call to auth.metadata.add_http and change the body argument from the current CelExpression string concat to a map-shaped CEL expression (using ValueOrSelector with CelExpression that returns a map) or move the long CEL into a named constant for readability, and add a short inline comment next to the substring(7) usage explaining it assumes a "Bearer " prefix on request.headers.authorization to make the intent explicit.
20-20: Consider adding anissuemarker linking to#949.The PR closes issue
#949; addingpytest.mark.issue("https://github.com/Kuadrant/testsuite/issues/949")(or the Kuadrant repo URL) topytestmarkhelps with traceability in Report Portal.As per coding guidelines: "Common markers include:
@pytest.mark.authorino, ...@pytest.mark.issue('https://github.com/...'), ...".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py` at line 20, Add the issue marker to the existing pytestmark declaration so the test is traceable to issue `#949`; update the pytestmark list (the pytestmark variable in this file) to include pytest.mark.issue("https://github.com/Kuadrant/testsuite/issues/949") alongside pytest.mark.kuadrant_only and pytest.mark.egress_gateway.
154-170: Consider also covering the "authenticated SA but no Vault role binding" case.The linked issue explicitly lists a 403 scenario for a workload whose ServiceAccount token is valid at the K8s TokenReview stage but has no Vault role binding. The current three tests cover valid/invalid/missing SA tokens (all rejected at Kubernetes identity stage for the negative paths), but none exercise the Vault-side authorization failure. A fourth test using a SA that is not bound to the Vault role would close that gap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py` around lines 154 - 170, Add a new test that exercises the Vault-side authorization failure: create a test function (e.g., test_egress_authenticated_no_vault_binding_returns_403) that sends a request using a ServiceAccount token that is valid for Kubernetes TokenReview but intentionally has no Vault role binding (use or add a fixture like sa_token_unbound_to_vault or construct such a token), call client.get(mockserver_expectation, headers={"Authorization": f"Bearer {sa_token_unbound_to_vault}"}), and assert the response.status_code == 403 and that the backend Authorization header was not overwritten; mirror assertions and structure from test_egress_credential_injection_via_vault to keep consistency.
22-23: S105 is a false positive here.
K8S_TOKEN_AUDIENCEholds an audience URL, not a credential. If Ruff noise becomes an issue, suppress with# noqa: S105or rename; otherwise safe to ignore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py` around lines 22 - 23, S105 is a false positive for K8S_TOKEN_AUDIENCE because it stores an audience URL, not a secret; fix by either appending a Ruff suppression or renaming the identifier: add "# noqa: S105" to the K8S_TOKEN_AUDIENCE definition or rename the constant to a clearly non-secret name such as K8S_TOKEN_AUDIENCE_URL (update all references) so the linter no longer flags it.testsuite/kubernetes/vault.py (1)
28-28: Consider makingttlconfigurable.The
ttl="1h"is hardcoded inside the wrapper, which prevents callers from overriding it for longer-running tests or negative scenarios. Exposing it as a keyword argument (with the current value as default) would keep the call sites concise while preserving flexibility.♻️ Suggested change
- def create_role(self, name: str, sa_names: list[str], sa_namespaces: list[str], policies: list[str]): + def create_role( + self, name: str, sa_names: list[str], sa_namespaces: list[str], policies: list[str], ttl: str = "1h" + ): """Create a Kubernetes auth role binding service accounts to policies""" self._client.auth.kubernetes.create_role( name, bound_service_account_names=sa_names, bound_service_account_namespaces=sa_namespaces, policies=policies, - ttl="1h", + ttl=ttl, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/kubernetes/vault.py` at line 28, The hardcoded ttl="1h" in testsuite/kubernetes/vault.py prevents callers from overriding token lifetime; add a ttl keyword argument to the enclosing function (default="1h") and replace the literal ttl="1h" with the parameter so callers can pass a custom ttl; update any internal calls/tests to keep behavior unchanged by default but allow overriding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testsuite/kubernetes/vault.py`:
- Around line 21-29: The method create_role in testsuite/kubernetes/vault.py
calls a non-existent helper self._client.auth.kubernetes.createq_role; change
that call to the correct hvac method name create_role so the function body uses
self._client.auth.kubernetes.create_role(...). Keep the existing argument names
(bound_service_account_names, bound_service_account_namespaces, policies, ttl)
and ensure the method name in the call matches the enclosing method create_role.
In
`@testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py`:
- Around line 115-140: The test fixture fails because of a typo: replace the
incorrect symbol createq_role with the correct Vault.create_role call in
testsuite/kubernetes/vault.py (fix the function name where the vault fixture
invokes createq_role), ensure the module imports or class Vault exposes
create_role and update any callers to use create_role so the vault fixture and
downstream authorization fixture can initialize properly.
---
Outside diff comments:
In `@testsuite/kuadrant/policy/authorization/sections.py`:
- Around line 202-234: In add_http, ensure callers supply exactly one of
endpoint or url_expression: add a validation at the start of the add_http method
(before building http_config) that raises a ValueError if neither endpoint nor
url_expression is provided, and optionally also raise if both are provided (to
enforce mutual exclusivity) so you don't construct an invalid "http" metadata
object; reference the add_http function and the endpoint and url_expression
parameters and perform this check before calling self.add_item.
---
Nitpick comments:
In `@testsuite/kubernetes/vault.py`:
- Line 28: The hardcoded ttl="1h" in testsuite/kubernetes/vault.py prevents
callers from overriding token lifetime; add a ttl keyword argument to the
enclosing function (default="1h") and replace the literal ttl="1h" with the
parameter so callers can pass a custom ttl; update any internal calls/tests to
keep behavior unchanged by default but allow overriding.
In
`@testsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.py`:
- Around line 115-127: Replace the fragile manually-escaped JSON CEL expression
passed to auth.metadata.add_http with a structured body using
ValueOrSelector/CelExpression that builds a map/object (e.g., {"jwt": <token>,
"role": <role>}) instead of concatenating JSON strings; locate the call to
auth.metadata.add_http and change the body argument from the current
CelExpression string concat to a map-shaped CEL expression (using
ValueOrSelector with CelExpression that returns a map) or move the long CEL into
a named constant for readability, and add a short inline comment next to the
substring(7) usage explaining it assumes a "Bearer " prefix on
request.headers.authorization to make the intent explicit.
- Line 20: Add the issue marker to the existing pytestmark declaration so the
test is traceable to issue `#949`; update the pytestmark list (the pytestmark
variable in this file) to include
pytest.mark.issue("https://github.com/Kuadrant/testsuite/issues/949") alongside
pytest.mark.kuadrant_only and pytest.mark.egress_gateway.
- Around line 154-170: Add a new test that exercises the Vault-side
authorization failure: create a test function (e.g.,
test_egress_authenticated_no_vault_binding_returns_403) that sends a request
using a ServiceAccount token that is valid for Kubernetes TokenReview but
intentionally has no Vault role binding (use or add a fixture like
sa_token_unbound_to_vault or construct such a token), call
client.get(mockserver_expectation, headers={"Authorization": f"Bearer
{sa_token_unbound_to_vault}"}), and assert the response.status_code == 403 and
that the backend Authorization header was not overwritten; mirror assertions and
structure from test_egress_credential_injection_via_vault to keep consistency.
- Around line 22-23: S105 is a false positive for K8S_TOKEN_AUDIENCE because it
stores an audience URL, not a secret; fix by either appending a Ruff suppression
or renaming the identifier: add "# noqa: S105" to the K8S_TOKEN_AUDIENCE
definition or rename the constant to a clearly non-secret name such as
K8S_TOKEN_AUDIENCE_URL (update all references) so the linter no longer flags 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e89ad610-0254-4e07-936f-67f5e647a4bc
📒 Files selected for processing (10)
config/settings.local.yaml.tplpyproject.tomltestsuite/config/__init__.pytestsuite/config/tools.pytestsuite/kuadrant/policy/authorization/__init__.pytestsuite/kuadrant/policy/authorization/sections.pytestsuite/kubernetes/vault.pytestsuite/tests/singlecluster/egress/credentials_injection/conftest.pytestsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection.pytestsuite/tests/singlecluster/egress/credentials_injection/test_credential_injection_by_destination.py
Signed-off-by: averevki <sandyverevkin@gmail.com>
averevki
force-pushed
the
egress-vault-tests
branch
from
efb9dc6 to
af684de
Compare
1 participant
Note
Requires Kuadrant/helm-charts-olm#86 to be merged first (Vault deployment in
tools-vaultnamespace).Closes #949
Description
hvacPython client for Vault management instead ofkubectl execinto the Vault podValueOrSelectorandNamedValueOrSelectordataclasses matching Authorino's API spec for typed metadata HTTP configurationfetch_service_ipto support cross-namespace service discovery for Vault intools-vaultnamespaceChanges
New Features
testsuite/kubernetes/vault.py— NewVaultclass usinghvaclibrary for managing policies, Kubernetes auth roles, and KV v2 secretsValueOrSelectordataclass wrappingValue | ValueFrom | CelExpressionto match Authorino'sValueOrSelectorspecNamedValueOrSelectorextendingValueOrSelectorwith anamefield, serializing as{name: {value/selector/expression}}CelPredicateadded to theRuletype union for CEL-based pattern matchingPlainResponse.plainnow acceptsCelExpressionin addition toABCValueRefactoring
MetadataSection.add_http()— Extended with keyword-only parameters:url_expression: str,content_type: Literal[...],body: ValueOrSelector,headers: list[NamedValueOrSelector];endpointandmethodare now optionalfetch_service_ip()— Added optionalnamespaceparameter for cross-namespace LoadBalancer lookupsmockserver_expectation,service1_api_secret,sa_token_secret,authorization) moved fromconftest.pyinto their respective test files todecouple the Vault-based and K8s-Secret-based credential injection tests
Tests
test_credential_injection.py— Rewritten to use Vault Kubernetes auth flow: creates aServiceAccount, configures Vault policy/role/secret, buildsAuthPolicywithvault_login+vault_secretmetadata steps, and validates credential injection via MockServertest_credential_injection_by_destination.py— Now self-contained with its own fixtures (previously shared from conftest)Configuration
config/settings.local.yaml.tpl— Addedvault.urlandvault.tokensettings templatetestsuite/config/__init__.py— AddedDefaultValueValidatorentries forvault.url(auto-fetched fromtools-vaultnamespace) andvault.token(defaults to"root")pyproject.toml— Addedhvacto main dependencies,types-hvacto dev dependenciesVerification steps
Summary by CodeRabbit
Release Notes
New Features
Tests