[AAP-63314] P4.4: Controller - Pass Workload TTL to Gateway#16303
[AAP-63314] P4.4: Controller - Pass Workload TTL to Gateway#16303arrestle merged 26 commits intoansible:develfrom
Conversation
|
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:
📝 WalkthroughWalkthroughPasses job.timeout as workload_ttl_seconds to Gateway when requesting workload identity JWTs (treats 0 as omitted, caps TTL at WORKLOAD_TTL_MAX_SECONDS). Adds detection for jobs needing workload JWTs, a token-request helper with logging/error handling, and invokes token fetch in pre_run_hook when the feature flag is enabled. Changes
Sequence DiagramsequenceDiagram
participant Job as Job
participant Controller as Controller
participant Gateway as Gateway
participant TokenService as TokenService
Job->>Controller: pre_run_hook(job, private_data_dir)
Controller->>Controller: Check FEATURE_WORKLOAD_IDENTITY_JWT_ENABLED
alt feature enabled
Controller->>Controller: _job_needs_workload_jwt(job)?
alt needs workload JWT
Controller->>Controller: Compute workload_ttl_seconds (0 → omitted, cap at WORKLOAD_TTL_MAX_SECONDS)
Controller->>Gateway: request JWT (claims, scope, audience, workload_ttl_seconds)
Gateway->>TokenService: fetch JWT
TokenService-->>Gateway: return JWT / error
Gateway-->>Controller: WorkloadIdentityTokenResponse / error
Controller->>Controller: Log success or TokenRequestError
else no JWT needed
Controller->>Controller: Skip JWT request
end
else feature disabled
Controller->>Controller: Skip JWT logic
end
Controller-->>Job: continue execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
arrestle
force-pushed
the
aap-63314-pass-workload-ttl
branch
2 times, most recently
from
edf2ba6 to
41bcf31
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/main/tasks/jobs.py`:
- Line 613: The workload_ttl is being derived from instance.timeout directly but
should use the effective runtime computed by get_instance_timeout() to account
for global fallbacks and negative-normalization; update the assignment that sets
workload_ttl (currently using instance.timeout and WORKLOAD_TTL_MAX_SECONDS) to
call get_instance_timeout(instance) (or the existing helper used in this module)
and then apply min(..., WORKLOAD_TTL_MAX_SECONDS) so the JWT TTL matches actual
job lifetime.
In `@awx/main/tests/unit/tasks/test_jobs.py`:
- Around line 143-147: The test has four `@mock.patch` decorators
(bulk_update_sorted_by_id, jobs.flag_enabled, facts.settings, create_partition)
but the test function test_pre_post_run_hook_facts_deleted_sliced only accepts
three mock parameters, causing a TypeError; update the test signature to accept
a fourth mock (matching the patched symbol bulk_update_sorted_by_id) — e.g. add
a parameter named mock_bulk_update_sorted_by_id (placed so parameters match the
patch order relative to create_partition, mock_facts_settings,
mock_flag_enabled) so the injected mocks align with the decorators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92f76957-5002-42b6-b614-68647793a6f7
📒 Files selected for processing (5)
AAP-63314.pr.mdawx/main/tasks/jobs.pyawx/main/tests/unit/conftest.pyawx/main/tests/unit/tasks/test_jobs.pyawx/main/tests/unit/test_tasks.py
davemulford
left a comment
davemulford
left a comment
There was a problem hiding this comment.
Added comment about rebasing that came up during a team review call.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
awx/main/tasks/jobs.py (1)
618-618:⚠️ Potential issue | 🟠 MajorUse effective runtime timeout for JWT TTL calculation.
instance.timeoutbypassesget_instance_timeout()semantics (global fallback + negative-timeout normalization), soworkload_ttl_secondscan be inconsistent with actual job runtime and may become negative.Proposed fix
- workload_ttl = min(instance.timeout, WORKLOAD_TTL_MAX_SECONDS) if instance.timeout else None + effective_timeout = self.get_instance_timeout(instance) + workload_ttl = min(effective_timeout, WORKLOAD_TTL_MAX_SECONDS) if effective_timeout > 0 else NoneAs per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/tasks/jobs.py` at line 618, The TTL calculation uses instance.timeout directly which bypasses get_instance_timeout() semantics; replace the use of instance.timeout with the effective timeout returned by get_instance_timeout(...) (preserving its normalization/fallback behavior) and compute workload_ttl as min(effective_timeout, WORKLOAD_TTL_MAX_SECONDS) if effective_timeout else None so workload_ttl_seconds matches actual job runtime; update references around workload_ttl, WORKLOAD_TTL_MAX_SECONDS, and instance.timeout accordingly.awx/main/tests/unit/tasks/test_jobs.py (1)
143-147:⚠️ Potential issue | 🔴 CriticalFix patch decorator/signature mismatch that breaks this test.
There are 4
@mock.patchdecorators but only 3 injected mock parameters in the function signature, so this test will fail at invocation time with aTypeError.Proposed fix
-def test_pre_post_run_hook_facts_deleted_sliced(mock_create_partition, mock_facts_settings, mock_flag_enabled, private_data_dir, execution_environment): +def test_pre_post_run_hook_facts_deleted_sliced( + mock_create_partition, + mock_facts_settings, + mock_flag_enabled, + _mock_bulk_update_sorted_by_id, + private_data_dir, + execution_environment, +):As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/tests/unit/tasks/test_jobs.py` around lines 143 - 147, The test has four `@mock.patch` decorators but only three mock parameters, causing a TypeError; update the test_pre_post_run_hook_facts_deleted_sliced signature to accept the mock for the first decorator (bulk_update_sorted_by_id). Specifically, add a parameter named e.g. mock_bulk_update_sorted_by_id as the first argument in test_pre_post_run_hook_facts_deleted_sliced so it matches the decorators (create_partition -> mock_create_partition, facts.settings -> mock_facts_settings, jobs.flag_enabled -> mock_flag_enabled, and the top `@mock.patch`('awx.main.tasks.facts.bulk_update_sorted_by_id') -> mock_bulk_update_sorted_by_id).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@awx/main/tasks/jobs.py`:
- Line 618: The TTL calculation uses instance.timeout directly which bypasses
get_instance_timeout() semantics; replace the use of instance.timeout with the
effective timeout returned by get_instance_timeout(...) (preserving its
normalization/fallback behavior) and compute workload_ttl as
min(effective_timeout, WORKLOAD_TTL_MAX_SECONDS) if effective_timeout else None
so workload_ttl_seconds matches actual job runtime; update references around
workload_ttl, WORKLOAD_TTL_MAX_SECONDS, and instance.timeout accordingly.
In `@awx/main/tests/unit/tasks/test_jobs.py`:
- Around line 143-147: The test has four `@mock.patch` decorators but only three
mock parameters, causing a TypeError; update the
test_pre_post_run_hook_facts_deleted_sliced signature to accept the mock for the
first decorator (bulk_update_sorted_by_id). Specifically, add a parameter named
e.g. mock_bulk_update_sorted_by_id as the first argument in
test_pre_post_run_hook_facts_deleted_sliced so it matches the decorators
(create_partition -> mock_create_partition, facts.settings ->
mock_facts_settings, jobs.flag_enabled -> mock_flag_enabled, and the top
`@mock.patch`('awx.main.tasks.facts.bulk_update_sorted_by_id') ->
mock_bulk_update_sorted_by_id).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c0bba26-03cd-4fff-b186-45b63146b6d2
📒 Files selected for processing (2)
awx/main/tasks/jobs.pyawx/main/tests/unit/tasks/test_jobs.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
awx/main/tests/unit/tasks/test_jobs.py (1)
143-147:⚠️ Potential issue | 🔴 CriticalPatch decorator/signature mismatch still breaks this test
Line 147 still has 4 patch decorators but only 3 injected mock parameters, so this test will raise
TypeErrorbefore executing assertions.Proposed fix
`@mock.patch`('awx.main.tasks.facts.bulk_update_sorted_by_id') `@mock.patch`('awx.main.tasks.jobs.flag_enabled', return_value=False) `@mock.patch`('awx.main.tasks.facts.settings') `@mock.patch`('awx.main.tasks.jobs.create_partition', return_value=True) -def test_pre_post_run_hook_facts_deleted_sliced(mock_create_partition, mock_facts_settings, mock_flag_enabled, private_data_dir, execution_environment): +def test_pre_post_run_hook_facts_deleted_sliced( + mock_create_partition, + mock_facts_settings, + mock_flag_enabled, + _mock_bulk_update_sorted_by_id, + private_data_dir, + execution_environment, +):As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/tests/unit/tasks/test_jobs.py` around lines 143 - 147, The test function test_pre_post_run_hook_facts_deleted_sliced has four `@mock.patch` decorators (awx.main.tasks.facts.bulk_update_sorted_by_id, awx.main.tasks.jobs.flag_enabled, awx.main.tasks.facts.settings, awx.main.tasks.jobs.create_partition) but only three mock parameters in its signature, causing a TypeError; fix by aligning the signature with the decorators—either add the missing mock parameter (e.g., add mock_bulk_update_sorted_by_id or mock_settings in the correct order) to the function parameters or remove the unneeded `@mock.patch` decorator so the number/order of injected mocks matches the function signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@awx/main/tests/unit/tasks/test_jobs.py`:
- Around line 143-147: The test function
test_pre_post_run_hook_facts_deleted_sliced has four `@mock.patch` decorators
(awx.main.tasks.facts.bulk_update_sorted_by_id,
awx.main.tasks.jobs.flag_enabled, awx.main.tasks.facts.settings,
awx.main.tasks.jobs.create_partition) but only three mock parameters in its
signature, causing a TypeError; fix by aligning the signature with the
decorators—either add the missing mock parameter (e.g., add
mock_bulk_update_sorted_by_id or mock_settings in the correct order) to the
function parameters or remove the unneeded `@mock.patch` decorator so the
number/order of injected mocks matches the function signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 523a20ce-d5cd-4bad-b624-aeeb3d6f8f3a
📒 Files selected for processing (1)
awx/main/tests/unit/tasks/test_jobs.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
awx/main/tasks/jobs.py (1)
613-613:⚠️ Potential issue | 🟠 MajorNormalize non-positive timeout values before sending TTL.
Line 613 can forward negative
instance.timeoutvalues as negative TTLs (min(-1, max)), which is invalid request shaping and inconsistent with the0 -> Nonebehavior.🔧 Proposed fix
- workload_ttl = min(instance.timeout, WORKLOAD_TTL_MAX_SECONDS) if instance.timeout else None + requested_ttl = instance.timeout or 0 + workload_ttl = min(requested_ttl, WORKLOAD_TTL_MAX_SECONDS) if requested_ttl > 0 else NoneAs per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/tasks/jobs.py` at line 613, The current assignment to workload_ttl can forward negative instance.timeout values (e.g., min(-1, WORKLOAD_TTL_MAX_SECONDS)), so normalize non-positive timeouts to None before computing TTL: when instance.timeout is None or <= 0, set workload_ttl to None; otherwise set workload_ttl to min(instance.timeout, WORKLOAD_TTL_MAX_SECONDS). Update the logic around the workload_ttl assignment (the expression using instance.timeout and WORKLOAD_TTL_MAX_SECONDS) to enforce this check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/main/tasks/jobs.py`:
- Around line 616-617: Replace the hardcoded audience
"https://vault.example.com" and scope "aap_controller_automation_job" with the
configured audience value and the enum/name value from
AutomationControllerJobScope; find the token request in awx/main/tasks/jobs.py
(the block setting audience=... and scope=...) and change audience to use the
appropriate configuration variable (e.g. settings.VAULT_AUDIENCE or the existing
configured audience accessor) and change scope to use
AutomationControllerJobScope.name (or
AutomationControllerJobScope.<MEMBER>.name) so the request uses environment
configuration and the canonical scope constant.
---
Duplicate comments:
In `@awx/main/tasks/jobs.py`:
- Line 613: The current assignment to workload_ttl can forward negative
instance.timeout values (e.g., min(-1, WORKLOAD_TTL_MAX_SECONDS)), so normalize
non-positive timeouts to None before computing TTL: when instance.timeout is
None or <= 0, set workload_ttl to None; otherwise set workload_ttl to
min(instance.timeout, WORKLOAD_TTL_MAX_SECONDS). Update the logic around the
workload_ttl assignment (the expression using instance.timeout and
WORKLOAD_TTL_MAX_SECONDS) to enforce this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ffa0aaa-1be2-4c90-a636-a78beb02ce81
📒 Files selected for processing (1)
awx/main/tasks/jobs.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
awx/main/tests/unit/tasks/test_jobs.py (1)
143-147:⚠️ Potential issue | 🔴 CriticalPatch decorators and test signature are out of sync.
Line 147 has four
@mock.patch(...)decorators but only three injected mock args in the function signature, so this test will fail before assertions run.Suggested change
`@mock.patch`('awx.main.tasks.facts.bulk_update_sorted_by_id') `@mock.patch`('awx.main.tasks.jobs.flag_enabled', return_value=False) `@mock.patch`('awx.main.tasks.facts.settings') `@mock.patch`('awx.main.tasks.jobs.create_partition', return_value=True) -def test_pre_post_run_hook_facts_deleted_sliced(mock_create_partition, mock_facts_settings, mock_flag_enabled, private_data_dir, execution_environment): +def test_pre_post_run_hook_facts_deleted_sliced( + mock_create_partition, + mock_facts_settings, + mock_flag_enabled, + _mock_bulk_update_sorted_by_id, + private_data_dir, + execution_environment, +):Run this read-only check; expected output should show
patch_decorators: 4andleading_mock_args: 3:#!/bin/bash python - <<'PY' import ast from pathlib import Path path = Path("awx/main/tests/unit/tasks/test_jobs.py") tree = ast.parse(path.read_text()) fn = next( n for n in tree.body if isinstance(n, ast.FunctionDef) and n.name == "test_pre_post_run_hook_facts_deleted_sliced" ) patch_decorators = [ d for d in fn.decorator_list if isinstance(d, ast.Call) and isinstance(d.func, ast.Attribute) and isinstance(d.func.value, ast.Name) and d.func.value.id == "mock" and d.func.attr == "patch" ] args = [a.arg for a in fn.args.args] leading_mock_args = 0 for arg in args: if arg.startswith("mock_") or arg.startswith("_mock_"): leading_mock_args += 1 else: break print("patch_decorators:", len(patch_decorators)) print("leading_mock_args:", leading_mock_args) print("args:", args) PYAs per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/tests/unit/tasks/test_jobs.py` around lines 143 - 147, The test has four `@mock.patch` decorators but the function signature only accepts three mock args; add the missing mock parameter corresponding to the topmost decorator ("awx.main.tasks.facts.bulk_update_sorted_by_id") to the test_pre_post_run_hook_facts_deleted_sliced signature (e.g. add mock_bulk_update_sorted_by_id as the fourth parameter after mock_flag_enabled) so the decorator-created mocks align with the function arguments.
🧹 Nitpick comments (1)
awx/settings/defaults.py (1)
1138-1141: Add an explicit default forFEATURE_WORKLOAD_IDENTITY_JWT_ENABLEDin defaults.
awx/main/tasks/jobs.pynow gates behavior on this flag, but there’s no matching default inawx/settings/defaults.py. Defining it explicitly (and mirroring in development defaults if that’s your convention) makes rollout behavior predictable.As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Suggested change
# feature flags FEATURE_INDIRECT_NODE_COUNTING_ENABLED = False +FEATURE_WORKLOAD_IDENTITY_JWT_ENABLED = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/settings/defaults.py` around lines 1138 - 1141, Add an explicit default boolean for FEATURE_WORKLOAD_IDENTITY_JWT_ENABLED in the settings defaults (e.g., set FEATURE_WORKLOAD_IDENTITY_JWT_ENABLED = False) so awx/main/tasks/jobs.py has a predictable value to gate on; place it near the other feature flags (alongside WORKLOAD_IDENTITY_VAULT_AUDIENCE) and also mirror the same default in your development defaults file if your repo convention requires matching dev settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/main/tasks/jobs.py`:
- Around line 609-611: When get_workload_identity_client() returns None inside
the JWT/workload identity path in jobs.py, do not silently return; instead log a
warning before returning so misconfiguration is visible. Update the block around
get_workload_identity_client() (the client = get_workload_identity_client()
check) to emit a clear warning-level message referencing the workload
identity/JWT flow and the missing client (use the module logger already used in
this file), then return as before.
---
Duplicate comments:
In `@awx/main/tests/unit/tasks/test_jobs.py`:
- Around line 143-147: The test has four `@mock.patch` decorators but the function
signature only accepts three mock args; add the missing mock parameter
corresponding to the topmost decorator
("awx.main.tasks.facts.bulk_update_sorted_by_id") to the
test_pre_post_run_hook_facts_deleted_sliced signature (e.g. add
mock_bulk_update_sorted_by_id as the fourth parameter after mock_flag_enabled)
so the decorator-created mocks align with the function arguments.
---
Nitpick comments:
In `@awx/settings/defaults.py`:
- Around line 1138-1141: Add an explicit default boolean for
FEATURE_WORKLOAD_IDENTITY_JWT_ENABLED in the settings defaults (e.g., set
FEATURE_WORKLOAD_IDENTITY_JWT_ENABLED = False) so awx/main/tasks/jobs.py has a
predictable value to gate on; place it near the other feature flags (alongside
WORKLOAD_IDENTITY_VAULT_AUDIENCE) and also mirror the same default in your
development defaults file if your repo convention requires matching dev
settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e91bb0c-efce-4b3c-b26d-62539872ce10
📒 Files selected for processing (3)
awx/main/tasks/jobs.pyawx/main/tests/unit/tasks/test_jobs.pyawx/settings/defaults.py
arrestle
requested review from
PabloHiro and
davemulford
and removed request for
davemulford
PabloHiro
left a comment
PabloHiro
left a comment
There was a problem hiding this comment.
I think the conflicts were not resolved properly. Please pay attention to the function retrieve_workload_identity_jwt and look to expand it
Hi Pablo, should be better now. Please check and let me know if I missed anything. |
dleehr
left a comment
dleehr
left a comment
There was a problem hiding this comment.
Thanks for revisiting this after the rebase discussion.
I think the logic to get the timeout and provide that to the client mostly (see item 1) makes sense, but I'm requesting changes for a few reasons:
- There is logic here that limits the max value of
workload_ttlto a hard-coded constant. This conflicts with the configurable approach you implemented in ansible/django-ansible-base#952 for the server-side logic. I suggest removing the check here. - Some existing/unrelated test functions have been modified with additional mocks/patches. I read the explanation in the PR description but the rationale doesn't make sense. Here's my reasoning: These same tests pass on
develtoday where we already have some flagged logic. We don't needflag_enabledpatches on them indevel. Is there something in this PR that changes that? I'd suggest reverting patches, push, and see if any checks fail. - Any feature-specific logic must be moved underneath the check if the flag is enabled. There's a couple things happening before checking the flag.
- I don't see a test for the new behavior. My understanding is the new behavior is that this code should get the value of
self.get_instance_timeout()and provide that value inretrieve_workload_identity_jwt. Is that a test that we can write? I was expecting to see something that mocked different return values forget_instance_timeoutand confirmed those values were provided to the client. - Code hygiene: This PR adds stray blank lines and 4 new pytest fixtures that don't appear to be used. Please make sure everything in the PR is here for a reason.
PabloHiro
left a comment
PabloHiro
left a comment
There was a problem hiding this comment.
LGTM, should be looking good once we rebase
Co-authored-by: Dan Leehr <dleehr@users.noreply.github.com>
PabloHiro
force-pushed
the
aap-63314-pass-workload-ttl
branch
from
6af1e43 to
e6ebb9c
Compare
There was a problem hiding this comment.
Overall LGTM. Functionally very clean. The diff is small which makes sense given the scope of work. It cleans up a couple mocks/fixture definitions too 👍
I'm approving, but please remove the logger.warning call that happens immediately before raising an error.
Edit: From my earlier review:
I don't see a test for the new behavior. My understanding is the new behavior is that this code should get the value of self.get_instance_timeout() and provide that value in retrieve_workload_identity_jwt. Is that a test that we can write? I was expecting to see something that mocked different return values for get_instance_timeout and confirmed those values were provided to the client.
Any thoughts?
) Only remove the collection directory the fixture created (redhat/indirect_accounting) instead of the entire /var/lib/awx/vendor_collections/ root, so we don't accidentally delete vendor collections that may have been installed by the build process. Forward-port of ansible/tower#7350. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* Remove pbr from requirements pbr was temporarily added to support ansible-runner installed from a git branch. It is no longer needed as a direct dependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Retrigger CI Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…yed as part of AAP (ansible#16283) After all settings are loaded, override DEFAULT_AUTHENTICATION_CLASSES to only allow Gateway JWT authentication when RESOURCE_SERVER__URL is set. This makes the lockdown immutable — no configuration file or environment variable can re-enable legacy auth methods (Basic, Session, OAuth2, Token). This is the same pattern used by Hub (galaxy_ng) and EDA (eda-server) for ANSTRAT-1840. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Made-with: Cursor
Made-with: Cursor
github-actions
Bot
added
the
dependencies
dleehr
left a comment
dleehr
left a comment
There was a problem hiding this comment.
The new test looks OK to me, but I'd need to defer to awx SMEs to review that pattern in the functional tests.
That said, there's a lot of files changed here that look unrelated, maybe a bad merge/rebase.
Please review the diff yourself (the Files Changed tab) before requesting reviews to make sure everything shown in the diff relates to your ticket.
github-actions
Bot
removed
the
dependencies
dleehr
left a comment
dleehr
left a comment
There was a problem hiding this comment.
LGTM.
Thanks for updating the functional test, that's what I was expecting to see in the previous review.
|
7 participants
[AAP-63314] P4.4: Controller - Pass Workload TTL to Gateway
Depends on: django-ansible-base PR #954 (adds
workload_ttl_secondstorequest_workload_jwt()).Description
What? Passes
job.timeoutasworkload_ttl_secondswhen Controller requests a workload identity JWT from Gateway.Why? Gateway (AAP-63312) calculates JWT TTL from workload duration but needs the value in the request. Controller supplies
job.timeoutso JWTs match job runtime.How? get_instance_timeout() → workload_ttl_seconds (0 → None for platform fallback). No capping in Controller — DAB/Gateway enforce ANSIBLE_BASE_WIT_MAX_TOKEN_TTL.
Related: DAB PR (client exposes param), AAP-63312 (Gateway), AAP-62693 (base JWT request), ANSTRAT-1019.
Handbook PR 1206
Type of Change
Component
Test Changes
flag_enabled(fromflags.state) checksFEATURE_WORKLOAD_IDENTITY_JWT_ENABLED. We only ever mock it toFalse.pre_run_hook: Mockflag_enabledtoFalse. Existing tests (fact cache, no-EE) need this to bypass the new JWT path. One new test uses it to verify that no JWT is requested when the flag is off._request_workload_identity_tokendirectly and do not use the flag at all.We add the mock only in tests that call
pre_run_hook, because that hook checks the flag before requesting a JWT.Testing
Unit tests:
make test_unit PYTEST_ARGS="-k workload"Manual (Gateway endpoint, requires service token):
Omit
workload_ttl_secondsfor platform default.Question for Reviewers
Feature flag:
FEATURE_WORKLOAD_IDENTITY_JWT_ENABLEDis not defined indefaults.py/development_defaults.py. Should we add it for consistency with other flags (e.g.FEATURE_INDIRECT_NODE_COUNTING_ENABLED)?Assisted-by: Claude
Summary by CodeRabbit
New Features
Reliability
Tests
Configuration