Add feature flag for OIDC workload identity credential types#16348
Add feature flag for OIDC workload identity credential types#16348matoval wants to merge 1 commit 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:
📝 WalkthroughWalkthroughFeature-flag gating for OIDC workload identity credential types was added. A new flag ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Views as Views
participant Serializers as Serializers
participant Flags as FeatureFlag
participant Constants as Constants
Client->>Views: GET list / GET detail / POST credential or input source
Views->>Flags: Check FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED
alt Flag Disabled
Views->>Constants: Get OIDC_CREDENTIAL_TYPE_NAMESPACES
Views->>Views: Exclude OIDC namespaces from queryset
Views->>Client: Return filtered list or 404 for excluded detail
else Flag Enabled
Views->>Client: Return full list/detail
end
Client->>Serializers: Validate submitted credential or source_credential
Serializers->>Flags: Check FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED
alt Flag Disabled and type.namespace in OIDC_CREDENTIAL_TYPE_NAMESPACES
Serializers->>Client: Raise ValidationError (enable feature flag)
else
Serializers->>Client: Validation passes
end
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)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![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/api/serializers.py`:
- Around line 3141-3148: The field-level gate in validate_source_credential only
runs when source_credential is present in the payload, so add an object-level
check in the serializer's validate(self, attrs) to enforce the
FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED flag on every update: implement validate
to determine the effective source_credential (use attrs.get('source_credential')
or, for PATCH/PUT, fall back to self.instance.source_credential if available),
then if not flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED") and the
credential_type.namespace of that effective source_credential is in
OIDC_CREDENTIAL_TYPE_NAMESPACES raise ValidationError with the same message;
keep or call validate_source_credential for create flows but ensure the
object-level validate always performs the gate so PATCH requests that omit
source_credential cannot bypass it.
In `@awx/main/tests/functional/api/test_oidc_credential_feature_flag.py`:
- Around line 11-13: Tests repeatedly call
CredentialType.setup_tower_managed_defaults which is redundant because the
registry is populated by the autouse session fixture load_all_credentials
(conftest fixture calling load_credentials); remove the per-test calls to
CredentialType.setup_tower_managed_defaults in this file and either (a) add a
module- or function-scoped fixture that depends on the session fixture to run
once per module/test if explicit setup is desired, or (b) add a short comment at
the top of the file stating that load_all_credentials in conftest populates the
registry so tests can assume credential types are registered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2dc65d56-1b6f-44af-9391-94c34ad3e2ef
📒 Files selected for processing (5)
awx/api/serializers.pyawx/api/views/__init__.pyawx/main/constants.pyawx/main/tests/functional/api/test_oidc_credential_feature_flag.pyawx/settings/defaults.py
awx/main/tests/functional/api/test_oidc_credential_feature_flag.py
Outdated
Show resolved
Hide resolved
matoval
force-pushed
the
AAP-64510
branch
2 times, most recently
from
39d3bbc to
c892a46
Compare
There was a problem hiding this comment.
Looks good. One nitpick suggestion - There's a flag_disabled function we can use instead of if not flag_enabled(), https://cfpb.github.io/django-flags/api/state/#flag_disabledflag_name-kwargs.
But I want to highlight the specific approach. What's here expects that we register the OIDC credential plugins and load them into the database, but then we filter them out from API responses. The feature flag controls only whether or not they're shown.
I think @AlanCoding suggested a different approach in our #16286 discussions - that we would only load them into the database if the feature flag is enabled. Thoughts?
I suppose my point is that feature flags should really disable the feature. I think part of the value we want to get out of them is that we could change the specifics before going GA. If we never load them in the first place on a system when the feature flag is never enabled, then we have a far easier time changing things before GA.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
awx/api/views/__init__.py (1)
1422-1428: Implementation is correct; consider extracting shared logic.The queryset filtering works correctly and ensures disabled OIDC types return 404 on detail access. The implementation is identical to
CredentialTypeList.get_queryset.♻️ Optional: Extract to a mixin to reduce duplication
class OIDCCredentialTypeFilterMixin: """Filters OIDC credential types when the feature flag is disabled.""" def get_queryset(self): qs = super().get_queryset() if not flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED"): qs = qs.exclude(namespace__in=OIDC_CREDENTIAL_TYPE_NAMESPACES) return qs class CredentialTypeList(OIDCCredentialTypeFilterMixin, ListCreateAPIView): # ... existing code, remove get_queryset override class CredentialTypeDetail(OIDCCredentialTypeFilterMixin, RetrieveUpdateDestroyAPIView): # ... existing code, remove get_queryset override🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/api/views/__init__.py` around lines 1422 - 1428, Duplicate get_queryset logic that excludes OIDC namespaces when FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED is false should be extracted into a mixin (e.g., OIDCCredentialTypeFilterMixin) to avoid duplication; implement a mixin that defines get_queryset by calling super().get_queryset(), applying the flag check and qs.exclude(namespace__in=OIDC_CREDENTIAL_TYPE_NAMESPACES) when needed, then have CredentialTypeList and CredentialTypeDetail inherit from this mixin and remove their local get_queryset overrides so the shared behavior is used.
🤖 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/api/serializers.py`:
- Around line 3033-3039: The validator validate_credential_type currently
rejects any OIDC credential_type when the feature flag is off, causing updates
that keep the same type to fail; change it so the OIDC block only applies to
creations or when the type is being changed by checking self.instance: if
self.instance exists and the submitted credential_type is the same as
self.instance.credential_type (compare by id/pk or equality), allow it,
otherwise raise the ValidationError; keep references to flag_enabled and
OIDC_CREDENTIAL_TYPE_NAMESPACES and raise ValidationError only when the flag is
disabled and the type is OIDC and either there is no self.instance or the
submitted type differs from the instance's current type.
---
Nitpick comments:
In `@awx/api/views/__init__.py`:
- Around line 1422-1428: Duplicate get_queryset logic that excludes OIDC
namespaces when FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED is false should be
extracted into a mixin (e.g., OIDCCredentialTypeFilterMixin) to avoid
duplication; implement a mixin that defines get_queryset by calling
super().get_queryset(), applying the flag check and
qs.exclude(namespace__in=OIDC_CREDENTIAL_TYPE_NAMESPACES) when needed, then have
CredentialTypeList and CredentialTypeDetail inherit from this mixin and remove
their local get_queryset overrides so the shared behavior is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 413e0764-350c-46c7-9abd-0fd60d02c3f5
📒 Files selected for processing (5)
awx/api/serializers.pyawx/api/views/__init__.pyawx/main/constants.pyawx/main/tests/functional/api/test_oidc_credential_feature_flag.pyawx/settings/defaults.py
🚧 Files skipped from review as they are similar to previous changes (2)
- awx/settings/defaults.py
- awx/main/tests/functional/api/test_oidc_credential_feature_flag.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
awx/api/serializers.py (1)
3033-3039:⚠️ Potential issue | 🟠 MajorAllow unchanged OIDC types on credential updates.
Line 3035 rejects any submitted OIDC
credential_typewhile the flag is off. That makes aPUT/PATCHfail when it resubmits the instance’s existing type, which is broader than the PR behavior of blocking creation or switching to an OIDC type.Suggested fix
def validate_credential_type(self, credential_type): - # Check if OIDC credential types are being used when the feature flag is disabled - if not flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED"): - if credential_type.namespace in OIDC_CREDENTIAL_TYPE_NAMESPACES: - raise ValidationError( - _('OIDC workload identity credential types are not available. The FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED feature flag must be enabled.') - ) + if ( + not flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED") + and credential_type.namespace in OIDC_CREDENTIAL_TYPE_NAMESPACES + and (not self.instance or credential_type.pk != self.instance.credential_type_id) + ): + raise ValidationError( + _('OIDC workload identity credential types are not available. The FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED feature flag must be enabled.') + ) if self.instance and credential_type.pk != self.instance.credential_type.pk:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/api/serializers.py` around lines 3033 - 3039, The current validate_credential_type method unconditionally rejects OIDC credential types when FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED is off, which also breaks PUT/PATCH that re-submit the same type; update validate_credential_type to allow the request to proceed when self.instance exists and the submitted credential_type is the same as the instance's current credential_type (compare PK/ID via self.instance.credential_type.pk or .id), but keep rejecting when attempting to create or change to an OIDC type; keep using flag_enabled and OIDC_CREDENTIAL_TYPE_NAMESPACES and raise ValidationError otherwise.
🤖 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/api/serializers.py`:
- Around line 3033-3039: The current validate_credential_type method
unconditionally rejects OIDC credential types when
FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED is off, which also breaks PUT/PATCH that
re-submit the same type; update validate_credential_type to allow the request to
proceed when self.instance exists and the submitted credential_type is the same
as the instance's current credential_type (compare PK/ID via
self.instance.credential_type.pk or .id), but keep rejecting when attempting to
create or change to an OIDC type; keep using flag_enabled and
OIDC_CREDENTIAL_TYPE_NAMESPACES and raise ValidationError otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6436b33c-bc9f-416f-ab9e-059c6ba86541
📒 Files selected for processing (5)
awx/api/serializers.pyawx/api/views/__init__.pyawx/main/constants.pyawx/main/tests/functional/api/test_oidc_credential_feature_flag.pyawx/settings/defaults.py
🚧 Files skipped from review as they are similar to previous changes (3)
- awx/api/views/init.py
- awx/main/tests/functional/api/test_oidc_credential_feature_flag.py
- awx/settings/defaults.py
I like the idea of having the feature completely disabled if the flag is disabled including not loading the credential types into the db but what would happen if OIDC creds are created with the flag enabled then the flag is disabled? |
That case was called out in https://github.com/ansible/awx/pull/16286/changes#r2880082875 and recorded in https://issues.redhat.com/browse/AAP-65973. Overall I'd still defer to @AlanCoding on the best way to cut this off 😄 |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
awx/api/serializers.py (1)
3033-3040:⚠️ Potential issue | 🟠 MajorAllow no-op updates for existing OIDC credentials.
When the flag is off, this rejects any payload that includes an OIDC
credential_type, even if the instance already has that same type. That makes ordinary PUT/PATCH updates of pre-existing OIDC credentials fail after the flag is disabled, which is broader than the create/switch gate described in this PR.Suggested fix
def validate_credential_type(self, credential_type): - # Check if OIDC credential types are being used when the feature flag is disabled - if not flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED"): - if credential_type.namespace in OIDC_CREDENTIAL_TYPE_NAMESPACES: - raise ValidationError( - _('OIDC workload identity credential types are not available. The FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED feature flag must be enabled.') - ) + if ( + not flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED") + and credential_type.namespace in OIDC_CREDENTIAL_TYPE_NAMESPACES + and (not self.instance or credential_type.pk != self.instance.credential_type_id) + ): + raise ValidationError( + _('OIDC workload identity credential types are not available. The FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED feature flag must be enabled.') + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/api/serializers.py` around lines 3033 - 3040, The validate_credential_type method currently blocks any payload with an OIDC credential_type when the FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED flag is off; change it to allow no-op updates by first checking if this is an update of an existing object (self.instance) and the instance already has the same credential_type, and if so return/accept the credential_type without raising ValidationError. In other words, inside validate_credential_type, if not flag_enabled(...) and credential_type.namespace in OIDC_CREDENTIAL_TYPE_NAMESPACES, then skip raising the error when self.instance is present and self.instance.credential_type == credential_type (or compare by id/pk if the instance stores a FK), otherwise raise the ValidationError as before.
🧹 Nitpick comments (1)
awx/main/tests/functional/api/test_oidc_credential_feature_flag.py (1)
111-150: Add a PATCH regression for the new input-source gate.The new object-level validation path is meant to block updates that omit
source_credential, but this module only exercises create. A PATCH test that edits an existing OIDC-backed input source with the flag off would lock in that fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/tests/functional/api/test_oidc_credential_feature_flag.py` around lines 111 - 150, Add a new PATCH regression test alongside test_create_credential_input_source_with_oidc_fails_when_flag_disabled that verifies object-level validation when editing an existing input source with the OIDC feature flag disabled: create an OIDC-backed Credential (using oidc_type and feature_flag_enabled) and an input-source record via the API (use post and reverse('api:credential_input_source_list')), then call patch on that input-source URL while feature_flag_disabled('FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED') and omit the source_credential field in the payload; assert response.status_code == 400 and that the response contains the OIDC error message and the FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED flag name to ensure updates are blocked as intended.
🤖 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/api/serializers.py`:
- Around line 3033-3040: The validate_credential_type method currently blocks
any payload with an OIDC credential_type when the
FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED flag is off; change it to allow no-op
updates by first checking if this is an update of an existing object
(self.instance) and the instance already has the same credential_type, and if so
return/accept the credential_type without raising ValidationError. In other
words, inside validate_credential_type, if not flag_enabled(...) and
credential_type.namespace in OIDC_CREDENTIAL_TYPE_NAMESPACES, then skip raising
the error when self.instance is present and self.instance.credential_type ==
credential_type (or compare by id/pk if the instance stores a FK), otherwise
raise the ValidationError as before.
---
Nitpick comments:
In `@awx/main/tests/functional/api/test_oidc_credential_feature_flag.py`:
- Around line 111-150: Add a new PATCH regression test alongside
test_create_credential_input_source_with_oidc_fails_when_flag_disabled that
verifies object-level validation when editing an existing input source with the
OIDC feature flag disabled: create an OIDC-backed Credential (using oidc_type
and feature_flag_enabled) and an input-source record via the API (use post and
reverse('api:credential_input_source_list')), then call patch on that
input-source URL while
feature_flag_disabled('FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED') and omit the
source_credential field in the payload; assert response.status_code == 400 and
that the response contains the OIDC error message and the
FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED flag name to ensure updates are blocked
as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4f2335c-2843-494e-9956-f98687e19118
📒 Files selected for processing (5)
awx/api/serializers.pyawx/api/views/__init__.pyawx/main/constants.pyawx/main/tests/functional/api/test_oidc_credential_feature_flag.pyawx/settings/defaults.py
🚧 Files skipped from review as they are similar to previous changes (2)
- awx/main/constants.py
- awx/api/views/init.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
awx/api/serializers.py (1)
3033-3039:⚠️ Potential issue | 🟠 MajorAllow updates that keep the same OIDC credential type
Line 3035 still rejects any OIDC
credential_typewhile the flag is off. That also breaks PUT/PATCH requests that re-submit the instance’s current type, so existing OIDC credentials become uneditable for unrelated changes after the flag is disabled. This gate should apply only on create or when switching types.Suggested fix
def validate_credential_type(self, credential_type): - # Check if OIDC credential types are being used when the feature flag is disabled - if not flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED"): - if credential_type.namespace in OIDC_CREDENTIAL_TYPE_NAMESPACES: - raise ValidationError( - _('OIDC workload identity credential types are not available. The FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED feature flag must be enabled.') - ) + if ( + not flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED") + and credential_type.namespace in OIDC_CREDENTIAL_TYPE_NAMESPACES + and (not self.instance or credential_type.pk != self.instance.credential_type_id) + ): + raise ValidationError( + _('OIDC workload identity credential types are not available. The FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED feature flag must be enabled.') + ) if self.instance and credential_type.pk != self.instance.credential_type.pk:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/api/serializers.py` around lines 3033 - 3039, The validator currently rejects any OIDC credential_type when the feature flag is off, preventing updates that re-submit the same type; modify validate_credential_type to only block OIDC types when creating a new instance or when changing the credential_type. Concretely, in validate_credential_type (and using self.instance), skip the FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED check if self.instance exists and self.instance.credential_type (or its id/namespace) matches the incoming credential_type so the same type is allowed to be re-submitted, but still raise ValidationError when attempting to set an OIDC namespace on create or when switching from a different type.
🧹 Nitpick comments (1)
awx/api/views/__init__.py (1)
1401-1406: Consider extracting the shared filtering logic to a mixin.Both
CredentialTypeListandCredentialTypeDetailhave identicalget_queryset()implementations. This could be extracted to a shared mixin to reduce duplication.♻️ Optional: Extract to a mixin
class OIDCCredentialTypeFilterMixin: """Mixin to filter OIDC credential types based on feature flag.""" def get_queryset(self): qs = super().get_queryset() if not flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED"): qs = qs.exclude(namespace__in=OIDC_CREDENTIAL_TYPE_NAMESPACES) return qs class CredentialTypeList(OIDCCredentialTypeFilterMixin, ListCreateAPIView): # ... existing code, remove get_queryset override class CredentialTypeDetail(OIDCCredentialTypeFilterMixin, RetrieveUpdateDestroyAPIView): # ... existing code, remove get_queryset overrideAlso applies to: 1422-1427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/api/views/__init__.py` around lines 1401 - 1406, Duplicate get_queryset logic in CredentialTypeList and CredentialTypeDetail should be extracted into a mixin; create OIDCCredentialTypeFilterMixin that implements get_queryset by calling super().get_queryset(), checking flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED"), and applying qs.exclude(namespace__in=OIDC_CREDENTIAL_TYPE_NAMESPACES) when disabled, then have CredentialTypeList and CredentialTypeDetail inherit from this mixin (removing their local get_queryset overrides) so both use the shared filtering behavior.
🤖 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/tests/functional/api/test_oidc_credential_feature_flag.py`:
- Around line 128-136: The test creates a new SSH CredentialType instance with
CredentialType.defaults['ssh']() and .save(), causing IntegrityError against the
unique (name, kind) constraint; instead fetch the existing default SSH type and
use it when creating target_cred—e.g., build ssh_type =
CredentialType.defaults['ssh']() only to derive name/kind and then replace the
.save() call with a query like CredentialType.objects.get(name=ssh_type.name,
kind=ssh_type.kind) (or CredentialType.objects.get(kind='ssh')) and use that
returned object for target_cred creation.
---
Duplicate comments:
In `@awx/api/serializers.py`:
- Around line 3033-3039: The validator currently rejects any OIDC
credential_type when the feature flag is off, preventing updates that re-submit
the same type; modify validate_credential_type to only block OIDC types when
creating a new instance or when changing the credential_type. Concretely, in
validate_credential_type (and using self.instance), skip the
FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED check if self.instance exists and
self.instance.credential_type (or its id/namespace) matches the incoming
credential_type so the same type is allowed to be re-submitted, but still raise
ValidationError when attempting to set an OIDC namespace on create or when
switching from a different type.
---
Nitpick comments:
In `@awx/api/views/__init__.py`:
- Around line 1401-1406: Duplicate get_queryset logic in CredentialTypeList and
CredentialTypeDetail should be extracted into a mixin; create
OIDCCredentialTypeFilterMixin that implements get_queryset by calling
super().get_queryset(), checking
flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED"), and applying
qs.exclude(namespace__in=OIDC_CREDENTIAL_TYPE_NAMESPACES) when disabled, then
have CredentialTypeList and CredentialTypeDetail inherit from this mixin
(removing their local get_queryset overrides) so both use the shared filtering
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bbbc132-f197-4115-ae58-1c65f1a1fb61
📒 Files selected for processing (5)
awx/api/serializers.pyawx/api/views/__init__.pyawx/main/constants.pyawx/main/tests/functional/api/test_oidc_credential_feature_flag.pyawx/settings/defaults.py
🚧 Files skipped from review as they are similar to previous changes (1)
- awx/settings/defaults.py
awx/main/tests/functional/api/test_oidc_credential_feature_flag.py
Outdated
Show resolved
Hide resolved
|
I thought we had agreement that we will not load the OIDC credential types if the feature flag is disabled. Filtering them out of the queryset is going to cause endless headaches, violating what-you-see-is-what-you get. And it pollutes the SQL we have to issue for every list. The flag as it was added in DAB was specifically tagged as install-time so that this on-load behavior would be possible. We run the loading logic (from installed plugins to DB) extremely aggressively, and there is a management command if anyone needs it. |
matoval
force-pushed
the
AAP-64510
branch
5 times, most recently
from
9d8c593 to
5950fed
Compare
|
Thank you @dleehr & @AlanCoding 🙏🏽 I have updated the PR to check the feature flag at install-time |
matoval
force-pushed
the
AAP-64510
branch
2 times, most recently
from
b25bd32 to
ada9ec5
Compare
awx/main/models/credential.py
Outdated
|
|
||
| for ns, ep in plugin_entry_points.items(): | ||
| # Skip OIDC credential types when feature flag is disabled | ||
| if ns in OIDC_CREDENTIAL_TYPE_NAMESPACES and not flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED"): |
There was a problem hiding this comment.
I think we are not supposed to use flag_enabled when Django is starting (ex: code to load the credentials could run before code to load the feature flags in the DB). We would need something like:
| if ns in OIDC_CREDENTIAL_TYPE_NAMESPACES and not flag_enabled("FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED"): | |
| if ns in OIDC_CREDENTIAL_TYPE_NAMESPACES and not if getattr(settings, 'FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED', False): |
We follow a similar approach here:
There was a problem hiding this comment.
I like this suggestion - it aligns with Alan's feedback that we should be respecting this as an install-time feature flag - even if we might be able to access Django model objects from this file (but we can check that locally, right?).
Only additional suggestion I have is to add a comment about why we're checking it from settings rather than flag_enabled. The former is fair for install-time feature flags and the latter requires django to be running with the DB and the entire framework.
There was a problem hiding this comment.
Only additional suggestion I have is to add a comment about why we're checking it from settings rather than flag_enabled. The former is fair for install-time feature flags and the latter requires django to be running with the DB and the entire framework.
Yeah, I did think we needed to stay with flag_enabled? We used to check via the setting, and if you saw any other cases doing that, I'll have a look and we might need to update those.
There was a problem hiding this comment.
Interesting. @AlanCoding, I think @PabloHiro's feedback is that maybe we shouldn't be using flag_enabled here because it relies on the database (and the app lifecycle), and it looks like this is happening pretty early in the process - and maybe checking flag_enabled here is not valid.
Sounds like you're saying that's not a problem and we should go ahead with checking flag state here. We can certainly test that (see if Django yells at us if we call flag_enabled here). But if it's not something we should worry about, that's fair too.
There was a problem hiding this comment.
Ah yes, don't get me wrong, it is awful that this requires database access (giving #15738 for evidence). I die a little inside every time the .ready method runs. It shouldn't have ever been merged like that.
But it already does this. Clarifying in a more technical sense, I believe load_credentials is already expected to make database queries. I wanted to give you more concrete verification of this but it has problems even running in the dev environment.
There was a problem hiding this comment.
After testing some more locally, flag_enabled isn't working at least not locally. The migration pod errors with AppRegistryNotReady error: django.core.exceptions.AppRegistryNotReady: Feature flag state cannot be checked before the app registry is ready.
I switched over to getattr(settings, ) and it's working properly hiding OIDC creds when the feature flag is false and showing them when true.
matoval
force-pushed
the
AAP-64510
branch
2 times, most recently
from
4dffff3 to
4560651
Compare
…ypes Implements FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED feature flag to gate HashiCorp Vault OIDC credential types as a Technology Preview feature. When the feature flag is disabled (default), OIDC credential types are not loaded into the plugin registry at application startup and do not exist in the database. When enabled, OIDC credential types are loaded normally and function as expected. Changes: - Add FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED setting (defaults to False) - Add OIDC_CREDENTIAL_TYPE_NAMESPACES constant for maintainability - Modify load_credentials() to skip OIDC types when flag is disabled - Add test coverage (2 test cases) This is an install-time flag that requires application restart to take effect. The flag is checked during application startup when credential types are loaded from plugins. Fixes: AAP-64510 Assisted-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
4 participants
SUMMARY
Implements FEATURE_OIDC_WORKLOAD_IDENTITY_ENABLED feature flag to gate HashiCorp Vault OIDC credential types (HashiCorp Vault Secret Lookup and HashiCorp Vault Signed SSH) as a Technology Preview feature.
When disabled (default):
When enabled:
Changes:
load_credentials()to skip OIDC types when feature flag is disabledThe feature flag is checked at application startup when credential types are loaded from plugins. This is an install-time flag that requires application restart to take effect.
Fixes: AAP-64510
Assisted by: Claude Sonnet 4.5 noreply@anthropic.com
ISSUE TYPE
COMPONENT NAME
STEPS TO REPRODUCE AND EXTRA INFO
Default behavior (flag disabled):
Enable the feature flag:
Set in defaults.py (for development/testing):
Verify OIDC types are now loaded:
Run tests:
make test TEST_DIRS=awx/main/tests/functional/api/test_oidc_credential_feature_flag.py