Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make image builds reproducible #4142

Closed
wants to merge 8 commits into from
Closed

Make image builds reproducible #4142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
095343c
Set access, creation, and last-modified time to zero
bjornbugge Nov 17, 2023
6376c32
Add small test that ensures that modtime doesn't change layer
bjornbugge Nov 17, 2023
05bd7f8
Also clear time-related PAX headers on parent-dir entries
bjornbugge Nov 17, 2023
9f048d1
Add blank line for checkstyle
bjornbugge Jan 24, 2024
24162dd
Fix the modification-time tests
bjornbugge Jan 25, 2024
8175386
Rename `clearTimeHeaders` to `clearPaxTimeHeaders`
bjornbugge Jan 25, 2024
de4ab25
Set PAX headers to EPOCH plus 1
bjornbugge Jan 29, 2024
dc3633b
Rename to`clearTimeHeaders` and call `setModTime` in helper
bjornbugge Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions jib-core/src/main/java/com/google/cloud/tools/jib/image/ReproducibleLayerBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,11 @@ private void add(TarArchiveEntry tarArchiveEntry) throws IOException {
if (namePath.getParent() != namePath.getRoot()) {
Path tarArchiveParentDir = Verify.verifyNotNull(namePath.getParent());
TarArchiveEntry dir = new TarArchiveEntry(DIRECTORY_FILE, tarArchiveParentDir.toString());
dir.setModTime(FileEntriesLayer.DEFAULT_MODIFICATION_TIME.toEpochMilli());
dir.setUserId(0);
dir.setGroupId(0);
dir.setUserName("");
dir.setGroupName("");
clearTimeHeaders(dir); // DEFAULT_MODIFICATION_TIME == EPOCH+1
add(dir);
}

Expand All @@ -95,6 +95,14 @@ private List<TarArchiveEntry> getSortedEntries() {
}
}

private static void clearTimeHeaders(TarArchiveEntry entry) {
bjornbugge marked this conversation as resolved.
Show resolved Hide resolved
entry.setModTime(FileEntriesLayer.DEFAULT_MODIFICATION_TIME.toEpochMilli());
entry.addPaxHeader("mtime", "1");
entry.addPaxHeader("atime", "1");
entry.addPaxHeader("ctime", "1");
entry.addPaxHeader("LIBARCHIVE.creationtime", "1");
}

private static void setUserAndGroup(TarArchiveEntry entry, FileEntry layerEntry) {
entry.setUserId(0);
entry.setGroupId(0);
Expand Down Expand Up @@ -156,8 +164,8 @@ public Blob build() throws IOException {
// Sets the entry's permissions by masking out the permission bits from the entry's mode (the
// lowest 9 bits) then using a bitwise OR to set them to the layerEntry's permissions.
entry.setMode((entry.getMode() & ~0777) | layerEntry.getPermissions().getPermissionBits());
entry.setModTime(layerEntry.getModificationTime().toEpochMilli());
setUserAndGroup(entry, layerEntry);
clearTimeHeaders(entry);

uniqueTarArchiveEntries.add(entry);
}
Expand Down
140 changes: 74 additions & 66 deletions jib-core/src/test/java/com/google/cloud/tools/jib/image/ReproducibleLayerBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.nio.file.StandardOpenOption;
import java.nio.file.attribute.FileTime;
import java.time.Instant;
import org.apache.commons.compress.archivers.ArchiveEntry;
import org.apache.commons.compress.archivers.tar.TarArchiveEntry;
import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
import org.junit.Rule;
Expand Down Expand Up @@ -84,12 +85,33 @@ private static void verifyNextTarArchiveEntryIsDirectory(
assertThat(extractionPathEntry.getMode()).isEqualTo(TarArchiveEntry.DEFAULT_DIR_MODE);
}

private static FileEntry defaultLayerEntry(Path source, AbsoluteUnixPath destination) {
/**
* Verifies that the modification time has been reset in the PAX headers.
*
* @param entry The archive entry in the layer.
*/
private static void verifyThatModificationTimeIsReset(ArchiveEntry entry) {
assertThat(entry.getLastModifiedDate().toInstant()).isEqualTo(FileEntriesLayer.DEFAULT_MODIFICATION_TIME);
}

private static FileEntry layerEntry(Path source, AbsoluteUnixPath destination, FilePermissions permissions, String ownership) throws IOException {
return new FileEntry(
source,
destination,
FileEntriesLayer.DEFAULT_FILE_PERMISSIONS_PROVIDER.get(source, destination),
FileEntriesLayer.DEFAULT_MODIFICATION_TIME);
permissions,
// Here we make sure to use the actual modification-time here because that's what would happen in
// regular use when copying the file from disk into the layer.
Comment on lines +102 to +103
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the regular use is to force setting the file timestamp to epoch+1 by default. The only other alternative for the user is to set a static time. Jib never sets the original timestamp of the source file.

From the Jib plugin doc:

The value should either be EPOCH_PLUS_SECOND to set the timestamps to Epoch + 1 second (default behavior), or an ISO 8601 date-time parsable with DateTimeFormatter.ISO_DATE_TIME such as 2019-07-15T10:15:30+09:00 or 2011-12-03T22:42:05Z.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can see my comment was a bit unclear. What I meant was that in normal use, Jib reads in files from disk and they have some "real" modification time. Then Jib is supposed to reset this, but that didn't happen (or at least didn't happen "enough" because there were still PAX headers with the timestamps in them).
Because the test helper created test files where the modtime was already reset, this issue wasn't visible in the test suite.

Copy link
Member

@chanseokoh chanseokoh Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then Jib is supposed to reset this, but that didn't happen (or at least didn't happen "enough" because there were still PAX headers with the timestamps in them).

This does happen. To clarify, the ordinary modification time is correctly set to epoch+1. It's just that the newer apache compress library added or set some new PAX headers with timestamp values. AFAICT, the mod time and the time values in PAX headers are independent. Everything worked fine without this PR, except that the new headers had dynamic values hence affected reproducibility. As long as we set the ordinary mod time to epoch+1, I think it shouldn't matter what time you set for the PAX headers. So ideally, I think what we need to test is these two:

  1. Verify that Jib continues to set the ordinary mod time to epoch+1 by default.
  2. Verify that Jib wipes out the time PAX headers. I think the value doesn't matter, as long as people don't complain.

Copy link
Contributor Author

@bjornbugge bjornbugge Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ad 1: The PAX headers take precedence it seems, so I'll set them to 1 and try to switch the ordering like you suggested below 👍
Ad 2: I believe that testing the wiping of the headers is captured in the existing test case testToBlob_reproducibility, as it compares the raw bytes. This test would fail after I updated the helper function here, hence my point with the old version of this helper "hiding" the issue with PAX headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. They're set to 1 now and it's asserted (again).
  2. If I disable the PAX header reset then the test testToBlob_reproducibility fails because of mismatched byte arrays. So I'd argue that this is covered as well :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes of course, I forgot that it's user-configurable 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although - I'm a bit at a loss as to where the modTime is supposed to be reset. In line 79 and 167 in ReproducibleLayerBuilder.kt there are calls to .setModTime. In the former case, for directories, it's always set to EPOCH+1 regardless of any user settings. In the latter case, for files, it's always set to the file's real value on disk, which is not reproducible. Are there any other places in the code base where the modification time could've been reset?

Copy link
Member

@chanseokoh chanseokoh Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

167 is the user-configured value. Just set both the PAX headers and setModTime() to the same timestamp. For the (Jib-created) directiories, it is not customizable and always set to epoch+1.

To recap, just set the same value as Jib does now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @chanseokoh - I've been away from this work for a while due to other things. I've read the code more carefully now and I'll now understand that I was confused earlier as to where the responsibility of (re)setting the datetime lies. I'll set the PAX headers to the same timestamp as setModTime 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjornbugge too bad that you missed the release 3.4.1.

Files.getLastModifiedTime(source).toInstant(),
ownership
);
}

private static FileEntry layerEntry(Path source, AbsoluteUnixPath destination, FilePermissions permissions) throws IOException {
return layerEntry(source, destination, permissions, "");
}

private static FileEntry layerEntry(Path source, AbsoluteUnixPath destination) throws IOException {
return layerEntry(source, destination, FileEntriesLayer.DEFAULT_FILE_PERMISSIONS_PROVIDER.get(source, destination));
}

@Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder();
Expand Down Expand Up @@ -169,14 +191,14 @@ public void testToBlob_reproducibility() throws IOException {
Blob layer =
new ReproducibleLayerBuilder(
ImmutableList.of(
defaultLayerEntry(fileA1, AbsoluteUnixPath.get("/somewhere/fileA")),
defaultLayerEntry(fileB1, AbsoluteUnixPath.get("/somewhere/fileB"))))
layerEntry(fileA1, AbsoluteUnixPath.get("/somewhere/fileA")),
layerEntry(fileB1, AbsoluteUnixPath.get("/somewhere/fileB"))))
.build();
Blob reproduced =
new ReproducibleLayerBuilder(
ImmutableList.of(
defaultLayerEntry(fileB2, AbsoluteUnixPath.get("/somewhere/fileB")),
defaultLayerEntry(fileA2, AbsoluteUnixPath.get("/somewhere/fileA"))))
layerEntry(fileB2, AbsoluteUnixPath.get("/somewhere/fileB")),
layerEntry(fileA2, AbsoluteUnixPath.get("/somewhere/fileA"))))
.build();

byte[] layerContent = Blobs.writeToByteArray(layer);
Expand All @@ -195,7 +217,7 @@ public void testBuild_parentDirBehavior() throws IOException {
Path ignoredParent = Files.createDirectories(testRoot.resolve("dirB-ignored"));
Path fileB = Files.createFile(ignoredParent.resolve("fileB"));
Path fileC =
Files.createFile(Files.createDirectories(testRoot.resolve("dirC-absent")).resolve("fileC"));
Files.createFile(Files.createDirectories(testRoot.resolve("dirC-absent")).resolve("fileC"));

Blob layer =
new ReproducibleLayerBuilder(
Expand Down Expand Up @@ -236,7 +258,7 @@ public void testBuild_parentDirBehavior() throws IOException {
// root (default folder permissions)
TarArchiveEntry root = in.getNextTarEntry();
assertThat(root.getMode()).isEqualTo(040755);
assertThat(root.getModTime().toInstant()).isEqualTo(Instant.ofEpochSecond(1));
bjornbugge marked this conversation as resolved.
Show resolved Hide resolved
verifyThatModificationTimeIsReset(root);
assertThat(root.getLongUserId()).isEqualTo(0);
assertThat(root.getLongGroupId()).isEqualTo(0);
assertThat(root.getUserName()).isEmpty();
Expand All @@ -245,7 +267,7 @@ public void testBuild_parentDirBehavior() throws IOException {
// parentAAA (custom permissions, custom timestamp)
TarArchiveEntry rootParentA = in.getNextTarEntry();
assertThat(rootParentA.getMode()).isEqualTo(040111);
assertThat(rootParentA.getModTime().toInstant()).isEqualTo(Instant.ofEpochSecond(10));
verifyThatModificationTimeIsReset(root);
assertThat(rootParentA.getLongUserId()).isEqualTo(0);
assertThat(rootParentA.getLongGroupId()).isEqualTo(0);
assertThat(rootParentA.getUserName()).isEmpty();
Expand All @@ -258,8 +280,7 @@ public void testBuild_parentDirBehavior() throws IOException {
TarArchiveEntry rootParentB = in.getNextTarEntry();
// TODO (#1650): we want 040444 here.
assertThat(rootParentB.getMode()).isEqualTo(040755);
// TODO (#1650): we want Instant.ofEpochSecond(40) here.
assertThat(rootParentB.getModTime().toInstant()).isEqualTo(Instant.ofEpochSecond(1));
verifyThatModificationTimeIsReset(root);
assertThat(rootParentB.getLongUserId()).isEqualTo(0);
assertThat(rootParentB.getLongGroupId()).isEqualTo(0);
assertThat(rootParentB.getUserName()).isEmpty();
Expand All @@ -271,7 +292,7 @@ public void testBuild_parentDirBehavior() throws IOException {
// parentCCC (default permissions - no entry provided)
TarArchiveEntry rootParentC = in.getNextTarEntry();
assertThat(rootParentC.getMode()).isEqualTo(040755);
assertThat(rootParentC.getModTime().toInstant()).isEqualTo(Instant.ofEpochSecond(1));
verifyThatModificationTimeIsReset(root);
assertThat(rootParentC.getLongUserId()).isEqualTo(0);
assertThat(rootParentC.getLongGroupId()).isEqualTo(0);
assertThat(rootParentC.getUserName()).isEmpty();
Expand All @@ -287,7 +308,7 @@ public void testBuild_timestampDefault() throws IOException {

Blob blob =
new ReproducibleLayerBuilder(
ImmutableList.of(defaultLayerEntry(file, AbsoluteUnixPath.get("/fileA"))))
ImmutableList.of(layerEntry(file, AbsoluteUnixPath.get("/fileA"))))
.build();

Path tarFile = temporaryFolder.newFile().toPath();
Expand All @@ -297,35 +318,29 @@ public void testBuild_timestampDefault() throws IOException {

// Reads the file back.
try (TarArchiveInputStream in = new TarArchiveInputStream(Files.newInputStream(tarFile))) {
assertThat(in.getNextEntry().getLastModifiedDate().toInstant())
.isEqualTo(Instant.EPOCH.plusSeconds(1));
verifyThatModificationTimeIsReset(in.getNextEntry());
}
}

@Test
public void testBuild_timestampNonDefault() throws IOException {
Path file = createFile(temporaryFolder.getRoot().toPath(), "fileA", "some content", 54321);

Blob blob =
new ReproducibleLayerBuilder(
ImmutableList.of(
new FileEntry(
file,
AbsoluteUnixPath.get("/fileA"),
FilePermissions.DEFAULT_FILE_PERMISSIONS,
Instant.ofEpochSecond(123))))
.build();

Path tarFile = temporaryFolder.newFile().toPath();
try (OutputStream out = new BufferedOutputStream(Files.newOutputStream(tarFile))) {
blob.writeTo(out);
}

// Reads the file back.
try (TarArchiveInputStream in = new TarArchiveInputStream(Files.newInputStream(tarFile))) {
assertThat(in.getNextEntry().getLastModifiedDate().toInstant())
.isEqualTo(Instant.EPOCH.plusSeconds(123));
}
Path file = createFile(temporaryFolder.getRoot().toPath(), "fileA", "some content", 54321);

Blob blob =
new ReproducibleLayerBuilder(
ImmutableList.of(
layerEntry(file, AbsoluteUnixPath.get("/fileA"))))
.build();

Path tarFile = temporaryFolder.newFile().toPath();
try (OutputStream out = new BufferedOutputStream(Files.newOutputStream(tarFile))) {
blob.writeTo(out);
}

// Reads the file back.
try (TarArchiveInputStream in = new TarArchiveInputStream(Files.newInputStream(tarFile))) {
verifyThatModificationTimeIsReset(in.getNextTarEntry());
}
}

@Test
Expand All @@ -338,17 +353,9 @@ public void testBuild_permissions() throws IOException {
Blob blob =
new ReproducibleLayerBuilder(
ImmutableList.of(
defaultLayerEntry(fileA, AbsoluteUnixPath.get("/somewhere/fileA")),
new FileEntry(
fileB,
AbsoluteUnixPath.get("/somewhere/fileB"),
FilePermissions.fromOctalString("123"),
FileEntriesLayer.DEFAULT_MODIFICATION_TIME),
new FileEntry(
folder,
AbsoluteUnixPath.get("/somewhere/folder"),
FilePermissions.fromOctalString("456"),
FileEntriesLayer.DEFAULT_MODIFICATION_TIME)))
layerEntry(fileA, AbsoluteUnixPath.get("/somewhere/fileA")),
layerEntry(fileB, AbsoluteUnixPath.get("/somewhere/fileB"), FilePermissions.fromOctalString("123")),
layerEntry(folder, AbsoluteUnixPath.get("/somewhere/folder"), FilePermissions.fromOctalString("456"))))
.build();

Path tarFile = temporaryFolder.newFile().toPath();
Expand Down Expand Up @@ -376,54 +383,46 @@ public void testBuild_ownership() throws IOException {
Blob blob =
new ReproducibleLayerBuilder(
ImmutableList.of(
defaultLayerEntry(someFile, AbsoluteUnixPath.get("/file1")),
new FileEntry(
layerEntry(someFile, AbsoluteUnixPath.get("/file1")),
layerEntry(
someFile,
AbsoluteUnixPath.get("/file2"),
FilePermissions.fromOctalString("123"),
Instant.EPOCH,
""),
new FileEntry(
layerEntry(
someFile,
AbsoluteUnixPath.get("/file3"),
FilePermissions.fromOctalString("123"),
Instant.EPOCH,
":"),
new FileEntry(
layerEntry(
someFile,
AbsoluteUnixPath.get("/file4"),
FilePermissions.fromOctalString("123"),
Instant.EPOCH,
"333:"),
new FileEntry(
layerEntry(
someFile,
AbsoluteUnixPath.get("/file5"),
FilePermissions.fromOctalString("123"),
Instant.EPOCH,
":555"),
new FileEntry(
layerEntry(
someFile,
AbsoluteUnixPath.get("/file6"),
FilePermissions.fromOctalString("123"),
Instant.EPOCH,
"333:555"),
new FileEntry(
layerEntry(
someFile,
AbsoluteUnixPath.get("/file7"),
FilePermissions.fromOctalString("123"),
Instant.EPOCH,
"user:"),
new FileEntry(
layerEntry(
someFile,
AbsoluteUnixPath.get("/file8"),
FilePermissions.fromOctalString("123"),
Instant.EPOCH,
":group"),
new FileEntry(
layerEntry(
someFile,
AbsoluteUnixPath.get("/file9"),
FilePermissions.fromOctalString("123"),
Instant.EPOCH,
"user:group")))
.build();

Expand All @@ -438,54 +437,63 @@ public void testBuild_ownership() throws IOException {
assertThat(entry1.getLongGroupId()).isEqualTo(0);
assertThat(entry1.getUserName()).isEmpty();
assertThat(entry1.getGroupName()).isEmpty();
verifyThatModificationTimeIsReset(entry1);

TarArchiveEntry entry2 = in.getNextTarEntry();
assertThat(entry2.getLongUserId()).isEqualTo(0);
assertThat(entry2.getLongGroupId()).isEqualTo(0);
assertThat(entry2.getUserName()).isEmpty();
assertThat(entry2.getGroupName()).isEmpty();
verifyThatModificationTimeIsReset(entry2);

TarArchiveEntry entry3 = in.getNextTarEntry();
assertThat(entry3.getLongUserId()).isEqualTo(0);
assertThat(entry3.getLongGroupId()).isEqualTo(0);
assertThat(entry3.getUserName()).isEmpty();
assertThat(entry3.getGroupName()).isEmpty();
verifyThatModificationTimeIsReset(entry3);

TarArchiveEntry entry4 = in.getNextTarEntry();
assertThat(entry4.getLongUserId()).isEqualTo(333);
assertThat(entry4.getLongGroupId()).isEqualTo(0);
assertThat(entry4.getUserName()).isEmpty();
assertThat(entry4.getGroupName()).isEmpty();
verifyThatModificationTimeIsReset(entry4);

TarArchiveEntry entry5 = in.getNextTarEntry();
assertThat(entry5.getLongUserId()).isEqualTo(0);
assertThat(entry5.getLongGroupId()).isEqualTo(555);
assertThat(entry5.getUserName()).isEmpty();
assertThat(entry5.getGroupName()).isEmpty();
verifyThatModificationTimeIsReset(entry5);

TarArchiveEntry entry6 = in.getNextTarEntry();
assertThat(entry6.getLongUserId()).isEqualTo(333);
assertThat(entry6.getLongGroupId()).isEqualTo(555);
assertThat(entry6.getUserName()).isEmpty();
assertThat(entry6.getGroupName()).isEmpty();
verifyThatModificationTimeIsReset(entry6);

TarArchiveEntry entry7 = in.getNextTarEntry();
assertThat(entry7.getLongUserId()).isEqualTo(0);
assertThat(entry7.getLongGroupId()).isEqualTo(0);
assertThat(entry7.getUserName()).isEqualTo("user");
assertThat(entry7.getGroupName()).isEmpty();
verifyThatModificationTimeIsReset(entry7);

TarArchiveEntry entry8 = in.getNextTarEntry();
assertThat(entry8.getLongUserId()).isEqualTo(0);
assertThat(entry8.getLongGroupId()).isEqualTo(0);
assertThat(entry8.getUserName()).isEmpty();
assertThat(entry8.getGroupName()).isEqualTo("group");
verifyThatModificationTimeIsReset(entry8);

TarArchiveEntry entry9 = in.getNextTarEntry();
assertThat(entry9.getLongUserId()).isEqualTo(0);
assertThat(entry9.getLongGroupId()).isEqualTo(0);
assertThat(entry9.getUserName()).isEqualTo("user");
assertThat(entry9.getGroupName()).isEqualTo("group");
verifyThatModificationTimeIsReset(entry9);
}
}

Expand Down