feat(docker): add support for a docker build image of datahub-mcp-server#92
feat(docker): add support for a docker build image of datahub-mcp-server#92
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Free Tier Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Docker image always reports version 0.0.0
- I passed a release-derived build arg through the Docker workflow and used it as
SETUPTOOLS_SCM_PRETEND_VERSIONduring project install so container builds no longer fall back to0.0.0without.git.
- I passed a release-derived build arg through the Docker workflow and used it as
- ✅ Fixed: Unpinned
uv:latesttag risks breaking Docker builds- I replaced
ghcr.io/astral-sh/uv:latestwith the pinned major tagghcr.io/astral-sh/uv:0.6to avoid unbounded upgrades fromlatest.
- I replaced
Or push these changes by commenting:
@cursor push 793c95037d
Preview (793c95037d)
diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml
--- a/.github/workflows/docker.yml
+++ b/.github/workflows/docker.yml
@@ -16,7 +16,9 @@
- name: Extract version from release tag
id: version
run: |
- echo "version=${{ github.event.release.tag_name }}" >> "$GITHUB_OUTPUT"
+ tag="${{ github.event.release.tag_name }}"
+ echo "version=${tag}" >> "$GITHUB_OUTPUT"
+ echo "package_version=${tag#v}" >> "$GITHUB_OUTPUT"
- name: Log in to Docker Hub
uses: docker/login-action@v3
@@ -39,6 +41,8 @@
with:
context: .
push: true
+ build-args: |
+ MCP_SERVER_DATAHUB_VERSION=${{ steps.version.outputs.package_version }}
tags: |
acryldata/mcp-server-datahub:${{ steps.version.outputs.version }}
acryldata/mcp-server-datahub:latest
diff --git a/Dockerfile b/Dockerfile
--- a/Dockerfile
+++ b/Dockerfile
@@ -3,8 +3,11 @@
WORKDIR /app
# Install uv
-COPY --from=ghcr.io/astral-sh/uv:latest /uv /usr/local/bin/uv
+COPY --from=ghcr.io/astral-sh/uv:0.6 /uv /usr/local/bin/uv
+# Build-time version override for setuptools-scm when .git is unavailable
+ARG MCP_SERVER_DATAHUB_VERSION=0.0.0
+
# Copy dependency files
COPY pyproject.toml uv.lock ./
@@ -15,7 +18,7 @@
COPY src/ ./src/
# Install the project itself
-RUN uv sync --frozen --no-dev
+RUN SETUPTOOLS_SCM_PRETEND_VERSION=${MCP_SERVER_DATAHUB_VERSION} uv sync --frozen --no-dev
ENV PATH="/app/.venv/bin:$PATH"This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| COPY src/ ./src/ | ||
|
|
||
| # Install the project itself | ||
| RUN uv sync --frozen --no-dev |
There was a problem hiding this comment.
Docker image always reports version 0.0.0
Medium Severity
The project uses setuptools-scm for versioning, which derives the version from git tags and writes _version.py. Since _version.py is in .gitignore (not tracked in git) and the Dockerfile never copies the .git directory, setuptools-scm cannot determine the version when uv sync runs during the build. It falls back to fallback_version = "0.0.0" from pyproject.toml. This means every Docker image — even those tagged with a real release version by the CI workflow — will report __version__ as "0.0.0", affecting the --version CLI output and the telemetry datahub_component string.
| WORKDIR /app | ||
|
|
||
| # Install uv | ||
| COPY --from=ghcr.io/astral-sh/uv:latest /uv /usr/local/bin/uv |
There was a problem hiding this comment.
Unpinned uv:latest tag risks breaking Docker builds
Medium Severity
The COPY --from=ghcr.io/astral-sh/uv:latest uses an unpinned :latest tag, making the Docker build non-reproducible. If uv releases a breaking change (e.g., moving the binary path from /uv, or changing CLI behavior), builds will silently break. The existing wheels.yml workflow pins astral-sh/setup-uv@v6, but this Dockerfile has no version constraint at all. Pinning to a specific version or major version tag (e.g., uv:0.6) would prevent unexpected build failures.
There was a problem hiding this comment.
ignoring this as other use cases use uv:latest
There was a problem hiding this comment.
why not pin? where are the other use cases that use uv:latest?
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 27. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
README.md
Outdated
| docker build -t mcp-server-datahub . | ||
| docker run -p 8000:8000 \ | ||
| -e DATAHUB_GMS_URL=https://your-datahub-instance \ | ||
| -e DATAHUB_GMS_TOKEN=your-token \ |
There was a problem hiding this comment.
this is a problem right? we are running datahub mcp without any authentication publically available.
alexsku
left a comment
alexsku
left a comment
There was a problem hiding this comment.
I think we should authenticate customer requests, I don't think it is ok for us to release docker files that promote unsecured services
I'll add support for token auth ad query param or header as default options |
|
This PR exposes the MCP server over HTTP with no authentication. Anyone who can reach port 8000 gets full access to all tools using the configured I'd recommend requiring an |
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 27. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
src/mcp_server_datahub/__main__.py
Outdated
|
|
||
| def __init__(self, client: DataHubClient) -> None: | ||
| self._client = client | ||
| def __init__(self, server_url: str, default_client: Optional[DataHubClient]) -> None: |
There was a problem hiding this comment.
I wonder if we need to have two separate modes - one that we use for http server mode (that would require authentication) and another for others. and we would have two mutually exclusive middleware - old _DataHubClientMiddleware and new one that will construct DataHubClient every time (it can use cache function that takes the token and the url as parameters)
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 27. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
@alexsku |
nwadams
force-pushed
the
na--docker-build-mcp-server
branch
from
e1adc70 to
0c74fc4
Compare
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 27. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
Nice progress on the auth — the Right now I think we'd be better off with two mutually exclusive middlewares selected by transport mode: stdio/SSE — keep the original HTTP with HTTP without
The distinction is simple: if This gives us:
|
|
One more thing — I'd remove Same for the README Docker examples — only show |
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 27. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
@alexsku refactored to simplfiy the middleware. I didn't end up setting up two different middleware because I made it so only the client is created in middleware and validation is done elsewhere. |
src/mcp_server_datahub/__main__.py
Outdated
|
|
||
| global_token = os.environ.get("DATAHUB_GMS_TOKEN") | ||
| if global_token: | ||
| _verify_client(_build_client(server_url, global_token)) |
There was a problem hiding this comment.
why do we need to do this here? not that it is a big deal either way, I'm just confused how this is related to the pr
There was a problem hiding this comment.
I want to error at startup if the token isn't valid since nothing will work without a valid token.
| _GET_ME_QUERY = "query getMe { me { corpUser { urn username } } }" | ||
|
|
||
|
|
||
| def _build_client(server_url: str, token: str) -> DataHubClient: |
There was a problem hiding this comment.
do we want to have a cache for this?
There was a problem hiding this comment.
I dont think we need to have a cache for this. There's no http calls here.
There was a problem hiding this comment.
it results in the new connection for every call, no? I'm pretty certain we need to cache
| super().__init__() | ||
| self._server_url = server_url | ||
|
|
||
| async def verify_token(self, token: str) -> Optional[AccessToken]: |
There was a problem hiding this comment.
do we want to have this cacheable?
src/mcp_server_datahub/__main__.py
Outdated
| return AccessToken( | ||
| client_id=f"mcp-server-datahub/{__version__}", scopes=[], token=token | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
if we are caching perhaps we should be more precise in what to catch here, we don't want to cache null if there was 500 server error from the server
There was a problem hiding this comment.
I think we would want to avoid caching errors in general
| try: | ||
| request = get_http_request() | ||
| except RuntimeError: | ||
| return None |
There was a problem hiding this comment.
when runtime error would be thrown? should we propagate the exception up instead of returning None here?
There was a problem hiding this comment.
If this is called outside of an http context. It should never happen in the current implementation.
There was a problem hiding this comment.
we should propagate the exception then, why changing it to None?
alexsku
left a comment
alexsku
left a comment
There was a problem hiding this comment.
PR Summary
PR: #92 — feat(docker): add support for a docker build image of datahub-mcp-server
Author: nwadams
Base: main ← na--docker-build-mcp-server
Type: Mixed — new infra (Docker/CI), behavior change (auth), API change (per-request tokens)
Size: +555 / -11 across 6 files
Adds Docker deployment support for running the MCP server in HTTP mode with per-request Bearer token authentication. Each HTTP request now carries its own DataHub token, validated against the DataHub GMS getMe query. When DATAHUB_GMS_TOKEN is set, it acts as a fallback (local/CLI usage); when absent, every request must authenticate (shared/Docker deployment). Includes a GitHub Actions workflow to build and publish Docker images on release, a Dockerfile, docker-compose, README docs, and unit tests.
Key invariants:
- HTTP requests without a valid Bearer token must be rejected with 401 when no global
DATAHUB_GMS_TOKENis configured - Per-request tokens must be used to build a DataHubClient scoped to that request's ContextVar
- stdio/SSE transport must continue working unchanged with env-based configuration
- Docker images must be tagged with the release version and published to both Docker Hub and GHCR
Risk Assessment
| Risk | Medium |
| Blast radius | All HTTP-mode deployments; stdio users affected by new DATAHUB_GMS_URL requirement |
| Rollback | Safe — no migrations or data changes |
| Rollout | Ship after fixes — Docker is new surface area so no existing users affected, but stdio regression needs attention |
| CI | Passing (lint + integration), but new unit tests are NOT run in CI |
Blocking Issues
[BLOCKER] Tests don't match implementation — will fail if actually run
Category: tests | Confidence: high
Location: tests/test_mcp/test_auth_middleware.py:80-88 — test_falls_back_to_default_client_when_no_request_token()
The middleware constructor is __init__(self, server_url: str, default_token: Optional[str]) — it stores a token string and builds a client per-request via _build_client(). But the test passes a MagicMock() as default_token and asserts result is default_client, expecting the middleware to return a pre-built client object directly. Since _build_client is not patched in this test, it will attempt to construct a real DataHubClient with a MagicMock token and crash, or at minimum the is assertion will fail.
Same issue in test_create_app_with_token_builds_default_client (line ~185): it asserts args[1] is mock_build.return_value, but create_app() passes global_token (the string "globaltoken") as the second arg to the middleware, not the built client.
These tests are not run in CI — the CI workflow only runs tests/test_mcp_integration.py, so the failures are invisible.
Suggested fix: Either:
- (a) Add
test_auth_middleware.pyto CI, then fix the tests to match the actual code (patch_build_clientwhere needed, assert on token strings not client objects), or - (b) Refactor the middleware to accept an optional pre-built default client (as the tests expect), which would also avoid rebuilding it every request when a global token is configured.
Option (b) is better — it fixes both the test mismatch and the performance issue below.
High-Priority Issues
[HIGH] No client caching — new DataHubClient + GraphQL roundtrip on every request
Category: performance | Confidence: high
Location: src/mcp_server_datahub/__main__.py:85-91 — verify_token() and _client_for_request()
Every HTTP request triggers two _build_client() calls:
_DataHubTokenVerifier.verify_token()builds a client and runs agetMeGraphQL query to validate the token_DataHubClientMiddleware._client_for_request()builds another client for the same token
For the verifier path, this means every MCP request does a synchronous GraphQL roundtrip just for auth validation — before the actual tool call even starts. There's no caching of verified tokens or clients.
Suggested fix: Add a cachetools.TTLCache (already a project dependency) keyed on (server_url, token) to cache validated clients for a short TTL (e.g. 5 minutes). The verifier can populate the cache, and the middleware can read from it.
[HIGH] Blocking synchronous I/O in async verify_token
Category: performance | Confidence: high
Location: src/mcp_server_datahub/__main__.py:85-91 — _DataHubTokenVerifier.verify_token()
verify_token is an async method but calls _verify_client() → client._graph.execute_graphql() which is synchronous blocking I/O. This blocks the event loop for every incoming HTTP request during token verification.
Suggested fix: Run the blocking call in a thread via asyncio.to_thread() or anyio.to_thread.run_sync() (the project already uses asyncer which wraps anyio).
[HIGH] Breaking change for stdio users — DATAHUB_GMS_URL now strictly required
Category: correctness | Confidence: medium
Location: src/mcp_server_datahub/__main__.py:159-161 — create_app()
Previously, create_app() used DataHubClient.from_env() which may have had default values (e.g. http://localhost:8080). Now it does a hard os.environ.get("DATAHUB_GMS_URL") check and raises RuntimeError if missing. Existing stdio users who relied on DataHubClient.from_env() defaults (or who set the URL via other means like ~/.datahubenv) will break.
Suggested fix: For stdio/SSE transport, consider falling back to DataHubClient.from_env() when DATAHUB_GMS_URL is not set. Only require explicit DATAHUB_GMS_URL for HTTP mode.
Other Issues
- [MEDIUM / security]
Dockerfile:6—COPY --from=ghcr.io/astral-sh/uv:latestis unpinned. A breakinguvrelease will silently break Docker builds. Fix: pin to a specific version likeghcr.io/astral-sh/uv:0.6. - [MEDIUM / operability]
__main__.py:88—verify_tokencatches all exceptions with a bareexcept Exception: return None. A network timeout, DNS failure, or DataHub outage will silently return 401 to all users with no logging. Fix: addlogger.warning("Token verification failed", exc_info=True)before returningNone. - [MEDIUM / correctness]
__main__.py:196-199—main()readsDATAHUB_GMS_URLa second time aftercreate_app()already validated it. The auth verifier decision is split betweenmain()andcreate_app(). Fix: move themcp.authassignment intocreate_app()based on transport mode. - [LOW / dx]
docker-compose.yml— Norestartpolicy. Docker deployments will silently stop on crash. Fix: addrestart: unless-stopped.
What's Missing
- Unit tests not in CI:
test_auth_middleware.pyis never executed. Add it to the CI workflow or add a separate unit test job. - No HTTPS guidance: The README Docker section doesn't mention HTTPS. Per-request Bearer tokens over plain HTTP are trivially interceptable. Add a note recommending a TLS-terminating reverse proxy.
- No rate limiting on token verification: Each
verify_tokencall does a GraphQL roundtrip. An attacker can DoS the DataHub GMS by sending many requests with different invalid tokens.
Test Plan
| Invariant | Covered? | Gap |
|---|---|---|
| 401 on missing/invalid token (HTTP, no global token) | Yes — tests exist | Not run in CI |
| Valid token accepted (HTTP) | Yes — test exists | Not in CI |
Fallback to global token when DATAHUB_GMS_TOKEN set |
Partially — test is buggy (blocker above) | Fix test, add to CI |
| stdio transport unaffected | No | Add test verifying stdio init without DATAHUB_GMS_URL |
| Docker image builds with correct version | No | Add CI job for test build |
Questions for Author
- Was the
DataHubClient.from_env()removal intentional? Are there stdio users who configure the client via~/.datahubenvor other mechanisms thatfrom_env()supports but directos.environ.getdoesn't? - What's the plan for getting the new unit tests into CI? Currently they're invisible — the CI workflow only runs integration tests.
- Have you considered caching verified clients (TTL cache keyed on token)? The current design does a GraphQL roundtrip on every single request, which could add significant latency under load.
| client = DataHubClient.from_env( | ||
| client_mode=ClientMode.SDK, | ||
| datahub_component=f"mcp-server-datahub/{__version__}", | ||
| ) |
There was a problem hiding this comment.
Claude is right - this is a breaking change
2 participants
Add support for running datahub-mcp-server in http mode with a docker-compose. Build a docker image for public consumption
Note
Medium Risk
Changes add a new container build/publish pipeline that relies on registry credentials and release tagging; misconfiguration could publish incorrect images or fail releases, but runtime code is unchanged.
Overview
Adds first-class Docker deployment support for running the server in HTTP mode, including a new
Dockerfile(Python 3.11-slim +uv) and adocker-compose.ymlthat wires required DataHub env vars and exposes port 8000.Introduces a GitHub Actions workflow that, on GitHub Release publish, builds the image and pushes versioned +
latesttags to both Docker Hub (acryldata/mcp-server-datahub) and GHCR, and updates the README with Docker/Compose run instructions and documented endpoints (/mcp,/health).Written by Cursor Bugbot for commit 2180f41. This will update automatically on new commits. Configure here.