feat: add checking in remote repo if manifest is a chart #3811

a1994sc · 2025-05-15T23:21:08Z

Description

Makes call to zarf registry when trying to mutate ocirepo resource, and checks if the manifest is a helm chart

Related Issue

Fixes #3435

Checklist before merging

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

netlify · 2025-05-15T23:21:15Z

✅ Deploy Preview for zarf-docs ready!

Name	Link
🔨 Latest commit	`7321088`
🔍 Latest deploy log	https://app.netlify.com/projects/zarf-docs/deploys/687ac04e4a937f00086470de
😎 Deploy Preview	https://deploy-preview-3811--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 project configuration.

src/internal/agent/hooks/flux-ocirepo.go

a1994sc · 2025-05-19T19:00:39Z

Hey team, I believe that this PR is in a better state to be reviewed. I have fixed the unit tests to maintained backwards-compatibility with the current logic, e.i. if A) it can not reach the zarf registry, or B) if the media type is not of type helm. I also included a new unit test for checking with an in-memory registry, shoot-out to @brandtkeller for the awesome help both here in the PR and in DM's .

Please let me know if there are any changes you would like to make

Edit:

I will be also updating the docs and what not, so well the code stuff is ready for review the UX/docs needs to be updated

AustinAbro321

This is an excellent start, thank you for doing research and driving this forward!

src/internal/agent/hooks/common_test.go

src/internal/agent/hooks/flux-ocirepo.go

src/internal/agent/hooks/common.go

src/internal/agent/hooks/flux-ocirepo.go

codecov · 2025-05-27T12:16:44Z

Codecov Report

Attention: Patch coverage is 79.03226% with 13 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/internal/agent/hooks/common.go	82.35%	4 Missing and 2 partials ⚠️
src/internal/agent/hooks/flux-ocirepo.go	87.50%	2 Missing and 1 partial ⚠️
src/internal/packager/images/pull.go	0.00%	2 Missing ⚠️
src/internal/packager/images/common.go	0.00%	1 Missing ⚠️
src/internal/packager/images/push.go	0.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
src/internal/packager/images/common.go	`31.57% <0.00%> (ø)`
src/internal/packager/images/push.go	`52.28% <0.00%> (ø)`
src/internal/packager/images/pull.go	`53.16% <0.00%> (ø)`
src/internal/agent/hooks/flux-ocirepo.go	`82.64% <87.50%> (+1.20%)`	⬆️
src/internal/agent/hooks/common.go	`85.00% <82.35%> (-15.00%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

a1994sc · 2025-05-27T19:24:26Z

This is an excellent start, thank you for doing research and driving this forward!

No problem at all!

src/internal/agent/hooks/flux-ocirepo.go

AustinAbro321

A few nits, and some test cleanup to be done. Overall changes are looking good.

src/internal/agent/hooks/flux-ocirepo_test.go

src/internal/agent/hooks/flux-ocirepo.go

src/internal/agent/hooks/common_test.go

src/internal/agent/hooks/flux-ocirepo.go

src/internal/agent/hooks/common_test.go

AustinAbro321

A few small tweaks, otherwise looks good

site/src/content/docs/ref/init-package.mdx

src/internal/agent/hooks/common_test.go

a1994sc · 2025-06-06T19:59:03Z

src/internal/agent/hooks/flux-ocirepo_test.go

+				s = &state.State{RegistryInfo: types.RegistryInfo{
+					Address: fmt.Sprintf("%s:%d", tt.svc.Spec.ClusterIP, tt.svc.Spec.Ports[0].Port),
+				}}
+			}


I have some mixed feelings on how I implemented this part, where I override the test endpoint of the zarf docker registry, the tests do now pass... but Please let me know if you want me to rework it

thanks github for not saying that this wasn't visible to anyone else...

A cleaner solution could be to move up setupRegistry above the table tests, then add a field registryAddress to admissionTest. Then we can avoid the if statement, and each entry could own it's address

I intend to dig into this more intentionally asap - I am curious if this pattern tests the logic accordingly or is working-around it - but I need to step through the code. Will report back.

Appreciate the patience. Core functionality looks great but I'd like to revisit this test so as to have the purpose of the svc object in the admissionTest appropriately testing our logic for internal service lookup.

Spent some time mapping these changes to the test on main and do believe this is working-around the issue and not testing service discovery logic.

Stepping through the test -> NewOCIRepositoryMutationHook -> mutateOCIRepo -> GetServiceInfoFromRegistryAddress -> serviceInfoFromNodePortURL -> content

The workaround currently pre-sets the state to the internal IP:port - which isn't a valid state. So it fails in the early detection stages.

I'd like to see registry still set as the nodeport url 127.0.0.1:31999 and then identifying the internal service:port as intended. The ReplacePatchOperation should then be expecting the original 127.0.0.1:31999.

All of that said - it probably puts you back in the original spot. I can help run through the debugging more but do believe some of the root of the issue is that localhost was used in some of the tests for not mutating andserviceInfoFromNodePortURL() cannot actually handle that as expected (maybe a bug - maybe expected - needs refinement).

Appreciate your bearing with us as we get better about responding to these PR's.

brandtkeller

I like the direction - appreciate your patience. Planning to step through the testing shortly but wanted to pass on some thoughts.

src/internal/agent/hooks/flux-ocirepo.go

brandtkeller · 2025-06-27T02:33:35Z

src/internal/agent/hooks/flux-ocirepo_test.go

+				s = &state.State{RegistryInfo: types.RegistryInfo{
+					Address: fmt.Sprintf("%s:%d", tt.svc.Spec.ClusterIP, tt.svc.Spec.Ports[0].Port),
+				}}
+			}


I intend to dig into this more intentionally asap - I am curious if this pattern tests the logic accordingly or is working-around it - but I need to step through the code. Will report back.

brandtkeller

great intent - want to see the testing provenance align otherwise looks great. Tough edge to solve.

brandtkeller · 2025-07-02T17:43:38Z

src/internal/agent/hooks/flux-ocirepo_test.go

+				s = &state.State{RegistryInfo: types.RegistryInfo{
+					Address: fmt.Sprintf("%s:%d", tt.svc.Spec.ClusterIP, tt.svc.Spec.Ports[0].Port),
+				}}
+			}


Appreciate the patience. Core functionality looks great but I'd like to revisit this test so as to have the purpose of the svc object in the admissionTest appropriately testing our logic for internal service lookup.

Spent some time mapping these changes to the test on main and do believe this is working-around the issue and not testing service discovery logic.

Stepping through the test -> NewOCIRepositoryMutationHook -> mutateOCIRepo -> GetServiceInfoFromRegistryAddress -> serviceInfoFromNodePortURL -> content

The workaround currently pre-sets the state to the internal IP:port - which isn't a valid state. So it fails in the early detection stages.

I'd like to see registry still set as the nodeport url 127.0.0.1:31999 and then identifying the internal service:port as intended. The ReplacePatchOperation should then be expecting the original 127.0.0.1:31999.

All of that said - it probably puts you back in the original spot. I can help run through the debugging more but do believe some of the root of the issue is that localhost was used in some of the tests for not mutating andserviceInfoFromNodePortURL() cannot actually handle that as expected (maybe a bug - maybe expected - needs refinement).

Appreciate your bearing with us as we get better about responding to these PR's.

Signed-off-by: Allen Conlon <[email protected]>

Signed-off-by: Brandt Keller <[email protected]>

AustinAbro321 · 2025-07-07T20:54:43Z

I tried the example here, but it seems that it's not connecting to the registry. I'm not sure why. Were you able to get this to work? I added a log to make sure that state is being pulled correctly and it seems that it is

a1994sc · 2025-07-07T21:25:23Z

~~I am seeing that as well... I will debug it and push an update~~

Correction.... I'm seeing the following error:

Signed-off-by: Allen Conlon <[email protected]>

src/internal/agent/hooks/common.go

Signed-off-by: Allen Conlon <[email protected]>

AustinAbro321

LGTM, only thing that's a bit awkward is that the example won't work if someone tries it on the latest release of Zarf. No changes requested, but I might wait until we're a bit closer to release (July 25th) to merge

a1994sc · 2025-07-11T13:59:58Z

Ok, that works from me, Thanks @AustinAbro321

brandtkeller

lgtm - we did just merge the migration of the agent orchestration from manifests to helm chart - so this will require a quick conflict resolve.

a1994sc commented May 15, 2025

View reviewed changes

src/internal/agent/hooks/flux-ocirepo.go Outdated Show resolved Hide resolved

a1994sc force-pushed the feat/oci-repo branch from 64df862 to 828723a Compare May 19, 2025 18:55

a1994sc marked this pull request as ready for review May 19, 2025 18:55

a1994sc requested review from a team as code owners May 19, 2025 18:55

a1994sc force-pushed the feat/oci-repo branch from 828723a to 1761dde Compare May 19, 2025 20:18

a1994sc requested a review from brandtkeller May 22, 2025 20:33

AustinAbro321 requested changes May 27, 2025

View reviewed changes

a1994sc force-pushed the feat/oci-repo branch from f83f552 to dcdec1f Compare May 27, 2025 19:22

a1994sc requested a review from AustinAbro321 May 27, 2025 19:23

AustinAbro321 reviewed May 28, 2025

View reviewed changes

src/internal/agent/hooks/flux-ocirepo.go Outdated Show resolved Hide resolved

a1994sc requested a review from AustinAbro321 May 28, 2025 19:54

AustinAbro321 requested changes Jun 2, 2025

View reviewed changes

a1994sc force-pushed the feat/oci-repo branch from 30a1110 to 420c4c9 Compare June 2, 2025 20:26

a1994sc requested a review from AustinAbro321 June 2, 2025 20:30

a1994sc force-pushed the feat/oci-repo branch 4 times, most recently from db059bf to 1a89c64 Compare June 6, 2025 20:27

a1994sc force-pushed the feat/oci-repo branch from 1a89c64 to a32c2cb Compare June 23, 2025 22:21

AustinAbro321 requested changes Jun 25, 2025

View reviewed changes

site/src/content/docs/ref/init-package.mdx Outdated Show resolved Hide resolved

src/internal/agent/hooks/common_test.go Outdated Show resolved Hide resolved

a1994sc commented Jun 25, 2025

View reviewed changes

a1994sc force-pushed the feat/oci-repo branch from 599519a to 4c9397b Compare June 25, 2025 20:47

brandtkeller reviewed Jun 27, 2025

View reviewed changes

a1994sc force-pushed the feat/oci-repo branch 2 times, most recently from 1cd6678 to 8a62fe8 Compare July 1, 2025 21:35

brandtkeller requested changes Jul 2, 2025

View reviewed changes

a1994sc added 4 commits July 2, 2025 19:08

feat(ocirepo): add logic to check mediatype

25c2558

Signed-off-by: Allen Conlon <[email protected]>

chore: update agent test to minimize pulled images

49dec12

Signed-off-by: Allen Conlon <[email protected]>

chore: update docs for oci repo

4386e28

Signed-off-by: Allen Conlon <[email protected]>

ci: fixed formatting

684b13b

Signed-off-by: Allen Conlon <[email protected]>

a1994sc force-pushed the feat/oci-repo branch from 8a62fe8 to 684b13b Compare July 2, 2025 23:08

brandtkeller added 2 commits July 6, 2025 04:01

fix(ocirepo): update testing to resolve internal service from nodeport

119d759

Signed-off-by: Brandt Keller <[email protected]>

fix(lint): resolve linting issues

45335a1

Signed-off-by: Brandt Keller <[email protected]>

feat: add helm-oci for podinfo example

e64cd2f

Signed-off-by: Allen Conlon <[email protected]>

AustinAbro321 reviewed Jul 8, 2025

View reviewed changes

src/internal/agent/hooks/common.go Outdated Show resolved Hide resolved

feat: update logic for checking registry http type

cc50a53

Signed-off-by: Allen Conlon <[email protected]>

a1994sc requested review from brandtkeller and AustinAbro321 July 9, 2025 13:16

AustinAbro321 previously approved these changes Jul 11, 2025

View reviewed changes

brandtkeller previously approved these changes Jul 18, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into feat/oci-repo

7321088

a1994sc dismissed stale reviews from brandtkeller and AustinAbro321 via 7321088 July 18, 2025 21:44

a1994sc requested review from AustinAbro321 and brandtkeller July 18, 2025 21:45

feat: add checking in remote repo if manifest is a chart #3811

Are you sure you want to change the base?

feat: add checking in remote repo if manifest is a chart #3811

Conversation

a1994sc commented May 15, 2025

Description

Related Issue

Checklist before merging

Uh oh!

netlify bot commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for zarf-docs ready!

Uh oh!

Uh oh!

a1994sc commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AustinAbro321 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

a1994sc commented May 27, 2025

Uh oh!

Uh oh!

AustinAbro321 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AustinAbro321 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

a1994sc Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

a1994sc Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

AustinAbro321 Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

brandtkeller Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

brandtkeller Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

brandtkeller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brandtkeller Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

brandtkeller left a comment

Choose a reason for hiding this comment

Uh oh!

brandtkeller Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

AustinAbro321 commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented May 15, 2025 •

edited

Loading

a1994sc commented May 19, 2025 •

edited

Loading

codecov bot commented May 27, 2025 •

edited

Loading

AustinAbro321 commented Jul 7, 2025 •

edited

Loading

a1994sc commented Jul 7, 2025 •

edited

Loading