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

Prepared queries: treat empty-string tags as no tags at all #2151

Closed
wants to merge 4 commits into from

Conversation

tpotega
Copy link

@tpotega tpotega commented Jun 29, 2016

Skipping tag filtering in prepared queries might fail as match() produces an empty-string tag.

When using standard DNS interface, it is easy to - optionally - filter by tag, just adding it in front of the service name.
Take the "web" service from the Consul intro as an example:

{"service": {"name": "web", "tags": ["rails"], "port": 80}}

First, a non-tagged query:

$ dig -p 8600 @127.0.0.1 web.service.consul
...
web.service.consul.     0       IN      A       127.0.0.1
...

Then, with tag prepended:

$ dig -p 8600 @127.0.0.1 rails.web.service.consul
...
rails.web.service.consul. 0     IN      A       127.0.0.1
...

A problem arises, if one would like to mimic this behaviour using prepared queries:

{
  "Name": "",
  "Template": {
    "Type": "name_prefix_match",
    "Regexp": "^(?:([^\\.]+)\\.)?([^\\.]+)$"
  },
  "Service": {
    "Service": "${match(2)}",
    "Tags": [
      "${match(1)}"
    ]
  }
}

This works for queries with a tag specified:

$ http 127.0.0.1:8500/v1/query/rails.web/explain | jq .Query.Service
{
  "Service": "web",
  "Failover": {
    "NearestN": 0,
    "Datacenters": null
  },
  "OnlyPassing": false,
  "Tags": [
    "rails"
  ]
}

But with the tag ommited - HIL match() function returns an empty string, and as such - one ends up with an "empty string" tag:

$ http 127.0.0.1:8500/v1/query/web/explain | jq .Query.Service
{
  "Service": "web",
  "Failover": {
    "NearestN": 0,
    "Datacenters": null
  },
  "OnlyPassing": false,
  "Tags": [
    ""
  ]
}

This is not equal to no tag filtering at all, and such queries will not return an answer:

$ dig -p 8600 @127.0.0.1 rails.web.query.consul
...
rails.web.query.consul. 0       IN      A       127.0.0.1
...

$ dig -p 8600 @127.0.0.1 web.query.consul
...
;; AUTHORITY SECTION:
consul.                 0       IN      SOA     ns.consul.
postmaster.consul. 1467201244 3600 600 86400 0
...

As a further confirmation - adding a ""-tag to the service definition would let it match:

{"service": {"name": "web", "tags": ["rails", ""], "port": 80}}

A potential solution would be to flatten or compact the tags list, omitting empty strings, and returning null instead of a list when no non-empty tags remain:

$ http 127.0.0.1:8500/v1/query/web/explain | jq .Query.Service
{
  "Service": "web",
  "Failover": {
    "NearestN": 0,
    "Datacenters": null
  },
  "OnlyPassing": false,
  "Tags": null
}

Seems to fix the problem mentioned, hope it does not break other things.

@slackpad
Copy link
Contributor

Hi @tpotega thanks for the PR. I think this is a good feature to add, though by default it seems a little dangerous since if you are selecting for a particular tag you don't want to just pick up any tag if the match didn't work out. I think if we add a boolean RemoveEmptyTags to the template definition that defaults to false then this would be good to go.

@tpotega
Copy link
Author

tpotega commented Jun 29, 2016

@slackpad - you're right, it all depends on the use case. Gave the RemoveEmptyTags a try - with just the basic test in place.

@slackpad slackpad added this to the 0.7.4 milestone Nov 18, 2016
@slackpad slackpad removed this from the Triaged milestone Apr 18, 2017
@sethvargo sethvargo added quick-win type/docs Documentation needs to be created/updated/clarified labels May 1, 2017
@sethvargo
Copy link
Contributor

Hey @tpotega

We are going to try to get this into the next release. We have to write docs before we can merge, unless you beat us to it 😉

@tpotega
Copy link
Author

tpotega commented May 5, 2017

A few words added, but now I see the docs have changed :)

@adambonny
Copy link

I was having this exact issue with queries and stumbled upon your branch, I hope it gets included as it adds a valuable feature that's currently missing from queries 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants