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

Support docker uri for lifecycle #2112

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rashadism
Copy link

@rashadism rashadism commented Apr 1, 2024

Summary

Now can provide a docker uri for the lifecycle image in builder.toml

Output

Before

After

Documentation

  • Should this change be documented?
    • Yes, the possibility of using a docker uri in builder.toml should be documented
    • No

Related

Resolves #2076

@github-actions github-actions bot added this to the 0.34.0 milestone Apr 1, 2024
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Apr 1, 2024
Copy link

codecov bot commented Apr 1, 2024

Codecov Report

Attention: Patch coverage is 3.10559% with 156 lines in your changes are missing coverage. Please review.

Project coverage is 79.15%. Comparing base (6fc092b) to head (43c900d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2112      +/-   ##
==========================================
- Coverage   79.72%   79.15%   -0.57%     
==========================================
  Files         176      176              
  Lines       13263    13362      +99     
==========================================
+ Hits        10573    10575       +2     
- Misses       2021     2118      +97     
  Partials      669      669              
Flag Coverage Δ
os_linux 78.06% <3.11%> (-0.59%) ⬇️
os_macos 75.81% <3.11%> (-0.53%) ⬇️
os_windows 78.54% <3.11%> (-0.57%) ⬇️
unit 79.15% <3.11%> (-0.57%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@rashadism rashadism marked this pull request as ready for review April 1, 2024 13:02
@rashadism rashadism requested review from a team as code owners April 1, 2024 13:02
@rashadism rashadism force-pushed the Support-docker-uri-for-lifecycle branch from 3ba1a29 to 15b94c9 Compare April 1, 2024 13:10
@github-actions github-actions bot added type/chore Issue that requests non-user facing changes. and removed type/chore Issue that requests non-user facing changes. labels Apr 1, 2024
return nil, err
}

lifecycleImageTar := filepath.Join(relativeBaseDir, "lifecycle-image.tar")
Copy link
Contributor

Choose a reason for hiding this comment

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

@rashadism Thank You for contributing to Buildpacks, I want to take it to your notice that "lifecycle-image.tar" might not going to be static, I think it will be based on imageName variable,
can you please take a look into it, and please ignore this message if I am wrong(as I am just a contributor like you and never worked on imageFetcher)
I recommend adding tests, and manually testing code as far as possible, I really appreciate your effort.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, thank you for going through this. Can you explain a bit more on

"lifecycle-image.tar" might not going to be static, I think it will be based on imageName
variable

I think I did not understand properly what you mentioned

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I did not understand properly what you mentioned

lifecycleImage, err = c.imageFetcher.Fetch(ctx, imageName, image.FetchOptions{Daemon: false})
if err != nil {
	return nil, err
}

lifecycleImageTar := filepath.Join(relativeBaseDir, "lifecycle-image.tar")

I feel like here the imageName in call c.imageFetcher.Fetch might decide whether the lifecycle file name should be lifecycle-image.tar or something else,
try changing the lifecycle uri in builder.toml to something else if you have provided lifecycle-image as lifecycle uri

Copy link
Member

Choose a reason for hiding this comment

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

I think what Sai was highlighting your hardcoded variable "lifecycle-image.tar" but checking your code I noticed you are just extracting the image from the docker daemon and saving it on disk using that hardcoded name. That's ok.

Now, I took sometime trying to think about this and I am going to do one suggestion (but I am not sure if it is going to work).

Your current code looks like this:

switch {
	case buildpack.HasDockerLocator(config.URI):
            var lifecycleImage imgutil.Image
            var blob blob.Blob
            imageName := buildpack.ParsePackageLocator(config.URI)
            c.logger.Debugf("Downloading lifecycle image: %s", style.Symbol(imageName))
            
            lifecycleImage, err = c.imageFetcher.Fetch(ctx, imageName, image.FetchOptions{Daemon: false})
            if err != nil {
	            return nil, err
            }
            // Good, till this point, we have imgItil.Image and we need to create a 
            // Blob from it
}

Your are going through a tricky path of trying to read/write the image from the daemon and saving it into a tar. We also have a layout implementation of the Image interface that maybe can help us here, could you try something like this:

       
	lifecycleLayoutImage, err := layout.NewImage("<path-to-safe-the-lifecyce>", layout.FromBaseImage(lifecycleImage.UnderlyingImage()))
	lifecycle.Save()
	blob := blob.NewBlob("<path-to-safe-the-lifecyce>")
         lifecycle, err := builder.NewLifecycle(blob)
         if err != nil {
	          return nil, errors.Wrap(err, "invalid lifecycle")
         }
         return lifecycle, nil
  • Save() is going to write the image on disk in OCI layout format at the specified path
  • Then we create the blob from that path

if this works, maybe you can avoid extracting all the other methods in the build.go file and simplify your current implementation. The initial part is exactly what I think needs to be done, the part of getting the lifecycle out of the daemon is where I have some doubts.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, that is smart and more convenient, I did not know about the layout implementation, will look into it thank you.

Copy link
Author

Choose a reason for hiding this comment

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

@WYGIN , what I meant was after doing lifecycleLayoutImage.Save(), the image is saved in OCI layout format. Couldn't find a way to inspect the file system of that(from within golang codebase)

Copy link
Member

Choose a reason for hiding this comment

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

@rashadism

I think I understand your problem, that's why I said I was not sure if it was going to work 😄

Checking the code, I have the following idea:

switch {
	case buildpack.HasDockerLocator(config.URI):
            var lifecycleImage imgutil.Image
            var blob blob.Blob
            imageName := buildpack.ParsePackageLocator(config.URI)
            c.logger.Debugf("Downloading lifecycle image: %s", style.Symbol(imageName))
            
            lifecycleImage, err = c.imageFetcher.Fetch(ctx, imageName, image.FetchOptions{Daemon: false})
            if err != nil {
	            return nil, err
            }
            // Good, till this point, we have imgItil.Image and we need to create a 
            // Blob from it
          lifecycleLayoutImage, err := layout.NewImage("<path-to-safe-the-lifecyce>", layout.FromBaseImage(lifecycleImage.UnderlyingImage()))
          lifecycle.Save()

          // You probably need to convert the <path-to-safe-the-lifecyce> to an URI
          // Not sure if it is relative or not
          uri, err = paths.FilePathToURI("<path-to-safe-the-lifecyce>", "")
}

blob, err := c.downloader.Download(ctx, URI)

// blob is going to be OCI layout format, we need to make NewLifecycle method capable of handling it
lifecycle, err := builder.NewLifecycle(blob)
	
return lifecycle, nil

As Sai mentioned, it's time to understand how to look for a lifecycle in OCI layout format. We do something similar for Buildpack Packages here I think that is what you want.

Copy link
Member

Choose a reason for hiding this comment

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

The OCI Layout format spec is here I think the tricky part is to open the blobs and find where the lifecycle binaries are located.

As @WYGIN mentioned, if you install dive tool, and then you pull the latest image from dockerhub

dive buildpacksio/lifecycle:latest

We can see that the blob containing the lifecycle is at the end, and also as mentioned by @WYGIN , it is under /cnb

Screenshot 2024-04-02 at 12 16 05 PM

I also used skopeo to copy the lifecycle from the daemon to disk in OCI layout

> skopeo copy docker-daemon:buildpacksio/lifecycle:latest oci:local-lifecycle
Getting image source signatures
Copying blob ac805962e479 done   |
Copying blob 1df9699731f7 done   |
Copying blob 70c35736547b done   |
Copying blob af5aa97ebe6c done   |
Copying blob 4d049f83d9cf done   |
Copying blob 6fbdf253bbc2 done   |
Copying blob bbb6cacb8c82 done   |
Copying blob 2a92d6ac9e4f done   |
Copying blob 1a73b54f556b done   |
Copying blob c048279a7d9f done   |
Copying blob 2388d21e8e2b done   |
Copying blob a03079e61f77 done   |
Copying config e5c54e7a25 done   |
Writing manifest to image destination
> ls
local-lifecycle
> tree local-lifecycle
local-lifecycle
├── blobs
│   └── sha256
│       ├── 02c91f6b395a6b06d17b8985a2db90aef7b09feedff77eff1f7c269161263a9b
│       ├── 0b2215fb07602fba31abe9852102a9a15baebc13afe0a1c8904bfaec08abb99b
│       ├── 12aa63c6434d9d6411f6f5316b053d1c869c83e24c974e31ca8491401e137e90
│       ├── 18c59b7472ef6bc22a3919f76e0928d6a595ba3350201be6b473c3f2fbc9488b
│       ├── 1e45aa110bce64887bed49d9216b825d63437fdca5d53a1f879e17bc35691e0e
│       ├── 50d2b2cd67880cbdac647edf0295c59ca5c7a54ae2512547960132c4861741d1
│       ├── 80f00805faa81bccdbddf9458369d8dede6a30de1dced521f32a6237aeca64c2
│       ├── 8284fef10e55c7fbd563239f69e0a94cbf76468489d74e7bab59c34ed61cccd9
│       ├── a97b9dcac04afbc295431c6a36a5ba65f42b2804647d34e4f578412d4475dca6
│       ├── b4aa04aa577f2b2f4b4a930e905d091b68b0719ec302b9abca710ffae50ebcaa
│       ├── e5c54e7a259ab941a22c38591dd8a0f08366003e0176e8723bcf0506cd71b154
│       ├── efd880768fbced8b0e7e6c514ee6fb69f2207088a5c5f15c9dbae25c45d2f852
│       ├── f5ceaaa2f4b47256c44e2cdae5cbbf663f8269e6ee345e31b75da5a97974ca62
│       └── ffdf8cfdbafeabe2762d2dde33e36f1b87dfa967ae34811ff5342ebe8233eb4b
├── index.json
└── oci-layout

3 directories, 16 files

If I am not wrong, each blob is a tar file, what I think you need to do is to find the blob containing the lifecycle binaries and then probably create another Blob instance to passthrough builder.NewLifecycle(blob)

The way I think you need to find the blob is reading the tar entries in the blob file, if you do the exercise for each blob above you will find the following

➜  sha256 tar -tf 8284fef10e55c7fbd563239f69e0a94cbf76468489d74e7bab59c34ed61cccd9
/cnb
/cnb/lifecycle.toml
/cnb/lifecycle
/cnb/lifecycle/analyzer
/cnb/lifecycle/builder
/cnb/lifecycle/creator
/cnb/lifecycle/detector
/cnb/lifecycle/exporter
/cnb/lifecycle/extender
/cnb/lifecycle/launcher
/cnb/lifecycle/launcher.sbom.cdx.json
/cnb/lifecycle/launcher.sbom.spdx.json
/cnb/lifecycle/launcher.sbom.syft.json
/cnb/lifecycle/lifecycle
/cnb/lifecycle/lifecycle.sbom.cdx.json
/cnb/lifecycle/lifecycle.sbom.spdx.json
/cnb/lifecycle/lifecycle.sbom.syft.json
/cnb/lifecycle/rebaser
/cnb/lifecycle/restorer

In this case the blob 8284fef10e55c7fbd563239f69e0a94cbf76468489d74e7bab59c34ed61cccd9 contains the binaries for the lifecycle, and because you know the path in the filesystem to that blob, I think you can create a new object calling.

blob, err := c.downloader.Download(ctx, URI)

Pointing to a URI to that particular blob and after that, I think, it should work. I am not sure about the cnb folder, maybe we need to remove it or make the code to create the lifecycle smarter.

I hope this can help you!

Copy link
Member

Choose a reason for hiding this comment

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

Another idea is:

Once you fetch the Lifecycle image:

lifecycleImage, err = c.imageFetcher.Fetch(ctx, imageName, image.FetchOptions{Daemon: false})

Try to find the v1.Layer containing the binaries, maybe, iterating over the Layers and inspecting them to find the cnb folder

lifecycleImage.UnderlyingImage().Layers()

Once you have the Layer, I think you can write it on disk, just that blob, and then keep doing all the other process maybe we don't need to save it as OCI layout

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the detailed insights @jjbustamante @WYGIN , will look into it

imageName := buildpack.ParsePackageLocator(config.URI)
c.logger.Debugf("Downloading lifecycle image: %s", style.Symbol(imageName))

lifecycleImage, err = c.imageFetcher.Fetch(ctx, imageName, image.FetchOptions{Daemon: false})
Copy link
Member

Choose a reason for hiding this comment

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

I think, we need to passthrough some key fetch options: Daemon, PullPolicy, Platform, those values should be available from method caller and we need to configure the Fetcher with them

@WYGIN
Copy link
Contributor

WYGIN commented Apr 2, 2024

@jjbustamante If this feature ships, isn't it possible for platform operators to create their own lifecycles? excited to see this feature shipping, and I think one should publish an article on this

@jjbustamante jjbustamante modified the milestones: 0.34.0, 0.35.0 Apr 3, 2024
@braunsonm
Copy link

braunsonm commented Apr 4, 2024

@WYGIN you could already create your own lifecycle binaries pretty sure. You just had to provide them in a tar if creating your own builder.

The pack CLI during build has supported --lifecycle-image for custom lifecycles with existing builders before this as well.

This definitely makes it more accessible though 😄

@WYGIN
Copy link
Contributor

WYGIN commented Apr 5, 2024

The pack CLI during build has supported --lifecycle-image for custom lifecycles with existing builders before this as well.

never noticed this flag, Thanks a lot for the information

@github-actions github-actions bot modified the milestones: 0.35.0, 0.34.0 Apr 19, 2024
@jjbustamante jjbustamante modified the milestones: 0.34.0, 0.35.0 Apr 19, 2024
@github-actions github-actions bot modified the milestones: 0.35.0, 0.34.0 May 6, 2024
@rashadism rashadism force-pushed the Support-docker-uri-for-lifecycle branch from 18c3a10 to a50d15c Compare May 6, 2024 07:37
@rashadism rashadism closed this May 6, 2024
@rashadism rashadism force-pushed the Support-docker-uri-for-lifecycle branch from a50d15c to 32d8db5 Compare May 6, 2024 07:37
@rashadism rashadism reopened this May 6, 2024
Signed-off-by: Rashad Sirajudeen <rashad.20@cse.mrt.ac.lk>
@rashadism rashadism force-pushed the Support-docker-uri-for-lifecycle branch from 643289c to abb2ac0 Compare May 6, 2024 10:56
Signed-off-by: Rashad Sirajudeen <rashad.20@cse.mrt.ac.lk>
@rashadism
Copy link
Author

rashadism commented May 6, 2024

@jjbustamante @WYGIN can u have a look

There is one thing I wasn't sure on how to handle. Since we are writing the lifecycle to disk, it should be removed after creating the builder. We can't defer removing that, because I think the reference is being maintained outside the function and we need it. I tried using os.MkDirTemp assuming that it would get cleaned up somewhere but it didn't work either.

@jjbustamante
Copy link
Member

Hi @rashadism

Sorry for being late reviewing this! thank you so much for your great effort!

Local test

To test this code I did the following:

  • I created a local registry running at localhost:5000
  • I ran make clean build package on lifecycle latest branch to compile the latest binaries
  • I ran go run ./tools/image/main.go -lifecyclePath ./out/lifecycle-*.tgz -tag localhost:5000/lifecycle:0.19.4-linux-x86-64 to create a lifecycle image and push it to my local registry (I also did it with a docker account)
  • I went to samples repo and build the jammy builder adding the following line to the builder.toml
[lifecycle]
uri = "docker://localhost:5000/lifecycle:0.19.4-linux-x86-64"

I also used my personal docker account.

[lifecycle]
uri = "docker://jbustamantevmware/lifecycle:0.19.4-linux-x86-64"

In both cases, the builder was created and I managed to see the lifecycle layer in the final builder image

Screenshot 2024-05-08 at 8 06 18 AM

Takeaways

  • At the end of the builder create execution, a lifecycle.tar is left in the filesystem, because of your comment that you can't remove the file because it is being used later.

My thoughts

  • I am not too worried about leaving that file there, but we should document that behavior in the docs. If we want to create a temp folder one idea is to add a TempDirectory field to CreateBuilderOptions struct, the caller can clean that folder after the client.CreateBuilder method is invoked.
  • Is it possible you add some unit tests scenarios?

Co-authored-by: Juan Bustamante <bustamantejj@gmail.com>
Signed-off-by: Rashad Sirajudeen <rashadsirajudeen@gmail.com>
@jjbustamante jjbustamante modified the milestones: 0.34.0, 0.35.0 May 8, 2024
@github-actions github-actions bot modified the milestones: 0.35.0, 0.34.0 May 17, 2024
Signed-off-by: Rashad Sirajudeen <rashad.20@cse.mrt.ac.lk>
@jjbustamante jjbustamante modified the milestones: 0.34.0, 0.35.0 May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support docker:// URIs for builder lifecycle parameter
4 participants