Skip to content

refactor: extract shared SQL tool wrapper helper#2

Open
Ghraven wants to merge 6 commits intomainfrom
refactor/extract-sql-wrapper-helper
Open

refactor: extract shared SQL tool wrapper helper#2
Ghraven wants to merge 6 commits intomainfrom
refactor/extract-sql-wrapper-helper

Conversation

@Ghraven
Copy link
Copy Markdown
Owner

@Ghraven Ghraven commented Apr 28, 2026

Closes Tracer-Cloud#894

What

Adds app/tools/utils/sql_wrapper.py with a single run_sql_tool() helper that encapsulates the repeated pattern shared by all six SQL tools:

resolve config → call integration fn → attach optional warning → return dict

Files

  • app/tools/utils/sql_wrapper.py — the helper with full docstring and example
  • tests/tools/utils/test_sql_wrapper.py — 7 tests covering:
    • Happy path returns integration result ✅
    • kwargs forwarded correctly ✅
    • No warning added by default ✅
    • Warning injected on success ✅
    • Warning NOT injected on failure (regression) ✅
    • Error dict preserved on failure ✅
    • Output keys unchanged (regression) ✅

Scope

This PR adds only the helper and its tests as specified in the issue. Migration of the six SQL tools (AzureSQL, PostgreSQL, MySQL, MariaDB) is left for a follow-up PR to keep the diff reviewable. Tool names, schemas, and output keys are unchanged.

Ghraven pushed a commit that referenced this pull request Apr 30, 2026
…ilder (Tracer-Cloud#976)

* fix(eks): forward stored AWS integration credentials to k8s client builder

Fixes Tracer-Cloud#969. Follow-up to Tracer-Cloud#724.

Problem
-------
PR Tracer-Cloud#724 let EKS be detected when the AWS integration is configured with
stored IAM user credentials (access_key_id + secret_access_key, no
role_arn), but build_k8s_clients still had no way to use those stored
credentials. After Tracer-Cloud#724 it supports:

  1. role_arn -> STS AssumeRole
  2. ambient botocore chain (env / shared config / instance profile / IRSA)

The stored integration credentials (persisted by resolve_integrations
and surfaced in _eks_int['credentials']) were silently dropped. On hosts
that happen to have ambient credentials pointing elsewhere, this picks
up the wrong principal; on hosts without ambient credentials the call
fails entirely, even though the integration has perfectly usable keys.

Fix (approach #2, pre-approved by Greptile in PR Tracer-Cloud#724 review)
-------------------------------------------------------------
Forward the stored credentials end-to-end through the EKS pipeline:

1. detect_sources.py: set eks_params['credentials'] = _eks_int.get(
   'credentials') or None alongside the other EKS integration fields.

2. EKSListClustersTool._eks_creds (shared helper used by every EKS tool
   for its extract_params): include 'credentials' in the returned dict
   so it flows into each tool's kwargs.

3. Seven EKS tool functions accept a new `credentials` kwarg and forward
   it to build_k8s_clients (and surface it in their input_schema):
     EKSPodLogsTool, EKSListPodsTool, EKSEventsTool, EKSNodeHealthTool,
     EKSListDeploymentsTool, EKSDeploymentStatusTool,
     EKSListNamespacesTool.

4. eks_k8s_client.build_k8s_clients accepts an optional credentials
   kwarg. Resolution priority:
     1. explicit credentials kwarg (stored-integration, new)
     2. role_arn AssumeRole         (existing production path)
     3. ambient botocore chain      (Tracer-Cloud#724 fallback)

The two secondary fixes from Tracer-Cloud#724 are preserved (empty SessionToken
coerced to None, regional STS endpoint).

Addresses Greptile P1
---------------------
The initial commit only wired the two ends of the pipeline (detect_sources
and build_k8s_clients); Greptile's review of Tracer-Cloud#976 correctly pointed out
that the shared _eks_creds helper and the seven tool call sites in between
were never updated, leaving the new credentials branch unreachable. This
revision wires credentials through every intermediate layer so each of
the seven EKS tools passes credentials=credentials to build_k8s_clients.

Test plan
---------
- py_compile passes on all 10 changed files.
- ruff check + ruff format clean on all 10 changed files.
- Regression safety: the role_arn (AssumeRole) and ambient-botocore
  branches are unchanged; the new explicit branch only runs when the
  credentials kwarg is truthy and has access_key_id + secret_access_key.

Related
-------
- Fixes Tracer-Cloud#969.
- Follow-up to Tracer-Cloud#724.
- Greptile pre-approved this approach in
  Tracer-Cloud#724 (comment)
- Greptile P1 on Tracer-Cloud#976 that drove the helper+tools fixups:
  Tracer-Cloud#976 (comment)

* chore: trigger Greptile re-review after end-to-end credentials wiring

The amend (ac2d841) wired credentials through _eks_creds helper + 7 EKS
tool call sites per Greptile P1 on the initial commit b03928d. Pushing
an empty commit to ensure Greptile re-reviews the new diff (10 files,
+114/-11 vs the original 2 files, +50/-4).

* chore: second re-trigger Greptile re-review

First re-trigger commit (61f15ed) did not produce a Greptile re-review after ~4h. Pushing another empty commit to kick Greptile again so the end-to-end credentials wiring (helper + 7 EKS tools in ac2d841) gets a fresh review.

* fix(eks): thread stored credentials through EKSClient-backed tools

PR Tracer-Cloud#976 wired stored AWS-integration credentials end-to-end for the
seven k8s-API EKS tools that route through `build_k8s_clients`, but
missed the four EKS tools that still go through `EKSClient`:
`list_eks_clusters`, `describe_eks_cluster`, `describe_eks_addon`,
`get_eks_nodegroup_health`. Their `_eks_creds` helper already exposes
`credentials`, but each tool absorbed it via `**_kwargs` and never
forwarded it, so `EKSClient._build` unconditionally called
`sts.assume_role(RoleArn="", ...)` and raised `ParamValidationError`
for IAM-user-only integrations at the cluster-discovery step.

Greptile flagged this exact gap on the (now-closed) parallel PR Tracer-Cloud#990.
This commit closes it on Tracer-Cloud#976.

Changes
-------
- `EKSClient.__init__` takes an optional `credentials` kwarg. `_build`
  prefers stored IAM user keys over STS AssumeRole; if neither is
  provided it raises `ValueError` (no silent fallthrough).
- New shared helper `_stored_credentials_to_aws_creds` normalises the
  stored dict into the AssumeRole-shaped triplet (empty `session_token`
  → None, both required keys present). `build_k8s_clients` now uses
  the same helper so both code paths stay in sync.
- The four EKSClient-backed tools accept `credentials` in the signature,
  expose it in `input_schema`, and pass it into the constructor.

Tests
-----
- New `tests/services/test_eks_client.py` covers the helper (full
  triplet / empty session token / missing required keys / None / empty
  dict) and EKSClient credential resolution (stored skips AssumeRole,
  empty session token coerced, partial dict falls back to AssumeRole,
  no creds + no role_arn raises ValueError, AssumeRole external_id
  passthrough stays intact).
- `test_eks_list_clusters_tool.py` grows two forwarding tests: the
  kwarg threads into EKSClient, and stays None by default for existing
  role-based callers.

Quality gate
------------
make lint / format-check / typecheck / test-cov all green
(3328 passed, 2 skipped, 1 xfailed).

Related
-------
- Extends Tracer-Cloud#976
- Fixes Tracer-Cloud#969
- Closes Greptile P1 on Tracer-Cloud#990
  (Tracer-Cloud#990 (comment))

* refactor(eks): address maintainer review feedback

Addresses all five inline comments from @muddlebee on Tracer-Cloud#976:

1. **eks_client.py** — add `from __future__ import annotations`
   (project style convention).

2. **utils module, public helper** — move
   `_stored_credentials_to_aws_creds` out of `eks_client.py` into a
   new `app/services/eks/utils.py` and drop the leading underscore
   (`stored_credentials_to_aws_creds`). The import contract between
   `eks_client` and `eks_k8s_client` is now explicit public API
   rather than two services reaching into each other for a private
   helper.

3. **eks_k8s_client.py** — promote the `import botocore.session`
   that lived in the ambient-fallback branch to the top-level imports.
   No circular-import concern (`botocore` is already imported at
   module level).

4. **eks_k8s_client.py** — normalize `frozen.token or None` at the
   source instead of relying on the downstream `or None` guard. This
   matches the behaviour of `stored_credentials_to_aws_creds` and
   removes the hidden dependency.

5. **All 11 EKS tool files** — tighten `credentials` parameter type
   annotation from `dict | None` to `dict[str, Any] | None`, matching
   the service-layer style and keeping mypy strict mode happy.

The `tests/services/test_eks_client.py` import also switches to the
new public name and module path.

Quality gate
------------
make lint          → All checks passed!
make format-check  → 906 files already formatted
make typecheck     → Success: no issues found in 392 source files
make test-cov      → 3328 passed, 2 skipped, 1 xfailed in 43.36s

No behaviour changes — pure refactor of public surface + style.

---------

Co-authored-by: weichao <[email protected]>
Co-authored-by: chaosreload <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extract one shared wrapper helper for the repeated SQL-tool flow

1 participant