AAP-68153 Add workload identity for inventory sync jobs#16380
AAP-68153 Add workload identity for inventory sync jobs#16380davemulford wants to merge 1 commit 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>
📝 WalkthroughWalkthroughModified Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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`:
- Around line 1731-1739: The current patch only overrides
inventory_update.get_cloud_credential to return a credential instance from
self._credentials, but inventory_update code may call
inventory.get_extra_credentials() which rebuilds from self.credentials.all() and
will return fresh DB instances lacking the workload-identity context; update the
same injector override logic to also replace
inventory_update.get_extra_credentials with a lambda that returns the extras
from the prefetched/context-bearing snapshot (i.e., derive extras by filtering
self._credentials for credential.kind/usage matching what get_extra_credentials
would return) so both single credential access (get_cloud_credential) and bulk
extras access (get_extra_credentials) return the contexted instances.
- Around line 1875-1878: The change returns every inventory_update credential
into self._credentials which causes BaseTask.run() to inject injector-owned
credentials twice; instead, leave _credentials semantics unchanged (so
BaseTask.run() continues to iterate only credentials meant for
inject_credential()), and create a separate snapshot used only for
workload-identity token generation (e.g., call
populate_workload_identity_tokens() with an all_credentials variable set from
inventory_update.credentials.prefetch_related('input_sources__source_credential').all());
update the code around the existing method (where
inventory_update.credentials.prefetch_related(...) was added) to return or
assign only the original injectable credentials for _credentials while keeping
the full list in a distinct name used solely by
populate_workload_identity_tokens().
🪄 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: 3d023b75-3e6d-4df2-83f6-77d246700863
📒 Files selected for processing (1)
awx/main/tasks/jobs.py
| # The injector's _get_shared_env() calls inventory_update.get_cloud_credential() | ||
| # which does a fresh DB fetch, losing the OIDC context populated by | ||
| # populate_workload_identity_tokens(). Override it on this instance to return | ||
| # the credential from self._credentials which already has the context. | ||
| cloud_cred = inventory_update.get_cloud_credential() | ||
| if cloud_cred and self._credentials: | ||
| contexted_cred = next((c for c in self._credentials if c.pk == cloud_cred.pk), None) | ||
| if contexted_cred is not None: | ||
| inventory_update.get_cloud_credential = lambda _cred=contexted_cred: _cred |
There was a problem hiding this comment.
This only fixes one of the inventory credential accessors.
In awx/main/models/inventory.py, Lines 1059-1074, get_extra_credentials() still rebuilds its list from self.credentials.all(). Anything in the inventory-update injector path that resolves extras through that accessor will still get fresh instances without the workload-identity token context, so the fix remains partial unless extras are also returned from the same prefetched/context-bearing snapshot.
🤖 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 1731 - 1739, The current patch only
overrides inventory_update.get_cloud_credential to return a credential instance
from self._credentials, but inventory_update code may call
inventory.get_extra_credentials() which rebuilds from self.credentials.all() and
will return fresh DB instances lacking the workload-identity context; update the
same injector override logic to also replace
inventory_update.get_extra_credentials with a lambda that returns the extras
from the prefetched/context-bearing snapshot (i.e., derive extras by filtering
self._credentials for credential.kind/usage matching what get_extra_credentials
would return) so both single credential access (get_cloud_credential) and bulk
extras access (get_extra_credentials) return the contexted instances.
| # Include all credentials (cloud + extra) with prefetch so that | ||
| # populate_workload_identity_tokens() can generate OIDC JWTs for | ||
| # credentials whose input sources reference an OIDC external credential. | ||
| return inventory_update.credentials.prefetch_related('input_sources__source_credential').all() |
There was a problem hiding this comment.
Returning all credentials here changes runtime injection semantics.
Line 1878 widens _credentials from “credentials that should run generic inject_credential() logic” to every inventory-update credential. But BaseTask.run() injects every entry in self._credentials on Lines 715-718, while InventoryUpdate.get_extra_credentials() exists specifically to keep the injector-owned credential out of that loop (awx/main/models/inventory.py, Lines 1059-1074). That can double-inject the special credential and clobber injector-managed env/file state; keep a separate all-credentials snapshot for workload-identity token generation instead.
🤖 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 1875 - 1878, The change returns every
inventory_update credential into self._credentials which causes BaseTask.run()
to inject injector-owned credentials twice; instead, leave _credentials
semantics unchanged (so BaseTask.run() continues to iterate only credentials
meant for inject_credential()), and create a separate snapshot used only for
workload-identity token generation (e.g., call
populate_workload_identity_tokens() with an all_credentials variable set from
inventory_update.credentials.prefetch_related('input_sources__source_credential').all());
update the code around the existing method (where
inventory_update.credentials.prefetch_related(...) was added) to return or
assign only the original injectable credentials for _credentials while keeping
the full list in a distinct name used solely by
populate_workload_identity_tokens().
PabloHiro
left a comment
PabloHiro
left a comment
There was a problem hiding this comment.
LGTM. I wonder if there is a way to add a test for this though. So in case this gets refactored in the future, we don't introduce regressions.
| if cloud_cred and self._credentials: | ||
| contexted_cred = next((c for c in self._credentials if c.pk == cloud_cred.pk), None) | ||
| if contexted_cred is not None: | ||
| inventory_update.get_cloud_credential = lambda _cred=contexted_cred: _cred |
There was a problem hiding this comment.
This is so so hacky, but I explored the code and I think there is no other way to do it.
There was a problem hiding this comment.
This is kind of crazy. The right answer would be a deeper refactor.
There was a problem hiding this comment.
You make the call here @AlanCoding , want us to dig deeper here or leave it as is for now until it becomes a problem?
|
I've moved this PR to draft status because #16382 may end up making these changes unnecessary. |
4 participants
SUMMARY
Adds OIDC workload identity support to inventory sync jobs.
RunInventoryUpdate had two gaps preventing OIDC workload identity tokens from reaching the cloud credential during inventory sync:
build_credentials_list() called get_extra_credentials() which excludes the cloud credential, so populate_workload_identity_tokens() never generated a JWT for it.
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.
ISSUE TYPE
COMPONENT NAME
Summary by CodeRabbit
Bug Fixes