bitflicker64 commented on code in PR #3362:
URL: https://github.com/apache/hugegraph/pull/3362#discussion_r4317532392
##########
helm/hugegraph/templates/store-statefulset.yaml:
##########
@@ -153,6 +153,11 @@ spec:
valueFrom:
fieldRef:
fieldPath: metadata.namespace
+ - name: HG_PD_AUTH_SECRET_KEY
Review Comment:
Minor: The chart now owns `HG_PD_AUTH_SECRET_KEY` on the Store container,
but the `store` entry of `$reservedEnv` in `_helpers.tpl` (line 948) does not
list it, while the `pd` entry does. A `store.extraEnv` entry with that name
therefore renders after this one and, as the comment above `$reservedEnv` says,
the last duplicate wins, so it silently replaces the chart's Secret reference.
A mismatched value only shows up as a warning at shutdown, because the report
is refused and the Store falls back to expiry.
Please add `HG_PD_AUTH_SECRET_KEY` to the `store` list and a chart test that
`store.extraEnv` with that name fails rendering, like the existing
reserved-name tests.
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/service/StoreShutdownAuthenticationTest.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hugegraph.pd.service;
+
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.hugegraph.pd.common.Consts;
+import org.apache.hugegraph.pd.config.PDConfig;
+import org.apache.hugegraph.pd.grpc.PDGrpc;
+import org.apache.hugegraph.pd.grpc.Pdpb;
+import org.apache.hugegraph.pd.service.interceptor.StoreShutdownAuthentication;
Review Comment:
Critical: This import, and `PDService` in `StoreShutdownReportTest` (line
39), resolve against the `hg-pd-service` artifact because `hg-pd-test` compiles
its tests as main sources. In a reactor `package` build that artifact is the
Spring Boot repackaged jar, with the classes under `BOOT-INF/classes/`, so
`hg-pd-test` fails with `package org.apache.hugegraph.pd.service.interceptor
does not exist` and `cannot find symbol: class PDService`, and the build stops
there.
This is why `pd_store / pd`, `pd_store / store`, `pd_store / hstore`,
`cluster / cluster-test`, `codeql` and `dependency-check` all fail at this head
in run 39294815115, each with exactly these two errors
(`StoreShutdownAuthenticationTest.java:[29,51]`,
`StoreShutdownReportTest.java:[39,13]`). None of the new or existing PD and
Store tests ran in CI. I reproduced it with `mvn -o clean package -pl
hugegraph-pd/hg-pd-test -am -Dmaven.test.skip=true` on JDK 17: `hg-pd-test`
FAILURE with the same two errors, and `unzip -l` on the `hg-pd-service` jar
shows `BOOT-INF/classes/org/...`. The local runs in the PR description pass
because `mvn test` resolves the reactor module to `target/classes` instead of
the repackaged jar.
Please move `StoreShutdownAuthenticationTest` and `StoreShutdownReportTest`
into `hugegraph-pd/hg-pd-service/src/test/java` (next to
`StoreHeartbeatAgeTest` and `StoreAPIReadyTest`), drop them from
`PDCoreSuiteTest`, and confirm the root `mvn clean package
-Dmaven.test.skip=true` passes.
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/PDService.java:
##########
@@ -438,6 +439,24 @@ public void getStore(Pdpb.GetStoreRequest request,
observer.onCompleted();
}
+ @Override
+ public void reportStoreShutdown(Pdpb.SetStoreRequest request,
+ StreamObserver<Pdpb.SetStoreResponse>
observer) {
+ if (!isLeader()) {
+ redirectToLeader(PDGrpc.getReportStoreShutdownMethod(), request,
observer);
Review Comment:
Important: A report that reaches a follower is forwarded without its
credential, so the leader refuses it and the Store's shutdown report never
takes effect in that case.
`StoreShutdownAuthentication` validates the `credential` header on the
follower, then this call forwards through `PDService.redirectToLeader`, which
sends `channel.newCall(method, CallOptions.DEFAULT)` (line 1335) on a bare
channel with no interceptor, so the forwarded call carries no `credential`
header. The same interceptor is registered on the leader, sees an empty
credential and closes the call with `UNAUTHENTICATED`. The follower relays that
to the Store, `HeartbeatService.reportOffline` logs a warning, and PD keeps the
Store `Up` until its lease expires, which is the pre-PR behavior this RPC was
added to fix.
This is a normal path in multi-PD deployments such as the Helm cluster
preset and `docker-compose-3pd-3store-3server.yml`: `PDClient.newLeaderStub`
(lines 315 to 317) keeps the stub on the first reachable host even when
`getMembers` names a different leader, and any call during a leader change also
lands on a follower. I checked the call shape with a probe test (not kept): two
local gRPC servers with `StoreShutdownAuthentication` and the same configured
secret, the follower forwarding with
`ClientCalls.asyncUnaryCall(leaderChannel.newCall(PDGrpc.getReportStoreShutdownMethod(),
CallOptions.DEFAULT), ...)`. A client sending the correct `store:<secret>`
credential to the follower got `UNAUTHENTICATED: Shutdown report requires an
internal credential`, and the leader handler ran zero times.
`StoreShutdownAuthenticationTest` uses one server, so it does not cover this
path.
Please carry the credential across the redirect, for example by having the
interceptor put the validated header into a gRPC `Context` key and attaching it
to the forwarded call with `MetadataUtils.newAttachHeadersInterceptor`, or by
having the follower sign the forwarded call with its own `auth.secret-key`.
Please add a test with a follower forwarding to a leader, both with the
interceptor installed.
--
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]
