fix: allow usage of local image archives again #2303

waveywaves · 2024-02-12T20:06:28Z

Description

a check in create_stages during package creation tries to parses references for the images provided for components in the zarf.yaml config. By default the reference parsing assumes that the source of the image is that of a container registry. If the image is a tarball instead, the reference parser errors out.

The changes in this commit check if the image reference is that of a tarball and skips the reference check if it is so allowing the image to be loaded by the cli.

Related Issue

Fixes #2181

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Other (security config, docs update, etc)

Checklist before merging

Test, docs, adr added or updated as needed
Contributor Guide Steps followed

netlify · 2024-02-12T20:06:33Z

✅ Deploy Preview for zarf-docs ready!

Name	Link
🔨 Latest commit	`c3bda46`
🔍 Latest deploy log	https://app.netlify.com/sites/zarf-docs/deploys/65e8ffba31927a00084ad6e5
😎 Deploy Preview	https://deploy-preview-2303--zarf-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Racer159

This is a good start (and likely all tests will pass currently) but this won't entirely resolve the issue - zarf package create will work fine, but zarf package deploy will fail. See the below example (after this patch was applied):

kind: ZarfPackageConfig
metadata:
  name: dos-games
  description: Simple example to load classic DOS games into K8s in the airgap
  version: 1.0.0

components:
  - name: baseline
    required: true
    manifests:
      - name: multi-games
        namespace: dos-games
        files:
          - manifests/deployment.yaml
          - manifests/service.yaml
    images:
      - image.tar
    actions:
      onDeploy:
        after:
          - wait:
              cluster:
                kind: deployment
                name: game
                namespace: dos-games
                condition: available

(The image was generated with the following:)

docker pull defenseunicorns/zarf-game:multi-tile-dark
docker save defenseunicorns/zarf-game:multi-tile-dark > examples/dos-games/image.tar

zarf package create:

zarf package deploy:

(note that the SBOMs say 0 images / that there is no SBOM - if an image was pulled properly this would not be showing - the image should pull on create and be properly referenced to push into the cluster on deploy)

Racer159 · 2024-02-12T21:07:38Z

Would be good to add something like the above as a test case under src/test/e2e/src/test/packages as well

waveywaves · 2024-02-13T13:56:02Z

@Racer159 I have updated the code to properly pull the image on create, and archives the package as required, upon checking inside the zarf package tar I am also able to see the layers for the images whereas before it was empty

On the other hand while I trying to deploy the zarf package the image is referenced as the zarf-game.tar I understand that this reference needs to be changed. Below is the package which zarf is trying to deploy. This is the one that contains the local image.

I get the following error on attempting a deployment. One error which stands out is the login error to the registry.

I understand that the image is pushed to the Zarf registry in the airgapped cluster before the package is deployed. I am currently trying to figure out the mutation for the image reference so that it is pushed correctly and to the cluster and inspecting the error regarding registry login.

waveywaves · 2024-02-14T07:13:39Z

@Racer159 The latest changes in this patch fix the bug and both package create and deploy command work as expected.

waveywaves · 2024-02-14T08:22:09Z

@Racer159 The PR is updated with a e2e test now, can we enable the CI on this PR ?

waveywaves · 2024-02-16T17:28:37Z

working through the tests

src/test/packages/36-tarball-image-references/zarf.yaml

Racer159 · 2024-02-21T03:10:43Z

src/internal/packager/images/push.go

@@ -31,6 +31,7 @@ func (i *ImageConfig) PushToZarfRegistry() error {
 	var totalSize int64
 	// Build an image list from the references
 	for _, refInfo := range i.ImageList {
+		message.Debug("Loading image", refInfo)


This is an uneeded debug statement

Suggested change

message.Debug("Loading image", refInfo)

src/test/packages/36-tarball-image-references/zarf.yaml

Racer159 · 2024-02-21T03:30:45Z

src/pkg/transform/image.go

+		out.Host = "docker.io"
+		out.Name = srcReference
+		out.Path = fmt.Sprintf("%s/%s/%s", out.Host, "library", strings.TrimSuffix(srcReference, ".tar"))
+		out.Reference = srcReference


Editing the reference like this won't result in a proper image reference in the end - while the package will create, the org.opencontainers.image.base.name remains "zarf-game.tar" inside images/index.json within the zarf package.

This is loaded on deploy successfully but then when pushed converts into docker.io/library/zarf-game which does not match the defenseunicorns/zarf-game:multi-tile-dark the tarball was made from. This means that the Deployment will never pull the image and will be in an image pull backoff state:

What needs to happen is that the image reference in the tarball needs to be parsed from it's manifest.json:

(you can see this if you vim into it, and it is accessible when the tarball is loaded with crane)

For this, the zarf.yaml image reference may need to be modified to turn zarf-game.tar into defenseunicorns/zarf-game:multi-tile-dark after pulling it

waveywaves · 2024-02-21T13:12:50Z

ok got it,
considering that we won't have the .tar image because of .gitignore, we are using #2303 (comment) to pull the image before the package create command runs so that the image will be available in the CI

working through the other notes

Racer159 · 2024-02-21T14:48:06Z

ok got it, considering that we won't have the .tar image because of .gitignore, we are using #2303 (comment) to pull the image before the package create command runs so that the image will be available in the CI

working through the other notes

Yep, the onCreate before action would make the tarball just in time before it was needed for the image pull.

waveywaves · 2024-02-21T15:00:50Z

@Racer159 ready to review

I changed the name and path in the reference because of which the deploy stopped working after pushing the latest commits, now the deploy works well again

waveywaves · 2024-02-22T07:55:34Z

seems like the windows binary test is flaking, can we run that test again ?

a check in create_stages during package creation tries to parses references for the images provided for components in the zarf.yaml config. By default the reference parsing assumes that the source of the image is that of a container registry. If the image is a tarball instead, the reference parser errors out. Check if the image reference is that of a tarball and skips the reference check if it is so allowing the image to be loaded by the cli. Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com>

Racer159 · 2024-02-23T02:14:31Z

src/pkg/transform/image.go

+	if IsTarball(srcReference) {
+		out.Host = "docker.io"
+		out.Name = strings.TrimSuffix(srcReference, ".tar")
+		out.Path = fmt.Sprintf("%s/%s/%s", out.Host, "library", out.Name)
+		out.Reference = srcReference


Re-commenting on this approach as the core of the issue - this will allow the package to be created but will never match the reference in the manifest.

shall make the changes
I was able to deploy the package though created from this logic 🤔 not sure how
also the tests which I wrote for the package deploy are passing as well

the reason is that the cluster caches the image and this same image is used in earlier tests (hence the passes in CI as well). On a fresh cluster / registry it will fail with an ImagePullBackoff.

Note 127.0.0.1:31999/defenseunicorns/zarf-game:multi-tile-dark-zarf-805192805 differs from 127.0.0.1:31999/docker.io/library/zarf-game:latest-zarf-27383200

Racer159 · 2024-02-26T16:37:05Z

src/pkg/transform/image.go

 	}

 	return out, nil
 }
+
+func IsTarball(srcReference string) bool {


lint (exported functions required comments) - see the GitHub actions check warning in the Files tab.

thanks for pointing it out, i'll check it

Racer159 · 2024-02-26T16:41:47Z

src/pkg/transform/image.go

+	if IsTarball(srcReference) {
+		out.Host = "docker.io"
+		out.Name = strings.TrimSuffix(srcReference, ".tar")
+		out.Path = fmt.Sprintf("%s/%s/%s", out.Host, "library", out.Name)
+		out.Reference = srcReference


the reason is that the cluster caches the image and this same image is used in earlier tests (hence the passes in CI as well). On a fresh cluster / registry it will fail with an ImagePullBackoff.

Note 127.0.0.1:31999/defenseunicorns/zarf-game:multi-tile-dark-zarf-805192805 differs from 127.0.0.1:31999/docker.io/library/zarf-game:latest-zarf-27383200

waveywaves · 2024-02-28T23:09:34Z

@Racer159 I have updated the logic to get the image information for the image manifest in the tarball

waveywaves · 2024-02-28T23:55:36Z

package create and deploy tests are also passing locally on a newly created and initiated minikube zarf instance, ptal

waveywaves · 2024-03-04T20:54:38Z

@Racer159 I am running this test locally which works well. The test requires the image to be present as a tarball locally which is being done by the onCreate - before action. This works well locally but seems like the image is not accessible to the packager in the CI.

…ix/2181

waveywaves marked this pull request as ready for review February 12, 2024 20:10

waveywaves requested review from jeff-mccoy, Racer159, Noxsios and lucasrod16 as code owners February 12, 2024 20:10

waveywaves changed the title ~~fix: allow usage of local image archives again~~ fix: allow usage of local image archives again (WIP) Feb 12, 2024

Racer159 requested changes Feb 12, 2024

View reviewed changes

Madeline-UX requested review from Madeline-UX and removed request for Madeline-UX February 12, 2024 22:47

waveywaves force-pushed the fix/2181 branch from 8a01607 to 538a726 Compare February 13, 2024 12:48

waveywaves changed the title ~~fix: allow usage of local image archives again (WIP)~~ fix: allow usage of local image archives again Feb 14, 2024

waveywaves force-pushed the fix/2181 branch from 1221b0b to 0bc23a2 Compare February 19, 2024 14:42

AustinAbro321 requested changes Feb 20, 2024

View reviewed changes

src/test/packages/36-tarball-image-references/zarf.yaml Show resolved Hide resolved

Racer159 requested changes Feb 21, 2024

View reviewed changes

waveywaves force-pushed the fix/2181 branch from 2a2d0fe to 0d35215 Compare February 21, 2024 14:52

waveywaves force-pushed the fix/2181 branch from 0d35215 to ebe8c91 Compare February 21, 2024 15:03

waveywaves force-pushed the fix/2181 branch from 6b13311 to ec63b21 Compare February 22, 2024 19:46

Racer159 requested changes Feb 23, 2024

View reviewed changes

Merge branch 'main' into fix/2181

574f4c4

Racer159 requested changes Feb 26, 2024

View reviewed changes

waveywaves added 3 commits February 29, 2024 03:20

try to get repotags

3c2efd1

add comment for exported function

0be3635

use repotags for the name, path and tag

48bc07c

waveywaves added 2 commits February 29, 2024 05:03

fix reference

7ef160b

refactor

b7dfbd7

waveywaves added 2 commits February 29, 2024 05:26

comment for the exported image

2b3bfd5

refactor

8a2830b

Merge branch 'main' of https://github.com/defenseunicorns/zarf into f…

c3bda46

…ix/2181

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow usage of local image archives again #2303

fix: allow usage of local image archives again #2303

waveywaves commented Feb 12, 2024 •

edited

netlify bot commented Feb 12, 2024 •

edited

Racer159 left a comment

Racer159 commented Feb 12, 2024

waveywaves commented Feb 13, 2024

waveywaves commented Feb 14, 2024

waveywaves commented Feb 14, 2024

waveywaves commented Feb 16, 2024

Racer159 Feb 21, 2024

Racer159 Feb 21, 2024

Racer159 Feb 21, 2024

Racer159 Feb 21, 2024

waveywaves commented Feb 21, 2024 •

edited

Racer159 commented Feb 21, 2024

waveywaves commented Feb 21, 2024

waveywaves commented Feb 22, 2024

Racer159 Feb 23, 2024

waveywaves Feb 23, 2024 •

edited

Racer159 Feb 26, 2024

waveywaves Feb 28, 2024

Racer159 Feb 26, 2024

waveywaves Feb 28, 2024

Racer159 Feb 26, 2024

waveywaves commented Feb 28, 2024

waveywaves commented Feb 28, 2024

waveywaves commented Mar 4, 2024

fix: allow usage of local image archives again #2303

Are you sure you want to change the base?

fix: allow usage of local image archives again #2303

Conversation

waveywaves commented Feb 12, 2024 • edited

Description

Related Issue

Type of change

Checklist before merging

netlify bot commented Feb 12, 2024 • edited

✅ Deploy Preview for zarf-docs ready!

Racer159 left a comment

Choose a reason for hiding this comment

Racer159 commented Feb 12, 2024

waveywaves commented Feb 13, 2024

waveywaves commented Feb 14, 2024

waveywaves commented Feb 14, 2024

waveywaves commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waveywaves commented Feb 21, 2024 • edited

Racer159 commented Feb 21, 2024

waveywaves commented Feb 21, 2024

waveywaves commented Feb 22, 2024

Choose a reason for hiding this comment

waveywaves Feb 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waveywaves commented Feb 28, 2024

waveywaves commented Feb 28, 2024

waveywaves commented Mar 4, 2024

waveywaves commented Feb 12, 2024 •

edited

netlify bot commented Feb 12, 2024 •

edited

waveywaves commented Feb 21, 2024 •

edited

waveywaves Feb 23, 2024 •

edited