-
Notifications
You must be signed in to change notification settings - Fork 46
Fix Remote Runtime API Authentication #1090
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
Changes from all commits
c53bee7
f2d5322
28fd153
363dd4a
be97d23
bec7b32
2c3dc35
198fb80
568fe4f
12406c6
cdb9fdf
3e4a065
314ee93
248b235
737d97e
8e1f2cb
7ec292d
1cb6f0c
4f02cfe
b349ea5
0fab162
422c539
0416c6a
bbae87d
67f73dd
3701e0f
b0fed18
6c9d97e
93598d8
76980b3
d1e9f7f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,8 +24,19 @@ jobs: | |
| runs-on: blacksmith-2vcpu-ubuntu-2404 | ||
| timeout-minutes: 60 | ||
| steps: | ||
| - name: Wait for agent server to finish build | ||
| uses: lewagon/[email protected] | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.ref }} | ||
| check-name: Build & Push (python-amd64) | ||
| repo-token: ${{ secrets.GITHUB_TOKEN }} | ||
| wait-interval: 10 | ||
|
|
||
| - name: Checkout | ||
| uses: actions/checkout@v5 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.ref }} | ||
| repository: ${{ github.event.pull_request.head.repo.full_name }} | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v7 | ||
|
|
@@ -45,10 +56,12 @@ jobs: | |
| LLM_API_KEY: ${{ secrets.LLM_API_KEY }} | ||
| LLM_MODEL: openhands/claude-haiku-4-5-20251001 | ||
| LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }} | ||
| RUNTIME_API_KEY: ${{ secrets.RUNTIME_API_KEY }} | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| REPO_OWNER: ${{ github.repository_owner }} | ||
| REPO_NAME: ${{ github.event.repository.name }} | ||
| GITHUB_SHA: ${{ github.event.pull_request.head.sha }} | ||
| run: | | ||
| # List of examples to test | ||
| # Excluded examples: | ||
|
|
@@ -85,6 +98,7 @@ jobs: | |
| "examples/02_remote_agent_server/01_convo_with_local_agent_server.py" | ||
| "examples/02_remote_agent_server/02_convo_with_docker_sandboxed_server.py" | ||
| "examples/02_remote_agent_server/03_browser_use_with_docker_sandboxed_server.py" | ||
| "examples/02_remote_agent_server/04_convo_with_api_sandboxed_server.py" | ||
| ) | ||
|
|
||
| # GitHub API setup (only for PR events) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,7 +54,7 @@ class APIRemoteWorkspace(RemoteWorkspace): | |
| default=1, description="Resource scaling (1, 2, 4, or 8)" | ||
| ) | ||
| runtime_class: str | None = Field( | ||
| default="sysbox", description="Runtime class (e.g., 'sysbox')" | ||
| default="sysbox-runc", description="Runtime class (e.g., 'sysbox')" | ||
| ) | ||
| init_timeout: float = Field( | ||
| default=300.0, description="Runtime init timeout (seconds)" | ||
|
|
@@ -71,6 +71,21 @@ class APIRemoteWorkspace(RemoteWorkspace): | |
| _runtime_url: str | None = PrivateAttr(default=None) | ||
| _session_api_key: str | None = PrivateAttr(default=None) | ||
|
|
||
| @property | ||
| def _api_headers(self): | ||
| """Headers for runtime API requests." | ||
|
|
||
| This is used to manage new container runtimes via Runtime API. | ||
|
|
||
| For actual interaction with the remote agent server, the | ||
| `client` property is used, which includes the session API key | ||
| defined by ._headers property. | ||
| """ | ||
| headers = {} | ||
| if self.runtime_api_key: | ||
| headers["X-API-Key"] = self.runtime_api_key | ||
| return headers | ||
|
|
||
| def model_post_init(self, context: Any) -> None: | ||
| """Set up the remote runtime and initialize the workspace.""" | ||
| if self.resource_factor not in [1, 2, 4, 8]: | ||
|
|
@@ -97,12 +112,18 @@ def _start_or_attach_to_runtime(self) -> None: | |
| logger.info(f"Runtime ready at {self._runtime_url}") | ||
| self.host = self._runtime_url.rstrip("/") | ||
| self.api_key = self._session_api_key | ||
| self._client = None # Reset HTTP client with new host and API key | ||
| _ = self.client # Initialize client by accessing the property | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks a bit weird, maybe error-prone, though I see where it's coming from...
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @OpenHands review the codebase and understand how we can do this better. Then create an issue about it as we can resolve it in a future PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm on it! xingyaoww can track my progress at all-hands.dev There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SummaryI've reviewed the codebase and created a GitHub issue documenting the architectural concern identified in PR #1090. What I Did
Key Problems DocumentedThe issue describes why this pattern is concerning:
Proposed SolutionsI documented four potential solutions in the issue, with Option 2 (dynamic client recreation) recommended as it:
Option 3 (separate clients for Runtime API vs Agent Server) would be ideal for a larger refactoring to make separation of concerns explicit. No Code Changes MadeAs requested, I did not modify any code - this analysis is captured in the GitHub issue for future resolution in a separate PR. |
||
| assert self.client is not None | ||
| assert self.client.base_url == self.host | ||
|
|
||
| def _check_existing_runtime(self) -> bool: | ||
| """Check if there's an existing runtime for this session.""" | ||
| try: | ||
| resp = self._send_api_request( | ||
| "GET", f"{self.runtime_api_url}/sessions/{self.session_id}" | ||
| "GET", | ||
| f"{self.runtime_api_url}/sessions/{self.session_id}", | ||
| headers=self._api_headers, | ||
| ) | ||
| data = resp.json() | ||
| status = data.get("status") | ||
|
|
@@ -149,6 +170,7 @@ def _start_runtime(self) -> None: | |
| f"{self.runtime_api_url}/start", | ||
| json=payload, | ||
| timeout=self.init_timeout, | ||
| headers=self._api_headers, | ||
| ) | ||
| self._parse_runtime_response(resp) | ||
| logger.info(f"Runtime {self._runtime_id} at {self._runtime_url}") | ||
|
|
@@ -160,6 +182,7 @@ def _resume_runtime(self) -> None: | |
| f"{self.runtime_api_url}/resume", | ||
| json={"runtime_id": self._runtime_id}, | ||
| timeout=self.init_timeout, | ||
| headers=self._api_headers, | ||
| ) | ||
| self._parse_runtime_response(resp) | ||
|
|
||
|
|
@@ -183,7 +206,9 @@ def _wait_until_runtime_alive(self) -> None: | |
| logger.info("Waiting for runtime to become alive...") | ||
|
|
||
| resp = self._send_api_request( | ||
| "GET", f"{self.runtime_api_url}/sessions/{self.session_id}" | ||
| "GET", | ||
| f"{self.runtime_api_url}/sessions/{self.session_id}", | ||
| headers=self._api_headers, | ||
| ) | ||
| data = resp.json() | ||
| pod_status = data.get("pod_status", "").lower() | ||
|
|
@@ -244,12 +269,15 @@ def _wait_until_runtime_alive(self) -> None: | |
| def _send_api_request(self, method: str, url: str, **kwargs: Any) -> httpx.Response: | ||
| """Send an API request with error handling.""" | ||
| logger.debug(f"Sending {method} request to {url}") | ||
| logger.debug(f"Client headers: {self._headers}") | ||
| logger.debug(f"Request kwargs: {kwargs.keys()}") | ||
|
|
||
| response = self.client.request(method, url, **kwargs) | ||
| try: | ||
| response.raise_for_status() | ||
| except httpx.HTTPStatusError: | ||
| logger.debug(f"Request headers: {response.request.headers}") | ||
| # Log only header keys, not values (to avoid exposing API keys) | ||
| header_keys = list(response.request.headers.keys()) | ||
| logger.debug(f"Request header keys: {header_keys}") | ||
| try: | ||
| error_detail = response.json() | ||
| logger.info(f"API request failed: {error_detail}") | ||
|
|
@@ -274,9 +302,10 @@ def cleanup(self) -> None: | |
| f"{self.runtime_api_url}/{action}", | ||
| json={"runtime_id": self._runtime_id}, | ||
| timeout=30.0, | ||
| headers=self._api_headers, | ||
| ) | ||
| except Exception as e: | ||
| logger.error(f"Cleanup error: {e}") | ||
| logger.warning(f"Cleanup error: {e}") | ||
| finally: | ||
| self._runtime_id = None | ||
| self._runtime_url = None | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is necessarily blocking, but this is a bit complex for an example. Maybe we could move this off into some other utility function in another file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OpenHands please create a new github issue tracking this - we can simplify this after we are able to customize image pull policy on runtime API (after https://github.com/OpenHands/runtime-api/pull/356)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
I've successfully created a GitHub issue to track the simplification work as requested.
Issue Created: #1104
Title: "Simplify image resolution logic in remote agent server example after runtime-api image pull policy support"
What the issue tracks:
get_latest_commit_sha()function and related image resolution logic inexamples/02_remote_agent_server/04_convo_with_api_sandboxed_server.py(lines 49-79)The issue has been labeled with
enhancementandtechnical-debttags, includes references to the runtime-api PR #356 and the current PR #1090, and contains a task checklist for tracking the work.View full conversation