bitflicker64 commented on code in PR #3322:
URL: https://github.com/apache/hugegraph/pull/3322#discussion_r4250356501
##########
helm/hugegraph/templates/_helpers.tpl:
##########
@@ -851,7 +851,7 @@ start-hugegraph-pd.sh, start-hugegraph-store.sh, and
hugegraph-server.sh).
{{- $reservedEnv := dict
"pd" (list "HG_PD_GRPC_HOST" "HG_PD_GRPC_PORT" "HG_PD_REST_PORT"
"HG_PD_RAFT_ADDRESS" "HG_PD_RAFT_PEERS_LIST" "HG_PD_INITIAL_STORE_LIST"
"HG_PD_INITIAL_STORE_COUNT" "HG_PD_DATA_PATH" "HG_PD_AUTH_SECRET_KEY"
"JAVA_OPTS" "JAVA_OPTIONS")
"store" (list "HG_STORE_PD_ADDRESS" "HG_STORE_GRPC_HOST"
"HG_STORE_GRPC_PORT" "HG_STORE_REST_PORT" "HG_STORE_RAFT_ADDRESS"
"HG_STORE_DATA_PATH" "JAVA_OPTS" "JAVA_OPTIONS")
- "server" (list "POD_IP" "HG_SERVER_BACKEND" "HG_SERVER_PD_PEERS"
"HG_SERVER_PD_REST_ENDPOINT" "STORE_REST" "HG_SERVER_INIT_STORE_ENABLED"
"HG_SERVER_URLS_TO_PD" "HG_SERVER_STARTUP_TIMEOUT_S" "PD_AUTH_PASSWORD"
"PASSWORD" "HG_SERVER_AUTH_TOKEN_SECRET" "JAVA_OPTS" "JAVA_OPTIONS")
+ "server" (list "POD_IP" "HG_SERVER_SERVICE_ID" "HG_SERVER_GRAPHSPACE"
"HG_SERVER_BACKEND" "HG_SERVER_PD_PEERS" "HG_SERVER_PD_REST_ENDPOINT"
"STORE_REST" "HG_SERVER_INIT_STORE_ENABLED" "HG_SERVER_URLS_TO_PD"
"HG_SERVER_STARTUP_TIMEOUT_S" "PD_AUTH_PASSWORD" "PASSWORD"
"HG_SERVER_AUTH_TOKEN_SECRET" "JAVA_OPTS" "JAVA_OPTIONS")
Review Comment:
Minor: the reserved list now blocks `HG_SERVER_SERVICE_ID` and
`HG_SERVER_GRAPHSPACE` in `server.extraEnv`, but the entrypoint in this PR also
maps the older `SERVICE_ID` and `GRAPH_SPACE` names, and those still pass
through.
Evidence: at this head, `helm template ci helm/hugegraph --set
'server.extraEnv[0].name=SERVICE_ID' --set-string
'server.extraEnv[0].value=café' --set 'server.extraEnv[1].name=GRAPH_SPACE'
--set-string 'server.extraEnv[1].value=TeamA'` renders both variables without
an error. With `server.serviceId` and `server.graphSpace` left at `DEFAULT` the
chart omits the `HG_SERVER_*` pair, so `migrate_env` in `docker-entrypoint.sh`
promotes the older names: a valid value changes the Server identity without
going through the chart values or the schema patterns, and an invalid one (as
above) is caught only at container start, where the Pod exits instead of the
render failing. `PASSWORD`, the other older name the entrypoint reads, is
already reserved here.
Requested change: add `SERVICE_ID` and `GRAPH_SPACE` to the `server`
reserved list, with a unittest case next to the two new shadow tests in
`server_discovery_test.yaml`.
##########
helm/hugegraph/templates/server-deployment.yaml:
##########
@@ -258,6 +258,14 @@ spec:
fieldPath: status.podIP
- name: HG_SERVER_BACKEND
value: {{ .Values.server.backend | quote }}
+ {{- if ne (.Values.server.serviceId | default "DEFAULT") "DEFAULT"
}}
+ - name: HG_SERVER_SERVICE_ID
+ value: {{ .Values.server.serviceId | quote }}
Review Comment:
Minor: Kubernetes expands `$(VAR)` references in an env `value` against
variables declared earlier in the same container, and `POD_IP` is declared just
above. The schema pattern `^[ -~]+$` accepts `$`, `(` and `)`, so a service ID
containing such a reference does not reach the entrypoint unchanged.
Evidence: `helm template ci helm/hugegraph --set-string
'server.serviceId=x-$(POD_IP)'` at this head renders `value: "x-$(POD_IP)"`,
which the kubelet resolves to a different string on every replica. That
contradicts the README text added here (printable ASCII is accepted as-is, and
all replicas in the Deployment share one identity). This was read from the
rendered manifest and the Kubernetes env expansion rule, not run on a cluster.
Requested change: escape the reference when rendering, for example `{{
.Values.server.serviceId | replace "$(" "$$(" | quote }}`, or reject `$(` in
the schema pattern and say so in the README.
--
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]
