chore(server): bump REST API version - #3159
Conversation
- identify the default-role REST contract as API 0.72 - preserve 1.7 releases at API 0.71 for client compatibility - document when the manifest version must change
67ff2e0 to
52035da
Compare
- install reactor outputs before assembling Server images - isolate and lock Maven caches by source revision - verify packaged API versions against source in CI - run Docker CI for every reactor source change
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3159 +/- ##
============================================
- Coverage 41.06% 0.34% -40.72%
+ Complexity 519 74 -445
============================================
Files 771 749 -22
Lines 65962 63520 -2442
Branches 8766 8345 -421
============================================
- Hits 27088 220 -26868
- Misses 36008 63277 +27269
+ Partials 2866 23 -2843 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- normalize configured and runtime peer addresses - enforce DNS-aware IP authorization for raft traffic - refresh peer allowlists during membership changes - cover service updates and raft authorization integration
- persist observer access against the all-graphs target - apply read-only access to existing and future graphs - migrate and remove legacy graph-scoped observer grants - document the GraphSpace-wide contract in API version 0.72
Why this branch contains PD Raft changesThe PD change in Before:
After:
Most of the apparent size is defensive behavior and tests: this commit is Scope note: this is not required to distinguish API Scope freezeThe Hubble dependency is limited to the API |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: No actionable code defect was independently confirmed at this head. Approval remains blocked by the current codecov/project failure. Evidence: Java 11 targeted tests passed: PD auth/raft 20/20, PDService raft update 6/6, GraphSpace API 13/13; current checks show codecov/project FAILURE.
- classify mutation steps from Gremlin bytecode - require write access for add and property steps - require delete access for drop steps - cover read write delete and nested traversals
- inspect realized traversal steps after script evaluation - cover vertex edge property and drop mutations - recurse through nested child traversals - keep read-only traversals executable
- keep auth wrappers when strategies are copied to script traversals - align strategy list behavior with its iterator - cover the copied-strategy contract - preserve structured mutation checks after script evaluation
- keep membership roles out of data action matching - preserve explicit read write and delete permissions - verify members can read without gaining mutations - retain direct GraphSpace administrator handling
- initialize audit limiter only after successful authentication - keep failed password and token attempts out of limiter state - invalidate limiter entries by username when deleting users - remove PD dynamic DNS and IP refresh from this PR - cover password token and cleanup paths with unit tests
- run metadata callbacks with an internal admin context - restore the previous context on success or failure - prevent admin propagation into callback child threads - cover task override and context restoration boundaries
- allow space managers to inspect users in their own space - reject users without current-space grants and global admins - cover cross-space and multi-space permission boundaries
- recognize custom global admins for user updates - allow custom global admins to delete ordinary users - preserve builtin admin behavior and deletion safeguards - cover builtin and custom admin mutation paths
- align template ownership with global admin semantics - preserve creator and GraphSpace manager access - cover all four template management roles
- align Checkstyle and EditorConfig at 120 columns - update contributor and module style guidance - compact only current PR code without legacy reformatting
- defer authenticator lookup until manager access is needed - keep anonymous creators on the owner mutation path - cover lazy owner and manager authorization paths
Bring in server REST API 0.72 changes (PR apache#3159) for local Hubble auth testing.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The new Gremlin mutation pre-check misses TinkerPop merge steps, so execute-only users can reach mutation paths without a required WRITE check. Evidence: traversalPermissions() only recognizes AddVertexStartStep, AddVertexStep, AddEdgeStartStep, AddEdgeStep, AddPropertyStep, and DropStep at HugeGraphAuthProxy.java:2533-2541; TinkerPop 3.7.6 also has MergeVertexStep and MergeEdgeStep, whose on-match path can call Vertex.property directly. Please classify both merge steps as WRITE (including nested children) and add execute-only regression tests for mergeV/mergeE create and onMatch cases.
- recognize mergeV and mergeE as write operations - preserve compatibility with the current TinkerPop baseline - reject execute-only create and onMatch traversals - verify recursive child traversal permissions
- match merge steps by exact TinkerPop class names - retain superclass traversal for provider implementations - reject unrelated steps sharing merge simple names - preserve recursive child permission coverage
- add pinned-3.5 test fixtures for TinkerPop merge steps - route vertex and edge merge shapes through strategy checks - retain external same-name and child traversal regressions
- move compatibility fixtures into test output only - add test output to the unit-test classpath - construct exact-package fixtures reflectively - keep main artifacts free of TinkerPop shadow classes
- select fixture sources from the pinned TinkerPop version - keep 3.5.1 compatibility classes in test output only - stop selecting fixtures automatically after a version change - preserve merge permission regression coverage
- prefer the official traversal and isStart constructor - use Merge.onMatch through reflection when available - keep an explicit 3.5.1 fixture child fallback - preserve merge authorization coverage
- restore the TinkerPop 3.5.1 authorization scope\n- remove future-version merge detection and fixtures\n- keep the Hubble permission closeout focused on reproduced behavior
- move dependency review action from v3 to the Node 24 v5 release - use the supported oversized-summary handling - keep existing severity and license policy unchanged
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The current head has authorization correctness and isolation regressions in GraphSpace role checks, Gremlin mutation enforcement, and callback context cleanup. Evidence: exact-head static review plus six independent read-only lanes; latest checks pass for builds/Docker/CodeQL, while dependency-review is cancelled and Codecov failures are non-blocking.
| if (hasGraph) { | ||
| result = authManager.isDefaultRole(name, graph, user, | ||
| defaultRole); | ||
| result = authManager.isDefaultRole(name, graph, user, defaultRole); |
There was a problem hiding this comment.
createSpaceDefaultRole() under the ALL_GRAPHS marker, but this graph-specific branch checks only isDefaultRole(name, graph, user, defaultRole). A user with the new space-wide role is therefore reported as false when the same check includes a concrete graph parameter; the parallel ManagerAPI path has the same omission. Please treat the ALL_GRAPHS role as covering the requested graph, and add a regression test for both endpoints.
| return rolePerm.contains(grant); | ||
| RolePermission grantedRole = RolePermission.fromJson(grant); | ||
| RolePerm rolePerm = RolePerm.fromJson(role); | ||
| if (resourceObject != null && |
There was a problem hiding this comment.
true when the operator is a SPACE manager for the current graph space and the target grant merely contains an entry for that space. UserAPI.role() then serializes the complete rolePermission(user) object, so a space-a manager can read the space-b entries of a multi-space grant. Please return a graph-space-scoped projection (or reject mixed grants) before allowing this path, and add a cross-space response-isolation test.
| Traversal.Admin<?, ?> traversal, | ||
| Set<HugePermission> permissions) { | ||
| for (Step<?, ?> step : traversal.getSteps()) { | ||
| if (step instanceof AddVertexStartStep || |
There was a problem hiding this comment.
Add*, AddPropertyStep, and DropStep instances. A Groovy lambda such as g.V().sideEffect { it.get().property('k', 'v') } contains none of these steps, while vertices() returns raw graph elements whose property() mutates the backend directly. An EXECUTE-only user can therefore bypass the new WRITE check. Please reject or sandbox lambda mutations, or route element mutations through an authorized proxy; add an execute-only strategy-level regression that calls apply() rather than only reflecting over the classifier.
| grpcThread = Thread.currentThread().getName().contains("grpc"); | ||
| if (grpcThread) { | ||
| HugeGraphAuthProxy.setAdmin(); | ||
| if (Thread.currentThread().getName().contains("grpc")) { |
There was a problem hiding this comment.
finally; this path now scopes only AuthContext through runAsAdmin(). HugeGraphAuthProxy.CONTEXTS is an InheritableThreadLocal, and REQUEST_GRAPH_SPACE is not cleared here, so a reused or inherited listener thread can retain a prior request identity/graph space after the callback or an exception. Please clear or save/restore all auth thread-locals at this listener boundary in finally, and add a thread-reuse/exception regression test.
| @@ -2327,7 +2377,9 @@ public TraversalStrategiesProxy(TraversalStrategies strategies) { | |||
|
|
|||
| @Override | |||
| public List<TraversalStrategy<?>> toList() { | |||
There was a problem hiding this comment.
TraversalStrategies.toList() contract requires an immutable list, and both the interface default and DefaultTraversalStrategies wrap their result with Collections.unmodifiableList. This override returns a mutable ArrayList, allowing callers to add/remove strategy proxies and changing the public API contract. Please return Collections.unmodifiableList(proxies) and add an immutability regression test.
| - 'hugegraph-server/hugegraph-dist/src/assembly/static/bin/util.sh' | ||
| - '.mvn/**' | ||
| - 'pom.xml' | ||
| - 'hugegraph-commons/**' |
There was a problem hiding this comment.
**/Dockerfile* filter leaves the repository's docker/hbase/** tree outside every pull_request.paths entry, although it still contains a Dockerfile, entrypoint, and HBase configuration. Changes to that image will no longer trigger Docker Build CI and can merge without any image validation. Please retain docker/hbase/** in the trigger or add a dedicated HBase build job with checks appropriate to that image.
| if (hasGraph) { | ||
| authManager.deleteDefaultRole(name, user, defaultRole, graph); | ||
| } else { | ||
| authManager.deleteDefaultRole(name, user, defaultRole); |
There was a problem hiding this comment.
ALL_GRAPHS role first and then performs independent per-graph deletes. If any later metadata delete fails, the request returns an error while the global role is already gone and some legacy graph-level grants remain active, leaving authorization state partially migrated. Please make this migration transactional or explicitly idempotent/resumable with compensation, and cover an injected mid-loop failure.
|
Superseded by hugegraph#198. The reviewed fixes and remaining compatibility acceptance are now tracked in the organization repository; please close this ASF PR. |
e37a8c4 to
8ca9601
Compare
8ca9601 to
40b5f42
Compare
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The selected head still has unresolved authorization and lifecycle correctness risks, and the workflow's exact-head runs are waiting for maintainer approval. Evidence: current-head checks show seven action_required workflow runs; the inline findings below are independent of that external gate.
| */ | ||
| String caller = Thread.currentThread().getName(); | ||
| if (!caller.contains(TraversalStrategiesProxy.REST_WORKER)) { | ||
| for (HugePermission permission : |
There was a problem hiding this comment.
Blocking: yes. Summary: The new traversal pre-check maps data mutations to ResourceType.GREMLIN WRITE/DELETE, so existing roles with data-resource WRITE/DELETE plus GREMLIN EXECUTE are rejected even though the public Gremlin contract only grants gremlin_execute. Evidence: lines 2469-2475 call verifyNamePermission with ResourceType.GREMLIN, while the existing role fixtures grant WRITE on VERTEX and EXECUTE on GREMLIN; resource matching is type-specific. Please check the actual graph data resource or introduce and migrate explicit GREMLIN WRITE/DELETE permissions with compatibility tests.
| Context old = getContext(); | ||
| AuthContext.useAdmin(); | ||
| return old; | ||
| public static void runAsAdmin(Runnable runnable) { |
There was a problem hiding this comment.
Blocking: yes. Summary: This new public admin closure can be invoked by untrusted Gremlin reflection unless it is explicitly hidden, and it changes the caller seen by every operation in the closure to User.ADMIN. Evidence: runAsAdmin() sets AuthContext to User.ADMIN at lines 207-211, getContext() gives that context priority at lines 221-225, while HugeFactoryAuthProxy.registerPrivateActions() filters getContext/resetContext but does not filter runAsAdmin. Please make this trusted bridge non-public or add it to the script method denylist, and add an unauthorized Gremlin regression test.
| } else { | ||
| authManager.deleteDefaultRole(name, user, defaultRole); | ||
| if (defaultRole.equals(HugeDefaultRole.OBSERVER)) { | ||
| for (String currentGraph : manager.graphs(name)) { |
There was a problem hiding this comment.
Blocking: yes. Summary: Space-wide OBSERVER cleanup only visits graphs currently returned by manager.graphs(name), so a legacy <graph>_observer role for a graph deleted earlier is never removed and can become active again if that graph name is recreated. Evidence: this loop is the only legacy-role cleanup at lines 276-280, while GraphManager.dropGraph() removes graph config before clearing the backend and does not remove the graph-scoped auth metadata. Please enumerate legacy observer roles from auth metadata or clean them during graph deletion, and cover delete/recreate with a user and group.
| LockUtil.lock(this.spaceGraphName(), LockUtil.GRAPH_LOCK); | ||
| try { | ||
| if (this.isHstore()) { | ||
| ((CachedSchemaTransactionV2) this.schemaTransaction()).clear(); |
There was a problem hiding this comment.
Blocking: yes. Summary: The new HStore schema clear is protected only by GRAPH_LOCK, but schema writes use independent per-type/per-id locks, so a concurrent schema write can recreate metadata after super.clear() and leave it inconsistent with the subsequently cleared store. Evidence: StandardHugeGraph.clearBackend() calls CachedSchemaTransactionV2.clear() at line 523, while SchemaTransactionV2.saveSchema() and removeSchema() acquire only hugeType2Group(schema.type()) locks. Please coordinate clear with the same graph-wide/distributed schema-write lock and add a concurrent clear/write regression.
| // Clear schema info firstly | ||
| super.clear(); | ||
| this.clearCache(false); | ||
| this.notifySchemaCacheClear(); |
There was a problem hiding this comment.
Blocking: no. Summary: HStore clear now broadcasts schema-cache-clear directly, and storeProvider.clear() immediately emits STORE_CLEAR, whose listener calls clearCache(true) and broadcasts the same event again under the default task.sync_deletion=false. Evidence: the new notifySchemaCacheClear() at line 470 follows clearCache(false), while the existing STORE_CLEAR listener uses notify = !STORE_INIT and clearCache(true). Please make one layer own the broadcast or add a clear-operation guard, and verify the event is emitted once.
| ACTUAL_PROPERTY=$(unzip -p "$COMMON_JAR" version.properties | | ||
| sed -n 's/^ApiVersion=//p' | tr -d '\r') | ||
|
|
||
| [[ "$ACTUAL_MANIFEST" == "$EXPECTED_MANIFEST" ]] || { |
There was a problem hiding this comment.
Blocking: no. Summary: This check validates the manifest and version.properties against separate source literals, but it does not verify the version returned by the runtime endpoint or that the two API version representations are equivalent. Evidence: ApiVersion.VERSION prefers the API JAR manifest and VersionAPI returns it, so this head can pass with manifest 0.72.0.0 and property 0.72; lines 112-118 compare each artifact independently and never exercise /versions. Please compare normalized canonical values and assert the packaged runtime API response as well.
Closeout TODO
0.72consistently through source properties, the APIJAR manifest, and the runtime version endpoint.
while keeping historical per-graph observer grants removable.
succeeds without a warm-up request.
templates within their authority.
authenticator in anonymous mode.
remove the earlier Server-side PD dynamic IP/allowlist refresh.
guidance, and current-PR formatting without reformatting legacy sources.
1.5–1.8 compatibility
0.72GraphSpace and templateauthorization contract used by current Hubble.
0.71contract; compatibility handlingstays in the Client/Hubble boundary.
Before → After
flowchart LR B["Before<br/>master and 1.7 both reported API 0.71<br/>future-graph roles were incomplete<br/>first writes needed prior limiter state<br/>PD address refresh leaked into Server"] A["After<br/>master reports API 0.72<br/>GraphSpace-wide default roles<br/>first authenticated write works directly<br/>Kubernetes DNS owns peer stability"] B --> A0.72Current state
7083242676277b2536614e4804c4f93313707bed28641f54; the only later diff is the dependency-review Action upgrade