From 27f5d1d6807bead317e9da9fe31020195020a264 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Thu, 30 Nov 2023 16:38:36 -0500 Subject: [PATCH] small changes Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- .../controllers/applicationset_controller.go | 6 +- .../{utils => controllers}/templatePatch.go | 16 +- .../controllers/templatePatch_test.go | 249 ++++++++++++++++++ applicationset/utils/templatePatch_test.go | 97 ------- .../applicationset/Template.md | 43 +-- 5 files changed, 289 insertions(+), 122 deletions(-) rename applicationset/{utils => controllers}/templatePatch.go (65%) create mode 100644 applicationset/controllers/templatePatch_test.go delete mode 100644 applicationset/utils/templatePatch_test.go diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go index 541c49279b585..527a4ae1e3883 100644 --- a/applicationset/controllers/applicationset_controller.go +++ b/applicationset/controllers/applicationset_controller.go @@ -568,10 +568,10 @@ func (r *ApplicationSetReconciler) applyTemplatePatch(app *argov1alpha1.Applicat replacedTemplate, err := r.Renderer.Replace(*applicationSetInfo.Spec.TemplatePatch, params, applicationSetInfo.Spec.GoTemplate, applicationSetInfo.Spec.GoTemplateOptions) if err != nil { - return nil, err + return nil, fmt.Errorf("error replacing values in templatePatch: %w", err) } - return utils.ApplyPatchTemplate(app, replacedTemplate) + return applyTemplatePatch(app, replacedTemplate) } func ignoreNotAllowedNamespaces(namespaces []string) predicate.Predicate { @@ -648,6 +648,8 @@ func (r *ApplicationSetReconciler) createOrUpdateInCluster(ctx context.Context, var firstError error // Creates or updates the application in appList for _, generatedApp := range desiredApplications { + // The app's namespace must be the same as the AppSet's namespace to preserve the appsets-in-any-namespace + // security boundary. generatedApp.Namespace = applicationSet.Namespace appLog := logCtx.WithFields(log.Fields{"app": generatedApp.QualifiedName()}) diff --git a/applicationset/utils/templatePatch.go b/applicationset/controllers/templatePatch.go similarity index 65% rename from applicationset/utils/templatePatch.go rename to applicationset/controllers/templatePatch.go index 36ed522d1a35a..f8efd9f376996 100644 --- a/applicationset/utils/templatePatch.go +++ b/applicationset/controllers/templatePatch.go @@ -1,4 +1,4 @@ -package utils +package controllers import ( "encoding/json" @@ -6,17 +6,18 @@ import ( "k8s.io/apimachinery/pkg/util/strategicpatch" + "github.com/argoproj/argo-cd/v2/applicationset/utils" appv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" ) -func ApplyPatchTemplate(app *appv1.Application, templatePatch string) (*appv1.Application, error) { +func applyTemplatePatch(app *appv1.Application, templatePatch string) (*appv1.Application, error) { appString, err := json.Marshal(app) if err != nil { return nil, fmt.Errorf("error while marhsalling Application %w", err) } - convertedTemplatePatch, err := ConvertYAMLToJSON(templatePatch) + convertedTemplatePatch, err := utils.ConvertYAMLToJSON(templatePatch) if err != nil { return nil, fmt.Errorf("error while converting template to json %q: %w", convertedTemplatePatch, err) @@ -26,7 +27,7 @@ func ApplyPatchTemplate(app *appv1.Application, templatePatch string) (*appv1.Ap return nil, fmt.Errorf("invalid templatePatch %q: %w", convertedTemplatePatch, err) } - data, err := strategicpatch.StrategicMergePatch([]byte(appString), []byte(convertedTemplatePatch), appv1.Application{}) + data, err := strategicpatch.StrategicMergePatch(appString, []byte(convertedTemplatePatch), appv1.Application{}) if err != nil { return nil, fmt.Errorf("error while applying templatePatch template to json %q: %w", convertedTemplatePatch, err) @@ -35,8 +36,11 @@ func ApplyPatchTemplate(app *appv1.Application, templatePatch string) (*appv1.Ap finalApp := appv1.Application{} err = json.Unmarshal(data, &finalApp) if err != nil { - return nil, fmt.Errorf("error while unmarhsalling patched application %w", err) + return nil, fmt.Errorf("error while unmarhsalling patched application: %w", err) } - return &finalApp, err + // Prevent changes to the `project` field. This helps prevent malicious template patches + finalApp.Spec.Project = app.Spec.Project + + return &finalApp, nil } diff --git a/applicationset/controllers/templatePatch_test.go b/applicationset/controllers/templatePatch_test.go new file mode 100644 index 0000000000000..c1a794077c8ee --- /dev/null +++ b/applicationset/controllers/templatePatch_test.go @@ -0,0 +1,249 @@ +package controllers + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + appv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" +) + +func Test_ApplyTemplatePatch(t *testing.T) { + testCases := []struct { + name string + appTemplate *appv1.Application + templatePatch string + expectedApp *appv1.Application + }{ + { + name: "patch with JSON", + appTemplate: &appv1.Application{ + TypeMeta: metav1.TypeMeta{ + Kind: "Application", + APIVersion: "argoproj.io/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster-guestbook", + Namespace: "namespace", + Finalizers: []string{"resources-finalizer.argocd.argoproj.io"}, + }, + Spec: appv1.ApplicationSpec{ + Project: "default", + Source: &appv1.ApplicationSource{ + RepoURL: "https://github.com/argoproj/argocd-example-apps.git", + TargetRevision: "HEAD", + Path: "guestbook", + }, + Destination: appv1.ApplicationDestination{ + Server: "https://kubernetes.default.svc", + Namespace: "guestbook", + }, + }, + }, + templatePatch: `{ + "metadata": { + "annotations": { + "annotation-some-key": "annotation-some-value" + } + }, + "spec": { + "source": { + "helm": { + "valueFiles": [ + "values.test.yaml", + "values.big.yaml" + ] + } + }, + "syncPolicy": { + "automated": { + "prune": true + } + } + } + }`, + expectedApp: &appv1.Application{ + TypeMeta: metav1.TypeMeta{ + Kind: "Application", + APIVersion: "argoproj.io/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster-guestbook", + Namespace: "namespace", + Finalizers: []string{"resources-finalizer.argocd.argoproj.io"}, + Annotations: map[string]string{ + "annotation-some-key": "annotation-some-value", + }, + }, + Spec: appv1.ApplicationSpec{ + Project: "default", + Source: &appv1.ApplicationSource{ + RepoURL: "https://github.com/argoproj/argocd-example-apps.git", + TargetRevision: "HEAD", + Path: "guestbook", + Helm: &appv1.ApplicationSourceHelm{ + ValueFiles: []string{ + "values.test.yaml", + "values.big.yaml", + }, + }, + }, + Destination: appv1.ApplicationDestination{ + Server: "https://kubernetes.default.svc", + Namespace: "guestbook", + }, + SyncPolicy: &appv1.SyncPolicy{ + Automated: &appv1.SyncPolicyAutomated{ + Prune: true, + }, + }, + }, + }, + }, + { + name: "patch with YAML", + appTemplate: &appv1.Application{ + TypeMeta: metav1.TypeMeta{ + Kind: "Application", + APIVersion: "argoproj.io/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster-guestbook", + Namespace: "namespace", + Finalizers: []string{"resources-finalizer.argocd.argoproj.io"}, + }, + Spec: appv1.ApplicationSpec{ + Project: "default", + Source: &appv1.ApplicationSource{ + RepoURL: "https://github.com/argoproj/argocd-example-apps.git", + TargetRevision: "HEAD", + Path: "guestbook", + }, + Destination: appv1.ApplicationDestination{ + Server: "https://kubernetes.default.svc", + Namespace: "guestbook", + }, + }, + }, + templatePatch: ` +metadata: + annotations: + annotation-some-key: annotation-some-value +spec: + source: + helm: + valueFiles: + - values.test.yaml + - values.big.yaml + syncPolicy: + automated: + prune: true`, + expectedApp: &appv1.Application{ + TypeMeta: metav1.TypeMeta{ + Kind: "Application", + APIVersion: "argoproj.io/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster-guestbook", + Namespace: "namespace", + Finalizers: []string{"resources-finalizer.argocd.argoproj.io"}, + Annotations: map[string]string{ + "annotation-some-key": "annotation-some-value", + }, + }, + Spec: appv1.ApplicationSpec{ + Project: "default", + Source: &appv1.ApplicationSource{ + RepoURL: "https://github.com/argoproj/argocd-example-apps.git", + TargetRevision: "HEAD", + Path: "guestbook", + Helm: &appv1.ApplicationSourceHelm{ + ValueFiles: []string{ + "values.test.yaml", + "values.big.yaml", + }, + }, + }, + Destination: appv1.ApplicationDestination{ + Server: "https://kubernetes.default.svc", + Namespace: "guestbook", + }, + SyncPolicy: &appv1.SyncPolicy{ + Automated: &appv1.SyncPolicyAutomated{ + Prune: true, + }, + }, + }, + }, + }, + { + name: "project field isn't overwritten", + appTemplate: &appv1.Application{ + TypeMeta: metav1.TypeMeta{ + Kind: "Application", + APIVersion: "argoproj.io/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster-guestbook", + Namespace: "namespace", + }, + Spec: appv1.ApplicationSpec{ + Project: "default", + Source: &appv1.ApplicationSource{ + RepoURL: "https://github.com/argoproj/argocd-example-apps.git", + TargetRevision: "HEAD", + Path: "guestbook", + }, + Destination: appv1.ApplicationDestination{ + Server: "https://kubernetes.default.svc", + Namespace: "guestbook", + }, + }, + }, + templatePatch: ` +spec: + project: my-project`, + expectedApp: &appv1.Application{ + TypeMeta: metav1.TypeMeta{ + Kind: "Application", + APIVersion: "argoproj.io/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster-guestbook", + Namespace: "namespace", + }, + Spec: appv1.ApplicationSpec{ + Project: "default", + Source: &appv1.ApplicationSource{ + RepoURL: "https://github.com/argoproj/argocd-example-apps.git", + TargetRevision: "HEAD", + Path: "guestbook", + }, + Destination: appv1.ApplicationDestination{ + Server: "https://kubernetes.default.svc", + Namespace: "guestbook", + }, + }, + }, + }, + } + + for _, tc := range testCases { + tcc := tc + t.Run(tcc.name, func(t *testing.T) { + result, err := applyTemplatePatch(tcc.appTemplate, tcc.templatePatch) + require.NoError(t, err) + assert.Equal(t, *tcc.expectedApp, *result) + }) + } +} + +func TestError(t *testing.T) { + app := &appv1.Application{} + + result, err := applyTemplatePatch(app, "hello world") + require.Error(t, err) + require.Nil(t, result) +} diff --git a/applicationset/utils/templatePatch_test.go b/applicationset/utils/templatePatch_test.go deleted file mode 100644 index 249b6f364b0f7..0000000000000 --- a/applicationset/utils/templatePatch_test.go +++ /dev/null @@ -1,97 +0,0 @@ -package utils - -import ( - "testing" - - appv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -func validate(t *testing.T, patch string) { - app := &appv1.Application{ - TypeMeta: metav1.TypeMeta{ - Kind: "Application", - APIVersion: "argoproj.io/v1alpha1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "my-cluster-guestbook", - Namespace: "namespace", - Finalizers: []string{"resources-finalizer.argocd.argoproj.io"}, - }, - Spec: appv1.ApplicationSpec{ - Project: "default", - Source: &appv1.ApplicationSource{ - RepoURL: "https://github.com/argoproj/argocd-example-apps.git", - TargetRevision: "HEAD", - Path: "guestbook", - }, - Destination: appv1.ApplicationDestination{ - Server: "https://kubernetes.default.svc", - Namespace: "guestbook", - }, - }, - } - - result, err := ApplyPatchTemplate(app, patch) - require.NoError(t, err) - require.Contains(t, result.ObjectMeta.Annotations, "annotation-some-key") - assert.Equal(t, result.ObjectMeta.Annotations["annotation-some-key"], "annotation-some-value") - - assert.Equal(t, result.Spec.SyncPolicy.Automated.Prune, true) - require.Contains(t, result.Spec.Source.Helm.ValueFiles, "values.test.yaml") - require.Contains(t, result.Spec.Source.Helm.ValueFiles, "values.big.yaml") -} - -func TestWithJson(t *testing.T) { - - validate(t, `{ - "metadata": { - "annotations": { - "annotation-some-key": "annotation-some-value" - } - }, - "spec": { - "source": { - "helm": { - "valueFiles": [ - "values.test.yaml", - "values.big.yaml" - ] - } - }, - "syncPolicy": { - "automated": { - "prune": true - } - } - } - }`) - -} - -func TestWithYaml(t *testing.T) { - - validate(t, ` -metadata: - annotations: - annotation-some-key: annotation-some-value -spec: - source: - helm: - valueFiles: - - values.test.yaml - - values.big.yaml - syncPolicy: - automated: - prune: true`) -} - -func TestError(t *testing.T) { - app := &appv1.Application{} - - result, err := ApplyPatchTemplate(app, "hello world") - require.Error(t, err) - require.Nil(t, result) -} diff --git a/docs/operator-manual/applicationset/Template.md b/docs/operator-manual/applicationset/Template.md index 4fd1a2233a977..76ff9132802d5 100644 --- a/docs/operator-manual/applicationset/Template.md +++ b/docs/operator-manual/applicationset/Template.md @@ -109,9 +109,9 @@ spec: In this example, the ApplicationSet controller will generate an `Application` resource using the `path` generated by the List generator, rather than the `path` value defined in `.spec.template`. -## Patch Template +## Template Patch -Templating is only available on string type. However some uses cases may require to apply templating on other types. +Templating is only available on string type. However, some uses cases may require to apply templating on other types. Example: @@ -119,7 +119,7 @@ Example: - Switch prune boolean to true - Add multiple helm value files -ArgoCD has a `templatePatch` feature to allow advanced templating. It supports both json and yaml +Argo CD has a `templatePatch` feature to allow advanced templating. It supports both json and yaml. ```yaml @@ -139,19 +139,6 @@ spec: valueFiles: - values.large.yaml - values.debug.yaml - templatePatch: | - spec: - source: - helm: - valueFiles: - {{- range $valueFile := .valueFiles }} - - {{ $valueFile }} - {{- end }} - {{- if .autoSync }} - syncPolicy: - automated: - prune: {{ .prune }} - {{- end }} template: metadata: name: '{{.cluster}}-deployment' @@ -164,4 +151,26 @@ spec: destination: server: '{{.url}}' namespace: guestbook -``` \ No newline at end of file + templatePatch: | + spec: + source: + helm: + valueFiles: + {{- range $valueFile := .valueFiles }} + - {{ $valueFile | toJson }} + {{- end }} + {{- if .autoSync }} + syncPolicy: + automated: + prune: {{ .prune | toJson }} + {{- end }} +``` + +!!! important + The `templatePatch` can apply arbitrary changes to the template. If parameters include untrustworthy user input, it + may be possible to inject malicious changes into the template. It is recommended to use `templatePatch` only with + trusted input or to carefully escape the input before using it in the template. Piping input to `toJson` should help + prevent, for example, a user from successfully injecting a string with newlines. + + The `spec.project` field is not supported in `templatePatch`. If you need to change the project, you can use the + `spec.project` field in the `template` field.