fix: maintain agent mutate even when already mutated

[a1994sc]

Description

Removes the checks on the agent webhook for effecting already patched gitops related resources:

• fluxcd-oci-repo
• fluxcd-helm-repo

The reasoning for removing the this check is that during the regular lifecycle of gitops tools is that they will re-apply the manifests to make sure the cluster reflects the correct state; this means that fluxcd will override the mutated repos to point to something outside of zarf's control, e.i. clear web endpoints that might not be accessible.

Note

So I did not add tests for removing the check on pods... My logic is that pods are static through their lifecycle once they start, so zarf would only need to mutate them once.

Related Issue

Fixes #3146

Checklist before merging

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

[netlify]

✅ Deploy Preview for zarf-docs canceled.

Name	Link
🔨 Latest commit	98b3229
🔍 Latest deploy log	https://app.netlify.com/sites/zarf-docs/deploys/673e4cc32d9af5000839a6be

[AustinAbro321]

Nice work so far! Since this PR is strictly removing code rather than adding code can you delete the test cases involving an already patched Zarf agent, both existing and new

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 12 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/internal/agent/hooks/flux-helmrepo.go	80.64%	4 Missing and 2 partials ⚠️
src/internal/agent/hooks/flux-ocirepo.go	85.36%	4 Missing and 2 partials ⚠️

Files with missing lines	Coverage Δ
src/internal/agent/hooks/flux-helmrepo.go	77.50% <80.64%> (+0.22%)	⬆️
src/internal/agent/hooks/flux-ocirepo.go	81.44% <85.36%> (-1.12%)	⬇️

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[a1994sc]

Nice work so far! Since this PR is strictly removing code rather than adding code can you delete the test cases involving an already patched Zarf agent, both existing and new

So the thought process in why I added the tests for patching even if they already have the labels is so that it leaves some tests for future developers to not remove that functionality... I can remove the tests if you would still like

[AustinAbro321]

@a1994sc Yeah we should delete the tests related to the Zarf agent patch. We don't want to test code that isn't there.

One thing I originally didn't consider was that as the code is right now for the Helm repo and OCI repo it will not be idempotent. This is because after the original mutation occurs, we will have another mutation. Because Zarf adds a checksum to repos and images that it points to, we would re-mutate the url and add another, now incorrect, checksum. There is logic in the argocd, and flux-gitrepo webhooks to address this

zarf/src/internal/agent/hooks/flux-gitrepo.go

Lines 67 to 72 in a21ed41

	if isUpdate {
	isPatched, err = helpers.DoHostnamesMatch(state.GitServer.Address, repo.Spec.URL)
	if err != nil {
	return nil, fmt.Errorf(lang.AgentErrHostnameMatch, err)
	}
	}

, but this logic is not in the helmrepo and ocirepo webhooks. Similar logic should be added to the ocirepo and helmrepo and tests should be added to all of the webhooks to ensure this logic works as intended.

As an example, this is the test you would add to the flux-gitrepo_test.go to test this behavior

		{
			name: "should not mutate URL if it has the same hostname as Zarf state",
			admissionReq: createFluxGitRepoAdmissionRequest(t, v1.Update, &flux.GitRepository{
				Spec: flux.GitRepositorySpec{
					URL: "https://git-server.com/a-push-user/podinfo-1646971829.git",
				},
			}),
			patch: []operations.PatchOperation{
				operations.ReplacePatchOperation(
					"/spec/url",
					"https://git-server.com/a-push-user/podinfo-1646971829.git",
				),
				operations.AddPatchOperation(
					"/spec/secretRef",
					fluxmeta.LocalObjectReference{Name: config.ZarfGitServerSecretName},
				),
				operations.ReplacePatchOperation(
					"/metadata/labels",
					map[string]string{
						"zarf-agent": "patched",
					},
				),
			},
			code: http.StatusOK,
		},

[a1994sc]

Thank you Austin, I will update the PR to apply the changes!

[a1994sc]

@AustinAbro321 Hey, updated the PR to remove the extra tests and added the logic for checking the url against the zarf state registry url. Let me know if that is good

[AustinAbro321]

LGTM, but I'd like to get a review from another one of the @zarf-dev/maintainers before merge

[a1994sc]

LGTM, but I'd like to get a review from another one of the @zarf-dev/maintainers before merge

No problem! I figured it was a medium sized change so it is all good to get more eyes on things

[phillebaba]

Define these variables where they are first used rather than at the top of the function.

[a1994sc]

I followed the pattern found in src/internal/agent/hooks/flux-gitrepo.go

	var (
		patches   []operations.PatchOperation
		isPatched bool

		isCreate = r.Operation == v1.Create
		isUpdate = r.Operation == v1.Update
	)

ref but I can update and get that up tonight

[phillebaba]

Small change otherwise things look good.

[a1994sc]

@AustinAbro321 and @phillebaba

I updated the PR per @phillebaba comments