Skip to content

Commit

Permalink
Revert "fix: Check for existence of layers in cache before returning …
Browse files Browse the repository at this point in the history
…cached base image (#3767)"

This reverts commit 7aa1110.
  • Loading branch information
emmileaf authored Sep 19, 2022
1 parent 7aa1110 commit 3899fde
Show file tree
Hide file tree
Showing 6 changed files with 18 additions and 171 deletions.
1 change: 0 additions & 1 deletion jib-core/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ All notable changes to this project will be documented in this file.
- Upgraded Google HTTP libraries to 1.42.2 ([#3745](https://github.com/GoogleContainerTools/jib/pull/3745))

### Fixed
- Fixed partially cached base image authorization issue by adding check for existence of layers in cache ([#3767](https://github.com/GoogleContainerTools/jib/pull/3767))

## 0.22.0

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -456,12 +456,19 @@ List<Image> getCachedBaseImages()

if (manifestList == null) {
Verify.verify(manifestsAndConfigs.size() == 1);
ManifestAndConfigTemplate manifestAndConfig = manifestsAndConfigs.get(0);
Optional<Image> cachedImage = getBaseImageIfAllLayersCached(manifestAndConfig, true);
if (!cachedImage.isPresent()) {
return Collections.emptyList();
ManifestTemplate manifest = manifestsAndConfigs.get(0).getManifest();
if (manifest instanceof V21ManifestTemplate) {
return Collections.singletonList(
JsonToImageTranslator.toImage((V21ManifestTemplate) manifest));
}
return Collections.singletonList(cachedImage.get());

ContainerConfigurationTemplate containerConfig =
Verify.verifyNotNull(manifestsAndConfigs.get(0).getConfig());
PlatformChecker.checkManifestPlatform(buildContext, containerConfig);

return Collections.singletonList(
JsonToImageTranslator.toImage(
(BuildableManifestTemplate) Verify.verifyNotNull(manifest), containerConfig));
}

// Manifest list cached. Identify matching platforms and check if all of them are cached.
Expand All @@ -477,52 +484,13 @@ List<Image> getCachedBaseImages()
if (!manifestAndConfigFound.isPresent()) {
return Collections.emptyList();
}
Optional<Image> cachedImage =
getBaseImageIfAllLayersCached(manifestAndConfigFound.get(), false);
if (!cachedImage.isPresent()) {
return Collections.emptyList();
}
images.add(cachedImage.get());
}
return images.build();
}

/**
* Helper method to retrieve a base image from cache given manifest and container config. Does not
* return image if any layers of base image are missing in cache.
*
* @param manifestAndConfig stores an image manifest and a container config
* @param isSingleManifest true if base image is not a manifest list
* @return the single cached {@link Image} found, or {@code Optional#empty()} if the base image
* not found in cache with all layers present.
* @throws BadContainerConfigurationFormatException if the container configuration is in a bad
* format
* @throws PlatformNotFoundInBaseImageException if build target platform is not found in the base
* image
* @throws LayerCountMismatchException LayerCountMismatchException if the manifest and
* configuration contain conflicting layer information
*/
private Optional<Image> getBaseImageIfAllLayersCached(
ManifestAndConfigTemplate manifestAndConfig, boolean isSingleManifest)
throws BadContainerConfigurationFormatException, PlatformNotFoundInBaseImageException,
LayerCountMismatchException {
Cache baseImageLayersCache = buildContext.getBaseImageLayersCache();
ManifestTemplate manifest = Verify.verifyNotNull(manifestAndConfig.getManifest());

// Verify all layers described in manifest are present in cache
if (!baseImageLayersCache.areAllLayersCached(manifest)) {
return Optional.empty();
}
if (manifest instanceof V21ManifestTemplate) {
return Optional.of(JsonToImageTranslator.toImage((V21ManifestTemplate) manifest));
ManifestTemplate manifest = Verify.verifyNotNull(manifestAndConfigFound.get().getManifest());
ContainerConfigurationTemplate containerConfig =
Verify.verifyNotNull(manifestAndConfigFound.get().getConfig());
images.add(
JsonToImageTranslator.toImage((BuildableManifestTemplate) manifest, containerConfig));
}
ContainerConfigurationTemplate containerConfig =
Verify.verifyNotNull(manifestAndConfig.getConfig());
if (isSingleManifest) {
// If base image is not a manifest list, check and warn misconfigured platforms.
PlatformChecker.checkManifestPlatform(buildContext, containerConfig);
}
return Optional.of(
JsonToImageTranslator.toImage((BuildableManifestTemplate) manifest, containerConfig));
return images.build();
}
}
11 changes: 0 additions & 11 deletions jib-core/src/main/java/com/google/cloud/tools/jib/cache/Cache.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import com.google.cloud.tools.jib.image.json.ContainerConfigurationTemplate;
import com.google.cloud.tools.jib.image.json.ImageMetadataTemplate;
import com.google.cloud.tools.jib.image.json.ManifestAndConfigTemplate;
import com.google.cloud.tools.jib.image.json.ManifestTemplate;
import com.google.cloud.tools.jib.image.json.V21ManifestTemplate;
import com.google.common.collect.ImmutableList;
import java.io.IOException;
Expand Down Expand Up @@ -193,16 +192,6 @@ public Optional<ImageMetadataTemplate> retrieveMetadata(ImageReference imageRefe
return cacheStorageReader.retrieveMetadata(imageReference);
}

/**
* Returns {@code true} if all image layers described in a manifest exist in the cache.
*
* @param manifest the image manifest
* @return a boolean
*/
public boolean areAllLayersCached(ManifestTemplate manifest) {
return cacheStorageReader.areAllLayersCached(manifest);
}

/**
* Retrieves the {@link CachedLayer} that was built from the {@code layerEntries}.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,38 +81,6 @@ static void verifyImageMetadata(ImageMetadataTemplate metadata, Path metadataCac
this.cacheStorageFiles = cacheStorageFiles;
}

/**
* Returns {@code true} if all image layers described in a manifest have a corresponding file
* entry in the cache.
*
* @param manifest the image manifest
* @return a boolean
*/
boolean areAllLayersCached(ManifestTemplate manifest) {

List<DescriptorDigest> layerDigests;

if (manifest instanceof V21ManifestTemplate) {
layerDigests = ((V21ManifestTemplate) manifest).getLayerDigests();
} else if (manifest instanceof BuildableManifestTemplate) {
layerDigests =
((BuildableManifestTemplate) manifest)
.getLayers().stream()
.map(BuildableManifestTemplate.ContentDescriptorTemplate::getDigest)
.collect(Collectors.toList());
} else {
throw new IllegalArgumentException("Unknown manifest type: " + manifest);
}

for (DescriptorDigest layerDigest : layerDigests) {
Path layerDirectory = cacheStorageFiles.getLayerDirectory(layerDigest);
if (!Files.exists(layerDirectory)) {
return false;
}
}
return true;
}

/**
* Retrieves the cached image metadata (a manifest list and a list of manifest/container
* configuration pairs) for an image reference.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package com.google.cloud.tools.jib.builder.steps;

import static com.google.common.truth.Truth.assertThat;
import static org.mockito.ArgumentMatchers.eq;

import com.google.cloud.tools.jib.api.DescriptorDigest;
Expand All @@ -41,7 +40,6 @@
import com.google.cloud.tools.jib.image.json.ContainerConfigurationTemplate;
import com.google.cloud.tools.jib.image.json.ImageMetadataTemplate;
import com.google.cloud.tools.jib.image.json.ManifestAndConfigTemplate;
import com.google.cloud.tools.jib.image.json.ManifestTemplate;
import com.google.cloud.tools.jib.image.json.OciIndexTemplate;
import com.google.cloud.tools.jib.image.json.PlatformNotFoundInBaseImageException;
import com.google.cloud.tools.jib.image.json.UnlistedPlatformInManifestListException;
Expand Down Expand Up @@ -157,7 +155,6 @@ public void testCall_digestBaseImage()
ImageMetadataTemplate imageMetadata =
new ImageMetadataTemplate(null, Arrays.asList(manifestAndConfig));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(cache.areAllLayersCached(manifestAndConfig.getManifest())).thenReturn(true);

ImagesAndRegistryClient result = pullBaseImageStep.call();
Assert.assertEquals("fat system", result.images.get(0).getOs());
Expand Down Expand Up @@ -199,7 +196,6 @@ public void testCall_offlineMode_cached()
ImageMetadataTemplate imageMetadata =
new ImageMetadataTemplate(null, Arrays.asList(manifestAndConfig));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(cache.areAllLayersCached(manifestAndConfig.getManifest())).thenReturn(true);

ImagesAndRegistryClient result = pullBaseImageStep.call();
Assert.assertEquals("fat system", result.images.get(0).getOs());
Expand Down Expand Up @@ -301,24 +297,6 @@ public void testGetCachedBaseImages_emptyCache()
Assert.assertEquals(Arrays.asList(), pullBaseImageStep.getCachedBaseImages());
}

@Test
public void testGetCachedBaseImages_partiallyCached_emptyListReturned()
throws InvalidImageReferenceException, CacheCorruptedException, IOException,
LayerCountMismatchException, PlatformNotFoundInBaseImageException,
BadContainerConfigurationFormatException, UnlistedPlatformInManifestListException {
ImageReference imageReference = ImageReference.parse("cat");
Mockito.when(buildContext.getBaseImageConfiguration())
.thenReturn(ImageConfiguration.builder(imageReference).build());
ManifestTemplate manifest = Mockito.mock(ManifestTemplate.class);
ImageMetadataTemplate imageMetadata =
new ImageMetadataTemplate(
null, Arrays.asList(new ManifestAndConfigTemplate(manifest, null)));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(cache.areAllLayersCached(manifest)).thenReturn(false);

assertThat(pullBaseImageStep.getCachedBaseImages()).isEmpty();
}

@Test
public void testGetCachedBaseImages_v21ManifestCached()
throws InvalidImageReferenceException, IOException, CacheCorruptedException,
Expand All @@ -338,7 +316,6 @@ public void testGetCachedBaseImages_v21ManifestCached()
null, Arrays.asList(new ManifestAndConfigTemplate(v21Manifest, null)));

Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(cache.areAllLayersCached(v21Manifest)).thenReturn(true);

List<Image> images = pullBaseImageStep.getCachedBaseImages();

Expand Down Expand Up @@ -367,7 +344,6 @@ public void testGetCachedBaseImages_v22ManifestCached()
ImageMetadataTemplate imageMetadata =
new ImageMetadataTemplate(null, Arrays.asList(manifestAndConfig));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(cache.areAllLayersCached(manifestAndConfig.getManifest())).thenReturn(true);

List<Image> images = pullBaseImageStep.getCachedBaseImages();

Expand Down Expand Up @@ -405,12 +381,6 @@ public void testGetCachedBaseImages_v22ManifestListCached()
new ManifestAndConfigTemplate(
new V22ManifestTemplate(), containerConfigJson2, "sha256:digest2")));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(
cache.areAllLayersCached(imageMetadata.getManifestsAndConfigs().get(0).getManifest()))
.thenReturn(true);
Mockito.when(
cache.areAllLayersCached(imageMetadata.getManifestsAndConfigs().get(1).getManifest()))
.thenReturn(true);

Mockito.when(containerConfig.getPlatforms())
.thenReturn(ImmutableSet.of(new Platform("arch1", "os1"), new Platform("arch2", "os2")));
Expand Down Expand Up @@ -446,9 +416,6 @@ public void testGetCachedBaseImages_v22ManifestListCached_partialMatches()
new ContainerConfigurationTemplate(),
"sha256:digest1")));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(
cache.areAllLayersCached(imageMetadata.getManifestsAndConfigs().get(0).getManifest()))
.thenReturn(true);

Mockito.when(containerConfig.getPlatforms())
.thenReturn(ImmutableSet.of(new Platform("arch1", "os1"), new Platform("arch2", "os2")));
Expand Down Expand Up @@ -487,7 +454,6 @@ public void testGetCachedBaseImages_v22ManifestListCached_onlyPlatforms()
Arrays.asList(
unrelatedManifestAndConfig, targetManifestAndConfig, unrelatedManifestAndConfig));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(cache.areAllLayersCached(targetManifestAndConfig.getManifest())).thenReturn(true);

Mockito.when(containerConfig.getPlatforms())
.thenReturn(ImmutableSet.of(new Platform("target-arch", "target-os")));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

package com.google.cloud.tools.jib.cache;

import static com.google.common.truth.Truth.assertThat;

import com.google.cloud.tools.jib.api.DescriptorDigest;
import com.google.cloud.tools.jib.api.ImageReference;
import com.google.cloud.tools.jib.blob.Blobs;
Expand Down Expand Up @@ -640,45 +638,4 @@ public void testVerifyImageMetadata_validOciImageIndex() throws CacheCorruptedEx
CacheStorageReader.verifyImageMetadata(metadata, Paths.get("/cache/dir"));
// should pass without CacheCorruptedException
}

@Test
public void testAllLayersCached_v21SingleManifest()
throws IOException, CacheCorruptedException, DigestException, URISyntaxException {
setupCachedMetadataV21(cacheDirectory);
ImageMetadataTemplate metadata =
cacheStorageReader.retrieveMetadata(ImageReference.of("test", "image", "tag")).get();
V21ManifestTemplate manifest =
(V21ManifestTemplate) metadata.getManifestsAndConfigs().get(0).getManifest();
DescriptorDigest firstLayerDigest = manifest.getLayerDigests().get(0);
DescriptorDigest secondLayerDigest = manifest.getLayerDigests().get(1);

// Create only one of the layer directories so that layers are only partially cached.
Files.createDirectories(cacheStorageFiles.getLayerDirectory(firstLayerDigest));
boolean checkWithPartialLayersCached = cacheStorageReader.areAllLayersCached(manifest);
// Create the other layer directory so that all layers are cached.
Files.createDirectories(cacheStorageFiles.getLayerDirectory(secondLayerDigest));
boolean checkWithAllLayersCached = cacheStorageReader.areAllLayersCached(manifest);

assertThat(checkWithPartialLayersCached).isFalse();
assertThat(checkWithAllLayersCached).isTrue();
}

@Test
public void testAllLayersCached_v22SingleManifest()
throws IOException, CacheCorruptedException, DigestException, URISyntaxException {
setupCachedMetadataV22(cacheDirectory);
ImageMetadataTemplate metadata =
cacheStorageReader.retrieveMetadata(ImageReference.of("test", "image", "tag")).get();
V22ManifestTemplate manifest =
(V22ManifestTemplate) metadata.getManifestsAndConfigs().get(0).getManifest();
DescriptorDigest layerDigest = manifest.getLayers().get(0).getDigest();

boolean checkBeforeLayerCached = cacheStorageReader.areAllLayersCached(manifest);
// Create the single layer directory so that all layers are cached.
Files.createDirectories(cacheStorageFiles.getLayerDirectory(layerDigest));
boolean checkAfterLayerCached = cacheStorageReader.areAllLayersCached(manifest);

assertThat(checkBeforeLayerCached).isFalse();
assertThat(checkAfterLayerCached).isTrue();
}
}

0 comments on commit 3899fde

Please sign in to comment.