Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add draft/bouncer-networks #478

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

emersion
Copy link
Contributor

@emersion emersion commented Dec 1, 2021

This is based on the vendored soju.im/bouncer-networks extension.

  • Consider using irc:// URLs instead of host/port attrs
  • Consider adding a way for servers to advertise supported network attrs
  • Consider adding an enabled attr
  • Consider adding a way for network notifications to remove attributes (e.g. BOUNCER NETWORK b33f -host,-port)
  • More attributes
  • More errors

cc @DarthGandalf @ItsOnlyBinary

@Herringway
Copy link

iirc there is a written spec for irc:// urls but I don't think it matches the de facto standard

@progval
Copy link
Contributor

progval commented Dec 1, 2021

discussion here: ircdocs/modern-irc#6

@emersion
Copy link
Contributor Author

emersion commented Dec 2, 2021

Well, I think we also don't want to allow all irc:// URLs. We probably only want to allow host/port and irc/ircs, nothing else (no user/password, no target, no query params, etc).

Additionally I'm not a fan of allowing irc:// (as opposed to ircs://). irc:// makes it extremely easy for clients to setup plain-text connections. In other words, setting up TLS requires extra effort (paying attention to use the ircs:// variant instead of the widely used irc:// variant). I'd rather have it the other way around (TLS by default, and explicit extra effort needed to setup plain-text connections).

@hhirtz
Copy link

hhirtz commented Dec 3, 2021

Server implementations:

Client implementations:

@ItsOnlyBinary
Copy link

  • Consider using irc:// URLs instead of host/port attrs
    seems this is just more parsing and no added benefit, currently all the needed data can be obtained using the same code as message-tags

  • Consider adding a way for servers to advertise supported network attrs
    this could possibly be handy for custom attrs, could also include a bouncer name/type/version kinda thing if the client wanted to support special cases/features for a particular bouncer
    maybe a BOUNCER_VERSION= RPL_ISUPPORT

  • Consider adding an enabled attr
    this could be useful

  • Consider adding a way for network notifications to remove attributes (e.g. BOUNCER NETWORK b33f -host,-port)
    surely if bouncer-notify it could just resend the whole network configuration on changes, as most attributes would change not just be removed

  • More attributes
    guess this falls under advertising supported attrs

  • More errors
    not sure on what extra errors could be included
    I do think the error format should be changed to start with BOUNCER though so it makes it easier to quickly decide if its something that needs processing by the BOUNCER code or passing down the event queue.
    the current FAIL BOUNCER does not feel consistent, and just adds more complexity to the code having to check two params if the first one is FAIL

@emersion
Copy link
Contributor Author

emersion commented Dec 3, 2021

could also include a bouncer name/type/version kinda thing if the client wanted to support special cases/features for a particular bouncer

I'd prefer to avoid version checks, and instead use feature detection. ie. enable $feature the bouncer advertises kiwiirc.com/asdf instead of checking for BOUNCER_VERSION >= x.y.z.

surely if bouncer-notify it could just resend the whole network configuration on changes, as most attributes would change not just be removed

It depends how many attributes we'll end up with I guess. Sending all the data always may not scale well.

I do think the error format should be changed to start with BOUNCER though so it makes it easier to quickly decide if its something that needs processing by the BOUNCER code or passing down the event queue.
the current FAIL BOUNCER does not feel consistent, and just adds more complexity to the code having to check two params if the first one is FAIL

Nack, the current spec uses standard replies.

@emersion
Copy link
Contributor Author

emersion commented Dec 8, 2021

It would also be nice to have a way for the bouncer to send a plain-text status message for each network. This would allow clients to show in their UI the reason why a network is disconnected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants