aws / aws/graph-explorer

Unify Docker image: remove separate SageMaker build

Open
#1,772 0 comments 0 reactions 1 assignee Claimed by @kmcginnes View on GitHub
ready-for-agent
Dominant language
TypeScript
Stars
481
Forks
108
Avg merge
8d 9h
Merged PRs (30d)
7

Description

## Problem Statement

Graph Explorer maintains two Docker image variants built from the same Dockerfile — a main image and a SageMaker image differing only in build-time environment variable defaults. This doubles CI build/scan time, creates a confusing tag namespace (`sagemaker-*` vs regular), and forces the SageMaker lifecycle script to track a separate tag lineage.

On the client side, users must manually configure a "Public or Proxy Endpoint" URL and enable a "Using Proxy-Server" checkbox, even though the proxy server and UI are always co-located on the same origin. This creates unnecessary configuration friction — especially in SageMaker where the proxy URL must be derived from notebook metadata.

## Solution

Ship a single Docker image that works in all deployment contexts. The frontend uses relative asset paths and relative API fetches, eliminating build-time knowledge of the deployment path prefix. The client always routes database queries through the same-origin proxy server, removing the need for users to provide a proxy URL. The `NEPTUNE_NOTEBOOK` environment variable is preserved as a runtime convenience preset that configures port, logging, and SSL defaults.

## User Stories

1. As a user deploying on SageMaker, I want to pull the same image tag as everyone else, so that I don't have to track a separate `sagemaker-*` tag lineage.
2. As a user deploying on SageMaker with an existing lifecycle script, I want my deployment to keep working after an image upgrade, so that I don't have to change anything.
3. As a user creating a Connection, I want to only provide my database endpoint and auth settings, so that I don't have to figure out what "Public or Proxy Endpoint" means or what URL to put there.
4. As a user deploying behind a custom reverse proxy, I want the app to work without build-time path configuration, so that I can mount it at any URL prefix.
5. As a user editing an existing Connection, I want my saved connections to keep working after upgrade, so that I don't have to reconfigure them.
6. As a user deploying with ECS Fargate, I want to simplify my task definition environment variables, so that I don't need to specify proxy endpoint and proxy toggle vars.
7. As a user deploying with Docker, I want to pass only the database URL and auth settings, so that the setup is minimal.
8. As a CI maintainer, I want to build one image instead of two, so that build time and vulnerability scan coverage are halved.
9. As a user running with `NEPTUNE_NOTEBOOK=true`, I want the container to automatically use port 9250 and cloudwatch logging, so that I don't need to pass those explicitly.
10. As a user passing explicit `PROXY_SERVER_HTTP_PORT`, I want my value to take precedence over the `NEPTUNE_NOTEBOOK` defaults, so that I have full control.
11. As a user with `NEPTUNE_NOTEBOOK=true`, I want SSL to be forced off regardless of other settings, so that the Jupyter proxy works correctly.
12. As a developer, I want the `proxyConnection` concept removed from the codebase, so that there's one path for all database queries.
13. As a user viewing the Connection detail, I want to see my database endpoint clearly, so that I know which database I'm connected to.
14. As a user hitting a critical error, I want the "reload" link to take me back to the app root, so that I get a clean slate.
15. As a user who previously had `proxyConnection: false`, I want my saved database URL to be preserved on upgrade, so that I don't lose my connection.
16. As a user who previously had `proxyConnection: true`, I want my saved graph database URL to be preserved on upgrade, so that I don't lose my connection.

## Implementation Decisions

### Relative asset paths
- Vite `base` changes from `env.GRAPH_EXP_ENV_ROOT_FOLDER` to `"./"`.
- `index.html` gets a `` tag to anchor `document.baseURI` regardless of client-side routing. This future-proofs for a potential move from HashRouter to proper URL routes.
- `RELOAD_URL` simplifies to `"."`.

### Relative API fetches
- All client API calls (sparql, gremlin, openCypher, defaultConnection, logger, summary, etc.) use `new URL("../endpoint", document.baseURI)` instead of constructing absolute URLs from `connection.url`.
- The static files are served at `/explorer` and API routes at `/`. The `../` from the static directory resolves to the API root. This relationship is fixed in one place on the server.
- The two-path fallback in `defaultConnection.ts` (trying both root and `/proxy/9250/`) is removed in favor of a single relative fetch.

### Connection model simplification
- `proxyConnection` and `url` are removed from the `ConnectionConfig` type.
- `graphDbUrl` becomes the canonical required field for the database endpoint.
- `fetchDatabaseRequest` always sends the `graph-db-connection-url` header — no conditional based on `proxyConnection`.
- A read-time normalization handles legacy stored data: if `proxyConnection` was true, use `graphDbUrl`; if false, use `url`.

### Connection form simplification
- The "Public or Proxy Endpoint" text field is removed.
- The "Using Proxy-Server" checkbox is removed.
- "Graph Connection URL" is always visible (not gated behind the proxy checkbox).
- The IAM auth toggle is always visible (not gated behind the proxy checkbox).

### Dockerfile
- `ARG NEPTUNE_NOTEBOOK` and all conditional ENV logic are removed.
- `GRAPH_EXP_ENV_ROOT_FOLDER` is removed.
- `PROXY_SERVER_HTTP_PORT` and `LOG_STYLE` are NOT set as ENV vars — the app's built-in defaults (80, "default") apply when absent.
- `EXPOSE 80`, `EXPOSE 443`, `EXPOSE 9250` remain.

### Runtime NEPTUNE_NOTEBOOK handling (process-environment.sh)
- When `NEPTUNE_NOTEBOOK=true`:
- Writes `PROXY_SERVER_HTTP_PORT=9250` to `.env` (respects explicit env var override via dotenv's no-override behavior).
- Writes `LOG_STYLE=cloudwatch` to `.env` (same override behavior).
- Forces `PROXY_SERVER_HTTPS_CONNECTION=false` and `GRAPH_EXP_HTTPS_CONNECTION=false` (not overridable).
- Stops writing `NEPTUNE_NOTEBOOK` itself to `.env` — nothing downstream reads it.
- `PUBLIC_OR_PROXY_ENDPOINT` and `USING_PROXY_SERVER` handling is removed from the script. These env vars are silently ignored if still passed.

### CI: dual-tag publishing
- CI builds one image.
- Publishes under both regular tags and `sagemaker-*` tags during a transition period.
- The `--build-arg NEPTUNE_NOTEBOOK=true` step is removed.

### Lifecycle script update
- Pulls the regular tag (not `sagemaker-*`).
- Removes `PUBLIC_OR_PROXY_ENDPOINT` and `USING_PROXY_SERVER` from `docker run`.

### Documentation updates
- `docs/references/configuration.md`
- `docs/guides/deploy-to-ecs-fargate.md`
- `docs/guides/deploy-to-sagemaker.md`
- `docs/guides/deploy-with-docker.md`
- `docs/features/connections.md`
- `docs/architecture.md`

## Testing Decisions

A good test for this work exercises observable external behavior through the public interfaces — not internal wiring. Tests should verify what a user or calling code observes, not how the internals are structured.

### Modules to test

1. **Connection migration (read-time normalization)** — Given legacy stored connection data with various combinations of `proxyConnection`, `url`, and `graphDbUrl`, verify that the normalized output always produces the correct `graphDbUrl`. Prior art: `defaultConnection.test.ts`, `configuration.ts` normalization tests.

2. **Relative URL resolution** — Given various `document.baseURI` values (representing `/explorer/`, `/proxy/9250/explorer/`, custom prefix), verify that the API URL construction resolves to the correct absolute path. This can be a pure function test with injected base URIs. Prior art: unit tests in `connector/` and `core/`.

3. **process-environment runtime defaults** — Given various combinations of environment variables and `NEPTUNE_NOTEBOOK=true/false`, verify the correct values are written to `.env` and `defaultConnection.json`. Verify explicit env vars take precedence. Verify SSL is forced off when `NEPTUNE_NOTEBOOK=true`. Prior art: `process-environment.test.ts` (extensive existing test suite).

4. **Connection form UI** — Verify the form renders the correct fields (no proxy endpoint, no proxy toggle), validates that `graphDbUrl` is required, and submits the correct shape. Prior art: existing component tests using React Testing Library patterns in the codebase.

5. **fetchDatabaseRequest** — Verify that `graph-db-connection-url` header is always sent. Verify AWS headers are sent when IAM is enabled. Verify the function works without `proxyConnection` in the connection object. Prior art: `fetchDatabaseRequest.test.ts` (comprehensive existing suite).

## Out of Scope

- Removing `NEPTUNE_NOTEBOOK` entirely (kept for backward compat with deployed lifecycle scripts).
- Dropping the `sagemaker-*` tag publishing (deferred to a future release after transition period).
- Moving from HashRouter to proper URL routes (future work, but `` prepares for it).
- Changing the `graphDbUrl` field name (kept for backward compat with stored data).
- Renaming the "Graph Connection URL" UI label.
- Server-side route restructuring (API routes stay at root, static files stay at `/explorer`).

## Further Notes

- The ADR for this decision is committed at `docs/adr/0001-unify-docker-image-remove-sagemaker-variant.md` on branch `remove-sagemaker-docker-image`.
- Users passing `PUBLIC_OR_PROXY_ENDPOINT` or `USING_PROXY_SERVER` as env vars to Docker will see no errors — the vars are simply ignored. No deprecation warning is emitted.
- The `defaultConnection.json` file still works as a pre-configuration mechanism for operators. It just no longer includes proxy endpoint or proxy toggle fields.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.