Skip to content

Conversation

shariff-6
Copy link
Contributor

@shariff-6 shariff-6 commented Oct 14, 2025

User description

Description

What

Improved AWS exception handling by enhancing is_resource_not_found_exception() and is_access_denied_exception() to correctly detect cases where AWS CloudControl API returns GeneralServiceException wrapping underlying errors such as ResourceNotFoundException or AccessDenied.

Why

The AWS CloudControl API, which the integration sometimes uses to fetch and manage resource states, sometimes returns a top-level error code of GeneralServiceException even when the underlying issue is more specific — such as ResourceNotFoundException or AccessDenied.

In these cases, CloudControl wraps the actual downstream service error (for example, from EC2 or S3) inside the GeneralServiceException message string instead of exposing it as a structured error code. As a result, our existing helper functions — is_resource_not_found_exception() and is_access_denied_exception() — failed to recognize these conditions because they only checked the top-level error code.

This led to legitimate “not found” or “permission denied” cases being treated as unhandled exceptions, which interrupted the entire resync process rather than skipping the problematic resource and continuing graceful

Example of the problematic error:

Error Code: "GeneralServiceException"
Message: "The Vpc Endpoint Id 'vpce-068e7950f9d5142c6' does not exist"

How

  • Enhanced is_resource_not_found_exception() to check for GeneralServiceException and inspect the error message for resource not found patterns: "not found", "notfound", "does not exist", "resourcenotfound"
  • Enhanced is_access_denied_exception() to check for GeneralServiceException and inspect the error message for access denied patterns: "access denied", "accessdenied", "unauthorized", "forbidden", "permission denied"
  • Made pattern matching case-insensitive using .lower()
  • Added comprehensive tests to validate the new behavior
  • Maintained backward compatibility - all existing error codes continue to work as before

Result: Resources with wrapped errors are now logged and skipped gracefully instead of breaking the resync, allowing the integration to continue processing remaining resources.

Type of change

Please leave one option from the following and delete the rest:

  • Bug fix (non-breaking change which fixes an issue)

All tests should be run against the port production environment(using a testing org).

Integration testing checklist

  • Integration able to create all default resources from scratch
  • Completed a full resync from a freshly installed integration and it completed successfully
  • Resync able to create entities
  • Resync able to update entities
  • Resync able to detect and delete entities
  • Resync finishes successfully
  • If new resource kind is added or updated in the integration, add example raw data, mapping and expected result to the examples folder in the integration directory.
  • If resource kind is updated, run the integration with the example data and check if the expected result is achieved
  • If new resource kind is added or updated, validate that live-events for that resource are working as expected
  • Docs PR link here

Preflight checklist

  • Handled rate limiting
  • Handled pagination
  • Implemented the code in async
  • Support Multi account

Screenshots

Include screenshots from your environment showing how the resources of the integration will look.

API Documentation

Provide links to the API documentation used for this integration.


PR Type

Bug fix


Description

  • Enhanced AWS exception handling to detect errors wrapped in GeneralServiceException

  • Added pattern matching for ResourceNotFoundException and AccessDenied in error messages

  • Prevents resync failures by gracefully handling CloudControl API wrapped errors

  • Added comprehensive test coverage for new exception detection logic


Diagram Walkthrough

flowchart LR
  A["AWS CloudControl API"] -- "Returns GeneralServiceException" --> B["is_resource_not_found_exception()"]
  A -- "Returns GeneralServiceException" --> C["is_access_denied_exception()"]
  B -- "Checks error message patterns" --> D["Detects wrapped ResourceNotFoundException"]
  C -- "Checks error message patterns" --> E["Detects wrapped AccessDenied"]
  D --> F["Gracefully skip resource"]
  E --> F
Loading

File Walkthrough

Relevant files
Bug fix
misc.py
Enhanced exception handlers to unwrap GeneralServiceException errors

integrations/aws/utils/misc.py

  • Enhanced is_access_denied_exception() to detect access denied patterns
    in GeneralServiceException messages
  • Enhanced is_resource_not_found_exception() to detect not found
    patterns in GeneralServiceException messages
  • Added case-insensitive pattern matching for error message inspection
  • Maintained backward compatibility with existing error code checks
+29/-2   
Tests
test_misc.py
Added tests for GeneralServiceException error unwrapping 

integrations/aws/tests/utils/test_misc.py

  • Added test for GeneralServiceException with "does not exist" message
    pattern
  • Added test for AccessDenied wrapped in GeneralServiceException
  • Validates new error detection behavior for wrapped exceptions
+26/-0   

- Enhanced is_access_denied_exception() to detect access denied errors
  wrapped in GeneralServiceException by checking error message patterns
- Enhanced is_resource_not_found_exception() to detect resource not found
  errors wrapped in GeneralServiceException by checking error message patterns
- Added tests to validate GeneralServiceException error unwrapping behavior
- Fixes resync failures when AWS wraps ResourceNotFoundException or
  AccessDenied errors inside GeneralServiceException

Fixes PORT-16491
@shariff-6 shariff-6 marked this pull request as ready for review October 14, 2025 09:19
@shariff-6 shariff-6 requested a review from a team as a code owner October 14, 2025 09:19
Copy link
Contributor

qodo-merge-pro bot commented Oct 14, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link
Contributor

qodo-merge-pro bot commented Oct 14, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Apply fix to the aws-v3 integration

The fix for GeneralServiceException handling should be applied not only to the
aws integration but also to the aws-v3 integration, which contains identical
helper functions that were overlooked in this PR.

Examples:

integrations/aws/utils/misc.py [61-86]
def is_access_denied_exception(e: Exception) -> bool:
    access_denied_error_codes = [
        "AccessDenied",
        "AccessDeniedException",
        "UnauthorizedOperation",
    ]

    if hasattr(e, "response") and e.response is not None:
        error_code = e.response.get("Error", {}).get("Code")


 ... (clipped 16 lines)
integrations/aws-v3/aws/core/helpers/utils.py [6-16]

Solution Walkthrough:

Before:

# file: integrations/aws-v3/aws/core/helpers/utils.py

def is_access_denied_exception(e: Exception) -> bool:
    access_denied_error_codes = [...]
    response = getattr(e, "response", None)
    if isinstance(response, dict):
        error_code = response.get("Error", {}).get("Code")
        return error_code in access_denied_error_codes
    return False

def is_resource_not_found_exception(e: Exception) -> bool:
    resource_not_found_error_codes = [...]
    response = getattr(e, "response", None)
    if isinstance(response, dict):
        error_code = response.get("Error", {}).get("Code")
        return error_code in resource_not_found_error_codes
    return False

After:

# file: integrations/aws-v3/aws/core/helpers/utils.py

def is_access_denied_exception(e: Exception) -> bool:
    ...
    response = getattr(e, "response", None)
    if isinstance(response, dict):
        error_code = response.get("Error", {}).get("Code")
        if error_code in access_denied_error_codes:
            return True
        if error_code == "GeneralServiceException":
            error_message = response.get("Error", {}).get("Message", "").lower()
            if any(p in error_message for p in ["access denied", "unauthorized", ...]):
                return True
    return False

def is_resource_not_found_exception(e: Exception) -> bool:
    ...
    # Similar logic to check for GeneralServiceException and message patterns
    # for "not found", "does not exist", etc.
    ...
    return False
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical oversight where the bug fix was not applied to the parallel aws-v3 integration, which contains nearly identical vulnerable functions, resulting in an incomplete solution.

High
General
Simplify logic and avoid redundant calls
Suggestion Impact:The commit refactored the GeneralServiceException logic into a helper function and returned the result of any() directly, simplifying the check. While it didn’t reuse a local error_details variable, it achieved the suggested conciseness and avoided duplicate logic.

code diff:

+def _check_general_service_exception(e: Exception, patterns: List[str]) -> bool:
+    """Check if GeneralServiceException message contains any of the given patterns."""
+    if hasattr(e, "response") and e.response is not None:
+        error_code = e.response.get("Error", {}).get("Code")
+        if error_code == "GeneralServiceException":
+            error_message = e.response.get("Error", {}).get("Message", "").lower()
+            return any(pattern in error_message for pattern in patterns)
+    return False
+
+
 def is_access_denied_exception(e: Exception) -> bool:
     access_denied_error_codes = [
         "AccessDenied",
@@ -71,17 +81,15 @@
         if error_code in access_denied_error_codes:
             return True
 
-        if error_code == "GeneralServiceException":
-            error_message = e.response.get("Error", {}).get("Message", "").lower()
-            access_denied_patterns = [
-                "access denied",
-                "accessdenied",
-                "unauthorized",
-                "forbidden",
-                "permission denied",
-            ]
-            if any(pattern in error_message for pattern in access_denied_patterns):
-                return True
+        access_denied_patterns = [
+            "access denied",
+            "accessdenied",
+            "unauthorized",
+            "forbidden",
+            "permission denied",
+        ]
+        if _check_general_service_exception(e, access_denied_patterns):
+            return True

Refactor the GeneralServiceException check to be more concise by directly
returning the result of any() and avoid a repeated call to
e.response.get("Error", {}) by using a variable.

integrations/aws/utils/misc.py [68-84]

 if hasattr(e, "response") and e.response is not None:
-    error_code = e.response.get("Error", {}).get("Code")
+    error_details = e.response.get("Error", {})
+    error_code = error_details.get("Code")
 
     if error_code in access_denied_error_codes:
         return True
 
     if error_code == "GeneralServiceException":
-        error_message = e.response.get("Error", {}).get("Message", "").lower()
+        error_message = error_details.get("Message", "").lower()
         access_denied_patterns = [
             "access denied",
             "accessdenied",
             "unauthorized",
             "forbidden",
             "permission denied",
         ]
-        if any(pattern in error_message for pattern in access_denied_patterns):
-            return True
+        return any(pattern in error_message for pattern in access_denied_patterns)

[Suggestion processed]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies an opportunity to refactor the code for better readability and to avoid a redundant dictionary lookup, which is a good practice.

Low
Possible issue
Safely handle missing status codes

Add a type check for the status variable before comparison to prevent a
TypeError when HTTPStatusCode is missing from the AWS response.

integrations/aws/utils/misc.py [89-94]

 def is_server_error(e: Exception) -> bool:
-    if hasattr(e, "response"):
+    if hasattr(e, "response") and isinstance(getattr(e, "response", None), dict):
         status = e.response.get("ResponseMetadata", {}).get("HTTPStatusCode")
-        return status >= 500
-
+        if isinstance(status, int):
+            return status >= 500
     return False

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential TypeError if HTTPStatusCode is missing from the response, and provides a robust fix by checking the type of status before comparison.

Medium
Sanitize error message type

Cast the error Message to a string before calling .lower() to prevent potential
AttributeError if the message is not a string.

integrations/aws/utils/misc.py [61-86]

 def is_access_denied_exception(e: Exception) -> bool:
     access_denied_error_codes = [
         "AccessDenied",
         "AccessDeniedException",
         "UnauthorizedOperation",
     ]
 
     if hasattr(e, "response") and e.response is not None:
-        error_code = e.response.get("Error", {}).get("Code")
+        error_details = e.response.get("Error", {})
+        error_code = error_details.get("Code")
 
         if error_code in access_denied_error_codes:
             return True
 
         if error_code == "GeneralServiceException":
-            error_message = e.response.get("Error", {}).get("Message", "").lower()
+            error_message = str(error_details.get("Message", "")).lower()
             access_denied_patterns = [
                 "access denied",
                 "accessdenied",
                 "unauthorized",
                 "forbidden",
                 "permission denied",
             ]
             if any(pattern in error_message for pattern in access_denied_patterns):
                 return True
 
     return False

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the error Message might not be a string, and casting it to str before calling .lower() prevents a potential AttributeError, making the exception handling more robust.

Medium
General
Broaden not-found message patterns

Add "no such resource" and "could not be found" to the list of patterns for
detecting "resource not found" exceptions within GeneralServiceException
messages.

integrations/aws/utils/misc.py [97-121]

 def is_resource_not_found_exception(e: Exception) -> bool:
     resource_not_found_error_codes = [
         "ResourceNotFoundException",
         "ResourceNotFound",
         "ResourceNotFoundFault",
     ]
 
     if hasattr(e, "response") and e.response is not None:
-        error_code = e.response.get("Error", {}).get("Code")
+        error_details = e.response.get("Error", {})
+        error_code = error_details.get("Code")
 
         if error_code in resource_not_found_error_codes:
             return True
 
         if error_code == "GeneralServiceException":
-            error_message = e.response.get("Error", {}).get("Message", "").lower()
+            error_message = str(error_details.get("Message", "")).lower()
             not_found_patterns = [
                 "not found",
                 "notfound",
                 "does not exist",
                 "resourcenotfound",
+                "no such resource",
+                "could not be found",
             ]
             if any(pattern in error_message for pattern in not_found_patterns):
                 return True
 
     return False

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion enhances the error detection logic by adding more patterns for "resource not found" messages, which improves the robustness of handling different AWS service exceptions.

Low
  • Update

Copy link
Member

@mk-armah mk-armah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@nivm-port nivm-port left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment

if error_code in resource_not_found_error_codes:
return True

if error_code == "GeneralServiceException":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don’t repeat yourself extract a function and pass the patterns on each occurrence

@shariff-6 shariff-6 requested a review from nivm-port October 17, 2025 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants