feat(api): support regionless OCI credentials#11741
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR makes Oracle Cloud API key region handling optional across serializer validation, schema generation, provider kwargs, provider session setup, and existing-secret migration. It also adds E2E secret preflight handling for forks and expands Vulture ignore names. ChangesOracle Cloud optional region support
Estimated code review effort: 4 (Complex) | ~60 minutes CI Workflow and Lint Config
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OracleCloudProviderSecret
participant get_prowler_provider_kwargs
participant OraclecloudProvider
participant OCI_SDK
Client->>OracleCloudProviderSecret: submit secret payload
OracleCloudProviderSecret->>OracleCloudProviderSecret: validate() region and regions
OracleCloudProviderSecret->>get_prowler_provider_kwargs: normalized secret
get_prowler_provider_kwargs->>OraclecloudProvider: build provider kwargs
OraclecloudProvider->>OCI_SDK: setup_session(region or bootstrap region)
OraclecloudProvider->>OCI_SDK: get_regions_to_audit()
OCI_SDK-->>OraclecloudProvider: subscribed regions or error
alt discovery fails and single explicit region
OraclecloudProvider->>OraclecloudProvider: fallback to explicit region as home region
else discovery fails and no/multiple regions
OraclecloudProvider-->>Client: raise OCISetUpSessionError
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
| {"required": ["key_file"]}, | ||
| {"required": ["key_content"]}, | ||
| ], | ||
| "not": {"required": ["region", "regions"]}, |
There was a problem hiding this comment.
Is it mandatory to submit region(s) or can I choose not to submit any?
If it's mandatory use this:
"oneOf": [ { "required": ["region"] }, { "required": ["regions"] } ]
If we choose to not submit any and is valid, the PR is perfect :)
| def _normalize_oraclecloud_provider_kwargs(secret: dict) -> dict: | ||
| """Normalize external OCI secret fields into SDK provider kwargs.""" | ||
| prowler_provider_kwargs = secret.copy() | ||
|
|
||
| if "regions" in prowler_provider_kwargs: | ||
| regions = prowler_provider_kwargs.pop("regions") | ||
| prowler_provider_kwargs["region"] = set(regions) | ||
| elif "region" in prowler_provider_kwargs: | ||
| prowler_provider_kwargs["region"] = {prowler_provider_kwargs["region"]} | ||
|
|
||
| return prowler_provider_kwargs |
There was a problem hiding this comment.
What do you think about something like this?
Avoids silent collisions
`def _normalize_oraclecloud_provider_kwargs(secret: dict) -> dict:
"""Normalize external OCI secret fields into SDK provider kwargs."""
prowler_provider_kwargs = secret.copy()
if "regions" in prowler_provider_kwargs:
# Extract the list from the new format
regions = prowler_provider_kwargs.pop("regions")
# Defensive programming: purge the legacy key if it somehow bypassed the serializer.
# Using .pop(..., None) ensures it doesn't raise an error if the key doesn't exist.
prowler_provider_kwargs.pop("region", None)
# Assign the final set required by Prowler
prowler_provider_kwargs["region"] = set(regions)
elif "region" in prowler_provider_kwargs:
# If only the legacy format is present, wrap it in a set
prowler_provider_kwargs["region"] = {prowler_provider_kwargs["region"]}
return prowler_provider_kwargs`
| raise serializers.ValidationError( | ||
| {"regions": "Regions cannot contain duplicate values."} | ||
| ) | ||
| attrs["regions"] = regions |
There was a problem hiding this comment.
I think this cleanup is not saved. validate_secret_based_on_provider() validates the serializer, but doesn't use serializer.validated_data, so the endpoint can store the region with spaces.
There was a problem hiding this comment.
Fixed in 50a6283c5a.
validate_secret_based_on_provider() now returns the nested serializer validated_data, and create/update assign it back to validated_attrs["secret"] before saving. That means legacy region cleanup is persisted instead of only being validated.
I also updated the provider-secret create/update tests to submit " us-ashburn-1 " and verify the stored secret keeps "us-ashburn-1".
| if "region" not in secret and "regions" not in secret: | ||
| continue | ||
|
|
||
| secret.pop("region", None) |
There was a problem hiding this comment.
Can we confirm this case? If an existing OCI provider had region and cannot list regions, removing region moves it to regionless mode and it may fail.
HugoPBrito
changed the base branch from
ci/oci-regionless-platform-11565
to
master
github-actions
Bot
added
github_actions
|
Merge conflict with the base branch. This PR cannot be merged cleanly. Update your branch with the latest base (rebase or merge) and resolve the conflicts. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/src/backend/api/utils.py (1)
292-320: 🩺 Stability & Availability | 🟠 MajorLegacy
regions(plural) key not defensively handled.Neither
_normalize_oraclecloud_provider_kwargsnor_normalize_oraclecloud_connection_test_kwargsstrip/convert a strayregionskey. If any stored secret still hasregions(e.g. pre-migration data, or a rolling-deploy window before migration0097runs), it will be forwarded verbatim via**prowler_provider_kwargs/**oraclecloud_kwargsintoOraclecloudProvider(...)/test_connection(...), neither of which accept aregionskwarg — causing an unhandledTypeErrorand a hard failure for that provider's scans/connection tests.This is the same concern raised in a prior review with a concrete suggested fix (pop
regions, fall back to it, and defensively purge strayregion), which does not appear to have been incorporated.🛡️ Proposed defensive fix
def _normalize_oraclecloud_provider_kwargs(secret: dict) -> dict: """Normalize external OCI secret fields into SDK provider kwargs.""" prowler_provider_kwargs = secret.copy() - if "region" in prowler_provider_kwargs: + if "regions" in prowler_provider_kwargs: + prowler_provider_kwargs["region"] = set( + prowler_provider_kwargs.pop("regions") + ) + elif "region" in prowler_provider_kwargs: prowler_provider_kwargs["region"] = {prowler_provider_kwargs["region"]} return prowler_provider_kwargs🤖 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 `@api/src/backend/api/utils.py` around lines 292 - 320, The OCI normalization helpers still pass a legacy regions key through to downstream calls, which can break Oracle Cloud scans and connection tests. Update _normalize_oraclecloud_provider_kwargs and _normalize_oraclecloud_connection_test_kwargs to defensively remove regions, treat it as a fallback for region when needed, and ensure only the accepted region kwarg is forwarded to OraclecloudProvider and test_connection.
🤖 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 `@api/src/backend/api/utils.py`:
- Around line 302-320: The OCI test-connection normalization in
_normalize_oraclecloud_connection_test_kwargs duplicates the provider bootstrap
region literal, which can drift from OraclecloudProvider._bootstrap_region.
Update this helper to source the fallback region from
OraclecloudProvider._bootstrap_region instead of hardcoding "us-ashburn-1", and
make sure the import/reference is used wherever the fallback is applied so the
API stays aligned with the provider default.
---
Duplicate comments:
In `@api/src/backend/api/utils.py`:
- Around line 292-320: The OCI normalization helpers still pass a legacy regions
key through to downstream calls, which can break Oracle Cloud scans and
connection tests. Update _normalize_oraclecloud_provider_kwargs and
_normalize_oraclecloud_connection_test_kwargs to defensively remove regions,
treat it as a fallback for region when needed, and ensure only the accepted
region kwarg is forwarded to OraclecloudProvider and test_connection.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 18404c0d-f8d3-45a9-8b6e-066ef1cef882
📒 Files selected for processing (14)
.github/workflows/ui-e2e-tests-v2.ymlapi/CHANGELOG.mdapi/src/backend/api/migrations/0097_remove_oraclecloud_secret_regions.pyapi/src/backend/api/tests/test_migrations.pyapi/src/backend/api/tests/test_serializers.pyapi/src/backend/api/tests/test_utils.pyapi/src/backend/api/tests/test_views.pyapi/src/backend/api/utils.pyapi/src/backend/api/v1/serializer_utils/providers.pyapi/src/backend/api/v1/serializers.pyprowler/CHANGELOG.mdprowler/providers/oraclecloud/oraclecloud_provider.pypyproject.tomltests/providers/oraclecloud/oraclecloud_provider_test.py
| def _normalize_oraclecloud_connection_test_kwargs(secret: dict) -> dict: | ||
| """Normalize external OCI secret fields into test_connection kwargs.""" | ||
| prowler_provider_kwargs = secret.copy() | ||
|
|
||
| if ( | ||
| "region" not in prowler_provider_kwargs | ||
| and prowler_provider_kwargs.get("user") | ||
| and prowler_provider_kwargs.get("fingerprint") | ||
| and prowler_provider_kwargs.get("tenancy") | ||
| and ( | ||
| prowler_provider_kwargs.get("key_content") | ||
| or prowler_provider_kwargs.get("key_file") | ||
| ) | ||
| ): | ||
| # Connection validation needs one OCI endpoint, but scans remain unfiltered. | ||
| prowler_provider_kwargs["region"] = "us-ashburn-1" | ||
|
|
||
| return prowler_provider_kwargs | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hardcoded bootstrap region duplicates OraclecloudProvider._bootstrap_region.
"us-ashburn-1" is a literal copy of the provider's _bootstrap_region class attribute. If that constant ever changes, this API helper silently diverges from the SDK default.
♻️ Reference the provider constant instead of duplicating the literal
+ from prowler.providers.oraclecloud.oraclecloud_provider import (
+ OraclecloudProvider,
+ )
+
if (
"region" not in prowler_provider_kwargs
and prowler_provider_kwargs.get("user")
and prowler_provider_kwargs.get("fingerprint")
and prowler_provider_kwargs.get("tenancy")
and (
prowler_provider_kwargs.get("key_content")
or prowler_provider_kwargs.get("key_file")
)
):
# Connection validation needs one OCI endpoint, but scans remain unfiltered.
- prowler_provider_kwargs["region"] = "us-ashburn-1"
+ prowler_provider_kwargs["region"] = OraclecloudProvider._bootstrap_region📝 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.
| def _normalize_oraclecloud_connection_test_kwargs(secret: dict) -> dict: | |
| """Normalize external OCI secret fields into test_connection kwargs.""" | |
| prowler_provider_kwargs = secret.copy() | |
| if ( | |
| "region" not in prowler_provider_kwargs | |
| and prowler_provider_kwargs.get("user") | |
| and prowler_provider_kwargs.get("fingerprint") | |
| and prowler_provider_kwargs.get("tenancy") | |
| and ( | |
| prowler_provider_kwargs.get("key_content") | |
| or prowler_provider_kwargs.get("key_file") | |
| ) | |
| ): | |
| # Connection validation needs one OCI endpoint, but scans remain unfiltered. | |
| prowler_provider_kwargs["region"] = "us-ashburn-1" | |
| return prowler_provider_kwargs | |
| def _normalize_oraclecloud_connection_test_kwargs(secret: dict) -> dict: | |
| """Normalize external OCI secret fields into test_connection kwargs.""" | |
| prowler_provider_kwargs = secret.copy() | |
| from prowler.providers.oraclecloud.oraclecloud_provider import ( | |
| OraclecloudProvider, | |
| ) | |
| if ( | |
| "region" not in prowler_provider_kwargs | |
| and prowler_provider_kwargs.get("user") | |
| and prowler_provider_kwargs.get("fingerprint") | |
| and prowler_provider_kwargs.get("tenancy") | |
| and ( | |
| prowler_provider_kwargs.get("key_content") | |
| or prowler_provider_kwargs.get("key_file") | |
| ) | |
| ): | |
| # Connection validation needs one OCI endpoint, but scans remain unfiltered. | |
| prowler_provider_kwargs["region"] = OraclecloudProvider._bootstrap_region | |
| return prowler_provider_kwargs |
🤖 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 `@api/src/backend/api/utils.py` around lines 302 - 320, The OCI test-connection
normalization in _normalize_oraclecloud_connection_test_kwargs duplicates the
provider bootstrap region literal, which can drift from
OraclecloudProvider._bootstrap_region. Update this helper to source the fallback
region from OraclecloudProvider._bootstrap_region instead of hardcoding
"us-ashburn-1", and make sure the import/reference is used wherever the fallback
is applied so the API stays aligned with the provider default.
3 participants
Context
OCI provider setup should support credentials without requiring users to provide region filters. Region discovery should be handled by the SDK.
Description
This PR updates API validation, provider initialization, schema metadata, and migration coverage so OCI secrets can omit region fields. The API no longer introduces
regionsas a replacement field. The existingregionfield is kept only as deprecated legacy compatibility, while new regionless payloads rely on SDK region discovery.Existing stored
regionsvalues are removed by the migration because that field is no longer part of the API contract. Existing legacyregionvalues are preserved for backward compatibility.Changed files:
api/CHANGELOG.mdapi/src/backend/api/migrations/0097_remove_oraclecloud_secret_regions.pyapi/src/backend/api/tests/test_migrations.pyapi/src/backend/api/tests/test_serializers.pyapi/src/backend/api/tests/test_utils.pyapi/src/backend/api/tests/test_views.pyapi/src/backend/api/utils.pyapi/src/backend/api/v1/serializer_utils/providers.pyapi/src/backend/api/v1/serializers.pypyproject.tomlSteps to review
Validation previously run for this slice:
Checklist
Community Checklist
SDK/CLI
UI
API
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Chain Context
feat/oci-regionless-sdk-chain-11565Chain Overview
Scope
Autonomy
Summary by CodeRabbit