Fix catalog tag filter backward compat

[ShimmerGlass]

Fixes: #4922

Fix catalog service node filtering (ex /v1/catalog/service/service?tag=tag1) between agent version <=v1.2.3 and server >=v1.3.0.
New server version did not account for the old field when filtering hence request made from old agent were not tag-filtered.

[pierresouchay]

LGTM: will allow to upgrade Consul from versions prior to 1.3.0 without troubles (otherwise, migration is broken)

[banks]

@Aestek does this same issue not affect /health/:service in the same way?

And thanks for the patch! We'll work out how to release this once we are happy we caught all this issues caused by the tags change.

[banks]

This seems like it will merge old and new fields...

Assuming that the old field is only set by <1.3 clients who didn't know about the new one I think it will do the job, but that assumption isn't mentioned in comments so I think it will get lost over time!

My marginal preference would be to make the actual logic explicit without that assumption:

if args.TagFilter {
  tags := args.ServiceTags
  // If there are no tags, fallback to the old singular tag field as this may be an RPC from a client running Consul < 1.3.0
  if len(tags) == 0 && len(args.ServiceTag) > 0 {
    tags = []string{args.ServiceTag} // Forget the actual type so it might not be this...
  }
  return s.ServiceTagNodes(ws, args.ServiceName, tags)
}

[ShimmerGlass]

@banks /health/:service has this check already: https://github.com/hashicorp/consul/blob/master/agent/consul/health_endpoint.go#L203 although the comment does not mention this backward compat issue.

[ShimmerGlass]

@banks I've updated the logic as you suggested

[pierresouchay]

LGTM

[banks]

nit: if you made the old and new fields have different values here you could effectively assert the non-merging behaviour I mentioned. As it is the implementation might be taking either or merging them and just happens the duplication doesn't matter. I'm sure it will pass so I can make that tweak when we merge but if you want to do that please do @Aestek, we'll talk about when to merge and release this later today.

[banks]

@banks /health/:service has this check already:

Thanks! Yeah So that case was caught because DNS interface tests effectively exercised it already internally - the only real reason I can see that DNS still exercises the old code path was that DNS syntax doesn't easily support multiple tags and so we left the code alone. Had we updated DNS to use ServiceTags anyway for consistency (arguably worthwhile) we'd have not patched health there either most likely.

@Aestek if you are making another change based on that nitpick comment, how do you feel about updating the comment in the health endpoint to also mention migration just in case we do update DNS and think it's redundant some time later? Minor thing but might be nice.

[ShimmerGlass]

@banks I've update this so /health and /catalog behave and are documented the same :

• If ServiceTag not empty -> use it
• Else use ServiceTags

[banks]

THanks @Aestek that's great. We'll figure out when to merge this later on.

[agy]

@Aestek thanks so much for the fix. It's really appreciated.

[ShimmerGlass]

@banks Thanks !