hashicorp / consul

Consul is a distributed, highly available, and data center aware solution to connect and configure applications across dynamic, distributed infrastructure.
https://www.consul.io
Other
28.25k stars 4.41k forks source link

Hope Service.Meta to contain a special characters such as dot(.) #8127

Open Ayden-Franklin opened 4 years ago

Ayden-Franklin commented 4 years ago

Feature Description

I encountered an exception OperationException(statusCode=400, statusMessage='Bad Request', statusContent='Invalid Service Meta: Couldn't load metadata pair ('user.name', 'spring_boot'): Key contains invalid characters') that was related to that issue #4422. I actually want to use Service.Meta to pass come configurations containing a special characters . such as user.name or user.password. Could you relax the restriction about Service.Meta?

Use Case(s)

I use spring boot admin in which BasicAuthHttpHeaderProvider requires the metadata(user.name and user.password) that can not be supported by consul. This project support many discovery client such as eureka, kubernetes service that all support to use user.name for metadata. In SpringFramework's perspective, metadata won't be related with DNS so I suggest that Service.Meta should keep relaxing.

Ayden-Franklin commented 4 years ago

I think Service.Meta like a dictionary to pass some K-V data except the service name. Service name is another significant term that not be related with metadata.

pierresouchay commented 4 years ago

I tend to agree that let users use the dot would be a good thing. In our case, we had to escape several times meta and replace with underscores, but it might create clashes. I imagine that's because dots are in use in various tools (consul-template, filters in queries...) to navigate in results of objects, but the biggest issue is probably DNS as dots have a special meaning here (dots cannot be used within DNS labels) and meta can be returned by DNS lookups. So if dots could be used, dots would have to be escaped in DNS responses as well.

Note that this limitation is the same for node.meta as well.

Here is a discussion in original PR https://github.com/hashicorp/consul/pull/3881#discussion_r177506154

HaojunRen commented 3 years ago

Hello guys,

I also met this issue. For Spring Cloud 2020, it refactorys metadata logics. And we register some config as metadata to Consul, like spring.boot.version. For Spring config, it often uses dot, so the issue will be

Caused by: com.ecwid.consul.v1.OperationException: OperationException(statusCode=400, statusMessage='Bad Request', statusContent='Invalid Service Meta: Couldn't load metadata pair ('spring.boot.version', '2.4.1'): Key contains invalid characters')
    at com.ecwid.consul.v1.agent.AgentConsulClient.agentServiceRegister(AgentConsulClient.java:278)
    at com.ecwid.consul.v1.ConsulClient.agentServiceRegister(ConsulClient.java:310)
pierresouchay commented 3 years ago

@banks It is true that for services meta, many people want to push ., mostly in the Java world because it is the common way to define properties. We had to patch in many places our SDKs/injections systems to replace those with _ for instance. Maybe allowing it in Service meta would not be such a big problem for Consul and would definitely help many people, but I see your point regarding the differences between Node.Meta and Service.Meta. However, there is a big difference: Node.Meta is mostly injected by Infrastructure (eg: chef), so it is possible easily to enforce some rules regarding the naming in 1 single place, while service.Meta might be injected by various systems (Java, Ruby, python, C# and friends) with many libraries that consider service.Meta like a commodity and do not take such limitations, so, for the the sake of UX, I would tend to say that it would be a good move to be a bit more permissive for Service.Meta

HaojunRen commented 3 years ago

@pierresouchay thanks for your answer. BTW, the spring cloud version before v2020.0.0, it use tags as metadata, so no limitation for dot. And if you want to refactory this logic, when it will release in 1.9.2 version?

pierresouchay commented 3 years ago

@HaojunRen I am not working for Hashicorp, so I cannot decide, the patch would not be very complicated to do, but it really depends on Hashicorp's decision

banks commented 3 years ago

I have always assumed this was a DNS limitation too since all Node meta is presented in TXT records:

$ dig @127.0.0.1 -p 8600 foo.node.consul ANY

; <<>> DiG 9.8.3-P1 <<>> @127.0.0.1 -p 8600 foo.node.consul ANY
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 24355
;; flags: qr aa rd; QUERY: 1, ANSWER: 1, AUTHORITY: 1, ADDITIONAL: 0
;; WARNING: recursion requested but not available

;; QUESTION SECTION:
;foo.node.consul.       IN  ANY

;; ANSWER SECTION:
foo.node.consul.    0   IN  A   10.1.10.12
foo.node.consul.    0   IN  TXT "meta_key=meta_value"
foo.node.consul.    0   IN  TXT "value only"

;; AUTHORITY SECTION:
consul.         0   IN  SOA ns.consul. postmaster.consul. 1392836399 3600 600 86400 0

(From https://www.consul.io/docs/discovery/dns#node-lookups)

But it turns out the contents of a TXT record per RFC1035 may contain dots and even the attribute syntax we use as specified in RFC1464 permits "any printable character" in an attribute name including = (assuming it is escaped).

So I think this was really just a case of starting off being conservative to not create potential issues later.

Only Node meta is used in DNS anyway, for Service Meta we just choose to keep the rules the same for consistency reasons.

So unless I've missed some other place we rely on Node meta, I don't see an issue with allowing periods in keys and values in both Node and Service meta.