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

fix: maintain agent mutate even when already mutated #3166

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

Conversation

a1994sc
Copy link
Contributor

@a1994sc a1994sc commented Oct 31, 2024

Description

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

  • fluxcd-oci-repo
  • fluxcd-helm-repo
  • fluxcd-git-repo
  • argocd-repository
  • argocd-application

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

Signed-off-by: Allen Conlon <software@conlon.dev>
@a1994sc a1994sc requested review from a team as code owners October 31, 2024 00:18
Copy link

netlify bot commented Oct 31, 2024

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit db8c955
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/673780971e8ea90008ab667e

Copy link
Contributor

@AustinAbro321 AustinAbro321 left a comment

Choose a reason for hiding this comment

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

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

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

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

Files with missing lines Patch % Lines
src/internal/agent/hooks/flux-helmrepo.go 80.00% 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 76.92% <80.00%> (-0.35%) ⬇️
src/internal/agent/hooks/flux-ocirepo.go 81.44% <85.36%> (-1.12%) ⬇️

@a1994sc
Copy link
Contributor Author

a1994sc commented Nov 4, 2024

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
Copy link
Contributor

@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

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
Copy link
Contributor Author

a1994sc commented Nov 7, 2024

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

Signed-off-by: Allen Conlon <software@conlon.dev>
@a1994sc
Copy link
Contributor Author

a1994sc commented Nov 8, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zarf-agent does not maintain mutate after it already has zarf-agent: patched label applied
2 participants