adk-python
adk-python copied to clipboard
fix: Allow disabling cert verify for RestApiTool and its dependency
Description
This PR aimed in fixing the SSL cert verification error while calling an endpoint with self-signed cert. As mentioned in the issue, build-in tool that supports calling external API should allow disabling the SSL cert verification for different purpose.
This PR fixes: https://github.com/google/adk-python/issues/1272
Root Cause
In RestApiTool.call() method, API call is made with the following statement in (rest_api_tool#L391):
request_params = self._prepare_request_params(api_params, api_args)
response = requests.request(**request_params)
There is no mechanism for the Tool or the AI Agent interface to fine-tune the behavior of the HTTP request with extra argument like "verify: False".
Both api_params and api_args are derived from the "operation". But we cannot expect operation to provide such detail because e.g. OpenAPI spec does not define runtime behavior like "cert verification". (rest_api_tool#L379):
As a result, SSL Verification failed the request when the endpoint comes with a self-signed cert.
Strategy
There are two approaches that can fix this problem. Option 2 is more ideal and implemented in this PR.
- Adding additional argument (e.g. "verify: bool") to the "RestApiTool.call() " function such that the caller can flag the request session (e.g. with verify=False) accordingly. This proposal is suboptimal because:
- it supports only the "verify" value to be overriden. requests supports many different parameters to fine-tune the HTTP request behavior.
- the caller that execute the tool (i.e. the Agent) will have to pass that setup in every call(). We argue that its cleaner to keep AI Agent decoupled from such API calling detail.
- the abstraction of the requests library is leak to the RestApiTool class. In other words, RestApiTool needs to kept its compatibility to the Request interface.
- Adding additional argument (e.g. "session: requests.Session") to the RestApiTool constructor. So, the creator of the RestApiTool can have full control to the behavior of the HTTP Request by injecting a custom Request.Session to the RestApiTool. This pattern make much more sense because:
- RestApiTool is now decoupled from many parameters in the requests.request()
- caller has access to all available features of requests to fine-tune the HTTP request behavior.
- Agent remains transparent to the Tool.call(). Everything else remain backward compatible.
Solution/Change
- Provide an optional argument session in the RestApiTool constructor to allow providing a customized requests.session to be used for HTTP Requests. AI Agent remains totally transparent to how HTTP Request is made by the RestApiTool. To support the reported issue, the application can:
import requests # Create a session session = requests.Session() # Disable SSL verification for the session session.verify = False tool = RestApiTool(..., session=session) - Add the same optional argument to enable such control in OpenAPITool and ApplicationIntegrationTool.
Proof that it works
Unit test to added to:
- Check (if any) custom request.session is provided in contructor, RestApiTool will use it to make the HTTP request.
- OpenAPITool and ApplicationIntegrationTool correctly passed the custom session (if any) down for constructing the RestApiTool.
- All existing unit test (without custom request.session) is behave as is. That is: fully backward compatible.
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
View this failed invocation of the CLA check for more information.
For the most up to date status, view the checks section at the bottom of the pull request.
Thanks for the fix @jeffreyrubi. However I feel we'd better not bound requests.Session to RestApiTool either. it's kind of exposing the implementation details (using requests library) and locks to that implementation. What if later we want to change the implementation to async using aiohttp or httpx ?
To summarize, the problem with your solution is :
- Implementation Lock-in: Directly exposing requests.Session locks the API to the requests library
- Async Migration Barrier: Moving to aiohttp/httpx would break the API
- Leaky Abstraction: HTTP client implementation details leak into the public API
- Limited Flexibility: Only supports requests-specific configuration
consider a more flexible and neutral approach , something like below:
class RequestOptions: """Generic HTTP request configuration.""" verify_ssl: bool = True timeout: Optional[float] = None headers: Optional[Dict[str, str]] = None cookies: Optional[Dict[str, str]] = None proxies: Optional[Dict[str, str]] = None # Add other common HTTP client options as needed
what do you think ?
I see your point. I think RequestOptions is a good tradeoff to keep the interface clean without leaking the abstraction for future enhancement. I'll submit a change.
Hi @jeffreyrubi , are you still working on the change?