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

support consul service health checks #102

Closed
wants to merge 3 commits into
base: 0.6.x
from

Conversation

Projects
None yet
9 participants
@johnrengelman
Contributor

johnrengelman commented Jul 14, 2014

Loads Consul service data into /_consul/service namespace. (#100)

Format for keys is:

/_consul/service/<serviceName>[/<tagName>]/<nodeIndex>

Consul datacenters are not supported yet. tagName is optional. Both sets of keys are generated.
Consul data is loaded as a JSON marshalled String as the Value in the keystore.
Using data in template requires unmarshalling the data using the new json or jsonArray functions bounds to the template.

Additionally, added some convenience methods parent and sibling to the template for easier navigation around the KV store.

Note: this is my 1st attempt at Go, so feel free to criticize style/naming/etc. I tried to follow standard where it was apparent.

Supporting change at: kelseyhightower/memkv#1

@@ -0,0 +1 @@
confd

This comment has been minimized.

@carlosdp

carlosdp Jul 18, 2014

idk how the maintainers feel about this, but it isn't really necessary if you use go install instead of go build. That will build the binary and then place it in $GOPATH/bin, which should be in your $PATH.

@carlosdp

carlosdp Jul 18, 2014

idk how the maintainers feel about this, but it isn't really necessary if you use go install instead of go build. That will build the binary and then place it in $GOPATH/bin, which should be in your $PATH.

This comment has been minimized.

@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

We should move this to a different commit.

@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

We should move this to a different commit.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Jul 22, 2014

Owner

Thanks for the PR, I'm be speaking at events related to OSCON, and I'll get a chance to review this later in the week.

Owner

kelseyhightower commented Jul 22, 2014

Thanks for the PR, I'm be speaking at events related to OSCON, and I'll get a chance to review this later in the week.

@carlosdp

This comment has been minimized.

Show comment
Hide comment
@carlosdp

carlosdp Jul 31, 2014

👍 pretty damn good first attempt 😄

One comment though: You're going to want to run go fmt ./... in your repo directory so that the code is properly formatted. Other than that, it looks good to me from an outsider perspective! Very excited to have this support in confd.

carlosdp commented Jul 31, 2014

👍 pretty damn good first attempt 😄

One comment though: You're going to want to run go fmt ./... in your repo directory so that the code is properly formatted. Other than that, it looks good to me from an outsider perspective! Very excited to have this support in confd.

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Jul 31, 2014

Contributor

thanks for the comments. I've pushed the format changes.

Contributor

johnrengelman commented Jul 31, 2014

thanks for the comments. I've pushed the format changes.

@mohitarora

This comment has been minimized.

Show comment
Hide comment
@mohitarora

mohitarora Aug 1, 2014

@kelseyhightower - Any update on this PR. It will be great if we have this functionality.

mohitarora commented Aug 1, 2014

@kelseyhightower - Any update on this PR. It will be great if we have this functionality.

@jetadmin

This comment has been minimized.

Show comment
Hide comment
@jetadmin

jetadmin Aug 4, 2014

@johnrengelman - This is a pretty important feature that we need. We don't want to do the work again. I guess author is not merging it anytime soon. Can you please publish the binary somewhere and share the link?

jetadmin commented Aug 4, 2014

@johnrengelman - This is a pretty important feature that we need. We don't want to do the work again. I guess author is not merging it anytime soon. Can you please publish the binary somewhere and share the link?

Show outdated Hide outdated backends/consul/client.go
service_vars, _ := getServicesHealth(*c, services)
for k, v := range service_vars {
vars[k] = v
}

This comment has been minimized.

@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

Will there every be conflicts between service names and key names?

@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

Will there every be conflicts between service names and key names?

This comment has been minimized.

@johnrengelman

johnrengelman Aug 4, 2014

Contributor

Potentially. I don't think there is a way to protect against that unless we move all the KV entries into it's own namespace internal to confd.

That's why I selected to prefix the service data with _consul.

@johnrengelman

johnrengelman Aug 4, 2014

Contributor

Potentially. I don't think there is a way to protect against that unless we move all the KV entries into it's own namespace internal to confd.

That's why I selected to prefix the service data with _consul.

Show outdated Hide outdated resource/template/resource.go
tplFuncMap["get"] = t.store.Get
tplFuncMap["gets"] = t.store.GetAll
tplFuncMap["getv"] = t.store.GetValue
tplFuncMap["getvs"] = t.store.GetAllValues
tplFuncMap["json"] = t.UnmarshalJsonObject
tplFuncMap["jsonArray"] = t.UnmarshalJsonArray

This comment has been minimized.

@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

Lets move adding the additional functions to a separate PR. This will allow us to combine other PRs that want the same set of functions. Looks good by the way.

@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

Lets move adding the additional functions to a separate PR. This will allow us to combine other PRs that want the same set of functions. Looks good by the way.

This comment has been minimized.

@johnrengelman

johnrengelman Aug 4, 2014

Contributor

Looks like someone else already split them out into a different PR: #103

@johnrengelman

johnrengelman Aug 4, 2014

Contributor

Looks like someone else already split them out into a different PR: #103

Show outdated Hide outdated backends/consul/client.go
@@ -38,5 +47,53 @@ func (c *Client) GetValues(keys []string) (map[string]string, error) {
vars[path.Join("/", p.Key)] = string(p.Value)
}
}
services, _ := listServices(*c)
service_vars, _ := getServicesHealth(*c, services)

This comment has been minimized.

@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

Does this mean we only grab healthy services?

@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

Does this mean we only grab healthy services?

This comment has been minimized.

@johnrengelman

johnrengelman Aug 4, 2014

Contributor

You can get all services from consul, but I chose to only get healthy. I could be swayed differently though. The actual call that dictates this is at Line 77.

@johnrengelman

johnrengelman Aug 4, 2014

Contributor

You can get all services from consul, but I chose to only get healthy. I could be swayed differently though. The actual call that dictates this is at Line 77.

Show outdated Hide outdated backends/consul/client.go
// /_consul/service/<service_name>/<idx>/address
// /_consul/service/<service_name>/<idx>/port
// /_consul/service/<service_name>/<tag>/<idx>/...
// <tag>.<service>.service.<datacenter>.<domain>

This comment has been minimized.

@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

Add a comment about what getServicesHealth is doing. What it returns and why.

@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

Add a comment about what getServicesHealth is doing. What it returns and why.

Show outdated Hide outdated integration/confdir/conf.d/service.toml
src = "service.conf.tmpl"
dest = "/tmp/confd-service-test.conf"
keys = [
"/_consul",

This comment has been minimized.

@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

Yay, seems this just becomes another key to watch for.

@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

Yay, seems this just becomes another key to watch for.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

@johnrengelman Ok, I did a review and everything looks good so far. Please do your best to address the concerns I've highlighted. Don't worry if you can't fix everything, I'm happy to help out as well.

For those that want to see this get merged please test this out. My plan is to merge this feature into 0.6.x today if the changes are made and I'll cut a release for testing.

Owner

kelseyhightower commented Aug 4, 2014

@johnrengelman Ok, I did a review and everything looks good so far. Please do your best to address the concerns I've highlighted. Don't worry if you can't fix everything, I'm happy to help out as well.

For those that want to see this get merged please test this out. My plan is to merge this feature into 0.6.x today if the changes are made and I'll cut a release for testing.

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 4, 2014

Contributor

Added function comments describing behavior and cleaned up a couple lines that were not doing anything.

Contributor

johnrengelman commented Aug 4, 2014

Added function comments describing behavior and cleaned up a couple lines that were not doing anything.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

@johnrengelman Ok, lets just move the out the json functions then I'll get this merged. Then I'll turn my attention to getting the json functions merged in, which will provide the complete solution.

Owner

kelseyhightower commented Aug 4, 2014

@johnrengelman Ok, lets just move the out the json functions then I'll get this merged. Then I'll turn my attention to getting the json functions merged in, which will provide the complete solution.

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 4, 2014

Contributor

Will do. Can you also take a look at kelseyhightower/memkv#1 and see what you think?
I needed this change because I ran into issues if there were no healthy servers in Consul then my templating would error because there were no keys. So instead, I changed it to return an empty list, which I think makes more sense.

Contributor

johnrengelman commented Aug 4, 2014

Will do. Can you also take a look at kelseyhightower/memkv#1 and see what you think?
I needed this change because I ran into issues if there were no healthy servers in Consul then my templating would error because there were no keys. So instead, I changed it to return an empty list, which I think makes more sense.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

I'm a bit torn about this feature. Mainly because we are trying to turn services into K/V pairs. I have a few questions that others can answer to help me understand.

Why not have this type of functionality live outside of confd?

It seems we are performing health checks on services, and if they pass using the service info as a value. I've seen something like this done for etcd as well. In that case the result of health checks updated K/V pairs in etcd. The nice thing about that approach it keeps confd simple and focused.

Owner

kelseyhightower commented Aug 4, 2014

I'm a bit torn about this feature. Mainly because we are trying to turn services into K/V pairs. I have a few questions that others can answer to help me understand.

Why not have this type of functionality live outside of confd?

It seems we are performing health checks on services, and if they pass using the service info as a value. I've seen something like this done for etcd as well. In that case the result of health checks updated K/V pairs in etcd. The nice thing about that approach it keeps confd simple and focused.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

@johnrengelman Do you want to jump on IRC and discuss this a bit further?

Owner

kelseyhightower commented Aug 4, 2014

@johnrengelman Do you want to jump on IRC and discuss this a bit further?

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 4, 2014

Contributor

Sure. Where?

Contributor

johnrengelman commented Aug 4, 2014

Sure. Where?

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower
Owner

kelseyhightower commented Aug 4, 2014

@johnrengelman #confd freenode

@carlosdp

This comment has been minimized.

Show comment
Hide comment
@carlosdp

carlosdp Aug 4, 2014

Consul Services are basically health-check aware / smart K/V pairs. Essentially, Consul is updating the K/V for us while etcd needs an outside component. Implementing something outside confd would just add extra complexity or end up re-writing confd, but just for Consul.

carlosdp commented Aug 4, 2014

Consul Services are basically health-check aware / smart K/V pairs. Essentially, Consul is updating the K/V for us while etcd needs an outside component. Implementing something outside confd would just add extra complexity or end up re-writing confd, but just for Consul.

@mohitarora

This comment has been minimized.

Show comment
Hide comment
@mohitarora

mohitarora Aug 4, 2014

+1 on comment by @carlosdp

Even though services concept is only present in consul not etcd but confd should have it for sure.

mohitarora commented Aug 4, 2014

+1 on comment by @carlosdp

Even though services concept is only present in consul not etcd but confd should have it for sure.

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 4, 2014

Contributor

After discussing with @kelseyhightower and @carlosdp the plan forward is to keep confd itself small and support 4 core backends:

  • etcd
  • consul K/V
  • env vars
  • custom (tcp/unix socket)

The custom backend will use either tcp or unix file sockets to connect confd to another process that can supply the backend implementation of your choosing. The Consul Service API backend will be developed here: https://github.com/johnrengelman/confd-consul-sd

While this does make the implementation slightly more complicated (i.e. it requires an intermediate process instead of talking directly to consul), it will allow confd to say thinner and more abstract to the various concepts from the various application a user may want to integrate with. Additionally, the custom backend scheme will allow new protocols or implementations with specific business logic to be develop and utilized without changes to confd core.

Contributor

johnrengelman commented Aug 4, 2014

After discussing with @kelseyhightower and @carlosdp the plan forward is to keep confd itself small and support 4 core backends:

  • etcd
  • consul K/V
  • env vars
  • custom (tcp/unix socket)

The custom backend will use either tcp or unix file sockets to connect confd to another process that can supply the backend implementation of your choosing. The Consul Service API backend will be developed here: https://github.com/johnrengelman/confd-consul-sd

While this does make the implementation slightly more complicated (i.e. it requires an intermediate process instead of talking directly to consul), it will allow confd to say thinner and more abstract to the various concepts from the various application a user may want to integrate with. Additionally, the custom backend scheme will allow new protocols or implementations with specific business logic to be develop and utilized without changes to confd core.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 4, 2014

Owner

@johnrengelman I'm going to close this out and start thinking about that custom backend.

Owner

kelseyhightower commented Aug 4, 2014

@johnrengelman I'm going to close this out and start thinking about that custom backend.

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 4, 2014

Contributor

Sounds good.

Contributor

johnrengelman commented Aug 4, 2014

Sounds good.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 6, 2014

Owner

I'm going to reopen this as more people are asking for this feature.

Owner

kelseyhightower commented Aug 6, 2014

I'm going to reopen this as more people are asking for this feature.

@mohitarora

This comment has been minimized.

Show comment
Hide comment
@mohitarora

mohitarora Aug 6, 2014

+1 need this feature in confd

mohitarora commented Aug 6, 2014

+1 need this feature in confd

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 6, 2014

Owner

For the sake of the community we are going to do this.

Owner

kelseyhightower commented Aug 6, 2014

For the sake of the community we are going to do this.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 6, 2014

Owner

@johnrengelman We are going to do this. But I'm going to have to refactor this a little.

We need to pick a key space for services.

/consol/services
Owner

kelseyhightower commented Aug 6, 2014

@johnrengelman We are going to do this. But I'm going to have to refactor this a little.

We need to pick a key space for services.

/consol/services
@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 6, 2014

Contributor

Sure...it should be /consul/services though, with a 'u'. Currently the namespace is /_consul/services in the PR. I though the _ would be a good way to delineate that this is an auto-generate key not from the K/V store.

Also, I just realized that I don't respect the key prefix that is requested in the GetValues method, it's appending the service data to the filtered keys from the K/V and returning that always...so we'll want to filter the service data.

Contributor

johnrengelman commented Aug 6, 2014

Sure...it should be /consul/services though, with a 'u'. Currently the namespace is /_consul/services in the PR. I though the _ would be a good way to delineate that this is an auto-generate key not from the K/V store.

Also, I just realized that I don't respect the key prefix that is requested in the GetValues method, it's appending the service data to the filtered keys from the K/V and returning that always...so we'll want to filter the service data.

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 6, 2014

Contributor

Is the plan then, to include this in a core and then eventually implement the custom backend like we were discussing?

Contributor

johnrengelman commented Aug 6, 2014

Is the plan then, to include this in a core and then eventually implement the custom backend like we were discussing?

@jhmartin

This comment has been minimized.

Show comment
Hide comment
@jhmartin

jhmartin Aug 6, 2014

Contributor

Having confd expose access to the catalog / health apis makes Consul useful to a very large set of applications. It is not feasible to get every significant app (varnish, haproxy, pound, stunnel, so on) to have native Consul access -- but they all support configuration files. DNS resolution works in some cases, but some apps need to be able to enumerate the entire list of backends (including port #s, as this is not stable under a Docker environment).

Zookeeper implementations generally take the approach of 'native integration required'; that high bar means that pretty much only bespoke applications or a few open source apps written specifically for it can take advantage of it.

Samples:

  • Varnish needs to enumerate its list of backends so that it can perform its own healthchecks against it and properly do retry logic. A single DNS-based backend breaks the saint/grace mode logic.
  • A custom PHP app may need to enumerate the list of living memcached backends.

I think the Health and Catalog APIs should be exposed:

  • The Health api in Consul allows querying for all nodes in a given service, and optionally pass the ?passing option to filter that list to just living nodes.
  • The Catalog API in consul allows querying the list of known services, even if no health data is associated.

There's also the agent, status, and internal APIs but I see no reason confd would need to expose those.

I do like the tcp/socket option for totally custom backends, but I think etc/consul are 'first class' backends that shouldn't require it.

Contributor

jhmartin commented Aug 6, 2014

Having confd expose access to the catalog / health apis makes Consul useful to a very large set of applications. It is not feasible to get every significant app (varnish, haproxy, pound, stunnel, so on) to have native Consul access -- but they all support configuration files. DNS resolution works in some cases, but some apps need to be able to enumerate the entire list of backends (including port #s, as this is not stable under a Docker environment).

Zookeeper implementations generally take the approach of 'native integration required'; that high bar means that pretty much only bespoke applications or a few open source apps written specifically for it can take advantage of it.

Samples:

  • Varnish needs to enumerate its list of backends so that it can perform its own healthchecks against it and properly do retry logic. A single DNS-based backend breaks the saint/grace mode logic.
  • A custom PHP app may need to enumerate the list of living memcached backends.

I think the Health and Catalog APIs should be exposed:

  • The Health api in Consul allows querying for all nodes in a given service, and optionally pass the ?passing option to filter that list to just living nodes.
  • The Catalog API in consul allows querying the list of known services, even if no health data is associated.

There's also the agent, status, and internal APIs but I see no reason confd would need to expose those.

I do like the tcp/socket option for totally custom backends, but I think etc/consul are 'first class' backends that shouldn't require it.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 6, 2014

Owner

@johnrengelman It's going in Core! I may merge this and fix up some things later.

Owner

kelseyhightower commented Aug 6, 2014

@johnrengelman It's going in Core! I may merge this and fix up some things later.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 6, 2014

Owner

@johnrengelman I'll fix up the prefix bits, but I now wonder how will prefixes play into this. Can we filter services with prefixes? Or do we just expose all the services we get back from consul?

Owner

kelseyhightower commented Aug 6, 2014

@johnrengelman I'll fix up the prefix bits, but I now wonder how will prefixes play into this. Can we filter services with prefixes? Or do we just expose all the services we get back from consul?

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 6, 2014

Contributor

Ah, I see were you are going....How do you want to handle services that don't have tags or if I want to get all instances of a service regardless of tag?

Contributor

johnrengelman commented Aug 6, 2014

Ah, I see were you are going....How do you want to handle services that don't have tags or if I want to get all instances of a service regardless of tag?

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 6, 2014

Owner

@johnrengelman That's where all this gets tricky. Since we are outside of the k/v semantics it's like wild-wild west.

Owner

kelseyhightower commented Aug 6, 2014

@johnrengelman That's where all this gets tricky. Since we are outside of the k/v semantics it's like wild-wild west.

@carlosdp

This comment has been minimized.

Show comment
Hide comment
@carlosdp

carlosdp Aug 6, 2014

@kelseyhightower I think the format @johnrengelman proposed is ideal, /_consul/services/<service>/<tag>. Makes most sense in the Consul and K/V model.

carlosdp commented Aug 6, 2014

@kelseyhightower I think the format @johnrengelman proposed is ideal, /_consul/services/<service>/<tag>. Makes most sense in the Consul and K/V model.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 6, 2014

Owner

@carlosdp Then your configs will not be portable between environments. You'll have to hardcode the tag in the templates.

Owner

kelseyhightower commented Aug 6, 2014

@carlosdp Then your configs will not be portable between environments. You'll have to hardcode the tag in the templates.

@carlosdp

This comment has been minimized.

Show comment
Hide comment
@carlosdp

carlosdp Aug 6, 2014

@kelseyhightower couldn't you use an ENV variable as the tag?

carlosdp commented Aug 6, 2014

@kelseyhightower couldn't you use an ENV variable as the tag?

@kshep

This comment has been minimized.

Show comment
Hide comment
@kshep

kshep Aug 6, 2014

Since the tag is applied to the service (or node) in Consul, it seems odd to invert the hierarchy and assume the tags are being used to designate an environment. I think some people might actually be using datacenter to designate different environments... so... I can see /consul/services/<datacenter>/<service>/<tag> with multiple keys for each tag... but flipping it around makes my head hurt a bit.

kshep commented Aug 6, 2014

Since the tag is applied to the service (or node) in Consul, it seems odd to invert the hierarchy and assume the tags are being used to designate an environment. I think some people might actually be using datacenter to designate different environments... so... I can see /consul/services/<datacenter>/<service>/<tag> with multiple keys for each tag... but flipping it around makes my head hurt a bit.

@carlosdp

This comment has been minimized.

Show comment
Hide comment
@carlosdp

carlosdp commented Aug 6, 2014

@kshep 👍

@kshep

This comment has been minimized.

Show comment
Hide comment
@kshep

kshep Aug 6, 2014

Hmm... will at'ing @mitchellh or @armon automatically pull them into this?

kshep commented Aug 6, 2014

Hmm... will at'ing @mitchellh or @armon automatically pull them into this?

@jhmartin

This comment has been minimized.

Show comment
Hide comment
@jhmartin

jhmartin Aug 6, 2014

Contributor

Just to record this, a sample template directive that references a service httpd with tag master:

{{range getvs "/_consul/service/httpd/master/*"}}
  Host  {{ (json .).Node.Address }}
  Port  {{ (json .).Service.Port }}
{{end}}
Contributor

jhmartin commented Aug 6, 2014

Just to record this, a sample template directive that references a service httpd with tag master:

{{range getvs "/_consul/service/httpd/master/*"}}
  Host  {{ (json .).Node.Address }}
  Port  {{ (json .).Service.Port }}
{{end}}
@armon

This comment has been minimized.

Show comment
Hide comment
@armon

armon Aug 6, 2014

Contributor

Hey not sure what the question is exactly, but I think the proposed scheme is good. I think having suffix provide more granularity makes sense (e.g. the more that is provided, the more granular, so datacenter is first, then service, lastly tag).

Contributor

armon commented Aug 6, 2014

Hey not sure what the question is exactly, but I think the proposed scheme is good. I think having suffix provide more granularity makes sense (e.g. the more that is provided, the more granular, so datacenter is first, then service, lastly tag).

@jhmartin

This comment has been minimized.

Show comment
Hide comment
@jhmartin

jhmartin Aug 6, 2014

Contributor

Is there any provision here to look for services that both tags 'xyz' and 'qa'?

Contributor

jhmartin commented Aug 6, 2014

Is there any provision here to look for services that both tags 'xyz' and 'qa'?

@carlosdp

This comment has been minimized.

Show comment
Hide comment
@carlosdp

carlosdp Aug 6, 2014

@jhmartin no, but I think this should be taken one step at a time. Figure out multiple tags later.

carlosdp commented Aug 6, 2014

@jhmartin no, but I think this should be taken one step at a time. Figure out multiple tags later.

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 6, 2014

Contributor

I think datacenters are a slightly different beast than tags though. Reading the Consul API doc, when dealing with datacenters, you either query your local by default (by not specifying one) or you specify the datacenter to query information about...there isn't a mechanism for querying all datacenters at once.

Is there a use case for wanting confd to have access to all service information across all datacenters during a single template run?

I guess we can support that query in the template by doing something like:

{{range getvs "/_consul/services/*/httpd/*}}

@kelseyhightower does that work in the templates?

But perhaps we should structure it a bit differently:

{{range getvs "/_consul/dc/*/services/httpd/*"}}

Then if you wanted the results from all datacenters, we could drop the dc part of the query so we'd have the original format:

{{range getvs "/_consul/services/httpd/*"}}

Would that be more clear?

Contributor

johnrengelman commented Aug 6, 2014

I think datacenters are a slightly different beast than tags though. Reading the Consul API doc, when dealing with datacenters, you either query your local by default (by not specifying one) or you specify the datacenter to query information about...there isn't a mechanism for querying all datacenters at once.

Is there a use case for wanting confd to have access to all service information across all datacenters during a single template run?

I guess we can support that query in the template by doing something like:

{{range getvs "/_consul/services/*/httpd/*}}

@kelseyhightower does that work in the templates?

But perhaps we should structure it a bit differently:

{{range getvs "/_consul/dc/*/services/httpd/*"}}

Then if you wanted the results from all datacenters, we could drop the dc part of the query so we'd have the original format:

{{range getvs "/_consul/services/httpd/*"}}

Would that be more clear?

@carlosdp

This comment has been minimized.

Show comment
Hide comment
@carlosdp

carlosdp Aug 6, 2014

@johnrengelman I don't think we want to have all DCs be the default, the node's DC should be the default. I also don't really know of a use case where we want something from all DCs in a template. It's scope-creep imo.

carlosdp commented Aug 6, 2014

@johnrengelman I don't think we want to have all DCs be the default, the node's DC should be the default. I also don't really know of a use case where we want something from all DCs in a template. It's scope-creep imo.

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 6, 2014

Contributor

I agree. After typing all the up, I think it would be best to not support DCs at the moment and add support for them later by using a different key path (/_consul/dc/<dcName>/services/<serviceName>[/<tag>]/<index>), then a user could combine the data using nested ranges.

{{range getvs "/_consul/datacenters/*"}}
  {{range getvs fmt.Sprintf("/_consul/dc/%s/services/httpd/*", .Key)}}
  {{end}}
{{end}}
Contributor

johnrengelman commented Aug 6, 2014

I agree. After typing all the up, I think it would be best to not support DCs at the moment and add support for them later by using a different key path (/_consul/dc/<dcName>/services/<serviceName>[/<tag>]/<index>), then a user could combine the data using nested ranges.

{{range getvs "/_consul/datacenters/*"}}
  {{range getvs fmt.Sprintf("/_consul/dc/%s/services/httpd/*", .Key)}}
  {{end}}
{{end}}
@armon

This comment has been minimized.

Show comment
Hide comment
@armon

armon Aug 6, 2014

Contributor

I agree. Most use cases are around the local DC.

Contributor

armon commented Aug 6, 2014

I agree. Most use cases are around the local DC.

@mohitarora

This comment has been minimized.

Show comment
Hide comment
@mohitarora

mohitarora Aug 8, 2014

Any update on this? What's the final key path?

mohitarora commented Aug 8, 2014

Any update on this? What's the final key path?

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 8, 2014

Owner

@mohitarora We really need to sort out the key layout here, then we should be ready to go. I'm leaning on the consul users here to help work this out.

Owner

kelseyhightower commented Aug 8, 2014

@mohitarora We really need to sort out the key layout here, then we should be ready to go. I'm leaning on the consul users here to help work this out.

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 8, 2014

Contributor

So this is from the Consul documentation:
When querying for services using the DNS interface, the following format is used:

<tag>.<service>.service.<datacenter>.<domain>

where <tag> and <datacenter> are optional, if <tag> is omitted, than all instances of the service are returned, if <datacenter> is omitted, then the local datacenter is queried.

For DNS results, only healthy nodes (nodes not failing the service healthcheck) are returned.

When querying for services using the REST API, the endpoint /v1/catalog/service/<service>
returns all instances of a service (without health information)

[
    {
        "Node": "foobar",
        "Address": "10.1.10.12",
        "ServiceID": "redis",
        "ServiceName": "redis",
        "ServiceTags": null,
        "ServicePort": 8000
    }
]

while the endpoint /v1/health/service/<service> returns the service instances with health information

[
    {
        "Node": {
            "Node": "foobar",
            "Address": "10.1.10.12"
        },
        "Service": {
            "ID": "redis",
            "Service": "redis",
            "Tags": null,
            "Port": 8000
        },
        "Checks": [
            {
                "Node": "foobar",
                "CheckID": "service:redis",
                "Name": "Service 'redis' check",
                "Status": "passing",
                "Notes": "",
                "Output": "",
                "ServiceID": "redis",
                "ServiceName": "redis"
            },
            {
                "Node": "foobar",
                "CheckID": "serfHealth",
                "Name": "Serf Health Status",
                "Status": "passing",
                "Notes": "",
                "Output": "",
                "ServiceID": "",
                "ServiceName": ""
            }
        ]
    }
]

Both rest endpoints will filter for tags by appending ?tag= and by datacenter by appending ?dc= query parameters. If no ?tag= is provided, then no filtering occurs, if no ?dc= is provided, then the local datacenter is queried.

The /v1/health/service/<service> additionally takes a ?passing query parameter to return only healthy nodes.

Based on this, I think we should try to replicate the DNS interface behavior in returning only healthy services using the same DNS name but in reverse:

<domain>[/<datacenter>]/service/<service>[/<tag>]

Internally, we would use the consul client to query the health endpoint and only get healthy nodes (same as the DNS interface).

If the /<datacenter> portion is omitted, then the local datacenter is queried:

<domain>/service/<service>[/<tag>]

In Consul, it appears the is always consul, so we can just use that (or _consul to try to indicate that it is generated and not K/V data).

This basically gets us to exactly what this PR implements and is the logic I used when creating this PR.

So the basic endpoint would be:

/_consul/service/<service>[/<tag>]

and then we can add support later for datacenters using:

/_consul/<datacenter>/service/<service>[/tag]

If people feel we need to support getting all nodes instead of just healthy nodes, I think we should support that via a backend configuration option instead of through the key structure (default to only healthy nodes, but allow configuration to return all nodes). Then the user could do their own filtering because the health information would be in the JSON stored at the key.

Contributor

johnrengelman commented Aug 8, 2014

So this is from the Consul documentation:
When querying for services using the DNS interface, the following format is used:

<tag>.<service>.service.<datacenter>.<domain>

where <tag> and <datacenter> are optional, if <tag> is omitted, than all instances of the service are returned, if <datacenter> is omitted, then the local datacenter is queried.

For DNS results, only healthy nodes (nodes not failing the service healthcheck) are returned.

When querying for services using the REST API, the endpoint /v1/catalog/service/<service>
returns all instances of a service (without health information)

[
    {
        "Node": "foobar",
        "Address": "10.1.10.12",
        "ServiceID": "redis",
        "ServiceName": "redis",
        "ServiceTags": null,
        "ServicePort": 8000
    }
]

while the endpoint /v1/health/service/<service> returns the service instances with health information

[
    {
        "Node": {
            "Node": "foobar",
            "Address": "10.1.10.12"
        },
        "Service": {
            "ID": "redis",
            "Service": "redis",
            "Tags": null,
            "Port": 8000
        },
        "Checks": [
            {
                "Node": "foobar",
                "CheckID": "service:redis",
                "Name": "Service 'redis' check",
                "Status": "passing",
                "Notes": "",
                "Output": "",
                "ServiceID": "redis",
                "ServiceName": "redis"
            },
            {
                "Node": "foobar",
                "CheckID": "serfHealth",
                "Name": "Serf Health Status",
                "Status": "passing",
                "Notes": "",
                "Output": "",
                "ServiceID": "",
                "ServiceName": ""
            }
        ]
    }
]

Both rest endpoints will filter for tags by appending ?tag= and by datacenter by appending ?dc= query parameters. If no ?tag= is provided, then no filtering occurs, if no ?dc= is provided, then the local datacenter is queried.

The /v1/health/service/<service> additionally takes a ?passing query parameter to return only healthy nodes.

Based on this, I think we should try to replicate the DNS interface behavior in returning only healthy services using the same DNS name but in reverse:

<domain>[/<datacenter>]/service/<service>[/<tag>]

Internally, we would use the consul client to query the health endpoint and only get healthy nodes (same as the DNS interface).

If the /<datacenter> portion is omitted, then the local datacenter is queried:

<domain>/service/<service>[/<tag>]

In Consul, it appears the is always consul, so we can just use that (or _consul to try to indicate that it is generated and not K/V data).

This basically gets us to exactly what this PR implements and is the logic I used when creating this PR.

So the basic endpoint would be:

/_consul/service/<service>[/<tag>]

and then we can add support later for datacenters using:

/_consul/<datacenter>/service/<service>[/tag]

If people feel we need to support getting all nodes instead of just healthy nodes, I think we should support that via a backend configuration option instead of through the key structure (default to only healthy nodes, but allow configuration to return all nodes). Then the user could do their own filtering because the health information would be in the JSON stored at the key.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 8, 2014

Owner

@johnrengelman Now we need everyone to agree about support for datacenters and tags. This is where I'm going to draw the line, if we really plan on ever adding datacenter and tag support for services then we should just avoid adding support for services at all.

Based on the conversation it seems people want something more robust than confd, and something that supports generic data sources and not just K/V. Maybe confd's run has come to an end and it maybe better to fork confd and add all of these features. This was the number one reason for rejecting this PR in the first place. While the idea and code are solid, the cost of adding it opens the door to a whole new world of features confd was never designed for.

Is it time for a "better" tool? I think this PR speaks to how broken all of this service discover stuff is. We all want data in a different formats to handle creating load balancer configuration files. To be honest we are just making a mess.

For the record I really support people taking the ideas of confd and creating a bunch of tools that integrate deeply with other tools in a way confd doesn't.

Owner

kelseyhightower commented Aug 8, 2014

@johnrengelman Now we need everyone to agree about support for datacenters and tags. This is where I'm going to draw the line, if we really plan on ever adding datacenter and tag support for services then we should just avoid adding support for services at all.

Based on the conversation it seems people want something more robust than confd, and something that supports generic data sources and not just K/V. Maybe confd's run has come to an end and it maybe better to fork confd and add all of these features. This was the number one reason for rejecting this PR in the first place. While the idea and code are solid, the cost of adding it opens the door to a whole new world of features confd was never designed for.

Is it time for a "better" tool? I think this PR speaks to how broken all of this service discover stuff is. We all want data in a different formats to handle creating load balancer configuration files. To be honest we are just making a mess.

For the record I really support people taking the ideas of confd and creating a bunch of tools that integrate deeply with other tools in a way confd doesn't.

@jhmartin

This comment has been minimized.

Show comment
Hide comment
@jhmartin

jhmartin Aug 8, 2014

Contributor

I think the PR as it stands is good enough, which is all this needs to do. It doesn't need to solve every possible use-case, but having a tool that solves the 80% use case available will draw much more attention to the overall problem -- and hopefully spawn the alternate tools that can handle the 20% / 'advanced' cases. Let confd handle the basic service-discovery cases that the PR already handles, get that out, and see where it goes from there.

Contributor

jhmartin commented Aug 8, 2014

I think the PR as it stands is good enough, which is all this needs to do. It doesn't need to solve every possible use-case, but having a tool that solves the 80% use case available will draw much more attention to the overall problem -- and hopefully spawn the alternate tools that can handle the 20% / 'advanced' cases. Let confd handle the basic service-discovery cases that the PR already handles, get that out, and see where it goes from there.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 8, 2014

Owner

@jhmartin The problem is once I hit the merge button, it's going to be hard to take it back. This does not integrate well with prefixes and will cause confusion, which I don't have the bandwidth to fully support at the moment.

I know I will not be able to make everyone happy, but I can at-least be sure to only merge things that we can actually support.

Owner

kelseyhightower commented Aug 8, 2014

@jhmartin The problem is once I hit the merge button, it's going to be hard to take it back. This does not integrate well with prefixes and will cause confusion, which I don't have the bandwidth to fully support at the moment.

I know I will not be able to make everyone happy, but I can at-least be sure to only merge things that we can actually support.

@armon

This comment has been minimized.

Show comment
Hide comment
@armon

armon Aug 8, 2014

Contributor

I like the key structure proposed by @johnrengelman. I think even without datacenter support it solves the 80% case as mentioned.

Contributor

armon commented Aug 8, 2014

I like the key structure proposed by @johnrengelman. I think even without datacenter support it solves the 80% case as mentioned.

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 8, 2014

Contributor

@kelseyhightower I'll take a look today at better supporting the prefix key structure. I'm thinking I can just parse the prefixes and use them to determine which services to look up data on a such.

I'll update this PR with my changes.

Contributor

johnrengelman commented Aug 8, 2014

@kelseyhightower I'll take a look today at better supporting the prefix key structure. I'm thinking I can just parse the prefixes and use them to determine which services to look up data on a such.

I'll update this PR with my changes.

@jhmartin

This comment has been minimized.

Show comment
Hide comment
@jhmartin

jhmartin Aug 8, 2014

Contributor

@johnrengelman That'll improve performance significantly in large environment as well, very nice.

Contributor

jhmartin commented Aug 8, 2014

@johnrengelman That'll improve performance significantly in large environment as well, very nice.

@carlosdp

This comment has been minimized.

Show comment
Hide comment
@carlosdp

carlosdp Aug 8, 2014

@kelseyhightower I think this is ready to go as is. Datacenter support can be added later without issue.

carlosdp commented Aug 8, 2014

@kelseyhightower I think this is ready to go as is. Datacenter support can be added later without issue.

@johnrengelman

This comment has been minimized.

Show comment
Hide comment
@johnrengelman

johnrengelman Aug 8, 2014

Contributor

Ok, I updated the PR to handle confd prefixes...Now it will only query for specific data in consul.
Examples:
Prefix = /_consul : Return all services and all tags using the above format
Prefix = /_consul/service : Same as /_consul currently
Prefix = /_consul/service/web : Get only data about the 'web' service, get data about all of the know s tags for 'web'
Prefix = /_consul/service/web/master : Get only data about the 'web' service with the 'master' tag.

Each prefix configure for confd is processed, so depending on the ordering there, you could resolve the same data a couple times...it will just overwrite itself in the internal keystore. This all happens before the the template is written out (same as the K/V).

Contributor

johnrengelman commented Aug 8, 2014

Ok, I updated the PR to handle confd prefixes...Now it will only query for specific data in consul.
Examples:
Prefix = /_consul : Return all services and all tags using the above format
Prefix = /_consul/service : Same as /_consul currently
Prefix = /_consul/service/web : Get only data about the 'web' service, get data about all of the know s tags for 'web'
Prefix = /_consul/service/web/master : Get only data about the 'web' service with the 'master' tag.

Each prefix configure for confd is processed, so depending on the ordering there, you could resolve the same data a couple times...it will just overwrite itself in the internal keystore. This all happens before the the template is written out (same as the K/V).

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 9, 2014

Owner

@johnrengelman Ok, I've taken a moment to play with this feature, and while I understand why people want this feature it's does not fit the confd model.

Don't get me wrong, it totally works, it just takes confd out of scope. I'm going to update the confd docs to express more clearly the goals for the project. confd is not a good solution for service discovery, even though it will work in some limited cases where data used by service discovery tools is made available via K/V pairs, it will not work for the uses cases outlined in this PR.

The hard line going froward for confd will be only to fetch data from K/V stores, I'm also leaning towards limiting those K/V stores to consul, etcd, and env vars.

I know this decision will mean confd losing some users, but I think the project still adds value in the form of simplicity and focus.

To quickly recap, this PR is being rejected based on the following:

  • querying non k/v endpoints is out of scope of confd. That includes consul services, catalog, and health checks.

This was a touch decision and I did go back and forth on this one. But I feel it's the right choice for confd long term.

Owner

kelseyhightower commented Aug 9, 2014

@johnrengelman Ok, I've taken a moment to play with this feature, and while I understand why people want this feature it's does not fit the confd model.

Don't get me wrong, it totally works, it just takes confd out of scope. I'm going to update the confd docs to express more clearly the goals for the project. confd is not a good solution for service discovery, even though it will work in some limited cases where data used by service discovery tools is made available via K/V pairs, it will not work for the uses cases outlined in this PR.

The hard line going froward for confd will be only to fetch data from K/V stores, I'm also leaning towards limiting those K/V stores to consul, etcd, and env vars.

I know this decision will mean confd losing some users, but I think the project still adds value in the form of simplicity and focus.

To quickly recap, this PR is being rejected based on the following:

  • querying non k/v endpoints is out of scope of confd. That includes consul services, catalog, and health checks.

This was a touch decision and I did go back and forth on this one. But I feel it's the right choice for confd long term.

@kshep

This comment has been minimized.

Show comment
Hide comment
@kshep

kshep Aug 9, 2014

@kelseyhightower I'm a little confused... consul isn't simply a k/v store, it's primarily a service registry. I think the core use case for consul+confd would be to register services with consul, then use confd to populate config files that point to those service. If I understand consul correctly, k/v storage is a secondary feature. If consul users need to use some other app to get service data into config files, I can't really see them also using confd.

kshep commented Aug 9, 2014

@kelseyhightower I'm a little confused... consul isn't simply a k/v store, it's primarily a service registry. I think the core use case for consul+confd would be to register services with consul, then use confd to populate config files that point to those service. If I understand consul correctly, k/v storage is a secondary feature. If consul users need to use some other app to get service data into config files, I can't really see them also using confd.

@kelseyhightower

This comment has been minimized.

Show comment
Hide comment
@kelseyhightower

kelseyhightower Aug 9, 2014

Owner

@kshep confd was built around etcd, before consul existed. The idea was simple, provide a bridge for people to get started with etcd until more apps supported it natively. etcd does not support additional APIs for service discovery, it's all done via K/V pairs.

Enter consul.

The idea was simple, provide the same support for consul that we do for etcd. We did. Consul supports a lot more features than etcd, including DNS and a services API for service discovery. Consul can be considered more feature rich and may benefit from a tool that supports all these features. confd is not that tool.

Owner

kelseyhightower commented Aug 9, 2014

@kshep confd was built around etcd, before consul existed. The idea was simple, provide a bridge for people to get started with etcd until more apps supported it natively. etcd does not support additional APIs for service discovery, it's all done via K/V pairs.

Enter consul.

The idea was simple, provide the same support for consul that we do for etcd. We did. Consul supports a lot more features than etcd, including DNS and a services API for service discovery. Consul can be considered more feature rich and may benefit from a tool that supports all these features. confd is not that tool.

@stevenjack

This comment has been minimized.

Show comment
Hide comment
@stevenjack

stevenjack Jun 18, 2015

Just a quick one if anyone stumbles across this PR, Consul has support for it's own templating:

https://github.com/hashicorp/consul-template#templating-language

stevenjack commented Jun 18, 2015

Just a quick one if anyone stumbles across this PR, Consul has support for it's own templating:

https://github.com/hashicorp/consul-template#templating-language

tomdee pushed a commit to tomdee/confd that referenced this pull request Jun 12, 2018

Merge pull request #102 from caseydavenport/cd-automated-libcalico-up…
…date

Automatic update of libcalico-go to 88291609bbefd0fe22982a7a78ec51d63583a651
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment