Enabling/Disabling of services/ports

[0xERR0R]

Configuration of ports should be changed to:

services:
  dns:
    ports:
       plain: 53
       tls: 853
       https: 443
    rate-limit: 60 # just example for further config
  metrics:
    ports:
       http: 4001
       https: 443 
    path: /mymetrics # moved from prometheus.path
  control-api:
    ports:
       http: 3000
    api-key: ASBC123 # just example for further config
  pprof:
    ports:
       http: 12345

prometheus configuration should be removed (migrated to services.metrics)

[kwitsch]

Just to verify: most settings of the basic configuration would also be moved(certFile, key file,minTlsServeVersion) since they are currently connected to ports, correct?

[ThinkChaos]

Sounds like that's what # just example for further config shows, so I'd say yeah.
Could be done in two steps if that's easier.

[ThinkChaos]

Started working on this though I'm doing a preliminary step without all the config changes, and just focusing on getting DoH separate from other HTTP services ATM, but doing it in a way that'll be re-usable for further splitting.
I think I can get a PR up this week. I assigned myself here just until then, but might unassign after that depending on motivation for refactoring the rest.

[ThinkChaos]

I like the original proposal, but ran into some ambiguity: if we add dns: ports: unix:, it's not clear what protocol should be sent over the Unix socket. For instance it could be DoH, or plain DNS.

I think we should add an optional transport layer to the config to solve that:

services:
  dns:
    expose: # instead of ports since you can specify an IP/hostname
      # Short version uses the protocol's default transport(s)
      plain: 53 # TCP+UDP
      http: localhost:80 # TCP, single value
      https: # TCP, YAML native list to replace splitting on `,`
        - 443
        - localhost:4430

      # Explicit transport(s)
      http:
        unix: /run/blocky/http.sock
      https:
        tcp: 443
        unix: /run/blocky/https.sock

  # Same thing applies to other services
  # Though not all services support all protocols
  metrics:
    expose:
      # `plain:` would be invalid in this context
      http:
        unix: /run/blocky/http.sock
      https: 443

    path: /mymetrics # support for extra config is the same as initial proposal

What do you think of this services format?

What do you think of the expose name?
Other ideas: addrs, listen (didn't go for this since Go uses Listener for TCP/streams only, UDP/datagrams are PacketConn, but I'm not really against it)

[ThinkChaos]

Also maybe we should do without services?
It's pretty general so thinking about it more I'm not sure how we'd distinguish what goes under x: ... vs just x:.
And one less level of nesting is always welcome :smile:

# DNS is just at the root
expose: # or addrs/listen/whatever
  plain: 53

metrics:
  expose:
    http: 80
  path: /mymetrics

[kwitsch]

I like the idea of all outbound accessibility in one config section. So I'm still in favor of the services section.

Multiple mappings could be configured if expose is an array of options containing an identifier and where to map it.

Alternative layout combining services, exposed and less nesting:

services:
  dns:
    expose:
      - plain: 53
      - plain: /run/blocky/dns.sock
      - tls: 853
      - https: 443
    rate-limit: 60 # just example for further config
  metrics:
    expose:
       - http: 4001
       - http: 172.16.0.2:4000
       - http: /run/blocky/metrics.sock
       - https: 443 
    path: /mymetrics # moved from prometheus.path
  control-api:
    expose:
       - http: localhost:3000
       - http: /run/blocky/control.sock
    api-key: ASBC123 # just example for further config
  pprof:
    expose:
       - http: 12345

[ThinkChaos]

I think the key idea there is putting the info unix:, tcp:, etc. provide into the address type/value:

• /a/path implies unix:
• 53 and localhost:53 imply tcp: and/or udp: (depending on the context)

That seems more user friendly to me!
I do see a couple downsides:

• we need to come up with unambiguous formats if we want to add things in the future. I don't think there's too many we'd need to add

  • Systemd was already requested (#1119)
  • and a path implying Unix doesn't explicit if it's a Unix TCP like sock (AF_STREAM) or UDP like (AF_DGRAM) (ref to link the issues #1118)

• it's not possible to express things like "plain DNS, but only TCP", which I'm not really concerned with, just want to note

Also, from a code perspective, I think pushing the lists down the hierarchy is best, as lists of heterogeneous types is awkward in Go.
But that's more a general remark, we can do that with your proposal:

expose:
  - plain: 53
  - plain: /run/blocky/dns.sock
  - tls: 853
  # becomes
  plain:
    - 53
    - /run/blocky/dns.sock
  tls: 853 # if we allow the short version

I like that better because it lets us use a struct per service, where only the supported protocols can be set and we need to do less manual checks.
Obviously we can have common types to avoid repeating all the unmarshaling logic.

Anyways, I think we can combine your idea with mine to get the best of both worlds:

• use the idea of "a value implies a transport" to make the "short version" more powerful Most users would just need this format
• use the long version to avoid needing "unambiguous formats" for Systemd or AF_STREAM vs AF_DGRAM

dns:
  expose:
    # Short version now guesses the right transport
    plain: 53 # TCP+UDP
    # or
    plain: /run/blocky/dns.sock # Unix (we can decide it's a stream by default for plain DNS)
    # or
    plain:
      - 53
      - /run/blocky/dns.sock

    # Long version is required for some values:
    plain:
      systemd: file-descriptor-name
      unix-datagram: /run/blocky/dns.sock
    # or
    plain:
      systemd:
        - file-descriptor-a
        - file-descriptor-b

I think that shouldn't be hard to achieve by composing the UnmarshalYAML pattern where we first try one type, and then another.