fix(azure/postgresql): isolate per-server collection failures#11595
Open
vahidg wants to merge 3 commits into
Open
fix(azure/postgresql): isolate per-server collection failures#11595vahidg wants to merge 3 commits into
vahidg wants to merge 3 commits into
Conversation
Collecting one PostgreSQL flexible server could abort collection of every remaining server in the subscription. The per-server work ran inside a single subscription-level try/except, so one failing call discarded all servers not yet processed (the failing one and the rest). This happened on PostgreSQL 16+, where the `connection_throttle.enable` server parameter no longer exists: `_get_connection_throttling` raised ConfigurationNotExists, which bubbled up and dropped the remaining servers in the subscription. It was silent because the error is logged at ERROR level, which is hidden from the console by default. - Wrap each server's collection in its own try/except so one server's failure no longer aborts the others in the subscription. - Make `_get_connection_throttling` tolerate the missing parameter (return None, like `_get_log_retention_days`), so PostgreSQL 16+ servers are still collected. Adds regression tests for the missing-config case and a hard per-server failure.
github-actions
Bot
added
the
provider/azure
📝 WalkthroughWalkthrough
ChangesAzure PostgreSQL flexible server error isolation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
✅ Conflict Markers Resolved All conflict markers have been successfully resolved in this pull request. |
![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
🤖 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 `@prowler/providers/azure/services/postgresql/postgresql_service.py`:
- Around line 165-174: The method shown in the diff now correctly returns None
as a fallback when the connection_throttle.enable parameter is not found, but
the Server model class still declares the connection_throttling attribute with a
type annotation of just str instead of str | None. Update the type annotation
for the Server.connection_throttling attribute (referenced at line 231) to str |
None to accurately reflect that this attribute can be either a string value or
None, aligning the model definition with the actual runtime behavior of the
method that populates it.
🪄 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: ba42f094-bbb8-41b6-afd5-2adbc2f651ad
📒 Files selected for processing (3)
prowler/CHANGELOG.mdprowler/providers/azure/services/postgresql/postgresql_service.pytests/providers/azure/services/postgresql/postgresql_service_test.py
Reflect the None fallback from _get_connection_throttling in the Server model and the method return annotation (per review feedback).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prowler/providers/azure/services/postgresql/postgresql_service.py (1)
163-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd parameter type hints and a Google-style docstring on
_get_connection_throttling.This changed method still violates the Prowler Python conventions: parameters are untyped and the method has no docstring.
Suggested patch
def _get_connection_throttling( - self, subscription, resouce_group_name, server_name + self, subscription: str, resouce_group_name: str, server_name: str ) -> str | None: + """Get connection throttling setting for a flexible server. + + Args: + subscription: Azure subscription identifier. + resouce_group_name: Resource group containing the server. + server_name: PostgreSQL flexible server name. + + Returns: + The uppercase throttling value when available, otherwise ``None``. + """ client = self.clients[subscription] try: connection_throttling = client.configurations.get( resouce_group_name, server_name, "connection_throttle.enable" )As per coding guidelines, "
prowler/**/*.py: Type hints are required for all public functions in Python code" and "Docstrings are required for all classes and methods in Python code, following Google style documentation".🤖 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 `@prowler/providers/azure/services/postgresql/postgresql_service.py` around lines 163 - 176, The `_get_connection_throttling` method is missing parameter type hints and a Google-style docstring, violating Prowler's Python conventions. Add type hints to the three parameters (subscription, resouce_group_name, server_name) based on their usage in the method body, and add a Google-style docstring above the method that documents the method's purpose, describes each parameter, specifies the return value, and explains the behavior regarding PostgreSQL 16+ compatibility mentioned in the existing comment.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@prowler/providers/azure/services/postgresql/postgresql_service.py`:
- Around line 163-176: The `_get_connection_throttling` method is missing
parameter type hints and a Google-style docstring, violating Prowler's Python
conventions. Add type hints to the three parameters (subscription,
resouce_group_name, server_name) based on their usage in the method body, and
add a Google-style docstring above the method that documents the method's
purpose, describes each parameter, specifies the return value, and explains the
behavior regarding PostgreSQL 16+ compatibility mentioned in the existing
comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 76c9ea1f-4003-4308-8779-4d88f4fe71cf
📒 Files selected for processing (1)
prowler/providers/azure/services/postgresql/postgresql_service.py
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
1 participant
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.
Context
While scanning an Azure tenant I noticed the Azure PostgreSQL service silently under-reported flexible servers: a subscription with two flexible servers (
...-devand...-prd) only ever produced findings for one of them.Root cause:
PostgreSQL._get_flexible_servers()iterates the servers of a subscription, but the per-server collection runs inside a single subscription-leveltry/except. If any per-server call raises, the handler catches it, logs atERROR, and abandons every server not yet processed — so one bad server discards the rest of the subscription.In this case the trigger was PostgreSQL 16+, where the
connection_throttle.enableserver parameter no longer exists._get_connection_throttling()therefore raisedResourceNotFoundError (ConfigurationNotExists), which bubbled up and dropped the remaining servers. It was invisible in normal use because thatERRORlog is hidden from the console at the default log level.No linked issue — happy to open one if preferred.
Description
try/exceptso a single server's failure no longer aborts collection of the other servers in the subscription (the subscription-level handler aroundservers.list()is kept)._get_connection_throttling()tolerate the missing parameter and returnNone(mirroring the existing_get_log_retention_days()), so PostgreSQL 16+ servers are still collected. Withconnection_throttling=Nonethepostgresql_flexible_server_connection_throttling_oncheck reports FAIL, matching its existing semantics.Adds regression tests for both the missing-config case and a hard per-server failure.
Steps to review
_get_flexible_servers()— per-servertry/exceptnow wraps the body; the subscription-level handler remains forservers.list()._get_connection_throttling()— now returnsNoneon exception, consistent with_get_log_retention_days().test_missing_connection_throttle_config_still_collects_server— both servers collected; the PG 16+ server getsconnection_throttling=None.test_one_server_hard_failure_does_not_drop_others— a server that fails onservers.getis skipped without dropping the rest.This is a bugfix and is likely a backport candidate.
Checklist
SDK/CLI
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Summary by CodeRabbit
connection_throttle.enableis treated as not enabled (no interruption).