feat(auth): add workload delegation - #1178
Open
ironcommit wants to merge 2 commits into
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThis change adds RFC 8693 actor claims, workload delegation storage, opaque Docker proof tokens, delegated token exchange validation, and Docker and Kubernetes workload identity integration. ChangesWorkload OBO authentication
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@packages/nmp_common/src/nmp/common/auth/jwt.py`:
- Around line 158-164: Update the actor subject validation in the claims parsing
method around actor_subject to strip surrounding whitespace and return None when
the normalized value is empty; pass the normalized subject into
ActorClaims.subject. Add a unit test covering a whitespace-only act.sub claim
and asserting it is rejected.
In `@packages/nmp_common/src/nmp/common/auth/workload_delegations.py`:
- Around line 239-264: Update the workload delegation validation routine after
validating the required fields to derive the expected name with
docker_delegation_name(...) for Docker delegations or
reference_delegation_name(...) for reference delegations, then reject
entity.name when it differs. Add mismatch tests covering both delegation types
while preserving the existing reference and expiry validations.
- Around line 165-182: Update the conflict handling in the delegation creation
method around _entity_client.add, get, and _copy_delegation_payload to compare
expected_db_version with the fetched existing row’s database version before
replacing fields. Raise WorkloadDelegationConflictError when the versions
differ, preserving the caller-version concurrency check; add tests covering
stale updates and stale replacements.
In `@plans/ready/2026-07-14-workload-token-exchange-obo-spec.md`:
- Around line 26-40: Update
plans/ready/2026-07-14-workload-token-exchange-obo-spec.md lines 26-40 to
describe the existing actor-claim parsing in jwt.py and delegated principal
mapping in token_resolver.py, including actor identity and top-level subject
preservation. Update plans/ready/2026-07-14-workload-token-exchange-obo-plan.md
lines 64-161 to mark the completed Phase 1 work accurately and replace
applicable middleware.py targets with token_resolver.py; both documentation
sites require changes.
🪄 Autofix
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: 8397c7ef-8887-4737-917b-ae2176b0df22
📒 Files selected for processing (10)
packages/nmp_common/src/nmp/common/auth/__init__.pypackages/nmp_common/src/nmp/common/auth/jwt.pypackages/nmp_common/src/nmp/common/auth/token_resolver.pypackages/nmp_common/src/nmp/common/auth/workload_delegations.pypackages/nmp_common/tests/auth/test_jwt.pypackages/nmp_common/tests/auth/test_middleware.pypackages/nmp_common/tests/auth/test_token_resolver.pypackages/nmp_common/tests/auth/test_workload_delegations.pyplans/ready/2026-07-14-workload-token-exchange-obo-plan.mdplans/ready/2026-07-14-workload-token-exchange-obo-spec.md
Contributor
|
ironcommit
force-pushed
the
workload-token-exchange-obo/rsadler
branch
from
August 7, 2026 21:56
9d68deb to
9670175
Compare
Contributor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
packages/nemo_platform_ext/src/nemo_platform_ext/auth/workload_exchange.py (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate token-classification logic across two Python packages.
DOCKER_OPAQUE_WORKLOAD_PROOF_TOKEN_TYPE,DOCKER_OPAQUE_WORKLOAD_PROOF_PREFIX, andsubject_token_type_for_exchangehere are byte-for-byte identical topackages/nemo_platform_plugin/src/nemo_platform_plugin/client/oidc.py(Line 129-130, Line 297-302). If the prefix or token-type string changes, both copies must change together.Move this constant and function to a shared internal module (e.g.
nmp_common) ifnemo_platform_plugincan depend on it.Also applies to: 77-82, 98-98
🤖 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 `@packages/nemo_platform_ext/src/nemo_platform_ext/auth/workload_exchange.py` around lines 29 - 30, Consolidate the duplicated DOCKER_OPAQUE_WORKLOAD_PROOF_TOKEN_TYPE, DOCKER_OPAQUE_WORKLOAD_PROOF_PREFIX, and subject_token_type_for_exchange definitions from auth/workload_exchange.py and client/oidc.py into a shared internal module such as nmp_common. Update both packages to import and reuse the shared symbols, preserving the existing token classification behavior and avoiding parallel definitions.services/core/auth/src/nmp/core/auth/api/v2/workload_token_exchange.py (2)
399-428: 🚀 Performance & Scalability | 🔵 TrivialBounded retry holds the request for up to 5 seconds by default.
With defaults, a missing delegation causes 51 store lookups and 5 seconds of wait on the request path. Many concurrent pod starts with missing rows can saturate the auth service worker pool. Consider a lower default timeout, or a concurrency cap on retrying lookups, plus a metric for exhausted retries.
🤖 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 `@services/core/auth/src/nmp/core/auth/api/v2/workload_token_exchange.py` around lines 399 - 428, Reduce the default retry timeout used by get_delegation_with_retry to avoid holding requests for roughly five seconds when delegations are missing. Preserve the bounded retry behavior and existing non-retry path, and add a metric emitted when the retry budget is exhausted.
614-658: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThe same RSA key signs issued access tokens and validates Docker proof tokens.
workload_signing_key_asyncreturns the key used at Line 908 to mint access tokens. Here its public half validates inbound proof tokens. Only theissandtoken_useclaims separate the two token classes, and both checks run afterjwt.decodesucceeds. The checks are correct today, so this is not exploitable, but a future claim change or a relaxed audience check turns a minted access token into a valid proof token.Use a distinct key pair for Docker proof tokens, or bind proof tokens with a dedicated
audvalue that access tokens never carry.🤖 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 `@services/core/auth/src/nmp/core/auth/api/v2/workload_token_exchange.py` around lines 614 - 658, The Docker proof-token validation in _decode_docker_jwt_proof_token must be cryptographically separated from access-token signing. Use a dedicated Docker proof-token key pair for verification and issuance, or enforce a dedicated proof-token audience that minted access tokens cannot contain; update the corresponding token-minting path while preserving the existing issuer and token_use checks.services/core/jobs/tests/controllers/test_workload_tokens.py (1)
33-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the
("jwt", True, "jwt")case.The parametrization omits the only combination where an explicit JWT configuration succeeds.
💚 Proposed fix
[ ("jwt", False, "error"), + ("jwt", True, "jwt"), ("opaque", True, "opaque"), ("auto", True, "jwt"), ("auto", False, "opaque"), ],🤖 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 `@services/core/jobs/tests/controllers/test_workload_tokens.py` around lines 33 - 41, Add the missing ("jwt", True, "jwt") parameter set to the parametrization for the workload token configuration test, preserving the existing cases and expected outcomes.services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py (2)
707-710: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis method is now a no-op that still runs on every sync.
_restore_workload_identity_refresher_for_containerperforms label and mount lookups, then only logs.create_step_updatecalls it for every ACTIVE and PENDING sync. Remove the method and its call site, or keep only the early-exit guard.🤖 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 `@services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py` around lines 707 - 710, The `_restore_workload_identity_refresher_for_container` method is now redundant because it only performs lookups and logging during every sync. Remove this method and its invocation from `create_step_update`, or reduce it to an early-exit guard without performing label or mount lookups.
471-491: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCollapse the duplicate workload-delegation wrappers. Let
_register_workload_delegationand_revoke_workload_delegationcallasyncio.rundirectly.🤖 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 `@services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py` around lines 471 - 491, Remove the duplicate `_run_workload_delegation_register` and `_run_workload_delegation_revoke` wrappers, and update `_register_workload_delegation` and `_revoke_workload_delegation` to call their respective async methods directly via `asyncio.run`.services/core/jobs/tests/controllers/test_docker_backend.py (1)
819-831: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an assertion that the written token matches the stored hash.
The test checks that
opaque_subject_token_hashis notNone, but not that the token placed in the volume hashes to that value. That pairing is the whole authentication contract with the exchange. Capture the token passed to_write_workload_identity_subject_tokenand verify it withverify_opaque_docker_proof_token_hash.🤖 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 `@services/core/jobs/tests/controllers/test_docker_backend.py` around lines 819 - 831, The test around register_delegation should capture the token passed to _write_workload_identity_subject_token and verify that token against delegation.opaque_subject_token_hash using verify_opaque_docker_proof_token_hash, replacing the current non-None-only assertion while preserving the existing delegation field checks.
🤖 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 `@services/core/auth/src/nmp/core/auth/api/v2/workload_token_exchange.py`:
- Around line 697-712: Prevent externally signed JWT claims from populating the
internal bound reference in the subject-token verification flow. Update the
decoder/return path around _decode_kubernetes_subject_token and the
VerifiedSubjectToken construction so the TokenReview-derived reference is
tracked separately from claims, or remove _BOUND_REFERENCE_NAME_CLAIM and
_BOUND_REFERENCE_VALUE_CLAIM from JWT-decoder output before claims are consumed;
preserve bound-reference behavior only for trusted TokenReview data.
- Around line 752-762: Update the Docker proof validation around
verified_subject.is_docker_proof and verified_subject.is_opaque_docker_proof so
any stored opaque_subject_token_hash binding is enforced for Docker JWT proofs
as well. Reject a non-opaque Docker proof when
delegation.opaque_subject_token_hash is present, while preserving the existing
opaque proof secret and hash validation paths.
- Around line 891-897: Update the scope handling in the workload token exchange
flow around `_build_delegated_claims` and `_build_workload_only_claims` so
requested scopes are intersected with the configured delegated-exchange
allow-list before being included in the JWT or sent to the PDP. Preserve the
existing configured/default scope behavior while ensuring unauthorized requested
scopes are excluded.
In `@services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py`:
- Around line 533-566: Update the caller that invokes
_prepare_workload_identity_for_step to revoke the Docker workload delegation
when _prepare_container_args_for_start or run_container fails after identity
preparation. Preserve the existing task-volume cleanup, ensure revocation is
attempted only after successful registration, and retain the current exception
logging and propagation behavior.
- Around line 508-531: The _build_docker_workload_delegation method currently
sets delegation expires_at from proof_token.expires_at, which is limited to the
subject-token TTL and can expire during a running job. Set the
WorkloadDelegationEntity expiration from the available job or step deadline so
it remains valid for the workload duration, or implement renewal that updates
both the delegation row and token file; preserve the existing proof-token hash
and delegation fields.
- Around line 413-424: Update _workload_delegation_audience to avoid silently
returning the "nemo-platform" fallback when get_auth_config() fails: raise
JobStorageError for the configuration error, or at minimum log it at warning
level before returning. Preserve the existing workload_audience, audience, and
default resolution when authentication configuration loads successfully.
- Around line 193-199: Update the DockerWorkloadProofToken dataclass so its
bearer-secret token field is excluded from the generated repr by using the
dataclasses field configuration, while preserving token as the first field and
keeping the existing defaults and field order valid.
---
Nitpick comments:
In `@packages/nemo_platform_ext/src/nemo_platform_ext/auth/workload_exchange.py`:
- Around line 29-30: Consolidate the duplicated
DOCKER_OPAQUE_WORKLOAD_PROOF_TOKEN_TYPE, DOCKER_OPAQUE_WORKLOAD_PROOF_PREFIX,
and subject_token_type_for_exchange definitions from auth/workload_exchange.py
and client/oidc.py into a shared internal module such as nmp_common. Update both
packages to import and reuse the shared symbols, preserving the existing token
classification behavior and avoiding parallel definitions.
In `@services/core/auth/src/nmp/core/auth/api/v2/workload_token_exchange.py`:
- Around line 399-428: Reduce the default retry timeout used by
get_delegation_with_retry to avoid holding requests for roughly five seconds
when delegations are missing. Preserve the bounded retry behavior and existing
non-retry path, and add a metric emitted when the retry budget is exhausted.
- Around line 614-658: The Docker proof-token validation in
_decode_docker_jwt_proof_token must be cryptographically separated from
access-token signing. Use a dedicated Docker proof-token key pair for
verification and issuance, or enforce a dedicated proof-token audience that
minted access tokens cannot contain; update the corresponding token-minting path
while preserving the existing issuer and token_use checks.
In `@services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py`:
- Around line 707-710: The `_restore_workload_identity_refresher_for_container`
method is now redundant because it only performs lookups and logging during
every sync. Remove this method and its invocation from `create_step_update`, or
reduce it to an early-exit guard without performing label or mount lookups.
- Around line 471-491: Remove the duplicate `_run_workload_delegation_register`
and `_run_workload_delegation_revoke` wrappers, and update
`_register_workload_delegation` and `_revoke_workload_delegation` to call their
respective async methods directly via `asyncio.run`.
In `@services/core/jobs/tests/controllers/test_docker_backend.py`:
- Around line 819-831: The test around register_delegation should capture the
token passed to _write_workload_identity_subject_token and verify that token
against delegation.opaque_subject_token_hash using
verify_opaque_docker_proof_token_hash, replacing the current non-None-only
assertion while preserving the existing delegation field checks.
In `@services/core/jobs/tests/controllers/test_workload_tokens.py`:
- Around line 33-41: Add the missing ("jwt", True, "jwt") parameter set to the
parametrization for the workload token configuration test, preserving the
existing cases and expected outcomes.
🪄 Autofix
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: e0500315-7e03-45f7-8911-382e16e3b9ea
⛔ Files ignored due to path filters (3)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/auth/workload_exchange.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/auth/test_workload_exchange.pyis excluded by!sdk/**
📒 Files selected for processing (16)
docs/set-up/config-reference.mdxopenapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlpackages/nemo_platform_ext/src/nemo_platform_ext/auth/workload_exchange.pypackages/nemo_platform_ext/tests/auth/test_workload_exchange.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/oidc.pypackages/nemo_platform_plugin/tests/test_client_auth.pyservices/core/auth/src/nmp/core/auth/api/v2/workload_token_exchange.pyservices/core/auth/tests/test_workload_token_exchange.pyservices/core/jobs/jobs-launcher/cmd/workload_auth.goservices/core/jobs/jobs-launcher/cmd/workload_auth_test.goservices/core/jobs/src/nmp/core/jobs/controllers/backends/docker.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/workload_tokens.pyservices/core/jobs/tests/controllers/test_docker_backend.pyservices/core/jobs/tests/controllers/test_workload_tokens.py
Contributor
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)
services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.py (1)
1018-1024: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve workload identity in all task clients
get_nemo_clientandget_async_nemo_clientstill read onlyNMP_PRINCIPALand do not configure workload token exchange. When this branch omitsNMP_PRINCIPAL, those clients send no authentication headers. Add workload identity support to both factories or retainNMP_PRINCIPAL.🤖 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 `@services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.py` around lines 1018 - 1024, Update get_nemo_client and get_async_nemo_client to preserve authentication when workload_identity_enabled is active: configure workload token exchange and required auth headers for those clients, or ensure NMP_PRINCIPAL remains available in the generated environment. Keep the existing principal-based environment setup unchanged for non-workload-identity tasks.
🧹 Nitpick comments (2)
services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.py (1)
140-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrivate SDK attribute access is fragile.
_custom_headers,_client.headers, and_nmp_request_routerare private to the SDK. An SDK upgrade silently degrades this to an unauthenticated client, becausegetattrdefaults mask the failure. Ask the SDK to expose a supported way to derive an async client from a sync client, or add an assertion that headers were resolved.🤖 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 `@services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.py` around lines 140 - 158, Update _create_async_nmp_sdk to avoid silently relying on private SDK attributes: use a supported SDK mechanism for deriving the async client from self._nmp_sdk if available, or explicitly validate that authentication headers were resolved and fail clearly when they are not. Do not use getattr defaults to construct an unauthenticated client, and preserve router propagation only through a supported interface.services/core/jobs/tests/controllers/test_kubernetes_backend.py (1)
2317-2471: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the failure paths.
All four tests patch
_register_workload_delegationand_revoke_workload_delegation, so the untested paths are the ones most likely to break in production:
- A pod without
metadata.uid, which must skip registration.- A job whose metadata is missing required labels, which must skip reconciliation.
- A registration error that is not
WorkloadDelegationConflictError.- Revocation when the pod list is empty, which currently revokes nothing.
Also, Line 2319 sets
workload_identity_token_expiration_seconds = 900but no assertion uses it.🤖 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 `@services/core/jobs/tests/controllers/test_kubernetes_backend.py` around lines 2317 - 2471, Add failure-path coverage alongside the existing Kubernetes workload delegation tests: verify pods without metadata.uid skip registration, jobs missing required labels skip reconciliation, non-WorkloadDelegationConflictError registration failures propagate or follow the expected error path, and empty pod lists still revoke the delegation. In test_kubernetes_job_registers_pod_uid_workload_delegation, assert the configured workload_identity_token_expiration_seconds=900 affects the delegation expiry instead of leaving the assignment unused.
🤖 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
`@services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.py`:
- Around line 311-320: Replace the unbounded delegation tracking sets at
services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.py:114-115
with a per-job mapping of registered delegation names, and update registration
to populate it. In _revoke_workload_delegations_for_job at
services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.py:311-320,
revoke the names recorded for the job instead of rederiving them from live pods,
then remove that job’s mapping entry after revocation completes.
- Around line 288-309: Update _ensure_workload_delegations_for_job to catch and
log non-conflict errors raised by _register_workload_delegation, allowing
create_step_update reconciliation to continue when the delegation store fails.
Keep conflict handling unchanged, and add delegation_name to
_registered_workload_delegations only after successful registration or the
existing conflict case; leave it unregistered for other failures so subsequent
syncs retry.
---
Outside diff comments:
In
`@services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.py`:
- Around line 1018-1024: Update get_nemo_client and get_async_nemo_client to
preserve authentication when workload_identity_enabled is active: configure
workload token exchange and required auth headers for those clients, or ensure
NMP_PRINCIPAL remains available in the generated environment. Keep the existing
principal-based environment setup unchanged for non-workload-identity tasks.
---
Nitpick comments:
In
`@services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.py`:
- Around line 140-158: Update _create_async_nmp_sdk to avoid silently relying on
private SDK attributes: use a supported SDK mechanism for deriving the async
client from self._nmp_sdk if available, or explicitly validate that
authentication headers were resolved and fail clearly when they are not. Do not
use getattr defaults to construct an unauthenticated client, and preserve router
propagation only through a supported interface.
In `@services/core/jobs/tests/controllers/test_kubernetes_backend.py`:
- Around line 2317-2471: Add failure-path coverage alongside the existing
Kubernetes workload delegation tests: verify pods without metadata.uid skip
registration, jobs missing required labels skip reconciliation,
non-WorkloadDelegationConflictError registration failures propagate or follow
the expected error path, and empty pod lists still revoke the delegation. In
test_kubernetes_job_registers_pod_uid_workload_delegation, assert the configured
workload_identity_token_expiration_seconds=900 affects the delegation expiry
instead of leaving the assignment unused.
🪄 Autofix
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: 3a3e08e6-5532-468f-962f-d537afe84e89
📒 Files selected for processing (3)
services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.pyservices/core/jobs/tests/controllers/test_kubernetes_backend.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
services/core/jobs/tests/controllers/test_kubernetes_backend.py (1)
2362-2366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the expiry from the constant.
The literal 1200 assumes
WORKLOAD_DELEGATION_TTL_BUFFER_SECONDSis 300. Usettl_seconds_active + WORKLOAD_DELEGATION_TTL_BUFFER_SECONDSso a constant change fails loudly instead of silently drifting.🤖 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 `@services/core/jobs/tests/controllers/test_kubernetes_backend.py` around lines 2362 - 2366, Update the expiry assertion around delegation.expires_at to replace the hard-coded 1200-second value with ttl_seconds_active + WORKLOAD_DELEGATION_TTL_BUFFER_SECONDS on both bounds, preserving the existing datetime comparisons.services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.py (1)
139-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated async SDK factory.
_create_async_nmp_sdkis byte-identical to the Docker backend copy, including the message "Kubernetes workload delegation requires authenticated SDK headers". The Docker backend therefore raises a Kubernetes-worded error. Move this helper into a shared module and pass a backend-specific message.🤖 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 `@services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.py` around lines 139 - 159, Extract the duplicated _create_async_nmp_sdk logic from the Kubernetes and Docker backends into a shared helper module, preserving header filtering, SDK configuration, and router reuse. Add a backend-specific missing-headers error-message parameter, and update both callers to pass their own delegation message so Docker never raises the Kubernetes wording.contrib/auth/README.md (1)
15-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the configured product substitution in all changed Markdown content.
contrib/auth/README.md#L15-L22: replace hardcodedNeMonames with|product-name|.contrib/auth/authentik/README.md#L8-L9: replace hardcodedNeMonames with|product-name|.contrib/auth/authentik/README.md#L89-L96: replace hardcodedNeMonames with|product-name|.contrib/auth/authentik/compose/implementation-details.md#L104-L107: replace hardcodedNeMonames with|product-name|.contrib/auth/authentik/compose/implementation-details.md#L147-L181: replace hardcodedNeMonames with|product-name|.contrib/auth/authentik/kubernetes/implementation-details.md#L107-L112: replace hardcodedNeMonames with|product-name|.As per coding guidelines, “Never hardcode product names; use substitutions in Sphinx configuration.”
🤖 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 `@contrib/auth/README.md` around lines 15 - 22, Replace hardcoded NeMo product names with the configured |product-name| substitution in all changed Markdown content: contrib/auth/README.md lines 15-22; contrib/auth/authentik/README.md lines 8-9 and 89-96; contrib/auth/authentik/compose/implementation-details.md lines 104-107 and 147-181; and contrib/auth/authentik/kubernetes/implementation-details.md lines 107-112. Preserve the surrounding wording and technical behavior.Source: Coding guidelines
packages/nemo_platform_plugin/src/nemo_platform_plugin/client_provider.py (1)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
WorkloadTokenExchangeProvider | Nonefor_workload_exchange_auth_from_env.
resolve_workload_exchange_providerreturnsWorkloadTokenExchangeProvider, and this helper returns only that provider orNone.🤖 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 `@packages/nemo_platform_plugin/src/nemo_platform_plugin/client_provider.py` around lines 19 - 24, Update the type annotation for _workload_exchange_auth_from_env to WorkloadTokenExchangeProvider | None, importing WorkloadTokenExchangeProvider from its defining module as needed. Keep the helper’s existing behavior of returning the resolved provider or None.Source: Coding guidelines
🤖 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 `@docs/auth/deployment/credential-propagation.mdx`:
- Around line 68-76: Update the managed job credential prohibition list in the
credential propagation documentation to include NMP_PRINCIPAL alongside the
existing forbidden workload-token variables. Preserve the exception allowing
direct SDK users to set NMP_WORKLOAD_IDENTITY_TOKEN_FILE when they own the
refreshed subject token file.
In
`@services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.py`:
- Around line 344-359: The cleanup_steps flow must retry previously recorded
failed workload-delegation revocations before their expires_at time, since
terminal steps are omitted by reconciliation. Update cleanup_steps and the
_revoke_workload_delegations_for_job state handling to include eligible
failed_delegations in the retry set, while retaining only still-failed names for
subsequent retries.
In `@tests/auth_idp/static/test_authentik_kubernetes_demo.py`:
- Around line 950-956: Replace the raw string assertions in the RBAC template
test with YAML parsing of the rendered template and inspect the resulting RBAC
rule list. Assert that tokenreviews/create is present while pod, pods/log, jobs,
and jobs/status permissions are absent regardless of YAML formatting or quoting;
retain the apiServiceAccountName assertion.
---
Nitpick comments:
In `@contrib/auth/README.md`:
- Around line 15-22: Replace hardcoded NeMo product names with the configured
|product-name| substitution in all changed Markdown content:
contrib/auth/README.md lines 15-22; contrib/auth/authentik/README.md lines 8-9
and 89-96; contrib/auth/authentik/compose/implementation-details.md lines
104-107 and 147-181; and
contrib/auth/authentik/kubernetes/implementation-details.md lines 107-112.
Preserve the surrounding wording and technical behavior.
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/client_provider.py`:
- Around line 19-24: Update the type annotation for
_workload_exchange_auth_from_env to WorkloadTokenExchangeProvider | None,
importing WorkloadTokenExchangeProvider from its defining module as needed. Keep
the helper’s existing behavior of returning the resolved provider or None.
In
`@services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.py`:
- Around line 139-159: Extract the duplicated _create_async_nmp_sdk logic from
the Kubernetes and Docker backends into a shared helper module, preserving
header filtering, SDK configuration, and router reuse. Add a backend-specific
missing-headers error-message parameter, and update both callers to pass their
own delegation message so Docker never raises the Kubernetes wording.
In `@services/core/jobs/tests/controllers/test_kubernetes_backend.py`:
- Around line 2362-2366: Update the expiry assertion around
delegation.expires_at to replace the hard-coded 1200-second value with
ttl_seconds_active + WORKLOAD_DELEGATION_TTL_BUFFER_SECONDS on both bounds,
preserving the existing datetime comparisons.
🪄 Autofix
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: 53dd1880-c491-4bf2-bdff-ecb8748052f1
📒 Files selected for processing (21)
contrib/auth/README.mdcontrib/auth/authentik/README.mdcontrib/auth/authentik/compose/implementation-details.mdcontrib/auth/authentik/kubernetes/implementation-details.mdcontrib/auth/authentik/manifest.yamldocs/auth/deployment/credential-propagation.mdxpackages/nemo_platform_ext/src/nemo_platform_ext/auth/workload_exchange.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/oidc.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client_provider.pypackages/nemo_platform_plugin/tests/test_client_auth.pypackages/nmp_common/src/nmp/common/auth/__init__.pypackages/nmp_common/src/nmp/common/auth/workload_delegations.pyservices/core/auth/src/nmp/core/auth/api/v2/workload_token_exchange.pyservices/core/auth/tests/test_workload_token_exchange.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/docker.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.pyservices/core/jobs/tests/controllers/test_docker_backend.pyservices/core/jobs/tests/controllers/test_kubernetes_backend.pyservices/core/jobs/tests/controllers/test_workload_tokens.pytests/auth_idp/contracts/test_jobs.pytests/auth_idp/static/test_authentik_kubernetes_demo.py
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/nmp_common/src/nmp/common/auth/init.py
- services/core/jobs/tests/controllers/test_workload_tokens.py
- services/core/auth/tests/test_workload_token_exchange.py
- packages/nmp_common/src/nmp/common/auth/workload_delegations.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
services/core/jobs/tests/controllers/test_kubernetes_backend.py (2)
2491-2493: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the failed registration is not recorded.
The retry only works if the failed delegation name stays out of
_workload_delegations_by_job. Assert that state directly so a future change that records before success is caught.💚 Proposed addition
assert register_delegation.call_count == 2 + job_key = f"test-namespace/{name_for_step(test_step_pending_with_auth_context)}" + assert kubernetes_job._workload_delegations_by_job.get(job_key, set()) == set()🤖 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 `@services/core/jobs/tests/controllers/test_kubernetes_backend.py` around lines 2491 - 2493, Add an assertion in the test covering the failed registration to verify the failed delegation name is absent from _workload_delegations_by_job after the retry flow. Keep the existing status and register_delegation.call_count assertions unchanged, and use the relevant job/delegation identifiers already established by the test.
2324-2330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated
auth_configstub into a fixture.The same
SimpleNamespaceOIDC stub is rebuilt in every delegation test. A shared fixture removes the duplication and keeps future config field additions in one place.♻️ Proposed fixture
+@pytest.fixture +def workload_exchange_auth_config(): + return SimpleNamespace( + oidc=SimpleNamespace( + workload_token_exchange_enabled=True, + workload_audience="nemo-platform", + audience=None, + ) + )Then use
patch("nmp.common.config.get_auth_config", return_value=workload_exchange_auth_config)in each test.🤖 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 `@services/core/jobs/tests/controllers/test_kubernetes_backend.py` around lines 2324 - 2330, Extract the repeated SimpleNamespace OIDC configuration into a shared fixture for the delegation tests, preserving workload_token_exchange_enabled, workload_audience, and audience values. Update each affected test to patch nmp.common.config.get_auth_config with the fixture value workload_exchange_auth_config instead of rebuilding auth_config locally.services/core/jobs/src/nmp/core/jobs/controllers/backends/workload_tokens.py (1)
35-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
nmp_sdkasNeMoPlatform, or define a protocol for the accessed SDK contract. The jobs call chain passesNeMoPlatform;Anyhides this contract and weakens type checking.🤖 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 `@services/core/jobs/src/nmp/core/jobs/controllers/backends/workload_tokens.py` around lines 35 - 36, Update the nmp_sdk parameter type in create_authenticated_async_nmp_sdk from Any to NeMoPlatform, or introduce and use a protocol describing the SDK members accessed by this function. Preserve the existing call behavior while enforcing the actual SDK contract through static typing.Source: Coding guidelines
🤖 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 `@contrib/auth/authentik/compose/implementation-details.md`:
- Around line 18-25: Replace every |product-name| placeholder throughout the
Markdown file with the literal product name “NeMo Platform,” including
configuration references, blueprint paths, and service descriptions.
---
Nitpick comments:
In
`@services/core/jobs/src/nmp/core/jobs/controllers/backends/workload_tokens.py`:
- Around line 35-36: Update the nmp_sdk parameter type in
create_authenticated_async_nmp_sdk from Any to NeMoPlatform, or introduce and
use a protocol describing the SDK members accessed by this function. Preserve
the existing call behavior while enforcing the actual SDK contract through
static typing.
In `@services/core/jobs/tests/controllers/test_kubernetes_backend.py`:
- Around line 2491-2493: Add an assertion in the test covering the failed
registration to verify the failed delegation name is absent from
_workload_delegations_by_job after the retry flow. Keep the existing status and
register_delegation.call_count assertions unchanged, and use the relevant
job/delegation identifiers already established by the test.
- Around line 2324-2330: Extract the repeated SimpleNamespace OIDC configuration
into a shared fixture for the delegation tests, preserving
workload_token_exchange_enabled, workload_audience, and audience values. Update
each affected test to patch nmp.common.config.get_auth_config with the fixture
value workload_exchange_auth_config instead of rebuilding auth_config locally.
🪄 Autofix
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: c20f9180-dffe-4e0a-aa30-4a69f25477dc
⛔ Files ignored due to path filters (2)
sdk/python/nemo-platform/src/nemo_platform/auth/workload_exchange.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/auth/test_workload_exchange.pyis excluded by!sdk/**
📒 Files selected for processing (22)
contrib/auth/README.mdcontrib/auth/authentik/README.mdcontrib/auth/authentik/compose/implementation-details.mdcontrib/auth/authentik/kubernetes/implementation-details.mdcontrib/auth/authentik/manifest.yamldocs/auth/deployment/credential-propagation.mdxpackages/nemo_platform_ext/src/nemo_platform_ext/auth/workload_exchange.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/oidc.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client_provider.pypackages/nemo_platform_plugin/tests/test_client_auth.pypackages/nmp_common/src/nmp/common/auth/__init__.pypackages/nmp_common/src/nmp/common/auth/workload_delegations.pyservices/core/auth/src/nmp/core/auth/api/v2/workload_token_exchange.pyservices/core/auth/tests/test_workload_token_exchange.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/docker.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/workload_tokens.pyservices/core/jobs/tests/controllers/test_docker_backend.pyservices/core/jobs/tests/controllers/test_kubernetes_backend.pyservices/core/jobs/tests/controllers/test_workload_tokens.pytests/auth_idp/contracts/test_jobs.pytests/auth_idp/static/test_authentik_kubernetes_demo.py
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/nemo_platform_plugin/tests/test_client_auth.py
- contrib/auth/README.md
- tests/auth_idp/static/test_authentik_kubernetes_demo.py
- packages/nmp_common/src/nmp/common/auth/init.py
- contrib/auth/authentik/manifest.yaml
- packages/nemo_platform_ext/src/nemo_platform_ext/auth/workload_exchange.py
- contrib/auth/authentik/README.md
- tests/auth_idp/contracts/test_jobs.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/client_provider.py
- services/core/jobs/tests/controllers/test_workload_tokens.py
- packages/nemo_platform_plugin/src/nemo_platform_plugin/client/oidc.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/kubernetes_job.py
- packages/nmp_common/src/nmp/common/auth/workload_delegations.py
- services/core/auth/tests/test_workload_token_exchange.py
- services/core/jobs/tests/controllers/test_docker_backend.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py
- services/core/auth/src/nmp/core/auth/api/v2/workload_token_exchange.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai review |
Contributor
|
15 tasks
Contributor
Author
|
@coderabbitai review |
Contributor
|
ironcommit
changed the title
Contributor
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 `@openapi/ga/individual/platform.openapi.yaml`:
- Around line 140-148: Update the jobs workload token exchange schema and
validation for resource, actor_token, and actor_token_type so any supplied field
is rejected based on presence, including empty or whitespace-only values and
explicitly empty occurrences. Do not rely on non-empty string validation;
preserve rejection for all occurrences of these unsupported fields.
🪄 Autofix
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: 7eb505f0-c68a-4bb0-8d98-b2a4b4e3884a
📒 Files selected for processing (5)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/core/auth/src/nmp/core/auth/api/v2/workload_token_exchange.pyservices/core/auth/tests/test_workload_token_exchange.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openapi/ga/openapi.yaml
ironcommit
force-pushed
the
workload-token-exchange-obo/rsadler
branch
3 times, most recently
from
August 8, 2026 19:30
11660f9 to
0370bfc
Compare
ironcommit
commented
Aug 9, 2026
ironcommit
commented
Aug 9, 2026
| if step.status in (PlatformJobStatus.ACTIVE, PlatformJobStatus.PENDING): | ||
| self._restore_workload_identity_refresher_for_container(container) | ||
|
|
||
| status, status_details, error_stack = self.map_docker_container_status_to_platform_status(step, container) |
Contributor
Author
There was a problem hiding this comment.
R3 - P1 blocking: Docker terminal reconciliation does not revoke the workload delegation.
Docker registers an opaque proof-token delegation before container start with expiry ttl_seconds_active + 300, but terminal create_step_update only updates task state and returns. Cleanup also only stops the refresher and removes the container/volumes. If a workload copied the proof token before exit, it can continue exchanging OBO access tokens until expiry. Kubernetes revokes the same delegation on terminal status.
Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
ironcommit
force-pushed
the
workload-token-exchange-obo/rsadler
branch
from
August 10, 2026 17:15
4da1682 to
a80042d
Compare
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 Phase 1 primitives for RFC 8693-aligned workload OBO token exchange. The change parses actor claims into delegated principals and adds auth-owned workload delegation entity helpers.
Related Issue
No linked issue.
Changes
Type of Change
Quality Gates
Verification
Targeted validation:
CI note:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation