router icon indicating copy to clipboard operation
router copied to clipboard

Reconsider the organization of configuration sections

Open SimonSapin opened this issue 2 years ago • 1 comments

Let's have a look before 1.0 at how we want to split configuration keys into sections

SimonSapin avatar Aug 12 '22 08:08 SimonSapin

so, the point I was raising there is that the configuration schema is too influenced by the plugins structure, and not enough by what would make sense from the usage point of view.

As an example, let's say we had a configuration file using URL override, header manipulation and rate limiting:

server:
  listen: 0.0.0.0:4000

override_subgraph_url:
  accounts: http://localhost:8080

headers:
  subgraphs:
    accounts:
      - propagate:
        named: "Authorization"
    products:
      - insert:
          name: "InsertSubgraph"
          value: "Value"

traffic_shaping:
  router:
    rate_limit:.
      capacity: 10
      interval: 5s
    timeout: 50sec
  subgraphs:
    products:
      rate_limit:
      timeout: 50sec

Here we see router level and subgraph level configuration in 3 different sections, because each one has its own configuration. This is still a manageable example, but it is bound to become more complex as more subgraphs are added and more plugins are developed.

What I would like to see is something of that shape:

server:
  listen: 0.0.0.0:4000
  traffic_shaping:
    rate_limit:.
      capacity: 10
      interval: 5s
    timeout: 50sec

subgraphs:
  accounts:
    override_url: http://localhost:8080
    headers:
      - propagate:
        named: "Authorization"
  products:
    headers:
      - insert:
        name: "InsertSubgraph"
        value: "Value"
    traffic_shaping:
      rate_limit:
      timeout: 50sec

Router level configuration is clearly separated, and each subgraph has its own configuration. Note that this is still the same configuration shape though, because each subsection still has the plugin name. I would like at some point that configuration be completely decorrelated from plugin name (example: by flattening the traffic_shaping options directly under the router or subgraph section), but that would be too much work for now.

What I am proposing would amount to extracting the global and subgraph level sections of a plugin from the configuration, and assembling it in a coherent configuration object for that plugin, which is much more doable than a complete configuration overhaul

Geal avatar Aug 12 '22 12:08 Geal

Discussed with @SimonSapin and concluded that this does seem like a more natural way to specify configuration for a router administrator. However, it's probably not something we should hold up 1.0 for.

garypen avatar Aug 18 '22 13:08 garypen

I would like at some point that configuration be completely decorrelated from plugin name

For external plugins that seems tricky to me.

A more specific change could be to keep config sections named after plugin names, but in addition to the router-global ones have optional per-subgraph config with a second associated type in the Plugin trait


Some possible changes for 1.0 IMO:

  • Optional per-subgraph config for plugins
  • Move cors out of server to the top-level: https://github.com/apollographql/router/issues/1483
  • Remove server entirely, move its contents to the top-level

SimonSapin avatar Aug 19 '22 10:08 SimonSapin

~We’ve decided not to change configuration for 1.0~ PR: https://github.com/apollographql/router/pull/1608

SimonSapin avatar Aug 25 '22 12:08 SimonSapin

From @garypen in https://github.com/apollographql/router/pull/1608#issuecomment-1232701149

cors:
  allow_credentials: true
 # renamed from landing_page
sandbox:
  listen: 127.0.0.1:4000
  path: /my_sandbox_path
  enabled: true <= NOT MANDATORY?
graphql:
  listen: 127.0.0.1:4000
  # renamed from endpoint
  path: /my_graphql_path
  introspection: false
  preview_defer_support: false
telemetry:
  metrics:
    prometheus:
      listen: 0.0.0.0:9090 # default
      path: /metrics # default
      enabled: true <= REQUIRED?
health-check:
      listen: 0.0.0.0:9090 # default
      path: /health # default

abernix avatar Sep 06 '22 14:09 abernix

@abernix did we define a default path for sandbox? The current configuration won't be reproducible once we make this change, we will have a conflict between the graphql and the sandbox path.

o0Ignition0o avatar Sep 07 '22 09:09 o0Ignition0o