bitflicker64 commented on code in PR #3331:
URL: https://github.com/apache/hugegraph/pull/3331#discussion_r4266299610
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/profile/GraphsAPI.java:
##########
@@ -693,6 +701,59 @@ public Map<String, GraphMode> mode(@Context GraphManager
manager,
return ImmutableMap.of("mode", g.mode());
}
+ @GET
+ @Timed
+ @Path("{name}/mode/owners")
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @RolesAllowed({"space", "$owner=$name"})
+ public Map<String, Object> loadingMode(@Context GraphManager manager,
+ @PathParam("graphspace") String
graphSpace,
+ @PathParam("name") String name) {
+ return graph(manager, graphSpace, name).loadingMode();
+ }
+
+ @PUT
+ @Timed
+ @Path("{name}/mode/owners")
+ @Consumes(APPLICATION_JSON)
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @RolesAllowed({"space", "$owner=$name"})
+ public Map<String, Object> loadingMode(@Context GraphManager manager,
+ @PathParam("graphspace") String
graphSpace,
+ @PathParam("name") String name,
+ JsonLoadingOwner owner,
+ @Context SecurityContext
securityContext) {
+ E.checkArgument(owner != null && owner.epoch != null, "Loading owner
body and epoch are required");
+ if ("recover".equals(owner.action)) {
+ if (securityContext == null ||
!securityContext.isUserInRole("admin")) {
+ throw new ForbiddenException("Loading recovery requires
administrator permission");
+ }
+ E.checkArgument(CONFIRM_RECOVER.equals(owner.confirm),
+ "Stop and drain all related imports, then confirm
with '%s'", CONFIRM_RECOVER);
+ E.checkArgument(owner.revision != null && owner.revision >= 0,
+ "Current loading state revision is required for
recovery");
+ }
+ try {
+ HugeGraph graph = graph(manager, graphSpace, name);
+ if ("recover".equals(owner.action)) {
+ return graph.recoverLoadingMode(owner.instance, owner.epoch,
owner.revision);
+ }
+ return graph.loadingMode(owner.action, owner.instance,
owner.epoch, owner.token, "");
Review Comment:
Minor: on a graph that is not wrapped by `HugeGraphAuthProxy`, every caller
acquires and releases as the principal `""`, so the binding the docs describe
("the authenticated principal owns the token") does not apply there. This line
passes `""` and relies on `HugeGraphAuthProxy.loadingMode` (line 827) to
substitute the context user, which never runs for a graph opened through
`org.apache.hugegraph.HugeFactory`: `GraphManager.createGraph` selects that in
PD mode for the `DEFAULT` space and for any space with auth off
(`GraphManager.java:1566-1567`), also when the Server authenticates requests.
On `LoadingMode` compiled from this head, `acquire` then `release` with `""` on
both calls returns `state=released`, while `alice` then `bob` is rejected at
`LoadingMode.java:87`. The Server never returns the token, so only the
principal check is lost. Requested change: pass `HugeGraphAuthProxy.username()`
here, as lines 164 and 511 of this file do; `HugeAuthenticator.authenticate`
sets that
context for every authenticated request, proxied or not. Otherwise say in
`docs/server-runtime.md` that the binding, and the graph-proxy half of the
recovery check, need a proxied graph.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/StandardHugeGraph.java:
##########
@@ -438,13 +438,29 @@ private void closeTx() {
@Override
public GraphMode mode() {
- return this.mode;
+ return this.loadingMode.mode();
}
@Override
public void mode(GraphMode mode) {
+ this.loadingMode.mode(mode);
LOG.info("Graph {} will work in {} mode", this, mode);
- this.mode = mode;
+ }
+
+ @Override
+ public Map<String, Object> loadingMode() {
+ return this.loadingMode.capability();
+ }
+
+ @Override
+ public Map<String, Object> loadingMode(String action, String instance,
long epoch,
+ String token, String principal) {
+ return this.loadingMode.update(action, instance, epoch, token,
principal);
+ }
+
+ @Override
+ public Map<String, Object> recoverLoadingMode(String instance, long epoch,
long revision) {
+ return this.loadingMode.recover(instance, epoch, revision);
Review Comment:
Minor: mode changes made through the owner API, and recovery, leave no log
line that records the change. `mode(GraphMode)` above logs `Graph {} will work
in {} mode` after a successful set, but `loadingMode(...)` and
`recoverLoadingMode(...)` return straight from `LoadingMode`, which has no
logger. Once the paired Loader (apache/hugegraph-toolchain#815) uses `acquire`
and `release`, the `NONE` to `LOADING` and `LOADING` to `NONE` switches that an
import logs today through the enum setter become silent, and an administrator
`recover` that drops every owner and forces `NONE` is not recorded either. What
remains is the per-request login line (user, client, path) and, for proxied
graphs, the rate-limited audit line in `HugeGraphAuthProxy.verifyResPermission`
(user, allowed or denied, permission, resource); neither has the owner action,
the epoch or the resulting mode. Requested change: log at INFO in
`LoadingMode`, or return the needed fields to this caller, when the effective
mode ch
anges through an owner (first acquire, last release) and on every recovery,
with the epoch, the previous mode and the number of owners cleared, and without
the token.
--
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]
