fix(auth): jobs-launcher support workload and service bearer token - #851
Conversation
ce177c4 to
449ea57
Compare
📝 WalkthroughWalkthroughJob log exporters now select workload-identity or service-identity authentication, isolate platform headers from workload OTEL headers, and remove environment-based launcher exporter configuration across Docker, Kubernetes, and subprocess backends. ChangesOTLP Log Authentication
Sequence Diagram(s)sequenceDiagram
participant JobLauncher
participant AuthDiscovery
participant AuthSource
participant OTLPLogEndpoint
JobLauncher->>AuthDiscovery: select workload or service identity
AuthDiscovery->>AuthSource: create authorization source
AuthSource-->>JobLauncher: authorization header
JobLauncher->>OTLPLogEndpoint: export platform logs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
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 `@services/core/jobs/jobs-launcher/cmd/otel.go`:
- Line 34: Move the workload_identity_token_exchange logOTLPLogAuthMechanism
call from immediately after newOTLPLogWorkloadAuthTokenSource to the point after
newRefreshableAuthLogExporter successfully validates AuthorizationHeader, and
apply the same ordering to the related auth paths. Ensure no
authentication-mechanism success log is emitted when discovery or token exchange
returns an error.
🪄 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: 8cd0b9b2-aaa5-4d62-865c-331483422461
📒 Files selected for processing (2)
services/core/jobs/jobs-launcher/cmd/otel.goservices/core/jobs/jobs-launcher/cmd/workload_auth_test.go
449ea57 to
e9fc5e9
Compare
e9fc5e9 to
8ff103e
Compare
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
`@services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.py`:
- Around line 258-271: The _build_otlp_log_exporter function always uses
SERVICE_JOBS_BEARER_HEADERS instead of honoring configured workload identity.
Update its header selection to detect configured workload identity, perform the
required identity exchange, and use the resulting credentials; retain
service:jobs headers only when no workload identity is configured. In
services/core/jobs/tests/controllers/test_subprocess_runtime.py lines 132-155,
add coverage for workload-identity selection and keep service-header assertions
limited to the fallback case.
🪄 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: ac92b762-c145-4b2b-a171-c127c2f28e6c
📒 Files selected for processing (13)
services/core/jobs/jobs-launcher/cmd/otel.goservices/core/jobs/jobs-launcher/cmd/otel_test.goservices/core/jobs/jobs-launcher/cmd/run.goservices/core/jobs/jobs-launcher/cmd/run_test.goservices/core/jobs/jobs-launcher/cmd/workload_auth_test.goservices/core/jobs/src/nmp/core/jobs/controllers/backends/base.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/docker.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.pyservices/core/jobs/tests/controllers/test_docker_backend.pyservices/core/jobs/tests/controllers/test_kubernetes_backend.pyservices/core/jobs/tests/controllers/test_subprocess_runtime.py
💤 Files with no reviewable changes (5)
- services/core/jobs/src/nmp/core/jobs/controllers/backends/base.py
- services/core/jobs/jobs-launcher/cmd/run.go
- services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py
🚧 Files skipped from review as they are similar to previous changes (1)
- services/core/jobs/jobs-launcher/cmd/workload_auth_test.go
|
Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
8ff103e to
92b82c4
Compare
There was a problem hiding this comment.
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/subprocess_runtime.py (1)
260-291: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRefresh the OTLP log token per request.
get_access_token()is called once here and the bearer header is frozen intoOTLPLogExporter, so long-running jobs will keep sending an expired workload token and log uploads will start failing with 401s. Use a refreshable auth hook or rebuild the exporter when credentials rotate.🤖 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/subprocess_runtime.py` around lines 260 - 291, Update _otlp_log_auth_headers and _build_otlp_log_exporter so workload identity credentials are refreshed for each OTLP request instead of resolving get_access_token() once during exporter construction. Use the exporter’s refreshable authentication hook or equivalent provider callback, while preserving static SERVICE_JOBS_BEARER_HEADERS behavior when no token file is configured.
🧹 Nitpick comments (1)
services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.py (1)
278-291: 📐 Maintainability & Code Quality | 🔵 TrivialEmit the selected auth mode and fallback reason.
This branch selects workload exchange or service identity silently. Add an informational breadcrumb for the selected mechanism and missing-token-file fallback reason; never log the token or authorization header.
🤖 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/subprocess_runtime.py` around lines 278 - 291, Update _otlp_log_auth_headers to emit an informational breadcrumb for the selected authentication mechanism: log workload identity exchange when WORKLOAD_IDENTITY_TOKEN_FILE_ENVVAR is present, and service identity when it is missing, including that missing-token-file fallback reason. Use the existing logger and never include the token, token-file contents, or Authorization header.
🤖 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.
Outside diff comments:
In
`@services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.py`:
- Around line 260-291: Update _otlp_log_auth_headers and
_build_otlp_log_exporter so workload identity credentials are refreshed for each
OTLP request instead of resolving get_access_token() once during exporter
construction. Use the exporter’s refreshable authentication hook or equivalent
provider callback, while preserving static SERVICE_JOBS_BEARER_HEADERS behavior
when no token file is configured.
---
Nitpick comments:
In
`@services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.py`:
- Around line 278-291: Update _otlp_log_auth_headers to emit an informational
breadcrumb for the selected authentication mechanism: log workload identity
exchange when WORKLOAD_IDENTITY_TOKEN_FILE_ENVVAR is present, and service
identity when it is missing, including that missing-token-file fallback reason.
Use the existing logger and never include the token, token-file contents, or
Authorization header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: eaae392e-5c04-49c3-9af0-458f8994050a
📒 Files selected for processing (13)
services/core/jobs/jobs-launcher/cmd/otel.goservices/core/jobs/jobs-launcher/cmd/otel_test.goservices/core/jobs/jobs-launcher/cmd/run.goservices/core/jobs/jobs-launcher/cmd/run_test.goservices/core/jobs/jobs-launcher/cmd/workload_auth_test.goservices/core/jobs/src/nmp/core/jobs/controllers/backends/base.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/docker.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess_runtime.pyservices/core/jobs/tests/controllers/test_docker_backend.pyservices/core/jobs/tests/controllers/test_kubernetes_backend.pyservices/core/jobs/tests/controllers/test_subprocess_runtime.py
💤 Files with no reviewable changes (5)
- services/core/jobs/jobs-launcher/cmd/run.go
- services/core/jobs/src/nmp/core/jobs/controllers/backends/kubernetes/common.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/base.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/docker.py
- services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py
🚧 Files skipped from review as they are similar to previous changes (6)
- services/core/jobs/jobs-launcher/cmd/otel_test.go
- services/core/jobs/tests/controllers/test_kubernetes_backend.py
- services/core/jobs/jobs-launcher/cmd/run_test.go
- services/core/jobs/tests/controllers/test_docker_backend.py
- services/core/jobs/jobs-launcher/cmd/otel.go
- services/core/jobs/jobs-launcher/cmd/workload_auth_test.go
Summary
Update jobs-launcher platform application log upload authentication on the launcher-private OTLP path.
NMP_JOB_LAUNCHER_OTLP_LOGS_ENDPOINT; exporter, protocol, timeout, compression, and auth headers are fixed in code.NMP_JOB_LAUNCHER_LOGS_EXPORTER,NMP_JOB_LAUNCHER_OTLP_LOGS_PROTOCOL,NMP_JOB_LAUNCHER_OTLP_LOGS_HEADERS,NMP_JOB_LAUNCHER_OTLP_LOGS_TIMEOUT, andNMP_JOB_LAUNCHER_OTLP_LOGS_COMPRESSIONfor application logs.NMP_WORKLOAD_IDENTITY_TOKEN_FILEis configured, then send the exchanged access token asAuthorization: Bearer <token>.Authorization: Bearer service:jobs.OTEL_EXPORTER_OTLP_LOGS_HEADERSfor platform application logs so workloads can reserve it for third-party OTEL export.log.infobreadcrumbs that identify the selected auth mechanism and reason before creating the platform log exporter.Notes
The two expected application log auth modes are
workload_identity_token_exchangeandservice_identity_bearer_token. There is no unauthenticated platform-log path.Summary by CodeRabbit