H5json - #450
Merged
Merged
H5json#450
Conversation
The branch diverged from master in Feb 2025. Resolves 20 conflicted files plus one modify/delete. Notes on the non-obvious resolutions: Relocations (took the h5json side; helpers moved into the h5json package, verified no remaining references): util/idUtil.py deleted (-> h5json.objid), util/chunkUtil.py and util/dsetUtil.py helper removals, node_runner.removeSitePackages, ctype_dn obj_class "datatype" -> "datatypes" (h5json.objid.validateUuid takes plural collection names), datanode_lib getLayoutClass -> getDatasetLayoutClass / getChunkLayout -> getChunkDims. Kept both sides (independent changes to adjacent lines): basenode: cluster_state="WAITING" on HTTPInternalServerError plus master's new HTTPServiceUnavailable handler. fileClient.get_object: master's bytes_in stat plus posix_delay. chunklocator: master's log_format/timestamps config plus getNow() (required - the paired stop_time already uses getNow()). chunk_crawl: the slimmed h5json imports plus master's `from . import metrics`, which has live call sites. pyproject/testall: version 1.0.0 and the h5json test list, plus master's prometheus-client dep and metrics_test. Behavioural picks: attr_sn: bytesToArray() - equivalent to master's explicit scalar reshape (getShapeDims returns () for H5S_SCALAR) and additionally handles vlen, which np.frombuffer does not. chunk_sn: select_dtype, not dset_dtype - the byte-count check above it uses select_item_size, so dset_dtype would contradict it for a "fields" selection. domain_test: dropped master's include_attrs assertions; the merged getDomainObjs() has no include_attrs and now reads .summary.json. requirements: the h5json pin set, but urllib3/requests keep master's forward move (2.5.0/2.32.4). The h5json branch never touched those pyproject constraints, so the merged pyproject carries master's urllib3>=2.4.0; keeping the 1.26.20 pin would contradict it. Also wrapped one line in chunk_crawl.py that master's added `with metrics.crawler_task(...)` indent pushed past the 99-col limit.
GET / with no domain returns the top-level domain list again, as HDFGroup#444 ("Nullreq") had it, rather than the 400 introduced on this branch in 3e87452. The 400 was not a security boundary: get_domains() does no per-domain authorization, and GET /domains - which the 400's own message points callers to - calls that same function with the same auth, so the identical listing was reachable either way. Restoring it avoids regressing HDFGroup#444's "support null requests" work. The block sits where HDFGroup#444 had it (after verbose/getobjs), and getBucketForDomain() already returns None for a falsy domain, so the bucket check above it is a no-op on this path. openapi.yml updated to match, and testGetRootNoDomain added - neither branch covered this path, which is how the behavior went missing in the merge unnoticed. Removed getDomainObjects() from domain_sn.py: no callers left, the logic moved into servicenode_lib.getDomainObjs(), which reads the consolidated .summary.json instead of crawling. GET_Domains built an hrefs list and then assigned rsp_json["hrefs"] = [], discarding it (came in verbatim from HDFGroup#444). Now assigns hrefs, so the response carries its self link. Added logger_test to testall.py's unit_tests. HDFGroup#446 added the test file but never touched testall.py, so it had never run; HDFGroup#447 added its metrics_test the same month, which is the intended convention. Added a release workflow: a pushed v* tag becomes a GitHub release, with notes from --generate-notes. HSDS is not on PyPI, so that is all it does; docker images are already built on the same tags by docker-image.yml. The version job resolves the version from pyproject.toml and fails if the tag disagrees with it, or if HSDS_VERSION in basenode.py has drifted - basenode's copy is what the service reports over /about, so releasing with the two out of sync would ship a service that misstates its own version. workflow_dispatch runs the validation half only, to rehearse off a branch.
mattjala
added a commit
to mattjala/h5pyd
that referenced
this pull request
Aug 31, 2026
h5pyd's h5json work needs the matching HSDS server, which is not on HDFGroup/hsds master yet - it is in HDFGroup/hsds#450. CI checking out master therefore tests h5pyd's new client against an old server and fails for reasons unrelated to h5pyd. Point the HSDS checkout at mattjala/hsds merge/master-into-h5json so CI produces useful signal now. Revert to repository: HDFGroup/hsds with no ref: once #450 lands.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Three debug statements wrote live credentials into the log in clear text. The deployments that would most want debug logging are the ones where these logs are shipped somewhere and retained, and HSDS's own CI runs at LOG_LEVEL=DEBUG. - authUtil.validateUserPasswordDynamoDB logged the stored password item fetched from DynamoDB. The two checks directly above it already validate the item's shape, so the line's only unique contribution was the secret; dropped. - jwtUtil.verifyBearerToken logged the raw bearer token, which is replayable until it expires. Now logs its presence and length. - s3Client logged AWS_SESSION_TOKEN, a live credential. Now logs only that it was found. Left alone: s3Client also logs the AWS access key id, which is an identifier rather than a secret - it travels in every signed request. These are pre-existing on master and unrelated to any in-flight branch, so they are split out here rather than folded into a larger change.
CodeQL flags py/path-injection in fileClient, and it is a real finding rather than noise. Buckets and keys come from the request, and neither validator caught a ".." segment: _validateBucket rejected "/" and "\" but not ".." itself, and _validateKey rejected only a *leading* slash. With root_dir=/var/hsds_data, bucket "hsdstest" and key "../../../etc/shadow" resolved to /etc/shadow - _getFilePath called normpath but never checked where the result landed. Rejects ".." path segments in keys and "."/".." as a bucket, and adds _checkPathInRoot() as a backstop that does not depend on having enumerated the traversal forms: it runs after normpath has collapsed the segments and requires the result to sit under root_dir. Uses commonpath rather than startswith, so a sibling directory sharing the root's prefix (/var/hsds_data_evil) is not mistaken for being inside it. Applied at all three places a path is built from caller input: _getFilePath, _mkdir - reached from put_object with a path assembled per key segment, which is the line CodeQL points at - and list_keys, which joins the prefix itself rather than going through _getFilePath. Adds tests/unit/file_client_test.py covering both the validators and the backstop, and registers it in testall.py.
mattjala
added a commit
to mattjala/h5pyd
that referenced
this pull request
Sep 1, 2026
h5pyd's h5json work needs the matching HSDS server, which is not on HDFGroup/hsds master yet - it is in HDFGroup/hsds#450. CI checking out master therefore tests h5pyd's new client against an old server and fails for reasons unrelated to h5pyd. Point the HSDS checkout at mattjala/hsds merge/master-into-h5json so CI produces useful signal now. Revert to repository: HDFGroup/hsds with no ref: once #450 lands. Kept as the last commit on the branch so it can be dropped without touching anything else.
This was referenced Sep 1, 2026
Closed
mattjala
added a commit
that referenced
this pull request
Sep 1, 2026
python-package.yml ran the HSDS suite and the h5pyd integration test in one workflow, so they shared a single status. A badge reports a whole workflow run and GitHub has no job-level badge - the ?job= query parameter is accepted but silently ignored - so an h5pyd failure made HSDS itself look broken, which is exactly what happened while #450 was in review: 22 green checks, one red h5pyd job, one red badge. Splits the workflow by what a red badge should mean: hsds-tests.yml reusable, holds the HSDS suite, takes the OS list and whether to run the socket tests hsds-linux.yml calls it for ubuntu-22.04 + ubuntu-latest, and the unix-socket suite, which is linux-only hsds-windows.yml calls it for windows-latest h5pyd-integration.yml h5pyd-integration + test-data-setup The callers are reusable-workflow stubs rather than copies, so the suite is still defined once. Three badges, and "linux green, windows red" is legible at a glance - worth having, since the last windows-only failure took a long time to distinguish from a general breakage. No change to what runs: the same four jobs, the same steps, and the same 18 build-and-test cells (12 linux + 6 windows). The only edits to the job bodies are the parameterised matrix OS list and an "if" on the socket job so the windows caller skips it. Note this renames the checks, so any branch protection rule naming "build-and-test (...)" needs updating - and it is now possible to require only the HSDS checks, so an h5pyd-side failure cannot block an HSDS merge.
d-gski
added a commit
to d-gski/hsds
that referenced
this pull request
Sep 29, 2026
Since 1.0, the chunk shape comes from h5json's getChunkDims, which returns the full dataset shape for any layout that isn't H5D_CHUNKED*. For a contiguous reference that makes the whole dataset one chunk: reading a single element makes the DN range-get, decode and cache the entire dataset from the linked file. This regressed in b6016e0 ("use hdf5-json util classes", HDFGroup#450). Through 0.9.x, POST_Dataset gave every contiguous reference a virtual chunk shape via getContiguousLayout, and testContiguousRefDataset required it (HDFGroup#393/HDFGroup#396 fixed its short last chunk on an NREL `meta` dataset). b6016e0 moved layouts under creationProperties, removed that computation, and switched to h5json's getChunkDims. h5json's rule holds for data HSDS stores itself, since generateLayout only picks H5D_CONTIGUOUS below chunk_min, but not for a reference, whose size comes from the linked file. The read path still assumes virtual chunks (per-chunk offsets in getChunkLocations, the DN's short-chunk padding). The reference-layout bug fixed by HDFGroup#468/HDFGroup#469 hid the regression for domains linked by 0.9.x. Seen on NREL's NSRDB TMY domains (nrel-pds-hsds). Each `meta` dataset is an H5D_CONTIGUOUS_REF (MSG: 2,693,287 x 134 B = 361 MB). One-row reads fetched the whole 361 MB (6-34 s from S3). The two such datasets no longer fit a 512m chunk cache and evicted each other. Concurrent requests during a fetch each started another full copy. Add getChunkDims to hsds.util.dsetUtil, used everywhere in place of h5json's. It defers to h5json except for H5D_CONTIGUOUS_REF. There it derives virtual chunks with getContiguousLayout from the dataset's shape, item size and the min/max_chunk_size config. That is the same split 0.9.x made: 21042 rows for the MSG `meta` above, as stored with the domain. Nothing is stored per chunk; each is a range get into the file. So the shape only has to agree between the SN (chunk index -> byte range) and the DN (decode + cache). Deriving it from shared inputs gives that on both, so GET_Dataset keeps reporting the reference layout without dims, as HDFGroup#468/HDFGroup#469 intend.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updated version of #420