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.
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