Skip to content

handle-invalid-gateway-url #600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TS0713
Copy link
Collaborator

@TS0713 TS0713 commented Jul 24, 2025

🐛 Bug-fix for PR 476


📌 Summary

💡 Fix Description

  • Added logic to validate gateway URL, if invalid raises exception. At present - implemented to handle both SSE & StreamableHHTP servers
  • modified 'forward_request' in 'GatewayService' Class (re-arranged statements) - Please review

🧪 Verification

Check Command Status
Unit tests make test pass
Unit tests make black pass
Unit tests make lint-web pass

📐 MCP Compliance (if relevant)

  • Matches current MCP spec
  • No breaking change to MCP clients

✅ Checklist

  • Code formatted (make black isort pre-commit)
  • No secrets/credentials committed

Example for testing invalid gateway servers

  1. www.example.com
  2. http://localhost:8200/sse (If server not reachable/ not started)
  3. http://localhost:8900
  4. www.google.com

All of the above will be handled in the 'validate_gateway_url', exception will be raised if they are invalid or not reachable

Signed-off-by: Satya [email protected]

@TS0713 TS0713 requested review from madhav165 and kevalmahajan July 24, 2025 19:13
@TS0713 TS0713 requested a review from crivetimihai as a code owner July 24, 2025 19:13
@TS0713 TS0713 marked this pull request as draft July 25, 2025 07:28
@TS0713 TS0713 marked this pull request as ready for review July 25, 2025 17:27
@TS0713 TS0713 force-pushed the handle-invalid-gateway-url branch from ac8dffd to 15c0303 Compare July 25, 2025 17:43
@crivetimihai
Copy link
Member

The PR successfully adds gateway connection validation with a new _validate_gateway_url method that checks transport-specific requirements before establishing connections. The implementation is sound but needs configuration options for timeout and SSL verification to be production-ready.

Implementation Recommendations

1. Make Validation Timeout Configurable

Add to config.py (after the health check settings):

# Gateway validation settings
gateway_validation_timeout: int = 5  # seconds

Add to .env.example (in Health Checks section):

# Timeout for gateway validation requests (seconds)
GATEWAY_VALIDATION_TIMEOUT=5

Update _validate_gateway_url method signature:

async def _validate_gateway_url(self, url: str, headers: dict, transport_type: str, timeout=None):
    """
    Validate if the given URL is a live Server-Sent Events (SSE) endpoint.
    
    Args:
        url (str): The full URL of the endpoint to validate.
        headers (dict): Headers to be included in the requests (e.g., Authorization).
        transport_type (str): SSE or STREAMABLEHTTP
        timeout (int, optional): Timeout in seconds. Defaults to settings.gateway_validation_timeout.
    
    Returns:
        bool: True if the endpoint is reachable and supports SSE/StreamableHTTP, otherwise False.
    """
    if timeout is None:
        timeout = settings.gateway_validation_timeout
    
    async with httpx.AsyncClient(verify=not settings.skip_ssl_verify) as client:
        timeout_obj = httpx.Timeout(timeout)
        # ... rest of implementation

2. Use Existing SSL Verification Setting

The current implementation should already use settings.skip_ssl_verify in the httpx client:

async with httpx.AsyncClient(verify=not settings.skip_ssl_verify) as client:

3. Add Debug Logging for Error Details

To maintain simple user-facing errors while preserving debugging information, update exception handling:

# In _validate_gateway_url
except Exception as e:
    logger.debug(f"Gateway validation failed for {url}: {str(e)}", exc_info=True)
    return False

# In _initialize_gateway
except Exception as e:
    logger.debug(f"Gateway initialization failed for {url}: {str(e)}", exc_info=True)
    raise GatewayConnectionError(f"Failed to initialize gateway at {url}")

4. Consider Retry Strategy

For consistency with other HTTP operations in the codebase, consider using ResilientHttpClient for validation requests. This would provide automatic retry logic:

# Alternative implementation using ResilientHttpClient
validation_client = ResilientHttpClient(
    client_args={
        "timeout": settings.gateway_validation_timeout,
        "verify": not settings.skip_ssl_verify
    }
)
try:
    async with validation_client.stream("GET", url, headers=headers) as response:
        # ... validation logic
finally:
    await validation_client.aclose()

5. Handle Edge Cases

Consider adding specific handling for common failure scenarios:

  • 401/403 errors: These indicate auth issues, not connectivity problems
  • Redirect chains: Current implementation follows one redirect - document this limitation or extend to handle chains
  • TLS errors: Log these separately as they often require admin intervention

Testing Recommendations

Add test coverage for:

  • Timeout behavior with slow/unresponsive endpoints
  • SSL verification bypass when SKIP_SSL_VERIFY=true
  • Authentication failures vs connectivity failures
  • StreamableHTTP redirect handling
  • Concurrent validation requests during bulk gateway registration

Performance Considerations

  • The added validation request will impact gateway registration time
  • For bulk operations, consider implementing parallel validation
  • Monitor impact on startup time when multiple gateways are configured

Final Notes

These changes maintain backward compatibility while adding necessary configuration flexibility. The validation timeout defaults to 5 seconds but can be tuned based on network conditions. SSL verification respects the existing global setting, avoiding the need for a separate configuration option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants