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

Run image in pack inspect is not correctly updated after rebasing #1098

Closed
harryli0108 opened this issue May 5, 2023 · 30 comments · Fixed by #1104
Closed

Run image in pack inspect is not correctly updated after rebasing #1098

harryli0108 opened this issue May 5, 2023 · 30 comments · Fixed by #1104
Assignees
Labels
status/ready type/bug Something isn't working

Comments

@harryli0108
Copy link

Summary

Run image in pack inspect <img-name-1> is not correctly updated after running pack rebase <img-name-1> --run-image <img-name-2>.


Reproduction

Steps
  1. Build an image with a buildpack and call it
  2. Build a run image with docker build <run-img> (Optional Step)
  3. Run pack inspect <img-name-1> --output json and note the content in the run image part
  4. Run pack rebase <img-name-1> --run-image <run-img> to rebase image 1 with a new run image
  5. Run pack inspect <img-name-1> --output json and note the content in the run image part again.
Current behavior

The run image field when running pack inspect is not reflected correctly after rebase command.

Expected behavior

The run image name and tag should be changed after rebasing with a new run image.

@harryli0108 harryli0108 added status/triage type/bug Something isn't working labels May 5, 2023
@hernandanielg
Copy link

Hey guys 👋 I'll be checking this issue during the weekend

@daniv-msft
Copy link

Hello @hernandanielg, thanks for offering to look into this!
I just wanted to know if you needed any more information from us regarding this issue?
Have a great day!

@hernandanielg
Copy link

hey @daniv-msft 👋🏻

I have checked and confirmed this issue

➜  pack inspect example --output json | jq .local_info.run_images
[
  {
    "name": "index.docker.io/paketobuildpacks/run:base-cnb"
  },
  {
    "name": "gcr.io/paketo-buildpacks/run:base-cnb"
  }
]

➜ pack rebase example --run-image gcr.io/paketo-buildpacks/run:full-cnb
full-cnb: Pulling from paketo-buildpacks/run
...
Digest: sha256:122ef4fac967afd1fec5a05d9a4ec61015fd1aa75a55188948beae61cff4c040
Status: Downloaded newer image for gcr.io/paketo-buildpacks/run:full-cnb
Rebasing example on run image gcr.io/paketo-buildpacks/run:full-cnb
Saving example...
*** Images (3aad57187a46):
      example
Rebased Image: 3aad57187a46babd571b4853c67059622f03677ef65e6f4e7b93ee6ba20ce490
Successfully rebased image example

➜  pack inspect example --output json | jq .local_info.run_images
[
  {
    "name": "index.docker.io/paketobuildpacks/run:base-cnb"
  },
  {
    "name": "gcr.io/paketo-buildpacks/run:base-cnb"
  }
]

I have spotted that https://github.com/buildpacks/lifecycle/blob/main/rebaser.go#L80 is missing to update the following:

origMetadata.Stack.RunImage.Image
origMetadata.Stack.RunImage.Mirrors

What do you think? am I going in the right direction?

It's been hard to identify the issue as I am not familiarized with the code and I am struggling to test this, I think I'll ask for help in Slack 😉

@natalieparellano
Copy link
Member

@hernandanielg thanks for the update - you are right, this would need to be changed in the lifecycle. It seems to me that the CNB spec as currently written does not cover the use case where in pack rebase example --run-image run-image-tag, run-image-tag is not one of origMetadata.Stack.RunImage.Image or origMetadata.Stack.RunImage.Mirrors. I think it would be fair to replace origMetadata.Stack.RunImage.Image with the newly provided run-image-tag, but in this case you would lose all mirror information. Is that acceptable?

As this seems to be more of a bug in the spec vs pack/lifecycle a good first step might be to open a PR against the platform spec, on or around here and then we can discuss it further.

cc'ing also @jabrown85 & @joeybrown-sf as folks who might have thoughts around run image mirrors & rebase.

@hernandanielg
Copy link

Created this PR buildpacks/spec#360 we can discuss it there and then continue with the implementation here as @natalieparellano suggested ;)

@natalieparellano
Copy link
Member

@harryli0108 @daniv-msft could we trouble you for a review on buildpacks/spec#360? Is this in line with what you were expecting?

I'm going to move this issue over to the lifecycle as it would be implemented there. It should be a small change. If we act soon we could fold this into the next minor release. Would anyone be willing to take this on?

@natalieparellano natalieparellano transferred this issue from buildpacks/pack May 22, 2023
@natalieparellano natalieparellano added this to the lifecycle 0.17.0 milestone May 22, 2023
@natalieparellano
Copy link
Member

Linked a PR that will close this issue

@harryli0108
Copy link
Author

Hi @natalieparellano and @hernandanielg. Thank you very much for the prompted replies and the quick PR! However, I'm still running into the same issue even with rebasing using the latest API version with the lts, prerelease, and the tar.gz created from the PR merge Github workflow. I use the following command to rebase the image using lifecycle:
./lifecycle/rebaser -previous-image <image-name-getting-rebased>:<old-tag> <image-name-after-rebased>:<new-tag> with the following environment variables set:
CNB_PLATFORM_API=0.11 (for lts) or CNB_PLATFORM_API=0.12 (for prerelease)
CNB_USE_DAEMONO=false
CNB_RUN_IMAGE=<latest-run-image-we-released>
CNB_REGISTRY_AUTH={"<reg-url>":"<auth-type> <token>"}

Afterwards, I run pack inspect <image-name-after-rebased>:<new-tag> to check the remote image info, and the run image is still using the old one.

Could you take a look if possible? Much appreciated!

@natalieparellano
Copy link
Member

Thanks for the report @harryli0108 - I'll take a look

@natalieparellano
Copy link
Member

@harryli0108 I am probably doing something different than you, but I'm not seeing this behavior in my testing. I'm using the daemon here, but when I do:

$  pack inspect hello-extensions
Inspecting image: hello-extensions

REMOTE:
(not present)

LOCAL:

Stack: io.buildpacks.samples.stacks.alpine

Base Image:
  Reference: b674d18e2cf7c7b17e63872e2d47ff7c5d767a430e22e32c391c78f3fe4adbe9
  Top Layer: sha256:499c7faef09066a72544ba5a62fb2752406b4058087d4aada433f4f2e169a973

Run Images:
  localhost:5000/run-image-curl

The original run image is localhost:5000/run-image-curl. Then:

$  CNB_USE_DAEMON=true CNB_PLATFORM_API=0.12 CNB_RUN_IMAGE=cnbs/sample-stack-run:alpine ./out/darwin-amd64/lifecycle/rebaser -previous-image=hello-extensions -report ./report.toml new-extensions-2
Saving new-extensions-2...
*** Images (bd0ba019f570):
      new-extensions-2

The run image changed from localhost:5000/run-image-curl -> cnbs/sample-stack-run:alpine and cnbs/sample-stack-run:alpine is NOT a known mirror. Then:

$  pack inspect new-extensions-2
Inspecting image: new-extensions-2

REMOTE:
(not present)

LOCAL:

Stack: io.buildpacks.samples.stacks.alpine

Base Image:
  Reference: 76c07164e3cb44d572116dbfc3469f5e785bb320d69d8fe4fe54201e9ed6eac3
  Top Layer: sha256:122328bebc9de54dd0a06091ae0e371896d33e72f7617f89d4621a37437dad04

Run Images:
  cnbs/sample-stack-run:alpine

The new run image is cnbs/sample-stack-run:alpine.

It's worth noting, the logic changed slightly from the linked PR commit. That one has:

	if r.PlatformAPI.AtLeast("0.12") {
		// update stack and runImage if needed
		if !origMetadata.RunImage.Contains(newBaseImage.Name()) {
			origMetadata.RunImage.Image = newBaseImage.Name()
			origMetadata.RunImage.Mirrors = []string{}
			newStackMD := origMetadata.RunImage.ToStackMetadata()
			origMetadata.Stack = &newStackMD
		}
	}

Whereas 0.17.0-rc.1 has:

	if appPlatformAPI != "" && api.MustParse(appPlatformAPI).AtLeast("0.12") {
		// update stack and runImage if needed
		if !origMetadata.RunImage.Contains(newBaseImage.Name()) {
			origMetadata.RunImage.Image = newBaseImage.Name()
			origMetadata.RunImage.Mirrors = []string{}
			newStackMD := origMetadata.RunImage.ToStack()
			origMetadata.Stack = &newStackMD
		}
	}

To emphasize, when checking if "platform API is at least 0.12" we are looking at the platform API of the previously built image vs the platform API in CNB_PLATFORM_API. There could be an argument for doing the opposite check, so please let us know if that isn't what you expected.

Would it be possible to repeat your test using lifecycle 0.17.0-rc.1?

@harryli0108
Copy link
Author

@natalieparellano Thanks for looking into it! I've just tested with 0.17.0-rc.1 with CNB_PLATFORM_API set to 0.12. Here's the outputs:

$ pack inspect testacr.azurecr.io/sample-app:7.0.5

Stack: io.buildpacks.stacks.capps.dotnet7

Base Image:
  Reference: a2c6be0bbeb8b1b9318706ddfdccd24a8e5db001554c8e0833de11073a45cba2
  Top Layer: sha256:bc0945687692bbc02973a195ff26b88de39dc0a4f07998149f596fae5b343278

Run Images:
  mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0-cbl-mariner2.0

And then rebasing it using lifecycle rebaser:

$ export CNB_PLATFORM_API=0.12
$ export CNB_RUN_IMAGE=mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0.7-cbl-mariner2.0
$ ./lifecycle/rebaser --log-level=debug -previous-image testacr.azurecr.io/sample-app:7.0.5 testacr.azurecr.io/sample-app:7.0.7

And then do another pack inspect:

$ pack inspect testacr.azurecr.io/sample-app:7.0.7

REMOTE:

Stack: io.buildpacks.stacks.capps.dotnet7

Base Image:
  Reference: mcr.microsoft.com/oryx/builder@sha256:d4c08fed1e2b411cbe1456e7fe1b343df234d69c37703deb1f47639f355dd2b5
  Top Layer: sha256:e2b501ea43827fd308ab637bddd6660dc66ccc7b2cb783d368db11a869ca4895

Run Images:
  mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0-cbl-mariner2.0

One note about the first pack insepct. The run image it is referring to was supposed to be 7.0.5 rather than 7.0. That one isn't reflected correctly after build.

@natalieparellano
Copy link
Member

Ah okay - what platform API was testacr.azurecr.io/sample-app:7.0.5 built with? You can get this with docker inspect testacr.azurecr.io/sample-app:7.0.5 | grep CNB_PLATFORM_API

@natalieparellano
Copy link
Member

One note about the first pack insepct. The run image it is referring to was supposed to be 7.0.5 rather than 7.0

That is very weird. Is it possible that the metadata IS getting updated, but pack inspect is truncating the tag somehow? What happens if you run docker inspect testacr.azurecr.io/sample-app:7.0.7 | jq -r .[0].Config.Labels.\"io.buildpacks.lifecycle.metadata\" | jq .runImage.image

@harryli0108
Copy link
Author

The CNB_PLATFORM_API of the 7.0.5 image was 0.9, and after I ran this command docker inspect testacr.azurecr.io/sample-app:7.0.7 | jq -r .[0].Config.Labels.\"io.buildpacks.lifecycle.metadata\" | jq .runImage.image, I got null

@natalieparellano
Copy link
Member

natalieparellano commented Jun 22, 2023

Thanks @harryli0108 - could you try running docker inspect testacr.azurecr.io/sample-app:7.0.7 | jq -r .[0].Config.Labels.\"io.buildpacks.lifecycle.metadata\" | jq .stack.runImage.image. As 7.0.5 was built with CNB_PLATFORM_API=0.9 it wouldn't include the newer runImage metadata. But it would be helpful to see if the stack metadata contains 7.0.5 or 7.0, because that could help us figure out if the bug is in pack or the lifecycle.

To emphasize, when checking if "platform API is at least 0.12" we are looking at the platform API of the previously built image vs the platform API in CNB_PLATFORM_API

(re: my comment above) We discussed this in today's CNB Working Group (6/22) and we think it makes more sense to do the opposite check. In the current implementation, as 7.0.5 was built with CNB_PLATFORM_API=0.9, the metadata will never get updated, even when you provide CNB_PLATFORM_API=0.12 to the rebaser.

What we thought makes more sense: when you provide CNB_PLATFORM_API=0.12 to the rebaser, always update the metadata, no matter what platform API version was used to build the image. However, when the run image provided to the rebaser is not found in the existing metadata, the lifecycle will fail to rebase unless --force is passed.

I'm going to put up a PR shortly to fix - when that's ready would you be able to test it out?

@harryli0108
Copy link
Author

@natalieparellano Thanks for the reply! I've just ran the command and got mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0-cbl-mariner2.0.

Also, I'm open to test things out if there's a potential fix for this! Please feel free to ping me if the PR is out.

@natalieparellano
Copy link
Member

@harryli0108 - thank you. I think the truncation/altering of the tag may be the real culprit here. I'll investigate...

@natalieparellano
Copy link
Member

@harryli0108 could you run one more command please: docker inspect testacr.azurecr.io/sample-app:7.0.5 | jq -r .[0].Config.Labels.\"io.buildpacks.lifecycle.metadata\" | jq .stack.runImage.image (differs from the above command in that 7.0.7 -> 7.0.5)

@harryli0108
Copy link
Author

yep, I still got mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0-cbl-mariner2.0

@natalieparellano
Copy link
Member

Ah okay - then there is still ambiguity here (maybe we're truncating the tag, maybe we're failing to update the run image metadata). Would you mind conducting a test on the artifact from https://github.com/buildpacks/lifecycle/actions/runs/5348592701/jobs/9698656837 (when it finishes). You will need to provide CNB_PLATFORM_API=0.12 to the rebaser.

@harryli0108
Copy link
Author

I've just tested out. Seems like the issue still lingers. Here's the env vars I set:
CNB_USE_DAEMON=false
CNB_REGISTRY_AUTH=<registry-url-and-auth-token>
CNB_PLATFORM_API=0.12
CNB_RUN_IMAGE=mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0.7-cbl-mariner2.0

After running ./lifecycle/rebaser --log-level=debug -previous-image testacr.azurecr.io/sample-app:7.0.5 testacr.azurecr.io/sample-app:7.0.7-test, the newly rebased image was pushed to registry.
Here's the output I got from running pack inspect on the new image:

$ pack inspect testacr.azurecr.io/sample-app:7.0.7-test

REMOTE:

Stack: io.buildpacks.stacks.capps.dotnet7

Base Image:
  Reference: mcr.microsoft.com/oryx/builder@sha256:d4c08fed1e2b411cbe1456e7fe1b343df234d69c37703deb1f47639f355dd2b5
  Top Layer: sha256:e2b501ea43827fd308ab637bddd6660dc66ccc7b2cb783d368db11a869ca4895

Run Images:
  mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0-cbl-mariner2.0

Here's the stack run image metadata of this image:

$ docker inspect testacr.azurecr.io/sample-app:7.0.7-test | jq -r .[0].Config.Labels.\"io.buildpacks.lifecycle.metadata\" | jq .stack.runImage.image

"mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0-cbl-mariner2.0"

Here's the run image metadata of this image:

$ docker inspect testacr.azurecr.io/sample-app:7.0.7-test | jq -r .[0].Config.Labels.\"io.buildpacks.lifecycle.metadata\" | jq .runImage.image

null

@natalieparellano
Copy link
Member

@harryli0108 are you able to paste the log output from the rebase operation?

@natalieparellano
Copy link
Member

I think I've figured out the issue - we weren't considering the tag part of the reference in our matching logic 😞 https://github.com/buildpacks/lifecycle/actions/runs/5349213062 should have the fix.

@harryli0108
Copy link
Author

are you able to paste the log output from the rebase operation?

Sorry for the delay of reply here. Here's the console log I have from the rebaser:

Starting rebaser...
Parsing inputs...
Ensuring privileges...
Executing command...
Saving testacr.azurecr.io/sample-app:7.0.7-test...
*** Images (sha256:372b0d962b93573b0ba0a2ff23244deefe26f175fd37e5ad7ef7e3a53659a8d4):
testacr.azurecr.io/sample-app:7.0.7-test

*** Digest: sha256:372b0d962b93573b0ba0a2ff23244deefe26f175fd37e5ad7ef7e3a53659a8d4

*** Manifest Size: 2052

https://github.com/buildpacks/lifecycle/actions/runs/5349213062 should have the fix.

I've just tested and got the following error when running rebaser:

failed to rebase: rebase app image: new base image 'mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0.7-cbl-mariner2.0' not found in existing run image metadata: {"image":"mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0-cbl-mariner2.0"}

I'm wondering if this error should be expected in this case?

@harryli0108
Copy link
Author

I just realized I forgot to pass the --force argument here. It is now working perfectly with lifecycle rebaser. Here's the outputs I got:

$ ./lifecycle/rebaser --log-level=debug -previous-image testacr.azurecr.io/sample-app:7.0.5 testacr.azurecr.io/sample-app:7.0.7-test

Starting rebaser...
Parsing inputs...
Ensuring privileges...
Executing command...
Warning: rebase app image: new base image 'mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0.7-cbl-mariner2.0' not found in existing run image metadata: {"image":"mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0-cbl-mariner2.0"}
Saving testacr.azurecr.io/sample-app:7.0.7-test...
*** Images (sha256:372b0d962b93573b0ba0a2ff23244deefe26f175fd37e5ad7ef7e3a53659a8d4):
testacr.azurecr.io/sample-app:7.0.7-test

*** Digest: sha256:372b0d962b93573b0ba0a2ff23244deefe26f175fd37e5ad7ef7e3a53659a8d4

*** Manifest Size: 2052

And when I do pack inspect, here's the result:

$ pack inspect testacr.azurecr.io/sample-app:7.0.7-test

REMOTE:

Stack: io.buildpacks.stacks.capps.dotnet7

Base Image:
  Reference: mcr.microsoft.com/oryx/builder@sha256:d4c08fed1e2b411cbe1456e7fe1b343df234d69c37703deb1f47639f355dd2b5
  Top Layer: sha256:e2b501ea43827fd308ab637bddd6660dc66ccc7b2cb783d368db11a869ca4895

Run Images:
  mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0.7-cbl-mariner2.0

Meanwhile, I'm wondering if this fix will also fix the pack rebase operation?

@natalieparellano
Copy link
Member

Great news @harryli0108! Thank you for checking. I've added 5edfb9b to provide some hint about the -force flag when rebase fails.

Meanwhile, I'm wondering if this fix will also fix the pack rebase operation?

Apologies, I've gotten a little turned around here... is there an outstanding issue with rebase that I missed?

@harryli0108
Copy link
Author

Sorry for not explaining it clearly. The pack CLI rebase is still having the issue where run image metadata is not updated correctly. For instance:

$ pack rebase --run-image mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0.7-cbl-mariner2.0 testacr.azurecr.io/sample-app:7.0.5

7.0.5: Pulling from sample-app
Digest: sha256:db9955b5b5eb3a3033023204be596e98ea73f8d78d110d8a1d1e4974e7ff9dbf
Status: Downloaded newer image for testacr.azurecr.io/sample-app:7.0.5
run-dotnet-aspnet-7.0.7-cbl-mariner2.0: Pulling from oryx/builder
Digest: sha256:d4c08fed1e2b411cbe1456e7fe1b343df234d69c37703deb1f47639f355dd2b5
Status: Image is up to date for mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0.7-cbl-mariner2.0
Rebasing testacr.azurecr.io/sample-app:7.0.5 on run image mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0.7-cbl-mariner2.0
Saving testacr.azurecr.io/sample-app:7.0.5...
*** Images (259a7593ecb6):
      testacr.azurecr.io/sample-app:7.0.5
Rebased Image: 259a7593ecb681f03d6bac2d67f10d919fc8b9b7b23a045118b907b23ba14e40
Successfully rebased image testacr.azurecr.io/sample-app:7.0.5

// Since pack cli currently doesn't allow creating new tag for rebased image, the following inspection output of 7.0.5 image is reflecting on the latest rebased image. 

$ pack inspect testacr.azurecr.io/sample-app:7.0.5

LOCAL:

Stack: io.buildpacks.stacks.capps.dotnet7

Base Image:
  Reference: 54a2931be8cef4daa9ab517dd2aa07da8c498931d2c7ae090fe4cb4e7e02bc83
  Top Layer: sha256:e2b501ea43827fd308ab637bddd6660dc66ccc7b2cb783d368db11a869ca4895

Run Images:
  mcr.microsoft.com/oryx/builder:run-dotnet-aspnet-7.0-cbl-mariner2.0

@natalieparellano
Copy link
Member

Ohh I see what you're saying - yes, pack rebase will be fixed once pack upgrades to the fixed lifecycle in go.mod (pack rebase uses the lifecycle as a library). This would be the PR to keep an eye on: buildpacks/pack#1717

@harryli0108
Copy link
Author

@natalieparellano Thanks for sharing the PR link! Just want to double check, will this also expose the -force option to the pack CLI as well?

@natalieparellano
Copy link
Member

Ah yes, the --force flag is already there in unreleased pack.

I'm going to go ahead and close this issue, but please let us know if you encounter any other issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/ready type/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants