Conversation
Contributor
|
Claude finished @mbevc1's task in 2m 56s —— View job Follow-up review complete
Reviewed Status of previously raised findings touched by this push
All corresponding review threads are already marked resolved on GitHub — nothing left to close. New issues in the diffNone. I checked in particular:
Note on verificationI was unable to run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
mbevc1
force-pushed
the
20260801_s3_metadata
branch
from
August 1, 2026 22:35
8aa8a82 to
e5e7859
Compare
mbevc1
force-pushed
the
20260801_s3_metadata
branch
from
August 4, 2026 14:14
e5e7859 to
f7e23d2
Compare
3 tasks
6 tasks
3 tasks
3 tasks
mbevc1
force-pushed
the
20260801_s3_metadata
branch
from
September 16, 2026 11:38
f7e23d2 to
25c39ea
Compare
mbevc1
force-pushed
the
20260801_s3_metadata
branch
2 times, most recently
from
September 16, 2026 14:33
6a95885 to
fa34711
Compare
mbevc1
commented
Sep 25, 2026
mbevc1
force-pushed
the
20260801_s3_metadata
branch
from
September 25, 2026 14:27
5489251 to
b2f9410
Compare
mbevc1
marked this pull request as ready for review
September 25, 2026 14:34
Fingerprinting a bucket from the checksums S3 already stores needs object metadata, not object content. Add S3HeadAPI to the S3API composite, backed by the same *s3.Client that already serves listing, and give FakeS3Client a HeadObject to match. The fake models stored checksums sparsely, via a Checksums map rather than deriving them from object bytes: an object uploaded without an explicit checksum algorithm has none, and that is the common case a caller has to handle. It also withholds the checksum unless the request sets ChecksumMode, exactly as S3 does -- a fake that always returned it would hide a caller that forgets to ask. The contract tests gain a sha256ChecksumKey parameter and cover metadata retrieval, the missing-key error, and both sides of the ChecksumMode behaviour. They skip when no checksum-bearing object is available, which is the case for kosli-cli-public today: adding one there would change the golden fingerprints TestGetS3Data pins.
fingerprintS3Objects hard-wired how each object's sha256 is obtained: download to a temp file and hash. Split that step out as s3DigestSource so a second source can supply digests without touching the disk, and keep everything else -- the key rule, the root .kosli_ignore download and its rules, the tree walk, the parallel fan-out -- in one shared pipeline, fingerprintS3Tree. Two sources cannot then fingerprint the same bucket differently, which is the property the ADR asks of metadata mode. The fan-out charges an object's listed size against the byte budget only when the source uses the disk; a source that reads metadata owes it nothing, so its concurrency is bounded by the worker count alone. fingerprintS3Objects keeps its signature as the content-mode entry point, so the parallel-download suite is unchanged and, with the pinned and attested-directory fingerprints, is the proof this is a pure refactor.
…tadata kosli snapshot s3 downloads every contributing object and hashes it. For a large bucket that is a full egress and a SHA256 pass over every byte, on top of the temp disk the download budget allows. --fingerprint-source metadata reads the SHA256 checksum S3 already stores for each object instead, with a HeadObject that asks for it. It is a second digest source plugged into the shared pipeline, so the key rule, the root .kosli_ignore and the tree walk are exactly content mode's and the fingerprint is byte for byte the same -- the pinned fingerprints and the attested-directory equality now hold for both sources. What this does not save is permissions. AWS requires s3:GetObject to read an object's checksum, the same permission downloading it needs, so the help text says so rather than letting anyone infer otherwise. Every contributing object must carry a full-object SHA256 checksum, which S3 only stores when the upload asked for one. A composite (multipart) checksum hashes the part checksums rather than the object and is rejected on both signals S3 gives -- the COMPOSITE type and the "-N" suffix the SDK's own response validation keys off -- with copy-object as the fix, which collapses the parts in place without the original file. Such problems describe the object rather than the connection, so the fan-out collects them and one run names every object that needs fixing, capped like key problems are; a transport error still stops the run at once. The root .kosli_ignore is downloaded as before, whatever the source, since its rules decide which objects contribute; objects the rules exclude are never fetched and so need no checksum. A source that reads metadata owes the byte budget nothing, so its HEADs are bounded by the worker count alone. decodeLambdaFingerprint becomes decodeBase64Sha256 now that Lambda's CodeSha256 is not the only Base64 digest AWS hands us.
Say what --fingerprint-source metadata changes and, as importantly, what it does not. The pipeline is shared, so keys, .kosli_ignore rules and the fingerprint itself are the same in both modes; only where each object's digest comes from differs. The two conditions a bucket must meet -- a stored full-object SHA256 on every contributing object, and no composite multipart checksums -- come with the aws command that fixes each. The obvious assumption is that reading metadata needs weaker permissions than downloading. AWS requires s3:GetObject for both, so the help says so plainly rather than leaving the reader to infer a benefit that is not there.
The help promises that metadata mode still downloads a root .kosli_ignore, since its rules decide which objects contribute. A bucket whose only object is that file broke the promise: the single-file branch runs before the ignore pass and fetched it through the metadata source, so the snapshot failed unless the ignore file itself had been uploaded with a checksum. Route that one case through the download source, so no source ever asks the ignore file for a checksum. Content mode is unchanged -- it downloaded the file either way. TestMatchesContentMode's lone-ignore case missed this because checksummedBucket gives every object a checksum; the new test leaves the ignore file without one and asserts it is downloaded, never HEADed.
Any "-" in a stored checksum was taken to mean a composite (multipart) value, following the SDK's own response validation. That is sound for standard Base64, which never contains a "-", but it misreports any other value that does: the user is told to collapse parts with copy-object, which does nothing for a checksum that was never composite. Require the "-" to be followed by the digits of a part count. A composite is still caught either by that suffix or by the COMPOSITE type, and any other "-" now reaches the Base64 decode and fails there, with an error that says the value cannot be decoded.
Metadata mode sized its fan-out from --download-concurrency, whose default of 8 exists because each download in flight may buffer up to 40 MB. A HeadObject buffers nothing and touches no disk, so that figure throttled metadata mode without describing anything it does: around an hour of checksum reads for a million-object bucket, the kind of bucket the mode is for. When --download-concurrency is not given, metadata mode now runs 32 reads at once -- a million objects in under twenty minutes at a 30 ms round trip, still far below S3's per-prefix request rate, with the adaptive retryer absorbing any throttling. A value given by flag, environment or config file still decides in either mode: bindFlags applies the latter two through Flags().Set before PreRunE runs, so all three mark the flag changed. The choice lives in the command, which is the only place that knows whether the user set the flag; the aws package keeps taking whatever limits it is handed.
…verns The help stated that each object is downloaded to a temporary file before the paragraph introducing --fingerprint-source said otherwise, so a reader of the generated docs met a claim that holds only for the default source. It now says "by default". The two download flags also read as if they applied to both sources. --download-concurrency now says it bounds checksum reads in metadata mode, where it defaults to 32, and a test keeps that figure equal to the constant, since cobra prints only the flag's own default. --download-budget says it has no effect in metadata mode, which uses no temporary disk. A bad budget is still rejected in either mode: an invalid value is invalid whether or not this run would have used it.
Keep each comment to the one fact a reader could not get from the code, stated once, where it applies. Several facts were repeated across files: that S3 returns a stored checksum only when asked appeared four times, that unusable checksums are collected rather than fatal three, and that the shared pipeline keeps the sources from disagreeing three. Each now lives in one place -- the interface, the error type and the digest source respectively -- and the copies are gone. Test comments that restated their own assertions or case names are dropped, and the rest are cut to their load-bearing sentence. Godoc on exported identifiers stays. No code changes; gofmt re-aligned two structs whose field comments were removed.
mbevc1
force-pushed
the
20260801_s3_metadata
branch
from
September 25, 2026 15:26
045d16a to
d4d7627
Compare
mbevc1
enabled auto-merge (squash)
September 25, 2026 15:27
This branch has not been deployed
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
1 participant
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.
Adds
--fingerprint-source content|metadatatokosli snapshot s3.Today the command downloads every contributing object and hashes it. For a large bucket that
means full egress and a SHA256 pass over every byte, on top of the temp disk the download
budget allows. With
--fingerprint-source metadatathe command reads the SHA256 checksum S3already stores for each object instead, via
HeadObjectwithChecksumModeenabled — nodownload, no hashing.
Default is
content, so existing behaviour is unchanged.Rebased onto the shared pipeline from #1180 and #1191
This is the landing the ADR (
docs/adr/20260911-s3-fingerprint-from-virtual-tree.md)described: metadata mode is a digest source plugged into the same list, normalise, exclude,
tree pipeline as content mode. The pipeline gained one seam,
s3DigestSource, and contentmode became its first implementation; nothing else about content mode changed, which
TestPinnedFingerprints,TestMatchesAttestedDirectoryand the parallel-download suite hold.Because the pipeline is shared:
a//b,/lead.txt,./c.txtall land wherefilepath.Joinput them), and the same collisions are reported thesame way. This PR's earlier stricter rule is gone, as the ADR called for.
.kosli_ignoreis applied, not rejected. It is downloaded — one object — as incontent mode, its rules decide what contributes, and objects the rules exclude are never
fetched and so need no checksum. The earlier "reject the bucket" behaviour is gone.
TestMatchesContentModecompares the wholeS3Datafrom both sources over the same fake buckets, including one with ignore rules and onewith unusual keys, and the pinned fingerprints from
mainare re-derived from checksums alone.Conditions a bucket must meet
upload asked for it (
aws s3api put-object --checksum-algorithm SHA256).object content. Detection keys off the observed
COMPOSITEtype and the-Nsuffix — thelatter being what the SDK's own response validation uses — so a full-object SHA256 is accepted
however it was uploaded. The fix offered is
aws s3api copy-object --checksum-algorithm SHA256, which collapses the parts in place without the original file.Both are properties of the object, not the connection, so the fan-out collects them and one run
names every object that needs fixing, capped like key problems are (ten). A transport error
still stops the run at once.
It does not save permissions: AWS requires
s3:GetObjectto read a checksum, the same onedownloading needs. The help says so.
Commits
feat(aws):HeadObjecton the seam; fake stores checksums sparsely and withholds themunless
ChecksumModeis set, as S3 does; contract sub-tests (skip against real AWS until achecksum-bearing object exists — not in
kosli-cli-public, whose fingerprintTestGetS3Datapins).
refactor(snapshot s3):s3DigestSource; pure refactor, existing suites are the proof.feat(snapshot s3): the flag and the metadata source.docs(snapshot s3): help text.Not run here:
SnapshotS3TestSuite(needs the local server) — five new cases were added withgoldens mirroring the pinned error text.
Checklist
charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR