From d75c6b1f609c8dfbb0bbd1cb7606bd3017cada52 Mon Sep 17 00:00:00 2001 From: Allen Conlon Date: Wed, 30 Oct 2024 20:17:16 -0400 Subject: [PATCH 1/2] fix: maintain agent mutate even when already mutated Signed-off-by: Allen Conlon --- .../agent/hooks/argocd-application_test.go | 43 +++++++++++++++++++ .../agent/hooks/argocd-repository_test.go | 38 ++++++++++++++++ src/internal/agent/hooks/flux-gitrepo_test.go | 31 +++++++++++++ src/internal/agent/hooks/flux-helmrepo.go | 7 --- .../agent/hooks/flux-helmrepo_test.go | 19 +++++++- src/internal/agent/hooks/flux-ocirepo.go | 7 --- src/internal/agent/hooks/flux-ocirepo_test.go | 23 +++++++++- 7 files changed, 151 insertions(+), 17 deletions(-) diff --git a/src/internal/agent/hooks/argocd-application_test.go b/src/internal/agent/hooks/argocd-application_test.go index 31ec452959..749e347c7e 100644 --- a/src/internal/agent/hooks/argocd-application_test.go +++ b/src/internal/agent/hooks/argocd-application_test.go @@ -14,6 +14,7 @@ import ( "github.com/zarf-dev/zarf/src/internal/agent/operations" "github.com/zarf-dev/zarf/src/types" v1 "k8s.io/api/admission/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) @@ -78,6 +79,48 @@ func TestArgoAppWebhook(t *testing.T) { }, code: http.StatusOK, }, + { + name: "should mutate even if agent patched", + admissionReq: createArgoAppAdmissionRequest(t, v1.Create, &Application{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "zarf-agent": "patched", + }, + }, + Spec: ApplicationSpec{ + Source: &ApplicationSource{RepoURL: "https://diff-git-server.com/peanuts"}, + Sources: []ApplicationSource{ + { + RepoURL: "https://diff-git-server.com/cashews", + }, + { + RepoURL: "https://diff-git-server.com/almonds", + }, + }, + }, + }), + patch: []operations.PatchOperation{ + operations.ReplacePatchOperation( + "/spec/source/repoURL", + "https://git-server.com/a-push-user/peanuts-3883081014", + ), + operations.ReplacePatchOperation( + "/spec/sources/0/repoURL", + "https://git-server.com/a-push-user/cashews-580170494", + ), + operations.ReplacePatchOperation( + "/spec/sources/1/repoURL", + "https://git-server.com/a-push-user/almonds-640159520", + ), + operations.ReplacePatchOperation( + "/metadata/labels", + map[string]string{ + "zarf-agent": "patched", + }, + ), + }, + code: http.StatusOK, + }, { name: "should return internal server error on bad git URL", admissionReq: createArgoAppAdmissionRequest(t, v1.Create, &Application{ diff --git a/src/internal/agent/hooks/argocd-repository_test.go b/src/internal/agent/hooks/argocd-repository_test.go index fdc99fe1c2..60a10a5121 100644 --- a/src/internal/agent/hooks/argocd-repository_test.go +++ b/src/internal/agent/hooks/argocd-repository_test.go @@ -83,6 +83,44 @@ func TestArgoRepoWebhook(t *testing.T) { }, code: http.StatusOK, }, + { + name: "should mutate even if agent patched", + admissionReq: createArgoRepoAdmissionRequest(t, v1.Create, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "argocd.argoproj.io/secret-type": "repository", + "zarf-agent": "patched", + }, + Name: "argo-repo-secret", + Namespace: "argo", + }, + Data: map[string][]byte{ + "url": []byte("https://diff-git-server.com/podinfo"), + }, + }), + patch: []operations.PatchOperation{ + operations.ReplacePatchOperation( + "/data/url", + b64.StdEncoding.EncodeToString([]byte("https://git-server.com/a-push-user/podinfo-1868163476")), + ), + operations.ReplacePatchOperation( + "/data/username", + b64.StdEncoding.EncodeToString([]byte(state.GitServer.PullUsername)), + ), + operations.ReplacePatchOperation( + "/data/password", + b64.StdEncoding.EncodeToString([]byte(state.GitServer.PullPassword)), + ), + operations.ReplacePatchOperation( + "/metadata/labels", + map[string]string{ + "argocd.argoproj.io/secret-type": "repository", + "zarf-agent": "patched", + }, + ), + }, + code: http.StatusOK, + }, { name: "matching hostname on update should stay the same, but secret should be added", admissionReq: createArgoRepoAdmissionRequest(t, v1.Update, &corev1.Secret{ diff --git a/src/internal/agent/hooks/flux-gitrepo_test.go b/src/internal/agent/hooks/flux-gitrepo_test.go index dc9c17a093..806f37aca0 100644 --- a/src/internal/agent/hooks/flux-gitrepo_test.go +++ b/src/internal/agent/hooks/flux-gitrepo_test.go @@ -73,6 +73,37 @@ func TestFluxMutationWebhook(t *testing.T) { }, code: http.StatusOK, }, + { + name: "should mutate even if agent patched", + admissionReq: createFluxGitRepoAdmissionRequest(t, v1.Create, &flux.GitRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mutate-this", + Labels: map[string]string{ + "zarf-agent": "patched", + }, + }, + Spec: flux.GitRepositorySpec{ + URL: "https://github.com/stefanprodan/podinfo.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, + }, { name: "should not mutate invalid git url", admissionReq: createFluxGitRepoAdmissionRequest(t, v1.Update, &flux.GitRepository{ diff --git a/src/internal/agent/hooks/flux-helmrepo.go b/src/internal/agent/hooks/flux-helmrepo.go index a2fca0b9a4..b1d15eca62 100644 --- a/src/internal/agent/hooks/flux-helmrepo.go +++ b/src/internal/agent/hooks/flux-helmrepo.go @@ -47,13 +47,6 @@ func mutateHelmRepo(ctx context.Context, r *v1.AdmissionRequest, cluster *cluste return &operations.Result{Allowed: true}, nil } - if src.Labels != nil && src.Labels["zarf-agent"] == "patched" { - return &operations.Result{ - Allowed: true, - PatchOps: nil, - }, nil - } - zarfState, err := cluster.LoadZarfState(ctx) if err != nil { return nil, err diff --git a/src/internal/agent/hooks/flux-helmrepo_test.go b/src/internal/agent/hooks/flux-helmrepo_test.go index d0e48a0074..d645b12d7a 100644 --- a/src/internal/agent/hooks/flux-helmrepo_test.go +++ b/src/internal/agent/hooks/flux-helmrepo_test.go @@ -68,7 +68,7 @@ func TestFluxHelmMutationWebhook(t *testing.T) { code: http.StatusInternalServerError, }, { - name: "should not mutate when agent patched", + name: "should mutate even if agent patched", admissionReq: createFluxHelmRepoAdmissionRequest(t, v1.Update, &flux.HelmRepository{ ObjectMeta: metav1.ObjectMeta{ Name: "already-patched", @@ -77,9 +77,26 @@ func TestFluxHelmMutationWebhook(t *testing.T) { }, }, Spec: flux.HelmRepositorySpec{ + URL: "oci://ghcr.io/stefanprodan/charts", Type: "oci", }, }), + patch: []operations.PatchOperation{ + operations.ReplacePatchOperation( + "/spec/url", + "oci://127.0.0.1:31999/stefanprodan/charts", + ), + operations.AddPatchOperation( + "/spec/secretRef", + fluxmeta.LocalObjectReference{Name: config.ZarfImagePullSecretName}, + ), + operations.ReplacePatchOperation( + "/metadata/labels", + map[string]string{ + "zarf-agent": "patched", + }, + ), + }, code: http.StatusOK, }, { diff --git a/src/internal/agent/hooks/flux-ocirepo.go b/src/internal/agent/hooks/flux-ocirepo.go index e8c3d21a0f..4fa182459d 100644 --- a/src/internal/agent/hooks/flux-ocirepo.go +++ b/src/internal/agent/hooks/flux-ocirepo.go @@ -50,13 +50,6 @@ func mutateOCIRepo(ctx context.Context, r *v1.AdmissionRequest, cluster *cluster message.Warnf(lang.AgentWarnSemVerRef, src.Spec.Reference.SemVer) } - if src.Labels != nil && src.Labels["zarf-agent"] == "patched" { - return &operations.Result{ - Allowed: true, - PatchOps: []operations.PatchOperation{}, - }, nil - } - zarfState, err := cluster.LoadZarfState(ctx) if err != nil { return nil, err diff --git a/src/internal/agent/hooks/flux-ocirepo_test.go b/src/internal/agent/hooks/flux-ocirepo_test.go index 5cab4d2530..733e395eba 100644 --- a/src/internal/agent/hooks/flux-ocirepo_test.go +++ b/src/internal/agent/hooks/flux-ocirepo_test.go @@ -39,7 +39,7 @@ func TestFluxOCIMutationWebhook(t *testing.T) { tests := []admissionTest{ { - name: "should not mutate when agent patched", + name: "should mutate even if agent patched", admissionReq: createFluxOCIRepoAdmissionRequest(t, v1.Update, &flux.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ Name: "already-patched", @@ -54,7 +54,26 @@ func TestFluxOCIMutationWebhook(t *testing.T) { }, }, }), - patch: nil, + patch: []operations.PatchOperation{ + operations.ReplacePatchOperation( + "/spec/url", + "oci://127.0.0.1:31999/stefanprodan/manifests/podinfo", + ), + operations.AddPatchOperation( + "/spec/secretRef", + fluxmeta.LocalObjectReference{Name: config.ZarfImagePullSecretName}, + ), + operations.ReplacePatchOperation( + "/spec/ref/tag", + "6.4.0-zarf-2823281104", + ), + operations.ReplacePatchOperation( + "/metadata/labels", + map[string]string{ + "zarf-agent": "patched", + }, + ), + }, code: http.StatusOK, }, { From b8e9f77d31fb6b3d320c04576b0d328aa773993d Mon Sep 17 00:00:00 2001 From: Allen Conlon Date: Thu, 7 Nov 2024 20:25:46 -0500 Subject: [PATCH 2/2] Updated logic for when to mutate Signed-off-by: Allen Conlon --- .../agent/hooks/argocd-application_test.go | 43 -------- .../agent/hooks/argocd-repository_test.go | 38 ------- src/internal/agent/hooks/flux-gitrepo_test.go | 31 ------ src/internal/agent/hooks/flux-helmrepo.go | 41 +++++-- .../agent/hooks/flux-helmrepo_test.go | 68 ++++++++++-- src/internal/agent/hooks/flux-ocirepo.go | 72 ++++++++----- src/internal/agent/hooks/flux-ocirepo_test.go | 102 +++++++++++++----- 7 files changed, 211 insertions(+), 184 deletions(-) diff --git a/src/internal/agent/hooks/argocd-application_test.go b/src/internal/agent/hooks/argocd-application_test.go index 456e283040..4bfdad674b 100644 --- a/src/internal/agent/hooks/argocd-application_test.go +++ b/src/internal/agent/hooks/argocd-application_test.go @@ -14,7 +14,6 @@ import ( "github.com/zarf-dev/zarf/src/internal/agent/operations" "github.com/zarf-dev/zarf/src/types" v1 "k8s.io/api/admission/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) @@ -79,48 +78,6 @@ func TestArgoAppWebhook(t *testing.T) { }, code: http.StatusOK, }, - { - name: "should mutate even if agent patched", - admissionReq: createArgoAppAdmissionRequest(t, v1.Create, &Application{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "zarf-agent": "patched", - }, - }, - Spec: ApplicationSpec{ - Source: &ApplicationSource{RepoURL: "https://diff-git-server.com/peanuts"}, - Sources: []ApplicationSource{ - { - RepoURL: "https://diff-git-server.com/cashews", - }, - { - RepoURL: "https://diff-git-server.com/almonds", - }, - }, - }, - }), - patch: []operations.PatchOperation{ - operations.ReplacePatchOperation( - "/spec/source/repoURL", - "https://git-server.com/a-push-user/peanuts-3883081014", - ), - operations.ReplacePatchOperation( - "/spec/sources/0/repoURL", - "https://git-server.com/a-push-user/cashews-580170494", - ), - operations.ReplacePatchOperation( - "/spec/sources/1/repoURL", - "https://git-server.com/a-push-user/almonds-640159520", - ), - operations.ReplacePatchOperation( - "/metadata/labels", - map[string]string{ - "zarf-agent": "patched", - }, - ), - }, - code: http.StatusOK, - }, { name: "should return internal server error on bad git URL", admissionReq: createArgoAppAdmissionRequest(t, v1.Create, &Application{ diff --git a/src/internal/agent/hooks/argocd-repository_test.go b/src/internal/agent/hooks/argocd-repository_test.go index 13e5c10b71..f735ee8a58 100644 --- a/src/internal/agent/hooks/argocd-repository_test.go +++ b/src/internal/agent/hooks/argocd-repository_test.go @@ -83,44 +83,6 @@ func TestArgoRepoWebhook(t *testing.T) { }, code: http.StatusOK, }, - { - name: "should mutate even if agent patched", - admissionReq: createArgoRepoAdmissionRequest(t, v1.Create, &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "argocd.argoproj.io/secret-type": "repository", - "zarf-agent": "patched", - }, - Name: "argo-repo-secret", - Namespace: "argo", - }, - Data: map[string][]byte{ - "url": []byte("https://diff-git-server.com/podinfo"), - }, - }), - patch: []operations.PatchOperation{ - operations.ReplacePatchOperation( - "/data/url", - b64.StdEncoding.EncodeToString([]byte("https://git-server.com/a-push-user/podinfo-1868163476")), - ), - operations.ReplacePatchOperation( - "/data/username", - b64.StdEncoding.EncodeToString([]byte(state.GitServer.PullUsername)), - ), - operations.ReplacePatchOperation( - "/data/password", - b64.StdEncoding.EncodeToString([]byte(state.GitServer.PullPassword)), - ), - operations.ReplacePatchOperation( - "/metadata/labels", - map[string]string{ - "argocd.argoproj.io/secret-type": "repository", - "zarf-agent": "patched", - }, - ), - }, - code: http.StatusOK, - }, { name: "matching hostname on update should stay the same, but secret should be added", admissionReq: createArgoRepoAdmissionRequest(t, v1.Update, &corev1.Secret{ diff --git a/src/internal/agent/hooks/flux-gitrepo_test.go b/src/internal/agent/hooks/flux-gitrepo_test.go index c89f425ed7..864f82e602 100644 --- a/src/internal/agent/hooks/flux-gitrepo_test.go +++ b/src/internal/agent/hooks/flux-gitrepo_test.go @@ -73,37 +73,6 @@ func TestFluxMutationWebhook(t *testing.T) { }, code: http.StatusOK, }, - { - name: "should mutate even if agent patched", - admissionReq: createFluxGitRepoAdmissionRequest(t, v1.Create, &flux.GitRepository{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mutate-this", - Labels: map[string]string{ - "zarf-agent": "patched", - }, - }, - Spec: flux.GitRepositorySpec{ - URL: "https://github.com/stefanprodan/podinfo.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, - }, { name: "should not mutate invalid git url", admissionReq: createFluxGitRepoAdmissionRequest(t, v1.Update, &flux.GitRepository{ diff --git a/src/internal/agent/hooks/flux-helmrepo.go b/src/internal/agent/hooks/flux-helmrepo.go index 0d97722251..f67a640384 100644 --- a/src/internal/agent/hooks/flux-helmrepo.go +++ b/src/internal/agent/hooks/flux-helmrepo.go @@ -37,6 +37,14 @@ func NewHelmRepositoryMutationHook(ctx context.Context, cluster *cluster.Cluster // mutateHelmRepo mutates the repository url to point to the repository URL defined in the ZarfState. func mutateHelmRepo(ctx context.Context, r *v1.AdmissionRequest, cluster *cluster.Cluster) (*operations.Result, error) { l := logger.From(ctx) + var ( + patches []operations.PatchOperation + isPatched bool + + isCreate = r.Operation == v1.Create + isUpdate = r.Operation == v1.Update + ) + src := &flux.HelmRepository{} if err := json.Unmarshal(r.Object.Raw, &src); err != nil { return nil, fmt.Errorf(lang.ErrUnmarshal, err) @@ -63,21 +71,36 @@ func mutateHelmRepo(ctx context.Context, r *v1.AdmissionRequest, cluster *cluste "name", src.Name, "registry", registryAddress) - patchedSrc, err := transform.ImageTransformHost(registryAddress, src.Spec.URL) - if err != nil { - return nil, fmt.Errorf("unable to transform the HelmRepo URL: %w", err) + patchedURL := src.Spec.URL + + // Check if this is an update operation and the hostname is different from what we have in the zarfState + // NOTE: We mutate on updates IF AND ONLY IF the hostname in the request is different than the hostname in the zarfState + // NOTE: We are checking if the hostname is different before because we do not want to potentially mutate a URL that has already been mutated. + if isUpdate { + zarfStateAddress := helpers.OCIURLPrefix + registryAddress + isPatched, err = helpers.DoHostnamesMatch(zarfStateAddress, src.Spec.URL) + if err != nil { + return nil, fmt.Errorf(lang.AgentErrHostnameMatch, err) + } } - patchedRefInfo, err := transform.ParseImageRef(patchedSrc) - if err != nil { - return nil, fmt.Errorf("unable to parse the HelmRepo URL: %w", err) + // Mutate the helm repo URL if necessary + if isCreate || (isUpdate && !isPatched) { + patchedSrc, err := transform.ImageTransformHost(registryAddress, src.Spec.URL) + if err != nil { + return nil, fmt.Errorf("unable to transform the HelmRepo URL: %w", err) + } + + patchedRefInfo, err := transform.ParseImageRef(patchedSrc) + if err != nil { + return nil, fmt.Errorf("unable to parse the HelmRepo URL: %w", err) + } + patchedURL = helpers.OCIURLPrefix + patchedRefInfo.Name } - patchedURL := helpers.OCIURLPrefix + patchedRefInfo.Name l.Debug("mutating the Flux HelmRepository URL to the Zarf URL", "original", src.Spec.URL, "mutated", patchedURL) - patches := populateHelmRepoPatchOperations(patchedURL, zarfState.RegistryInfo.IsInternal()) - + patches = populateHelmRepoPatchOperations(patchedURL, zarfState.RegistryInfo.IsInternal()) patches = append(patches, getLabelPatch(src.Labels)) return &operations.Result{ diff --git a/src/internal/agent/hooks/flux-helmrepo_test.go b/src/internal/agent/hooks/flux-helmrepo_test.go index c9b5ad6478..b5c861331f 100644 --- a/src/internal/agent/hooks/flux-helmrepo_test.go +++ b/src/internal/agent/hooks/flux-helmrepo_test.go @@ -68,13 +68,10 @@ func TestFluxHelmMutationWebhook(t *testing.T) { code: http.StatusInternalServerError, }, { - name: "should mutate even if agent patched", - admissionReq: createFluxHelmRepoAdmissionRequest(t, v1.Update, &flux.HelmRepository{ + name: "should be mutated with no internal service registry", + admissionReq: createFluxHelmRepoAdmissionRequest(t, v1.Create, &flux.HelmRepository{ ObjectMeta: metav1.ObjectMeta{ - Name: "already-patched", - Labels: map[string]string{ - "zarf-agent": "patched", - }, + Name: "mutate-this", }, Spec: flux.HelmRepositorySpec{ URL: "oci://ghcr.io/stefanprodan/charts", @@ -100,7 +97,7 @@ func TestFluxHelmMutationWebhook(t *testing.T) { code: http.StatusOK, }, { - name: "should be mutated with no internal service registry", + name: "should be mutated with internal service registry", admissionReq: createFluxHelmRepoAdmissionRequest(t, v1.Create, &flux.HelmRepository{ ObjectMeta: metav1.ObjectMeta{ Name: "mutate-this", @@ -110,6 +107,55 @@ func TestFluxHelmMutationWebhook(t *testing.T) { Type: "oci", }, }), + patch: []operations.PatchOperation{ + operations.ReplacePatchOperation( + "/spec/url", + "oci://10.11.12.13:5000/stefanprodan/charts", + ), + operations.AddPatchOperation( + "/spec/secretRef", + fluxmeta.LocalObjectReference{Name: config.ZarfImagePullSecretName}, + ), + operations.ReplacePatchOperation( + "/metadata/labels", + map[string]string{ + "zarf-agent": "patched", + }, + ), + }, + svc: &corev1.Service{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "Service", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "zarf-docker-registry", + Namespace: "zarf", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + Ports: []corev1.ServicePort{ + { + NodePort: int32(31999), + Port: 5000, + }, + }, + ClusterIP: "10.11.12.13", + }, + }, + code: http.StatusOK, + }, + { + name: "should not mutate URL if it has the same hostname as Zarf state", + admissionReq: createFluxHelmRepoAdmissionRequest(t, v1.Update, &flux.HelmRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-mutate-this", + }, + Spec: flux.HelmRepositorySpec{ + URL: "oci://127.0.0.1:31999/stefanprodan/charts", + Type: "oci", + }, + }), patch: []operations.PatchOperation{ operations.ReplacePatchOperation( "/spec/url", @@ -129,13 +175,13 @@ func TestFluxHelmMutationWebhook(t *testing.T) { code: http.StatusOK, }, { - name: "should be mutated with internal service registry", - admissionReq: createFluxHelmRepoAdmissionRequest(t, v1.Create, &flux.HelmRepository{ + name: "should not mutate URL if it has the same hostname as Zarf state internal repo", + admissionReq: createFluxHelmRepoAdmissionRequest(t, v1.Update, &flux.HelmRepository{ ObjectMeta: metav1.ObjectMeta{ - Name: "mutate-this", + Name: "no-mutate-this", }, Spec: flux.HelmRepositorySpec{ - URL: "oci://ghcr.io/stefanprodan/charts", + URL: "oci://10.11.12.13:5000/stefanprodan/charts", Type: "oci", }, }), diff --git a/src/internal/agent/hooks/flux-ocirepo.go b/src/internal/agent/hooks/flux-ocirepo.go index ffcd288c3e..3024705ddc 100644 --- a/src/internal/agent/hooks/flux-ocirepo.go +++ b/src/internal/agent/hooks/flux-ocirepo.go @@ -36,6 +36,14 @@ func NewOCIRepositoryMutationHook(ctx context.Context, cluster *cluster.Cluster) // mutateOCIRepo mutates the oci repository url to point to the repository URL defined in the ZarfState. func mutateOCIRepo(ctx context.Context, r *v1.AdmissionRequest, cluster *cluster.Cluster) (*operations.Result, error) { l := logger.From(ctx) + var ( + patches []operations.PatchOperation + isPatched bool + + isCreate = r.Operation == v1.Create + isUpdate = r.Operation == v1.Update + ) + src := &flux.OCIRepository{} if err := json.Unmarshal(r.Object.Raw, &src); err != nil { return nil, fmt.Errorf(lang.ErrUnmarshal, err) @@ -45,7 +53,7 @@ func mutateOCIRepo(ctx context.Context, r *v1.AdmissionRequest, cluster *cluster src.Spec.Reference = &flux.OCIRepositoryRef{} } - // If we have a semver we want to continue since we wil still have the upstream tag + // If we have a semver we want to continue since we will still have the upstream tag // but should warn that we can't guarantee there won't be collisions if src.Spec.Reference.SemVer != "" { l.Warn("Detected a semver OCI ref, continuing but will be unable to guarantee against collisions if multiple OCI artifacts with the same name are brought in from different registries", "ref", src.Spec.Reference.SemVer) @@ -67,37 +75,51 @@ func mutateOCIRepo(ctx context.Context, r *v1.AdmissionRequest, cluster *cluster "name", src.Name, "registry", registryAddress) - ref := src.Spec.URL - if src.Spec.Reference.Digest != "" { - ref = fmt.Sprintf("%s@%s", ref, src.Spec.Reference.Digest) - } else if src.Spec.Reference.Tag != "" { - ref = fmt.Sprintf("%s:%s", ref, src.Spec.Reference.Tag) - } - - patchedSrc, err := transform.ImageTransformHost(registryAddress, ref) - if err != nil { - return nil, fmt.Errorf("unable to transform the OCIRepo URL: %w", err) - } - - patchedRefInfo, err := transform.ParseImageRef(patchedSrc) - if err != nil { - return nil, fmt.Errorf("unable to parse the transformed OCIRepo URL: %w", err) - } + patchedURL := src.Spec.URL patchedRef := src.Spec.Reference - patchedURL := helpers.OCIURLPrefix + patchedRefInfo.Name + // Check if this is an update operation and the hostname is different from what we have in the zarfState + // NOTE: We mutate on updates IF AND ONLY IF the hostname in the request is different than the hostname in the zarfState + // NOTE: We are checking if the hostname is different before because we do not want to potentially mutate a URL that has already been mutated. + if isUpdate { + zarfStateAddress := helpers.OCIURLPrefix + registryAddress + isPatched, err = helpers.DoHostnamesMatch(zarfStateAddress, src.Spec.URL) + if err != nil { + return nil, fmt.Errorf(lang.AgentErrHostnameMatch, err) + } + } - if patchedRefInfo.Digest != "" { - patchedRef.Digest = patchedRefInfo.Digest - } else if patchedRefInfo.Tag != "" { - patchedRef.Tag = patchedRefInfo.Tag + // Mutate the oci repo URL if necessary + if isCreate || (isUpdate && !isPatched) { + if src.Spec.Reference.Digest != "" { + patchedURL = fmt.Sprintf("%s@%s", patchedURL, src.Spec.Reference.Digest) + } else if src.Spec.Reference.Tag != "" { + patchedURL = fmt.Sprintf("%s:%s", patchedURL, src.Spec.Reference.Tag) + } + + patchedSrc, err := transform.ImageTransformHost(registryAddress, patchedURL) + if err != nil { + return nil, fmt.Errorf("unable to transform the OCIRepo URL: %w", err) + } + + patchedRefInfo, err := transform.ParseImageRef(patchedSrc) + if err != nil { + return nil, fmt.Errorf("unable to parse the transformed OCIRepo URL: %w", err) + } + + patchedURL = helpers.OCIURLPrefix + patchedRefInfo.Name + + if patchedRefInfo.Digest != "" { + patchedRef.Digest = patchedRefInfo.Digest + } else if patchedRefInfo.Tag != "" { + patchedRef.Tag = patchedRefInfo.Tag + } } l.Debug("mutating the Flux OCIRepository URL to the Zarf URL", "original", src.Spec.URL, "mutated", patchedURL) - - patches := populateOCIRepoPatchOperations(patchedURL, zarfState.RegistryInfo.IsInternal(), patchedRef) - + patches = populateOCIRepoPatchOperations(patchedURL, zarfState.RegistryInfo.IsInternal(), patchedRef) patches = append(patches, getLabelPatch(src.Labels)) + return &operations.Result{ Allowed: true, PatchOps: patches, diff --git a/src/internal/agent/hooks/flux-ocirepo_test.go b/src/internal/agent/hooks/flux-ocirepo_test.go index d250b47deb..a7d5447e51 100644 --- a/src/internal/agent/hooks/flux-ocirepo_test.go +++ b/src/internal/agent/hooks/flux-ocirepo_test.go @@ -39,13 +39,23 @@ func TestFluxOCIMutationWebhook(t *testing.T) { tests := []admissionTest{ { - name: "should mutate even if agent patched", + name: "bad oci url", admissionReq: createFluxOCIRepoAdmissionRequest(t, v1.Update, &flux.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ - Name: "already-patched", - Labels: map[string]string{ - "zarf-agent": "patched", - }, + Name: "bad oci url", + }, + Spec: flux.OCIRepositorySpec{ + URL: "bad://ghcr.io/$", + }, + }), + errContains: "unable to transform the OCIRepo URL", + code: http.StatusInternalServerError, + }, + { + name: "should be mutated with no internal service registry", + admissionReq: createFluxOCIRepoAdmissionRequest(t, v1.Update, &flux.OCIRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mutate-this", }, Spec: flux.OCIRepositorySpec{ URL: "oci://ghcr.io/stefanprodan/manifests/podinfo", @@ -54,7 +64,7 @@ func TestFluxOCIMutationWebhook(t *testing.T) { }, }, }), - patch: []operations.PatchOperation{ + patch: []operations.PatchOperation{ operations.ReplacePatchOperation( "/spec/url", "oci://127.0.0.1:31999/stefanprodan/manifests/podinfo", @@ -74,47 +84,61 @@ func TestFluxOCIMutationWebhook(t *testing.T) { }, ), }, - code: http.StatusOK, + code: http.StatusOK, }, { - name: "bad oci url", + name: "test semver tag", admissionReq: createFluxOCIRepoAdmissionRequest(t, v1.Update, &flux.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ - Name: "bad oci url", + Name: "mutate-this", }, Spec: flux.OCIRepositorySpec{ - URL: "bad://ghcr.io/$", + URL: "oci://ghcr.io/stefanprodan/manifests/podinfo", + Reference: &flux.OCIRepositoryRef{ + SemVer: ">= 6.4.0", + }, }, }), - errContains: "unable to transform the OCIRepo URL", - code: http.StatusInternalServerError, + patch: []operations.PatchOperation{ + operations.ReplacePatchOperation( + "/spec/url", + "oci://127.0.0.1:31999/stefanprodan/manifests/podinfo", + ), + operations.AddPatchOperation( + "/spec/secretRef", + fluxmeta.LocalObjectReference{Name: config.ZarfImagePullSecretName}, + ), + operations.ReplacePatchOperation( + "/metadata/labels", + map[string]string{ + "zarf-agent": "patched", + }, + ), + }, + code: http.StatusOK, }, { - name: "should be mutated with no internal service registry", - admissionReq: createFluxOCIRepoAdmissionRequest(t, v1.Update, &flux.OCIRepository{ + name: "should be mutated with internal service registry", + admissionReq: createFluxOCIRepoAdmissionRequest(t, v1.Create, &flux.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ Name: "mutate-this", }, Spec: flux.OCIRepositorySpec{ - URL: "oci://ghcr.io/stefanprodan/manifests/podinfo", + URL: "oci://ghcr.io/stefanprodan/charts", Reference: &flux.OCIRepositoryRef{ - Tag: "6.4.0", + Digest: "sha256:6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b", }, }, }), patch: []operations.PatchOperation{ operations.ReplacePatchOperation( "/spec/url", - "oci://127.0.0.1:31999/stefanprodan/manifests/podinfo", + "oci://10.11.12.13:5000/stefanprodan/charts", ), operations.AddPatchOperation( "/spec/secretRef", fluxmeta.LocalObjectReference{Name: config.ZarfImagePullSecretName}, ), - operations.ReplacePatchOperation( - "/spec/ref/tag", - "6.4.0-zarf-2823281104", - ), operations.ReplacePatchOperation( "/metadata/labels", map[string]string{ @@ -122,18 +146,38 @@ func TestFluxOCIMutationWebhook(t *testing.T) { }, ), }, + svc: &corev1.Service{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "Service", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "zarf-docker-registry", + Namespace: "zarf", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + Ports: []corev1.ServicePort{ + { + NodePort: int32(31999), + Port: 5000, + }, + }, + ClusterIP: "10.11.12.13", + }, + }, code: http.StatusOK, }, { - name: "test semver tag", + name: "should not mutate URL if it has the same hostname as Zarf state", admissionReq: createFluxOCIRepoAdmissionRequest(t, v1.Update, &flux.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ Name: "mutate-this", }, Spec: flux.OCIRepositorySpec{ - URL: "oci://ghcr.io/stefanprodan/manifests/podinfo", + URL: "oci://127.0.0.1:31999/stefanprodan/manifests/podinfo", Reference: &flux.OCIRepositoryRef{ - SemVer: ">= 6.4.0", + Tag: "6.4.0-zarf-2823281104", }, }, }), @@ -146,6 +190,10 @@ func TestFluxOCIMutationWebhook(t *testing.T) { "/spec/secretRef", fluxmeta.LocalObjectReference{Name: config.ZarfImagePullSecretName}, ), + operations.ReplacePatchOperation( + "/spec/ref/tag", + "6.4.0-zarf-2823281104", + ), operations.ReplacePatchOperation( "/metadata/labels", map[string]string{ @@ -156,13 +204,13 @@ func TestFluxOCIMutationWebhook(t *testing.T) { code: http.StatusOK, }, { - name: "should be mutated with internal service registry", - admissionReq: createFluxOCIRepoAdmissionRequest(t, v1.Create, &flux.OCIRepository{ + name: "should not mutate URL if it has the same hostname as Zarf state internal repo", + admissionReq: createFluxOCIRepoAdmissionRequest(t, v1.Update, &flux.OCIRepository{ ObjectMeta: metav1.ObjectMeta{ Name: "mutate-this", }, Spec: flux.OCIRepositorySpec{ - URL: "oci://ghcr.io/stefanprodan/charts", + URL: "oci://10.11.12.13:5000/stefanprodan/charts", Reference: &flux.OCIRepositoryRef{ Digest: "sha256:6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b", },