bitflicker64 commented on code in PR #3322:
URL: https://github.com/apache/hugegraph/pull/3322#discussion_r4257614564
##########
helm/hugegraph/values.schema.json:
##########
@@ -762,6 +762,23 @@
"probes"
],
"properties": {
+ "serviceId": {
+ "type": "string",
+ "minLength": 1,
+ "pattern": "^[ -~]+$",
Review Comment:
Important: the `server.serviceId` pattern accepts every printable ASCII
character, but the Server's own service bookkeeping uses `/` and `-` as
separators, so two ordinary-looking IDs are accepted here and then mishandled
after startup.
Evidence (static read at dd10fda, not run on a cluster; `helm template ci
helm/hugegraph --set-string server.serviceId=a/b` and `...=analytics-prod` both
render at this head):
- `/`: `GraphManager.loadServices` stores the Server's own record through
`ServiceMetaManager.serviceConfKey`, which joins
`.../GRAPHSPACE/<space>/SERVICE_CONF/<name>` with `/`. `serviceConfigs()` reads
the records back with `key.split("/")` and keeps only the last part. A Server
started with `team/analytics` in `team_a` is written under
`.../SERVICE_CONF/team/analytics` and is loaded by every later start, and by
every other Server, as the service `analytics`. If `team_a` also has a real
`analytics` service, both land on the same map key and one record replaces the
other in the cache.
- `-`: `GraphManager.clearGraphSpace` walks the cached `<space>-<service>`
keys with `key.split("-")` and calls `dropService(parts[0], parts[1])`. For
`analytics-prod` that is `dropService("team_a", "analytics")`: the record of
`analytics-prod` is left in PD when the space is cleared or dropped, and a
different service named `analytics` is the one looked up.
The entrypoint applies the same printable-ASCII check, and the new tests use
hyphenated IDs (`service-a`, `legacy-service`), so this is the common case, not
a corner one. The README text added here says printable ASCII round-trips
unchanged, which holds for the properties file but not for the registry.
Requested change: reject `/` in `server.serviceId` (schema pattern and the
entrypoint check, with a `must_fail` case), and for `-` either reject it the
same way or make `clearGraphSpace` split with a limit of 2 (graph-space names
cannot contain `-`). If the wider character set is intended, please state these
two limits in the "Server service identity" sections of both READMEs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
