AAP-68153 Introduce new internal classes for durable data in job preparation#16382
Open
AlanCoding wants to merge 9 commits intoansible:develfrom
Open
AAP-68153 Introduce new internal classes for durable data in job preparation#16382AlanCoding wants to merge 9 commits intoansible:develfrom
AlanCoding wants to merge 9 commits intoansible:develfrom
Conversation
RunInventoryUpdate had two gaps preventing OIDC workload identity tokens from reaching the cloud credential during inventory sync: 1. build_credentials_list() called get_extra_credentials() which excludes the cloud credential, so populate_workload_identity_tokens() never generated a JWT for it. 2. The inventory plugin injector calls inventory_update.get_cloud_credential() internally, which does a fresh DB fetch that loses the OIDC context set by populate_workload_identity_tokens(). Fix (1) by returning all credentials with prefetched input sources. Fix (2) by overriding get_cloud_credential() on the instance to return the credential that already carries the OIDC context. Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughCredential handling in job task execution has been refactored. The system now uses a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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)
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 |
AlanCoding
force-pushed
the
credential_cargo
branch
from
0000292 to
fe7a28b
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| elif instance.__dict__.get('credential_id'): | ||
| creds = [instance.credential] | ||
| else: | ||
| creds = [] |
There was a problem hiding this comment.
Dead elif branch loses credentials for FK-based jobs
High Severity
from_instance has an unreachable elif branch for FK-based credential types. All UnifiedJob subclasses inherit a credentials M2M field, so hasattr(instance, 'credentials') is always True for saved instances. This means AdHocCommand and ProjectUpdate, which store their credential via a FK (credential) rather than the M2M, will always enter the first branch and get an empty queryset from the M2M. The docstring even states the elif is meant for "types that have a single credential (ProjectUpdate, AdHocCommand)" but the condition never fires. This causes all credential-dependent behavior (SSH keys, passwords, args) to silently break for ad hoc commands and project updates.
Additional Locations (1)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 223-240: populate_workload_identity_tokens currently only iterates
prep.credentials so organization-level Galaxy credentials never get minted;
change the generator in populate_workload_identity_tokens to iterate over both
prep.credentials and the organization-level credentials loaded by
TaskPrepData.from_instance (e.g. prep.organization_credentials or
prep.organization_galaxy_credentials) — combine them (chain or simple
concatenation) and then run the same input_sources check so
workoad_identity_token JWTs are created for org-attached OIDC-backed Galaxy
credentials as well.
- Around line 688-691: The current if guard around
flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED") prevents
populate_workload_identity_tokens(prep) from running when the feature flag is
off, which hides the codepath that sets self.instance.status = 'error'; in run()
remove the outer flag check so populate_workload_identity_tokens(prep) is always
invoked (so it can mark the job errored when workload-identity-backed
credentials are present and the feature is disabled); if you want to keep the
log message, keep logger.info(...) behind the flag but call
populate_workload_identity_tokens(prep) unconditionally so the disabled-flag
failure path can execute.
In `@awx/main/tasks/prep.py`:
- Around line 111-118: TaskPrepData.__init__ currently wraps entries in
credentials with PreparedCredential but leaves self.galaxy_credentials as raw
Credential objects; change the constructor to normalize galaxy_credentials the
same way as credentials by iterating over the incoming galaxy_credentials,
extracting raw = g._credential if isinstance(g, PreparedCredential) else g, and
appending PreparedCredential(raw, self) to self.galaxy_credentials so OIDC token
resolution runs through PreparedCredential (do the same fix for the second
occurrence referenced around the other block at 138-142).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b9912d4-8210-482f-8785-126118da2a4f
📒 Files selected for processing (7)
awx/main/models/credential.pyawx/main/tasks/jobs.pyawx/main/tasks/prep.pyawx/main/tests/functional/test_jobs.pyawx/main/tests/unit/models/test_credential.pyawx/main/tests/unit/tasks/test_jobs.pyawx/main/tests/unit/test_tasks.py
Comment on lines
+223
to
+240
| def populate_workload_identity_tokens(self, prep): | ||
| """ | ||
| Credentials for the task execution. | ||
| Fetches credentials once using build_credentials_list() and stores | ||
| them for the duration of the task to avoid redundant database queries. | ||
| """ | ||
| credentials_list = self.build_credentials_list(self.instance) | ||
| # Convert to list to prevent re-evaluation of QuerySet | ||
| return list(credentials_list) | ||
|
|
||
| def populate_workload_identity_tokens(self): | ||
| """ | ||
| Populate credentials with workload identity tokens. | ||
| Populate prep.workload_tokens with workload identity JWTs. | ||
|
|
||
| Sets the context on Credential objects that have input sources | ||
| using compatible external credential types. | ||
| Writes JWTs keyed by CredentialInputSource PK. These tokens are | ||
| later consumed by PreparedCredential.get_input() when resolving | ||
| dynamic fields via the shared workload_tokens dict. | ||
| """ | ||
| credential_input_sources = ( | ||
| (credential.context, src) | ||
| for credential in self._credentials | ||
| src | ||
| for credential in prep.credentials | ||
| for src in credential.input_sources.all() | ||
| if any( | ||
| field.get('id') == 'workload_identity_token' and field.get('internal') | ||
| for field in src.source_credential.credential_type.inputs.get('fields', []) | ||
| ) | ||
| ) | ||
| for credential_ctx, input_src in credential_input_sources: | ||
| for input_src in credential_input_sources: |
There was a problem hiding this comment.
Mint workload tokens for Galaxy credentials as well.
TaskPrepData.from_instance() now loads organization Galaxy credentials, and RunProjectUpdate.build_env() later consumes them, but this generator only walks prep.credentials. Any OIDC-backed Galaxy credential attached at the organization level will never receive a workload_identity_token.
Suggested fix
- credential_input_sources = (
- src
- for credential in prep.credentials
+ credential_input_sources = (
+ src
+ for credential in [*prep.credentials, *getattr(prep, 'galaxy_credentials', [])]
for src in credential.input_sources.all()
if any(
field.get('id') == 'workload_identity_token' and field.get('internal')
for field in src.source_credential.credential_type.inputs.get('fields', [])
)
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def populate_workload_identity_tokens(self, prep): | |
| """ | |
| Credentials for the task execution. | |
| Fetches credentials once using build_credentials_list() and stores | |
| them for the duration of the task to avoid redundant database queries. | |
| """ | |
| credentials_list = self.build_credentials_list(self.instance) | |
| # Convert to list to prevent re-evaluation of QuerySet | |
| return list(credentials_list) | |
| def populate_workload_identity_tokens(self): | |
| """ | |
| Populate credentials with workload identity tokens. | |
| Populate prep.workload_tokens with workload identity JWTs. | |
| Sets the context on Credential objects that have input sources | |
| using compatible external credential types. | |
| Writes JWTs keyed by CredentialInputSource PK. These tokens are | |
| later consumed by PreparedCredential.get_input() when resolving | |
| dynamic fields via the shared workload_tokens dict. | |
| """ | |
| credential_input_sources = ( | |
| (credential.context, src) | |
| for credential in self._credentials | |
| src | |
| for credential in prep.credentials | |
| for src in credential.input_sources.all() | |
| if any( | |
| field.get('id') == 'workload_identity_token' and field.get('internal') | |
| for field in src.source_credential.credential_type.inputs.get('fields', []) | |
| ) | |
| ) | |
| for credential_ctx, input_src in credential_input_sources: | |
| for input_src in credential_input_sources: | |
| def populate_workload_identity_tokens(self, prep): | |
| """ | |
| Populate prep.workload_tokens with workload identity JWTs. | |
| Writes JWTs keyed by CredentialInputSource PK. These tokens are | |
| later consumed by PreparedCredential.get_input() when resolving | |
| dynamic fields via the shared workload_tokens dict. | |
| """ | |
| credential_input_sources = ( | |
| src | |
| for credential in [*prep.credentials, *getattr(prep, 'galaxy_credentials', [])] | |
| for src in credential.input_sources.all() | |
| if any( | |
| field.get('id') == 'workload_identity_token' and field.get('internal') | |
| for field in src.source_credential.credential_type.inputs.get('fields', []) | |
| ) | |
| ) | |
| for input_src in credential_input_sources: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@awx/main/tasks/jobs.py` around lines 223 - 240,
populate_workload_identity_tokens currently only iterates prep.credentials so
organization-level Galaxy credentials never get minted; change the generator in
populate_workload_identity_tokens to iterate over both prep.credentials and the
organization-level credentials loaded by TaskPrepData.from_instance (e.g.
prep.organization_credentials or prep.organization_galaxy_credentials) — combine
them (chain or simple concatenation) and then run the same input_sources check
so workoad_identity_token JWTs are created for org-attached OIDC-backed Galaxy
credentials as well.
Comment on lines
688
to
691
| if flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED"): | ||
| logger.info(f'Generating workload identity tokens for {self.instance.log_format}') | ||
| self.populate_workload_identity_tokens() | ||
| self.populate_workload_identity_tokens(prep) | ||
| if self.instance.status == 'error': |
There was a problem hiding this comment.
Don’t bypass the disabled-flag failure path in run().
populate_workload_identity_tokens() already marks the job errored when workload-identity-backed credentials are present but the feature flag is off. Guarding the call here makes that branch unreachable during normal execution, so prep continues until a later dynamic-input failure.
Suggested fix
- if flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED"):
- logger.info(f'Generating workload identity tokens for {self.instance.log_format}')
- self.populate_workload_identity_tokens(prep)
- if self.instance.status == 'error':
- raise RuntimeError('not starting %s task' % self.instance.status)
+ self.populate_workload_identity_tokens(prep)
+ if self.instance.status == 'error':
+ raise RuntimeError('not starting %s task' % self.instance.status)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@awx/main/tasks/jobs.py` around lines 688 - 691, The current if guard around
flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED") prevents
populate_workload_identity_tokens(prep) from running when the feature flag is
off, which hides the codepath that sets self.instance.status = 'error'; in run()
remove the outer flag check so populate_workload_identity_tokens(prep) is always
invoked (so it can mark the job errored when workload-identity-backed
credentials are present and the feature is disabled); if you want to keep the
log message, keep logger.info(...) behind the flag but call
populate_workload_identity_tokens(prep) unconditionally so the disabled-flag
failure path can execute.
Comment on lines
+111
to
+118
| def __init__(self, instance, credentials, galaxy_credentials=None): | ||
| self._instance = instance | ||
| self.workload_tokens = {} | ||
| self.credentials = [] | ||
| self.galaxy_credentials = galaxy_credentials or [] | ||
| for c in credentials: | ||
| raw = c._credential if isinstance(c, PreparedCredential) else c | ||
| self.credentials.append(PreparedCredential(raw, self)) |
There was a problem hiding this comment.
Wrap galaxy_credentials in PreparedCredential too.
TaskPrepData now carries organization Galaxy credentials, but they stay as raw Credential objects here. After awx/main/models/credential.py:368-372 stopped passing prep context through raw Credential.get_input(), RunProjectUpdate.build_env() in awx/main/tasks/jobs.py:1391-1398 will bypass PreparedCredential for dynamic token resolution, so OIDC-backed organization Galaxy credentials still fail during prep.
Suggested fix
def __init__(self, instance, credentials, galaxy_credentials=None):
self._instance = instance
self.workload_tokens = {}
self.credentials = []
- self.galaxy_credentials = galaxy_credentials or []
+ self.galaxy_credentials = []
for c in credentials:
raw = c._credential if isinstance(c, PreparedCredential) else c
self.credentials.append(PreparedCredential(raw, self))
+ for c in galaxy_credentials or []:
+ raw = c._credential if isinstance(c, PreparedCredential) else c
+ self.galaxy_credentials.append(PreparedCredential(raw, self))- galaxy_creds = list(instance.project.organization.galaxy_credentials.all())
+ galaxy_creds = list(
+ instance.project.organization.galaxy_credentials.prefetch_related('input_sources__source_credential').all()
+ )Also applies to: 138-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@awx/main/tasks/prep.py` around lines 111 - 118, TaskPrepData.__init__
currently wraps entries in credentials with PreparedCredential but leaves
self.galaxy_credentials as raw Credential objects; change the constructor to
normalize galaxy_credentials the same way as credentials by iterating over the
incoming galaxy_credentials, extracting raw = g._credential if isinstance(g,
PreparedCredential) else g, and appending PreparedCredential(raw, self) to
self.galaxy_credentials so OIDC token resolution runs through PreparedCredential
(do the same fix for the second occurrence referenced around the other block at
138-142).
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
2 participants
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
Created stemming from conversation in #16380
I didn't like it when the context was introduced on the
Credentialmodel. But I accepted it begrudgingly. However, if this results in the situation getting worse and worse and monkey patches being introduced everywhere, then I'd rather do this.This does an internal refactor that is hopefully specific to the job "prep" phase. This is where the runner kwargs and private_data_dir are getting populated.
ISSUE TYPE
COMPONENT NAME
Note
Medium Risk
Moderate risk because it refactors the job prep path (credential fetching, dynamic input resolution, and credential injection) across multiple job types; regressions could break job launches or external credential/OIDC resolution.
Overview
Introduces new in-memory prep structures (
TaskPrepData/PreparedCredential) to own credentials and runtime workload-identity JWTs during the job preparation phase, and removes runtime state from theCredentialmodel.Updates
BaseTaskand all job task implementations to build prep artifacts (private data dir/files, env, args, extra vars, inventory/playbook params) from the shared prep snapshot, and changes OIDC workload identity token generation to populateprep.workload_tokenskeyed byCredentialInputSourcePK.Adjusts
RunInventoryUpdateinjection to avoid double-injecting the cloud credential, and rewrites affected tests to construct/useTaskPrepDatainstead of relying on cached credential properties or per-credentialcontext.Written by Cursor Bugbot for commit fe7a28b. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Release Notes
Refactor
Tests