feat(auth): publish SPIFFE credential health metrics#169
Open
dmathur0 wants to merge 3 commits into
Open
Conversation
dmathur0
requested review from
jojung1,
polarweasel,
ryanheffernan,
spidercensus,
susanhooks and
zsblevins
as code owners
July 23, 2026 01:57
📝 WalkthroughWalkthroughChangesSPIFFE configuration now loads a JWT-SVID file path. A Prometheus collector reports trust-bundle readability, key counts, certificate expiry, and JWT-SVID readability and expiration, with registration wired into identity probe setup and covered by unit and integration tests. SPIFFE credential health metrics
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant IdentityProbe as install_identity_probe
participant Registry as CollectorRegistry
participant Collector as SpiffeCredentialCollector
participant Files as Local credential files
participant MetricsEndpoint as Service /metrics endpoint
IdentityProbe->>Registry: install_spiffe_bundle_metrics()
MetricsEndpoint->>Registry: scrape metrics
Registry->>Collector: collect()
Collector->>Files: read trust bundle and JWT-SVID
Files-->>Collector: credential contents or read errors
Collector-->>Registry: health gauge metrics
Registry-->>MetricsEndpoint: Prometheus exposition output
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
src/tests/common/test_metrics.py (1)
182-231: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test: already-expired JWT-SVID.
No test decodes a token whose
expis already in the past. This is the central scenario the metric exists to detect (a decayingexppast "now" signals a deadspiffe-helper), and it depends on PyJWT'sverify_signature=Falseimplicitly disablingverify_expso the token still decodes instead of raisingExpiredSignatureError. Worth an explicit test locking in that assumption.🧪 Suggested test for already-expired JWT-SVID
+def test_jwt_svid_already_expired_still_readable(monkeypatch, tmp_path): + """An expired SVID must still report readable=1 with a past expiry, not unreadable.""" + svid = tmp_path / "jwt-svid" + exp = datetime.now(UTC) - timedelta(minutes=5) + _write_svid(svid, exp) + + spiffe = SpiffeConfig( + jwks_uri="https://spire:8443/keys", audiences=["spiffe://td"], jwt_svid_path=str(svid) + ) + _use_spiffe_config(monkeypatch, spiffe) + + samples = _samples(SpiffeCredentialCollector()) + assert samples[f"{_SVID}_readable"] == 1.0 + assert samples[f"{_SVID}_expiry_timestamp_seconds"] == pytest.approx(exp.timestamp(), abs=1.0)🤖 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 `@src/tests/common/test_metrics.py` around lines 182 - 231, Add a test alongside test_jwt_svid_expiry_emitted that writes a JWT-SVID with an expiration time in the past, collects metrics through SpiffeCredentialCollector, and asserts it remains readable while reporting the expired timestamp. Ensure the test exercises decoding with signature verification disabled so the expired token is accepted.
🤖 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 `@src/nv_config_manager/common/auth.py`:
- Around line 950-956: Wrap the install_spiffe_bundle_metrics() call in
install_identity_probe with exception handling so any metrics registration
failure is isolated and the remaining auth middleware, /whoami endpoint, and
enforcement setup continues. Preserve the existing local import and make the
failure non-fatal using the module’s established logging mechanism if available.
---
Nitpick comments:
In `@src/tests/common/test_metrics.py`:
- Around line 182-231: Add a test alongside test_jwt_svid_expiry_emitted that
writes a JWT-SVID with an expiration time in the past, collects metrics through
SpiffeCredentialCollector, and asserts it remains readable while reporting the
expired timestamp. Ensure the test exercises decoding with signature
verification disabled so the expired token is accepted.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dc183f3c-ba45-4185-bb85-e81420d9d378
📒 Files selected for processing (3)
src/nv_config_manager/common/auth.pysrc/nv_config_manager/common/metrics.pysrc/tests/common/test_metrics.py
dmathur0
added a commit
that referenced
this pull request
Jul 23, 2026
Wrap install_spiffe_bundle_metrics() in install_identity_probe with a try/except so a failure in this ancillary observability feature can never abort the primary auth wiring (middleware, /whoami, enforcement). Addresses CodeRabbit review finding on PR #169.
Add a lazy Prometheus collector that reports the health of the SPIFFE credentials written by the spiffe-helper sidecar, so operators can alert before service-to-service auth silently breaks. Trust bundle (inbound validation): - spiffe_trust_bundle_readable / _keys: bundle present and parseable. - spiffe_trust_bundle_earliest_cert_expiry_timestamp_seconds: emitted only when JWKS keys carry x5c chains. JWT-SVID (outbound credential liveness): - spiffe_jwt_svid_readable + spiffe_jwt_svid_expiry_timestamp_seconds: the short-lived token's exp decays toward now if spiffe-helper stops refreshing, which is the real "auth about to break" signal. Trust-bundle file age is deliberately not published because the bundle only changes on key rotation and would be a misleading liveness metric. The collector reads both files at scrape time (no background poller, no per-request overhead) and is registered via install_identity_probe so every FastAPI service exposes it on /metrics. It no-ops when SPIFFE is unconfigured or the bundle is served over HTTP. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
…n tests - Add a low-cardinality trust_domain label (derived from the SPIFFE ID in the token 'sub') to spiffe_jwt_svid_expiry_timestamp_seconds. Only the domain is used, never the workload path, to bound cardinality and avoid leaking identity. - Add live-cluster integration tests (src/tests/integration/test_spiffe_metrics.py) that scrape each FastAPI service's /metrics from inside a spiffe-enabled pod and assert the trust-bundle and JWT-SVID health metrics, including the trust_domain label matching the SVID's trust domain. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
Wrap install_spiffe_bundle_metrics() in install_identity_probe with a try/except so a failure in this ancillary observability feature can never abort the primary auth wiring (middleware, /whoami, enforcement). Addresses CodeRabbit review finding on PR #169. Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
dmathur0
force-pushed
the
davesh/spiffe-credential-metrics
branch
from
July 23, 2026 19:53
5dd9532 to
e605f7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/nv_config_manager/common/auth.py`:
- Around line 950-960: Move the local import of install_spiffe_bundle_metrics
inside the existing try block so import-time and registration failures are both
caught. Preserve the circular-import workaround, warning log, and continued auth
setup behavior.
In `@src/nv_config_manager/common/metrics.py`:
- Around line 230-234: Update the collector registration logic around the
module-global _collector guard so _collector is assigned only after
registry.register() succeeds. Track successful registration per
CollectorRegistry rather than using a single global early-return, allowing
installation into different registries while preserving idempotency for each
registry and retrying after registration failures.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ec0abddf-9881-4126-a477-af149ee22312
📒 Files selected for processing (4)
src/nv_config_manager/common/auth.pysrc/nv_config_manager/common/metrics.pysrc/tests/common/test_metrics.pysrc/tests/integration/test_spiffe_metrics.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/integration/test_spiffe_metrics.py
Comment on lines
+950
to
+960
| # Register SPIFFE credential health metrics on the shared registry. | ||
| # Imported here (not at module top) to avoid a circular import: metrics | ||
| # imports this module for auth-config access. | ||
| from nv_config_manager.common.metrics import install_spiffe_bundle_metrics | ||
|
|
||
| # Registration failure of an ancillary observability feature must never | ||
| # abort the primary auth wiring (middleware, /whoami, enforcement). | ||
| try: | ||
| install_spiffe_bundle_metrics() | ||
| except Exception: | ||
| logger.warning("Failed to register SPIFFE credential health metrics", exc_info=True) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Locate the relevant files and inspect their structure.
git ls-files 'src/nv_config_manager/common/auth.py' 'src/nv_config_manager/common/metrics.py'
printf '\n--- outline auth.py ---\n'
ast-grep outline src/nv_config_manager/common/auth.py --view expanded | sed -n '1,220p'
printf '\n--- outline metrics.py ---\n'
ast-grep outline src/nv_config_manager/common/metrics.py --view expanded | sed -n '1,220p'
printf '\n--- auth.py around target lines ---\n'
nl -ba src/nv_config_manager/common/auth.py | sed -n '930,980p'
printf '\n--- metrics.py imports / auth references ---\n'
rg -n 'from nv_config_manager\.common\.auth|import nv_config_manager\.common\.auth|auth' src/nv_config_manager/common/metrics.py src/nv_config_manager/common -g '*.py' | sed -n '1,200p'Repository: NVIDIA/nv-config-manager
Length of output: 4745
🏁 Script executed:
set -euo pipefail
# Fallback: inspect only the relevant slice if outline output is unavailable.
nl -ba src/nv_config_manager/common/auth.py | sed -n '940,970p'
rg -n 'from nv_config_manager\.common\.auth|import nv_config_manager\.common\.auth|install_spiffe_bundle_metrics|spiffe' src/nv_config_manager/common/metrics.py src/nv_config_manager/common -g '*.py' | sed -n '1,200p'Repository: NVIDIA/nv-config-manager
Length of output: 202
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/nv_config_manager/common/auth.py')
text = p.read_text()
for needle in [
'from nv_config_manager.common.metrics import install_spiffe_bundle_metrics',
'try:',
'install_spiffe_bundle_metrics()',
'logger.warning("Failed to register SPIFFE credential health metrics", exc_info=True)',
]:
print(f'-- {needle} --')
idx = text.find(needle)
print(idx)
PY
python3 - <<'PY'
from pathlib import Path
p = Path('src/nv_config_manager/common/metrics.py')
text = p.read_text()
for needle in [
'from nv_config_manager.common.auth',
'import nv_config_manager.common.auth',
'install_spiffe_bundle_metrics',
'spiffe',
]:
if needle in text:
print(f'FOUND {needle}')
PYRepository: NVIDIA/nv-config-manager
Length of output: 497
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
for file, start, end in [
('src/nv_config_manager/common/auth.py', 948, 962),
('src/nv_config_manager/common/metrics.py', 1, 120),
('src/nv_config_manager/common/metrics.py', 120, 260),
]:
print(f'\n--- {file}:{start}-{end} ---')
lines = Path(file).read_text().splitlines()
for i in range(start, min(end, len(lines)) + 1):
print(f'{i:4d}: {lines[i-1]}')
PYRepository: NVIDIA/nv-config-manager
Length of output: 11894
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
def show(path, start, end):
print(f'\n--- {path}:{start}-{end} ---')
lines = Path(path).read_text().splitlines()
for i in range(start, min(end, len(lines)) + 1):
print(f'{i:4d}: {lines[i-1]}')
show('src/nv_config_manager/common/auth.py', 945, 965)
show('src/nv_config_manager/common/metrics.py', 1, 90)
show('src/nv_config_manager/common/metrics.py', 220, 250)
PYRepository: NVIDIA/nv-config-manager
Length of output: 6595
Move the local import inside the try block. src/nv_config_manager/common/auth.py:953 runs before the failure boundary, so an import-time error from nv_config_manager.common.metrics still aborts auth setup before middleware, /whoami, and enforcement are installed. Keep the circular-import workaround, but catch import and registration failures together.
🤖 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 `@src/nv_config_manager/common/auth.py` around lines 950 - 960, Move the local
import of install_spiffe_bundle_metrics inside the existing try block so
import-time and registration failures are both caught. Preserve the
circular-import workaround, warning log, and continued auth setup behavior.
Source: Coding guidelines
Comment on lines
+230
to
+234
| global _collector | ||
| if _collector is not None: | ||
| return | ||
| _collector = SpiffeCredentialCollector() | ||
| registry.register(_collector) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file around the reported lines.
sed -n '1,320p' src/nv_config_manager/common/metrics.py
# Find all references to the collector and registration helper.
rg -n "_collector|register\\(" src/nv_config_manager/common/metrics.py src -g '*.py'Repository: NVIDIA/nv-config-manager
Length of output: 10765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure and inspect nearby definitions.
python3 - <<'PY'
from pathlib import Path
p = Path('src/nv_config_manager/common/metrics.py')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 'class ' in line or 'def ' in line or '_collector' in line or 'registry' in line:
print(f"{i}: {line}")
PY
# Inspect the collector class and registry-related imports if present.
python3 - <<'PY'
from pathlib import Path
p = Path('src/nv_config_manager/common/metrics.py')
lines = p.read_text().splitlines()
for start in (1, 100, 200, 220, 230):
end = min(len(lines), start + 30)
print(f"\n--- lines {start}-{end} ---")
for i in range(start, end + 1):
print(f"{i:4}: {lines[i-1]}")
PYRepository: NVIDIA/nv-config-manager
Length of output: 6616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the full file with line numbers; keep output bounded by focusing on the reported region.
sed -n '1,320p' src/nv_config_manager/common/metrics.py | nl -ba | sed -n '1,320p'Repository: NVIDIA/nv-config-manager
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test coverage around metrics registration.
sed -n '1,360p' src/tests/common/test_metrics.py
# Look for any custom-registry usage of the installer.
rg -n "install_spiffe_bundle_metrics|CollectorRegistry|REGISTRY|register\\(" src/tests src/nv_config_manager -g '*.py'Repository: NVIDIA/nv-config-manager
Length of output: 11662
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the metrics tests, especially any fixtures and registry-specific cases.
sed -n '1,340p' src/tests/common/test_metrics.py
# Narrow search for custom registry usage around this feature.
rg -n "install_spiffe_bundle_metrics|CollectorRegistry\\(|REGISTRY|_collector|Collector" src/tests/common/test_metrics.py src/tests -g '*.py'Repository: NVIDIA/nv-config-manager
Length of output: 14946
Register only after registry.register() succeeds. _collector is set before the registry call, so a failure leaves later retries as no-ops. The same module-global guard also blocks installing the collector into a different CollectorRegistry; track successful registrations per registry instead.
🤖 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 `@src/nv_config_manager/common/metrics.py` around lines 230 - 234, Update the
collector registration logic around the module-global _collector guard so
_collector is assigned only after registry.register() succeeds. Track successful
registration per CollectorRegistry rather than using a single global
early-return, allowing installation into different registries while preserving
idempotency for each registry and retrying after registration failures.
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
1 participant
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.
Summary
Adds a lazy Prometheus collector that reports the health of the SPIFFE credentials the spiffe-helper sidecar writes, so operators can alert before service-to-service auth silently breaks. Registered via
install_identity_probe, so every FastAPI service exposes it on its existing/metricsendpoint.Metrics
Trust bundle (
bundle.json— inbound JWT-SVID validation):nv_config_manager_spiffe_trust_bundle_readable—1if the bundle file reads + parses, else0nv_config_manager_spiffe_trust_bundle_keys— number of signing keys (0= broken bundle)nv_config_manager_spiffe_trust_bundle_earliest_cert_expiry_timestamp_seconds— earliestx5cnotAfter, emitted only when keys carry certificate chainsJWT-SVID (
jwt-svid— outbound credential liveness):nv_config_manager_spiffe_jwt_svid_readable—1if the token file reads + decodes, else0nv_config_manager_spiffe_jwt_svid_expiry_timestamp_seconds— the token'sexp; decays toward now if spiffe-helper stops refreshing (the real "auth about to break" alert target)Design notes
expis the correct liveness/pre-breakage signal.jwt-svidper outbound call).install_identity_probevia a local import to avoid a circular dependency (metricsimportsauthfor config).Test plan
uv run ruff check/ruff formatcleanuv run mypy src/nv_config_manager/common/metrics.pycleanuv run pytest src/tests/common/test_metrics.py src/tests/common/test_auth.py— 65 passed/metricsin a SPIFFE-enabled deploymentSummary by CodeRabbit
jwt_svid_path(defaulting to empty when unset)./metricsendpoint.