Add option to allow Fabio to register frontend services in Consul on behalf of user services #426

rileyje · 2018-01-24T21:01:08Z

We would like to have Fabio re-register itself in Consul under user-specified names, so that users can access their services through Fabio via Consul-provided A records. The motivation for this is:

Users can immediately access their service "foo" directly at foo.service.example.com using Consul DNS
Users do not need to create CNAMES to Fabio for their service in an external DNS system, e.g. foo.example.com IN CNAME fabio.service.example.com
The service will be reachable via a DNS health-checked A record so that Kerberos auth in browsers will work correctly with a Kerberos principal unique to the user's service name. Currently this fails if the service is accessed via a CNAME to Fabio, unless principals are made too generically to match (e.g.) fabio.service.example.com.

The intended usage is that users will give their service a backend-specific name, and pass to Fabio the name they wish to use for the front end via a new "alias" option. For example:

    "ServiceName": "foo-backend",
    "ServiceTags": [
        "urlprefix-foo.service.example.com/ alias=foo",
        "urlprefix-foo/"
    ],

Fabio then registers new service foo.service.example.com with its own host and port information and creates routes "foo.service.example.com/" and "foo/" to foo-backend.service.example.com as usual.

magiconair

I think this is a cool idea. I would structure the implementation a bit different and keep the parsing code together. Also, I wouldn't distinguish between registering and updating registrations since the update call does all the heavy lifting and register then becomes a special case.

I'm not sure if alias is a good name for the option since it is quite generic and it isn't clear what this is an alias for. Maybe register?

magiconair · 2018-02-01T08:15:32Z

registry/backend.go

@@ -2,10 +2,13 @@ package registry

 type Backend interface {
 	// Register registers fabio as a service in the registry.
-	Register() error
+	Register(serviceName string) error


I think a better approach would be to just extend the register function since you need to keep the state of what is registered in the backend anyway.

type Backend interface { Register(name ...string) error DeregisterAll() error ... }

magiconair · 2018-02-01T08:16:04Z

registry/consul/backend.go

@@ -36,26 +37,47 @@ func NewBackend(cfg *config.Consul) (registry.Backend, error) {
 	return &be{c: c, dc: dc, cfg: cfg}, nil
 }

-func (b *be) Register() error {
-	if !b.cfg.Register {
+func (b *be) Register(serviceName string) error {


Then here you first unregister names that no longer exist and register new names.

magiconair · 2018-02-01T08:16:25Z

registry/consul/backend.go

-	if b.dereg != nil {
-		b.dereg <- true // trigger deregistration
-		<-b.dereg       // wait for completion
+func (b *be) Deregister(serviceName string) error {


This function then deregisters all currently registered services.

magiconair · 2018-02-01T08:17:43Z

registry/consul/backend.go

@@ -100,6 +122,54 @@ func (b *be) WatchNoRouteHTML() chan string {
 	return html
 }

+func (b *be) UpdateAliases(config string) error {


Move the parsing code into the route/parse_new.go since there you already have all the parse helpers. Return a list of alias names to register, e.g.

func ParseAliases(config string) (names []string, error) {...}

magiconair · 2018-02-01T08:19:10Z

main.go

@@ -396,6 +397,8 @@ func watchBackend(cfg *config.Config, first chan bool) {
 			continue
 		}

+		registry.Default.UpdateAliases(next)


Then here you call

aliases, err := route.ParseAliases(config) registry.Default.Register(aliases)

rileyje · 2018-02-03T00:36:23Z

Thanks @magiconair. I tried to address your feedback with these changes:

removed UpdateAliases() and moved parse code to ParseAliases() in route/parse_new.go. Implements a subset of Parse(), so there is some redundancy. Better to just call Parse()?. Had to import config to check if fabio should register itself. Can't get that from the private config member of the backend. Brought in log for some visibility.
Added DeregisterAll(), but kept Deregister() for single deregistrations. I wasn't sure if that's what you had in mind.
Extended Register() to handle removal and addition of aliases.
Changed option name to register (much better :)
Still tracking registered services in b.dereg map. Is that OK? Wasn't clear on "you need to keep the state of what is registered in the backend anyway"

I had a couple of questions while working on this:

Would you be OK with adding "DeregisterCriticalServiceAfter" to registrations so that if fabio exits uncleanly, Consul will clean up services? That was added in Consul 0.7.0, not sure if you have a minimum support Consul version or we could check version with the API before including that option.
Any concerns about duplicate health checks? Would be nice if Consul allowed you to specify an existing health check when registering a service. I can open a feature request against Consul.
Do you have any thoughts on test coverage for Register() and Deregister(All)()?

I see the Travis CI build failed, I'll look in to that.

aaronhurt · 2018-02-03T15:02:57Z

You may be able to register existing services and checks with CatalogRegistration but I'm not sure that has the intended result. I'll check locally and see how it works.

Update: No, that does not have the intended effect.

magiconair

I've added some more comments, some on style and some on dependencies. The Commands const in https://github.com/fabiolb/fabio/blob/master/route/parse_new.go#L19-L58 needs to be updated to contain the new option.

Also, could you please update the docs? I've just noticed that I forgot to migrate https://github.com/fabiolb/fabio/wiki/Routing to the new website. I'll do that now and that's where the new option should go.

Update: added https://github.com/fabiolb/fabio/blob/master/docs/content/cfg/_index.md

magiconair · 2018-02-04T14:11:21Z

registry/consul/backend.go

-		b.dereg <- true // trigger deregistration
-		<-b.dereg       // wait for completion
+func (b *be) Deregister(serviceName string) error {
+	if len(serviceName) == 0 {


Since DeregisterAll() exists I wouldn't use a magic value "" to deregister all services. Every now and then I am tempted to do that but then opt for simplicity. Deregister(service) deregisters a service if it is currently registered, otherwise it does nothing. DeregisterAll() deregisters all currently registered services.

magiconair · 2018-02-04T14:13:11Z

registry/consul/backend.go

+	// register new service names
+	for _, serviceName := range services {
+		if _, ok := b.dereg[serviceName]; ok {
+			log.Printf("[DEBUG]: %q already registered (appears in dereg map)\n", serviceName)


pls drop the text in (...) since that is an implementation detail.

magiconair · 2018-02-04T14:15:18Z

registry/consul/backend.go

+func (b *be) DeregisterAll() error {
+	log.Printf("[DEBUG]: consul: Deregistering all registered Fabio aliases.")
+	for name, dereg := range b.dereg {
+		if dereg != nil {


I prefer less indents

for name, dereg := range b.dereg { if dereg == nil { continue } ... }

magiconair · 2018-02-04T14:16:58Z

registry/consul/backend.go

+	}
+
+	// register new service names
+	for _, serviceName := range services {


less indents

for _, name := range services { if b.dereg[name] == nil { continue } ... }

magiconair · 2018-02-04T14:17:31Z

registry/consul/register.go

 			return ""
 		}

+		log.Printf("[INFO] consul: Registered fabio with name %q", service.Name)


Registered fabio as %q

magiconair · 2018-02-04T14:18:46Z

route/parse_new.go

@@ -87,6 +90,42 @@ func Parse(in string) (defs []*RouteDef, err error) {
 	return defs, nil
 }

+func ParseAliases(in string, cfg *config.Config) (names []string, err error) {


Pls add a comment on what this function does.

magiconair · 2018-02-04T14:19:52Z

route/parse_new.go

+
+	var aliases []string
+
+	if cfg.Registry.Backend == "consul" && cfg.Registry.Consul.Register {


Why is that important here? I'd just parse the aliases and let the backend decide what to do with them.

magiconair · 2018-02-04T14:20:54Z

route/parse_new.go

+	for _, d := range defs {
+		registerName, ok := d.Opts["register"]
+		if ok {
+			log.Printf("[DEBUG]: consul: Service %q wants alias %q", d.Service, registerName)


I don't think we should log that here.

magiconair · 2018-02-04T14:24:16Z

I think adding DeregisterCriticalServiceAfter with a sane default makes sense. Can you please add that as a separate commit within this PR?

magiconair · 2018-02-04T14:26:37Z

Ah, and please add some tests to https://github.com/fabiolb/fabio/blob/master/route/parse_test.go

This patch adds a "register=<name>" option which tells Fabio to register a new frontend service in Consul on behalf of the requesting backend service. The new service provides Consul-hosted A records to Fabio with the desired service name, provided as the <name> argument to "register". Fabio will deregister front end services when the backend services leave, and will deregister all Fabio-registered services on exit.

Consul can automatically deregister services if the health check is critical for a given amount of time. This is useful if fabio exits abnormally without deregistering services.

rileyje · 2018-02-07T03:20:27Z

@magiconair here is the latest version of the patch.

Updated Commands const to included register. I noticed that host was missing and took the liberty of adding it.
Updated docs to include register option.
Removed config check from ParseAliases() and moved it to Consul backend
Added comment for ParseAliases()
Added tests for ParseAliases()
Removed unneeded magic value from Deregister()
Factored out excess indents
Cleaned up logging statements
Added DeregisterCriticalServiceAfter as a separate commit. Used the 90m default from Consul docs. Not sure if that fits your definition of sane :) We will likely set it to something much shorter in production.

magiconair · 2018-02-07T09:19:21Z

I think this is good enough as is. Thanks a lot!

rileyje · 2018-02-07T14:27:29Z

Great, thank you!

rileyje force-pushed the add-alias-option branch from 46c2cec to 760a15e Compare January 24, 2018 21:09

magiconair reviewed Feb 1, 2018

View reviewed changes

rileyje force-pushed the add-alias-option branch from 760a15e to 4823267 Compare February 3, 2018 00:27

magiconair reviewed Feb 4, 2018

View reviewed changes

rileyje added 2 commits February 6, 2018 18:02

Add DeregisterCriticalServiceAfter to Consul checks

9ece33a

Consul can automatically deregister services if the health check is critical for a given amount of time. This is useful if fabio exits abnormally without deregistering services.

rileyje force-pushed the add-alias-option branch from 4823267 to 9ece33a Compare February 7, 2018 03:13

magiconair merged commit cb5de44 into fabiolb:master Feb 7, 2018

magiconair added this to the 1.5.8 milestone Feb 7, 2018

pires mentioned this pull request Jun 11, 2019

fabio service entries may stay in Consul on dirty exit #663

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to allow Fabio to register frontend services in Consul on behalf of user services #426

Add option to allow Fabio to register frontend services in Consul on behalf of user services #426

rileyje commented Jan 24, 2018

magiconair left a comment

magiconair Feb 1, 2018

magiconair Feb 1, 2018

magiconair Feb 1, 2018

magiconair Feb 1, 2018

magiconair Feb 1, 2018

rileyje commented Feb 3, 2018

aaronhurt commented Feb 3, 2018 •

edited

Loading

magiconair left a comment •

edited

Loading

magiconair Feb 4, 2018

magiconair Feb 4, 2018

magiconair Feb 4, 2018

magiconair Feb 4, 2018

magiconair Feb 4, 2018

magiconair Feb 4, 2018

magiconair Feb 4, 2018

magiconair Feb 4, 2018

magiconair commented Feb 4, 2018

magiconair commented Feb 4, 2018

rileyje commented Feb 7, 2018

magiconair commented Feb 7, 2018

rileyje commented Feb 7, 2018


		var aliases []string

		if cfg.Registry.Backend == "consul" && cfg.Registry.Consul.Register {

Add option to allow Fabio to register frontend services in Consul on behalf of user services #426

Add option to allow Fabio to register frontend services in Consul on behalf of user services #426

Conversation

rileyje commented Jan 24, 2018

magiconair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rileyje commented Feb 3, 2018

aaronhurt commented Feb 3, 2018 • edited Loading

magiconair left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiconair commented Feb 4, 2018

magiconair commented Feb 4, 2018

rileyje commented Feb 7, 2018

magiconair commented Feb 7, 2018

rileyje commented Feb 7, 2018

aaronhurt commented Feb 3, 2018 •

edited

Loading

magiconair left a comment •

edited

Loading