Allow setting of Consul ServiceMeta tags from config file

[unRob]

Hi there!

I'd love to be able to set ServiceMeta on the Consul service registered by Vault by specifying the desired tags in the config file, and I'm submitting this PR to that effect.

I'm not happy about unmarshalling json during NewServiceRegistration but considered that a simpler change than the couple of alternatives I came up from a quick glance at the codebase. That is to say, I'd love feedback on the approach (and perhaps suggestions on a proper testing strategy should folks feel this approach works).

fixes #9121
see also #10233

[hashicorp-cla]

All committers have signed the CLA.

[unRob]

Still very interested in seeing this through, any chance it could be reviewed?

[VioletHynes]

Hey there @unRob ! I'm really very sorry this one has waited so long, but I'm pretty eager to get it merged in as long as you are.

There's just a few things I'd like to confirm/address before we do. I'll be leaving a review shortly with some comments, but I'll be watching this closely for updates, and please feel free to tag me just in case :)

[VioletHynes]

Thanks for bearing with us on this. A few comments and things I'd like to see answered/addressed, but I'm eager to get this merged. I agree that the approach of deserializing the JSON where you have seems to be the easiest place and I don't see it as too problematic.

I know this took a while to be reviewed but I'm committed to helping you see this to the finish line, as long as you're still willing :)

[VioletHynes]

How come the config must be a list? Since we're only taking the first of these values, I'd expect to be able to configure this as a set of JSON strings, e.g.

conf: map[string]string{"service_meta": "{\"key\":\"value\", \"bar\":\"baz\"}"},

[VioletHynes]

As mentioned elsewhere, I'm a little confused why we're unmarshaling this into a list first and then taking the first element. Could this be changed so that we don't expect a list?

[unRob]

I don't recall why I made this choice in the context of the codebase I originally submitted this PR to, but agree it does not make any sense at all as in config, these values—as part of storage or service_registration— will be maps. Vaguely remember having trouble understanding if I should be using HCL blocks or objects.

I think my original intention was to allow something like the following config snippet

storage "consul" {
  service_meta = {some_key = "some value"}
}

It's been a while, and I eventually ended up hacking around this shortcoming, so don't really have an informed opinion. Fixing.

[VioletHynes]

Suggested change

	if logger.IsDebug() {
	logger.Debug("config service_meta set", "service_meta", metaTags)
	}
	c.logger.Debug("config service_meta set", "service_meta", metaTags)

[VioletHynes]

I think an example here would be great

[VioletHynes]

The formatting looks a little strange here -- could you give this a make fmt/align this?

[unRob]

Yeah, I used github's UI to resolve the refactor of NewServiceRegistration / serverRegistration.merge and did not really pay attention, while I had hope, I was not really expecting someone to bring this PR back from the dead 😂 . Fixing!

[VioletHynes]

It seems the code has changed around this, and this should just be
return errors.New("service tags must be a dictionary of string keys and values")

[VioletHynes]

Oh, one last thing, we would require a changelog for this. I'm happy to do it myself if you don't want to fiddle, but it'd be a file in the changelog directory called 11084.txt with something like the following text:

```release-note:improvement
serviceregistration: Added support for Consul ServiceMeta tags from config file from the new `service_meta` config field.
-```

(Delete the - preceding the three backticks, I couldn't get it to format properly otherwise. The other changelogs in that directory should show you the right formatting though)

[VioletHynes]

Looks great! Just a few more things before we can get this over the finish line. Thanks for the quick response so far!

[VioletHynes]

We should do this irrespective of if ok, since otherwise there will be a behaviour change as part of this PR. Previously, the tag would have been set, but now, it won't be set unless I provide my own meta tags.

[VioletHynes]

To put it another way: if I don't provide my own service_meta, it should still be:

map[string]string{
	"external-source": metaExternalSource,
},

[VioletHynes]

I'd suggest moving this line until after the if ok, that way the behaviour is preserved.

[unRob]

good catch, fixed!

[VioletHynes]

Nit: We should have a godoc describing what this test does and is about

[VioletHynes]

Suggested change

	func TestConsul_serviceMeta(t *testing.T) {
	func TestConsul_ServiceMeta(t *testing.T) {

[VioletHynes]

I'd love to see a test in some respect here that validates that if I don't provide my own service_meta, that it's still set to what it was before ("external-source": metaExternalSource)

[VioletHynes]

One way we could do this is by improving TestConsul_ServiceMeta to test the resultant map for the three configurations, but I'm open to whatever you feel is easiest :)

[VioletHynes]

Thank you! I appreciate your patience for bearing with us on this, but thanks for being so understanding and great to work with. Approved! I'll look to get this merged as soon as all the CI passes etc.