Page MenuHomePhabricator

Add `notification.bind-uri` to configure Aphlict server
AbandonedPublic

Authored by joshuaspence on Jan 13 2015, 7:41 AM.
Tags
None
Referenced Files
F13179237: D11359.diff
Wed, May 8, 9:03 PM
Unknown Object (File)
Sat, May 4, 6:29 PM
Unknown Object (File)
Thu, May 2, 5:12 AM
Unknown Object (File)
Wed, Apr 24, 11:21 PM
Unknown Object (File)
Fri, Apr 19, 2:14 AM
Unknown Object (File)
Thu, Apr 11, 7:52 AM
Unknown Object (File)
Apr 3 2024, 9:33 PM
Unknown Object (File)
Apr 1 2024, 3:08 PM
Subscribers

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary

Originally (in D11288) --client-host and --client-port options were added to the Aphlict server. The intention was to reduce the amount of configuration required to use the Aphlict server. I feel that moving this flags into configuration would be easier to manage.

Test Plan

Started the Aphlict server with ./bin/aphlict debug. Set notification.bind-uri and repeat.

Diff Detail

Repository
rP Phabricator
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3894
Build 3906: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence retitled this revision from to Add `notification.bind-uri` to configure Aphlict server.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.

@epriestley, this diff isn't finished yet but let me know if you'd be likely to accept this diff before I finish it off. Basically, the proposition is to add notification.client-bind-uri and notification.adminbind-uri config options.

I'm very hesitant to introduce this because this setup seems very niche to me: essentially, it seems like you add a bunch of nginx configuration to save yourself a little bit of SSL configuration. I suspect no one else will ever configure things this way, and don't want to add configuration which is used by only one install.

In the Phacility cluster where we have more than one server, it's probably easier to use flags than try to change the config: we can pass flag values easily and clearly when starting servers, but changing config from there is a mess.

I can understand your concerns and I'm happy to live with passing explicit flags to ./bin/aphlict start. I don't think that simplifying SSL configuration is the only argument in favor of proxying Aphlict WebSockets through nginx however. My main motivations were:

  • Less mucking around with AWS security groups.
  • Less mucking around with firewalls.
  • Easier to later scale the Aphlict server to span multiple hosts (T6915).

Maybe we should think about moving Aphlict most config to a separate file? Phabricator needs to know about enabled, client-uri and server-uri but not any of the other stuff (SSL, log, PID) or this stuff (client and admin bind addresses) and is probably not a good way to do more advanced configuration.

T6915 implies multiple different servers running with different configuration. T7012 likely implies some more config.

One thing that's nice about this approach is that it's hard for the client and server to be configured incompatibly, but we could still check that on startup and raise a warning if things looked suspicious.

What's the benefit of moving it to a separate file? Is the main concern you have that it pollutes the UI? If so, couldn't we just hide it?

Yes, my main concern is that we have >200 config settings. I don't want to add new settings which only a handful of installs are likely to use. I think hiding settings is an anti-solution -- it's worse to have 150 config settings and 50 top-secret hidden config settings than 200 config settings.

We have reasonable plans to get rid of a lot of the config settings in the future (like T5952, T6030, T5315). Moving to a config file would let us side-grade some of these settings away, which I think is a small net improvement. Broadly, in many cases the only way we can get rid of a lot of settings is by moving them to a contextual UI, which is sort of not really getting rid of them, but is often a big improvement in the actual user experience. For instance, the new application email management stuff from T5952 is more powerful and easier to use than having a bunch of different metamta.<app>.public-create-email settings.

Moving things to a separate file is likely only a major advantage in a multi-server situation (T6915). If multiple nodes need different config, and the config is stored in the Phabricator central config, it probably all needs to be maps from servers to their configuration -- so to see the configuration for aphlict003.company.com, you'd have to go pop open a bunch of notification.<thing> options and check the aphlict003 key in them. That seems worse than opening aphlict003.json somewhere.

We could also use Almanac, and that's likely the option which makes the most sense in the Phacility cluster, but it's probably not ready for production use.

Overall:

  • I'm strongly resistant to adding new configuration.
  • I'm fine with adding more flags to bin/aphlict to do weird edge-casey things in the short term.
  • In the long term, I imagine cluster support means the configuration for complex cases either moves to Almanac or config files, since those options make more sense for multi-host configurations.