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.

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

Prepare JSON template classes for new manifest cache design #2707

Merged
merged 3 commits into from
Aug 20, 2020

Conversation

chanseokoh
Copy link
Member

@chanseokoh chanseokoh commented Aug 18, 2020

  1. Renaming ManifestAndConfig to ManifestAndConfigTemplate.

    • The class is a simple pair of manifest and config. In the new design, it will be used as a JSON template for de-/serialization.
    • As a JSON template, converted Optional<> to @Nullable.
  2. New class: ImageMetadataTemplate.

    • Holds
      1. a manifest list (if not Docker schema 1). Can be null (when a base image reference is not a manifest list).
      2. a list of manifest+config pairs (List<ManifestAndConfigTemplate>).
        For example,
    {
      "manifestList": {  # manifest list content
        "@class": "com.google.cloud.tools.jib.image.json.V22ManifestListTemplate",
        "manifests": [ { ... }, { ... }, { ... }, ... ]
      },
    
      "manifestsAndConfigs": [
        {
          "manifest": {
            "@class": "com.google.cloud.tools.jib.image.json.V22ManifestTemplate",
            "schemaVersion": 2,
            "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
            "config": { ... }
            ...
          },
          "config": { ... }
        },
        { ... },
        ...
      ]
    
  3. OciImageIndex now inherits ManifestTemplate (previously JsonTemplate). This is to have a slightly more specialized class as a common super class of OciImageIndex and V22ManifestListTemplate.

    • Note currently ManfiestTemplate is also a super class of OciManifestTemplate, V21ManifestTemplate and V22ManifestTemplate. It does make sense to consider all of these manifest list classes and manifest classes as ManifestTemplate.

@louismurerwa

@chanseokoh chanseokoh requested a review from a team August 18, 2020 15:02
@chanseokoh
Copy link
Member Author

chanseokoh commented Aug 18, 2020

  1. OciImageIndex now inherits ManifestTemplate (previously JsonTemplate).

Note ManifestTemplate is not significantly different from JsonTemplate.

/** Parent class for image manifest JSON templates. */
@JsonIgnoreProperties(ignoreUnknown = true)
public interface ManifestTemplate extends JsonTemplate {
  int getSchemaVersion();
}

Also the only place where OciImageIndex is used is ImageTarball.ociWriteTo() to write a local OCI-format image tarball (deserialization only).

OciIndexTemplate index = new OciIndexTemplate();
// TODO: figure out how to tag with allTargetImageTags
index.addManifest(manifestDescriptor, imageReference.toStringWithQualifier());
tarStreamBuilder.addByteEntry(JsonTemplateMapper.toByteArray(index), "index.json");
tarStreamBuilder.writeAsTarArchiveTo(out);

Note we don't support reading an OCI image index for a base image at all.

return Arrays.asList(
OciManifestTemplate.MANIFEST_MEDIA_TYPE,
V22ManifestTemplate.MANIFEST_MEDIA_TYPE,
V21ManifestTemplate.MEDIA_TYPE,
V22ManifestListTemplate.MANIFEST_MEDIA_TYPE);
}

@loosebazooka
Copy link
Member

loosebazooka commented Aug 19, 2020

I wonder if we're using explicit type information, then it makes sense to only use a complex json cache entry for manifest list/oci index and simpler entries for single images?
A cache file would be either:

{
  "@class": "com.google.cloud.tools.jib.image.json.AbstractManifestListType"
   ...
  // all entries for manifest list/oci indexes
}

or

{
  "@class": "com.google.cloud.tools.jib.image.json.AbstractSingleImageType"
  ...
  // all entries for a single image ref
}

I'm not super sure what the consequences of this are though.

@chanseokoh
Copy link
Member Author

chanseokoh commented Aug 19, 2020

Ah, that's a very natural idea. There's one thing related though. For the cache retrieval method Cache.retrieveMetadata(), I'm making it return the entire metadata info (both a manifest list and a manifest+config list) with ImageMetadataTemplate:

  public Optional<ImageMetadataTemplate> retrieveMetadata(ImageReference imageReference)

(Did not want to create another class for a return type that is fundamentally same as ImageMetadataTemplate, although this isn't necessarily a good thing).

So, if we were to use different top-level classes for manifests_configs.json, then retrieveMetadata() needs to either 1) reassemble this complex metadata structure again; or 2) return a nominal common super class of a manifest list+manifests structure and a single manifest structure. For 2), I guess the caller would then need to check the actual sub-type with instanceof and cast it to either a manifest list+manifests type or just a single manifest type (which may not be a bad thing).

Initially I thought returning ImageMetadataTemplate is reasonable and simple in implementation. There are certainly some disadvantages though; some assumptions that cannot be enforced with strong typing need to be made. For example, an absence of ImageMetadataTemplate.manifestList means ImageMetadataTemplate.manifestsAndConfigs should be a single-element list. I'm not so decisive of what's the best way or whether there is a better solution. But at least it seems like having a single ImageMetadataTemplate type is simpler in implementation and resulting in creating a less number of JSON-related types that are only required to exist due to the cache.

Copy link
Member

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

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

This seems fine. Just questions for my understanding.

property = "@class")
@JsonSubTypes({
@JsonSubTypes.Type(value = OciManifestTemplate.class),
@JsonSubTypes.Type(value = V21ManifestTemplate.class),
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to persist a V21 manifest? Don't we do conversions of this to V22?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't actually do a JSON-to-JSON conversion. We are currently persisting V21ManifestTemplate in the cache.

It's just that we can properly instantiate a Java Image instance from both V21 and V22.

@@ -110,7 +110,7 @@

if (schemaVersion == 1) {
return Optional.of(
new ManifestAndConfig(
new ManifestAndConfigTemplate(
Copy link
Member

Choose a reason for hiding this comment

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

It seems strange that we would generate a JsonTemplate type from separately parsed out jsonTemplates? Is this something that we would need to change eventually?

Can we go directly to ManifestAndConfigTemplate directly from file?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the next PR, all of these parsing and conversion operations will be gone, and this retrieveMetadata() will directly de-serialize and return ImageMetadataTemplate without any processing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants