-
Notifications
You must be signed in to change notification settings - Fork 143
ostree-ext: make OCI history reproducible #1339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @champtar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request to help everyone get up to speed quickly.
This PR addresses issue #5406 by making the OCI image history more reproducible. Previously, when pushing layers to an OCI image, the created timestamp in the layer history entry was set to the current time (chrono::offset::Utc::now()). This meant that pushing the exact same content at different times would result in different OCI image configurations due to the varying timestamps, breaking reproducibility.
The core change in this PR is to modify the layer pushing logic (push_layer calls, now replaced by push_layer_full) to use a stable timestamp instead of the current time. Specifically, it now extracts the created timestamp from the OCI image configuration itself (or the previous layer's history entry when re-exporting) and uses that value for the new layer's history entry. This ensures that the timestamp is consistent regardless of when the push operation occurs, leading to reproducible OCI image history.
Highlights
- Reproducible OCI History: The primary goal is to fix non-reproducible OCI image history caused by using the current time for layer creation timestamps.
- Timestamp Source Change: Layer history entries now use the timestamp from the image configuration or the previous layer's history instead of the current time.
- API Usage Update: Switched from the
push_layermethod to the more flexiblepush_layer_fullmethod in theocidircrate to allow specifying thecreatedtimestamp.
Changelog
Click here to see the changelog
- ostree-ext/src/container/encapsulate.rs
- Replaced calls to
ociw.push_layerwithociw.push_layer_full(lines 147, 162). - Added logic to extract and parse the
createdtimestamp from the image configuration (imgcfg) (lines 136-144). - Passed the extracted
createdtimestamp to thepush_layer_fullcalls (lines 153, 168).
- Replaced calls to
- ostree-ext/src/container/store.rs
- Replaced call to
dest_oci.push_layerwithdest_oci.push_layer_full(line 156). - Added logic to extract and parse the
createdtimestamp from the previous history entry in the source image configuration (srcinfo.configuration.history()) (lines 144-154). - Passed the extracted
previous_createdtimestamp to thepush_layer_fullcall (line 153).
- Replaced call to
- ostree-ext/src/fixture.rs
- Added
use std::collections::HashMap;import (line 29). - Replaced call to
self.src_oci.push_layerwithself.src_oci.push_layer_full(line 1028). - Added logic to extract and parse the
createdtimestamp from the current image configuration (config) (lines 1018-1026). - Passed the extracted
createdtimestamp to thepush_layer_fullcall (line 1034).
- Added
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Time flies, code changes fast,
But history, built to last,
Should stand the test of time,
Reproducible, sublime.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses the reproducibility issue with OCI history by ensuring a deterministic created timestamp is used for layers, rather than Utc::now(). The changes are consistently applied across the relevant files (encapsulate.rs, store.rs, and fixture.rs) by switching from push_layer to push_layer_full and providing a controlled timestamp.
The approach of defaulting to the epoch (1970-01-01T00:00:00Z) if a created timestamp is not available or is malformed is sound for ensuring reproducibility. My main suggestion revolves around making this fallback behavior more observable through logging, to help users debug cases where a provided timestamp might be invalid.
Overall, this is a good fix for the reported issue.
Summary of Findings
- Silent fallback for malformed 'created' timestamps: In
encapsulate.rs,store.rs, andfixture.rs, when parsingcreatedtimestamps from image configurations or history, the code usesunwrap_or_default(). This correctly ensures a deterministic fallback (to epoch) for reproducibility if a timestamp is missing or malformed. However, it silently ignores parsing errors for provided timestamps. Adding a warning log when a provided timestamp string fails to parse would improve debuggability for users who might provide an invalid format.
Merge Readiness
The pull request correctly addresses the reproducibility issue by ensuring deterministic created timestamps for OCI layers. The implementation is sound.
I have one medium-severity suggestion regarding logging for malformed timestamps, which would improve user experience and debuggability. Addressing this point would make the changes more robust.
As an AI reviewer, I am not authorized to approve pull requests. Please ensure this PR is reviewed and approved by a human maintainer before merging.
2e59930 to
de9a659
Compare
Signed-off-by: Etienne Champetier <[email protected]>
OciDir push_layer() calls push_layer_full() with created = chrono::offset::Utc::now() Signed-off-by: Etienne Champetier <[email protected]>
This allows convenient control over all parts of the history, including the timestamp. Replaces #36 Related to bootc-dev/bootc#1339 Signed-off-by: Colin Walters <[email protected]>
This allows convenient control over all parts of the history, including the timestamp. Replaces #36 Related to bootc-dev/bootc#1339 Signed-off-by: Colin Walters <[email protected]>
This allows convenient control over all parts of the history, including the timestamp. Replaces #36 Related to bootc-dev/bootc#1339 Signed-off-by: Colin Walters <[email protected]>
This allows convenient control over all parts of the history, including the timestamp. Replaces #36 Related to bootc-dev/bootc#1339 Signed-off-by: Colin Walters <[email protected]>
OciDir push_layer() calls push_layer_full() with
created = chrono::offset::Utc::now()
This should fix coreos/rpm-ostree#5406