fix(oci): scan identity in known valid region#10529
Conversation
|
✅ Conflict Markers Resolved All conflict markers have been successfully resolved in this pull request. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #10529 +/- ##
===========================================
- Coverage 93.65% 61.36% -32.29%
===========================================
Files 230 87 -143
Lines 33937 2876 -31061
===========================================
- Hits 31784 1765 -30019
+ Misses 2153 1111 -1042
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
HugoPBrito
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the issue is real and the approach (using the provider's known regions instead of hardcoding us-ashburn-1) makes sense. However, there are a few issues that need addressing before this can be merged:
is notvs!=— all 5 string comparisons use identity check instead of equality. This is the most critical issue.- Removed guards on search methods —
__search_root_compartment_resources__and__search_active_non_root_compartments__lost their region guard entirely, which will cause redundant calls and potentially duplicate results. - Provider change breaks multi-region —
single_regionnow always picks the first region even when multiple are specified. - Missing test updates — Codecov shows 41% patch coverage with 7 uncovered lines.
|
Thanks for the feedback Hugo! I've implemented the changes requested so that domains are filtered at insertion with the provided code, the test now tests both password policies and list_domains with multiple regional clients. Home region's default is fixed along with the other issues listed. Let me know if there's anything else I should look into! |
HugoPBrito
left a comment
There was a problem hiding this comment.
Found a regression while testing this branch. With an empty _regions list the new initializer at oraclecloud_provider.py:164-167 crashes the entire scan:
INFO: Found 0 subscribed regions
CRITICAL: IndexError[347]: list index out of range
Root cause: the default value passed to next() is evaluated eagerly, so self._regions[0].key raises IndexError whenever _regions is empty (e.g. when --region/-f filters don't match any subscribed region):
self._home_region = next(
(region.key for region in self._regions if region.is_home_region),
self._regions[0].key, # IndexError when self._regions == []
)Pre-PR this same scenario completed silently with 0 findings; now it aborts the whole run. Could you guard against the empty-list case and surface a clear error (e.g. "no subscribed regions match the requested filter") instead of letting it crash here?
HugoPBrito
left a comment
There was a problem hiding this comment.
Could you also share evidence that the fix behaves as expected in a tenancy whose home region is not us-ashburn-1? Ideally a full --log-level INFO run of --service identity showing the new Home region is: <non-ashburn-region> log line and identity findings being produced where the previous behavior would have returned none. We aren't able to validate the original scenario end-to-end on our side, so concrete evidence from your test environment would be very helpful.
HugoPBrito
left a comment
There was a problem hiding this comment.
That will do the job, thanks!
Just take into account this last comment an it will be ready to be merged.
Thank you for the effort put into this.
|
Changelog moved, thanks Hugo! |
`__list_domains__` runs concurrently per region via `__threading_call__`, so the read-then-modify on `self.domains` could allow duplicates or lose the home-region preference when two regions returned the same domain at once. Serialize the dedupe-and-append with a lock and add a concurrent test that exercises the path under a thread pool.
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |

Context
Please include relevant motivation and context for this PR.
Fix #10528
Description
Scans found region for identity misconfigurations instead of generally defaulting to us-ashburn-1
Steps to review
Scanning any tenancy with an identity domain outside us-ashburn-1 or not supporting us-ashburn-1
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.