Embedded Sandbox default connection endpoint of http://0.0.0.0:4000/ results in query errors

[prasek]

Describe the bug
Embedded Sandbox at http://locahost:4000 defaults to connection endpoint of http://0.0.0.0:4000/ resulting in query errors

To Reproduce
Steps to reproduce the behavior:

1. Console window 1:

git clone https://github.com/apollographql/supergraph-demo-fed2.git
cd supergraph-demo-fed2
make run-router-main

Note this runs with acme_router --dev -c router.yaml (using router as a lib)

cors:
  allow_any_origin: true
supergraph:
  listen: 0.0.0.0:4000
  introspection: true
sandbox: # temporary workaround until RC1 is released
  listen: 0.0.0.0:4000
include_subgraph_errors:
  all: true # Propagate errors from all subraphs
telemetry:
  apollo:
    # The percentage of requests will include HTTP request and response headers in traces sent to Apollo Studio.
    # This is expensive and should be left at a low value.
    # This cannot be higher than tracing->trace_config->sampler
    field_level_instrumentation_sampler: 1 # (default)
  tracing:
    trace_config:
      sampler: 1 # The percentage of requests that will generate traces (a rate or `always_on` or `always_off`)
include_subgraph_errors:
  all: true # Propagate errors from all subraphs

2. Verify router has started

3: Console window 2:

make smoke

4. verify smoke tests pass

5. open http://localhost:4000 to load the Embedded Sandbox using Chrome

6. see the Sandbox connection endpoint is set to http://0.0.0.0:4000/

7. Run the following query in Sandbox

fragment MyFragment on Product {
    variation { name }
  }
}
query Dimensions {
  allProducts {
    ...MyFragment @defer
    sku
    id
  }
}

8. See the following error

{
  "error": {
    "message": "Failed to fetch",
    "stack": "TypeError: Failed to fetch\n    at <anonymous>:1:876\n    at handleRequest.s.includeCookies (https://embeddable-sandbox.cdn.apollographql.com/_latest/embeddable-sandbox.umd.production.min.js:1:52074)\n    at https://embeddable-sandbox.cdn.apollographql.com/_latest/embeddable-sandbox.umd.production.min.js:1:54762\n    at l (https://embeddable-sandbox.cdn.apollographql.com/_latest/embeddable-sandbox.umd.production.min.js:1:60081)"
  }
}

9. Adjust the Sandbox connection endpoint to: http://localhost:4000/

10. See the query execute successfully

Expected behavior
Embedded Sandbox connection endpoint should default to a host/IP where the router can be queried.

Where possible Sandbox should default to the browser window URL, but regardless when accessing Sandbox at http://localhost:4000 the queries should just work without having to adjust the Sandbox connection endpoint.

Output
See above

Desktop (please complete the following information):
Apollo Router v1.0.0-rc.0
OS: Mac OS 12.5.1 [64-bit]
Shell: /bin/zsh

Additional context
Add any other context about the problem here.

[abernix]

this is a great set of reproduction steps. thanks much!

[o0Ignition0o]

The problem:

The sandbox used to rely on window.location.href to load the correct router url to point to.

However since we're now allowing users to bind Sandbox to a custom listenAddress (ip and port) and path, using window.location.href would yield an incorrect result. For example, the sandbox being available on /sandbox would make it wrongly assume the supergraph endpoint is located at localhost:4000/sandbox.

This is the reason why sandbox now used the supergraph.listen and supergraph.path values to generate the url.

In a scenario where the supergraph endpoint is served on 127.0.0.1:5000/graphql and sandbox is served on 0.0.0.0:10000/sandbox, the loaded sandbox will point to 127.0.0.1:5000/graphql.

The constraints:

• We cannot rely on window.location.href to serve sandbox on the browser, since both endpoints can be served on a different URL
• A server's bind address is only correct on the client side if it uses 127.0.0.1
• Customizing endpoints was one of our goals, and since we're at 1.0 we want to make additive changes only as much as we can.
• The backend doesn't have enough context at startup to confidently know what public supergraph url will allow clients to make graphql requests (localhost is only correct if the supergraph is running on a local machine, container environments can change any of this ad any point, and dns will come in play as well)

Possible solutions and outcomes:

1: add a supergraph.public_endpoint_url field in the configuration, to give awareness of the url:

Pros:

• the user specifies the actual url router clients will use to make GraphQL requests to the supergraph.
• if we fallback to localhost:<supergraph_port><supergraph_path>, adding this is not a breaking change.
• the fallback will work for the currently common scenario, which is local dev.
• This would work for sandbox AND homepage (if homepage provides us with a way to interpolate the url).

Cons:

• One more field in the config

2: use the host header from a graphql request as hostname, and derive the supergraph endpoint url by replacing the port and path:

Pros:

• No need to change the configuration
• This will work in most cases
• This would work for sandbox AND homepage (if homepage provides us with a way to interpolate the url).

Cons:

• In container scenarios, This won't work if the publicly exposed port doesn't match the private one.

3: don't allow sandbox to bind to an other listen address (path is fine)

Pros:

• much less hassle trying to figure out the right url, we strip the path, put the supergraph endpoint's

Cons:

• Endpoints won't be customizable anymore
• It's a breaking change

I'm in favor of 1, and against 3.

[o0Ignition0o]

Here's what we discussed during huddle:

Most of our products assume Sandbox and Homepage are exposed on the same endpoint as the one serving GraphQL requests. Allowing sandbox to bind to an other path / url will require deeper validation across the whole platform.

Homepage and Sandbox are mutually exclusive: if both are enabled: print an error message and fail at startup
Sandbox cannot work if introspection is disabled: print an error message and fail at startup.

We have decided the safer approach is 3: Sandbox will be an enabled boolean, and it will bind to the Supergraph endpoint.

I'll work on a PR today, which will supersede #1781 that won't be needed anymore, and fix #1783

[abernix]

The above sounds good to me. I think we can re-consider the flexibility we (very temporarily) offered in a future version. Thanks!

[o0Ignition0o]

fixed by #1796