Configuration: Update default web endpoints, and make them configurable

[o0Ignition0o]

Fixes #1500

Configuration: Update metrics and healthcheck web endpoints, and make them configurable (#1500)

The web endpoints exposed by the router listen to 127.0.0.1 by default, and the ports and paths for health check and prometheus have changed.

Here's the list of the endpoints exposed by the router:

• GraphQL: http://127.0.0.1:4000/ (unchanged)
• The GraphQL sandbox: http://127.0.0.1:4000/ (unchanged)
• Prometheus metrics: http://127.0.0.1:9090/metrics (used to be http://127.0.0.1:4000/plugins/apollo.telemetry/prometheus)
• Healthcheck: http://127.0.0.1:9494/health (used to be http://127.0.0.1:4000/.well-known/apollo/server-health)

While you could previously only customize the path for these endpoints, you can now customize the full IP address, PORT and PATH.

In order to enable this new feature, various server attributes such as listen, graphql_path and landing_page moved to more relevant sections.
Likewise, introspection and preview_defer_support have moved from the server section to the supergraph section:

This previous configuration:

server:
  listen: 127.0.0.1:4000
  graphql_path: /graphql
  health_check_path: /health
  introspection: false
  preview_defer_support: true
  landing_page: true
telemetry:
  metrics:
    prometheus:
      enabled: true

Now becomes:

# landing_page configuration
sandbox: 
  listen: 127.0.0.1:4000
  path: /
  enabled: false # default
# graphql_path configuration
supergraph:
  listen: 127.0.0.1:4000
  path: /
  introspection: false
  preview_defer_support: true
# health_check_path configuration
health-check:
  listen: 127.0.0.1:9494
  path: /health
  enabled: true # default
# prometheus scraper configuration
telemetry:
  metrics:
    prometheus:
      listen: 127.0.0.1:9090
      path: /metrics
      enabled: true

By @o0Ignition0o in #1718

[garypen]

Nice work!

I think the helm chart will need to be updated to reflect these changes. At least the health check stuff and I expect a lot of internal yaml attribute chasing will need to be updated. Let me know if you want me to work on the helm stuff.

[o0Ignition0o]

Not sure why the CI fails on linux, and just on linux, rerunning the job

Edit: it just passed Oo

[Geal]

could there be a comment here explaining the goal of this function?

[Geal]

what's the reason behind this? Do we consider that a sandbox with a different IP but same port would be a misconfiguration? What about this:

• graphql server listening on 0.0.0.0:4000
• sandbox listening on <IP accessible only from the internal network>:4000

[o0Ignition0o]

We want the test to fail if two different listenaddrs want to bind to the same port. This would be a security issue (binding to 127.0.0.1:4000 and 0.0.0.0:4000 would effectively mean the 127.0.0.1 endpoint is accessible from WAN)

The reason why this test fails this way is because we're using the default graphql endpoint (127.0.0.1:4000) and 0.0.0.0:4000 sandbox, which would "leak the graphql endpoint".

The example you have in mind is currently not supported (this would require more efforts like binding to 0.0.0.0:4000 and then manually "route" the connections according to the configuration, so we decided to not support it yet.

[Geal]

This would be a security issue (binding to 127.0.0.1:4000 and 0.0.0.0:4000 would effectively mean the 127.0.0.1 endpoint is accessible from WAN)

It looks like I don't have the right understanding of this PR then. In which case could this happen? What would it look like in the configuration file?

[o0Ignition0o]

Let's pretend you wanna expose the graphql endpoint and the prometheus one on the same port:

sandbox:
  listen: 0.0.0.0:4000
  path: /
  enabled: true # NOT MANDATORY?
telemetry:
  metrics:
    prometheus:
      listen: 127.0.0.1:4000
      path: /metrics
      enabled: true

With this setup /metrics would be exposed to WAN AFAICT, which would be a security issue.

ensure_listenaddrs_consistency makes sure the configuration doesn't have this, and raises an error if it's the case.

[Geal]

Ok, so the way I imagined the feature was more aligned with this:

The example you have in mind is currently not supported (this would require more efforts like binding to 0.0.0.0:4000 and then manually "route" the connections according to the configuration, so we decided to not support it yet.

But since it is not implemented yet we can end up with a security issue, right?

[o0Ignition0o]

We can’t, because the router explicitly refuses to start if that’s the case

[Geal]

wasn't the 9090 port supposed to be used only by the prometheus endpoint?

[o0Ignition0o]

I don't think so, the comment i ve worked from says otherwise

[Geal]

Originally the health check was on the same port as the graphql server, that's why it was under the .well-known path

[o0Ignition0o]

yes, this is an intentional breaking change, the goal is to not expose it to internet by default.

[garypen]

Great work. I think this is a big improvement on what we had.

[bnjjj]

To be able to run cargo xtask test locally without killing my local router instance

[BrynCooke]

Great work!