argoproj / argo-cd

Declarative Continuous Deployment for Kubernetes
https://argo-cd.readthedocs.io
Apache License 2.0
16.74k stars 5.07k forks source link

Encoding ApplicationSet's generators to JSON have unexpected "template" field #18535

Open exklamationmark opened 1 month ago

exklamationmark commented 1 month ago

Checklist:

Describe the bug

When creating an ApplicationSet programmatically, various generators (ListGenerator, ClusterGenerator, etc) generates an extra template field when encoded to JSON. This happen even if we leave their Template field zero-valued.

To Reproduce

package main

import (
    "encoding/json"
    "fmt"

        // go mod download github.com/argoproj/argo-cd/v2@v2.11.3
    apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
    argocdappv1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
)

func main() {
    appSet := argocdappv1alpha1.ListGenerator{
        Elements: []apiextensionsv1.JSON{},
    }
    b, _ := json.Marshal(&appSet)
    fmt.Println(string(b))
}

Output:

go run main.go | jq -r '.'
{
  "elements": [],
  "template": {            <--- Extra field that wasn't expected
    "metadata": {},
    "spec": {
      "destination": {},
      "project": ""
    }
  }
}

This is not impacting the ApplicationSet's functionality, but could be confusing when users see something they didn't declared.

We are observing this as we have another program that creates ApplicationSet through API calls, instead of kubectl apply.

Expected behavior

Definition:

# https://github.com/argoproj/argo-cd/blob/v2.11.3/pkg/apis/application/v1alpha1/applicationset_types.go#L265
type ListGenerator struct {
    // +kubebuilder:validation:Optional
    Elements     []apiextensionsv1.JSON `json:"elements" protobuf:"bytes,1,name=elements"`
    Template     ApplicationSetTemplate `json:"template,omitempty" protobuf:"bytes,2,name=template"` // <--- omitempty
    ElementsYaml string                 `json:"elementsYaml,omitempty" protobuf:"bytes,3,opt,name=elementsYaml"`
}

At a glance, the field is defined with omitempty. So we expected it to not be included in the JSON. However, the Template field is actually a struct, so it is not considered a "zero-value" in encoding/json:

The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

Screenshots

N/A

Version

argocd: v2.11.3+3f344d5
  BuildDate: 2024-06-06T09:27:36Z
  GitCommit: 3f344d54a4e0bbbb4313e1c19cfe1e544b162598
  GitTreeState: clean
  GoVersion: go1.21.10
  Compiler: gc
  Platform: linux/amd64

Logs

N/A
exklamationmark commented 1 month ago

I believe this could be changed by making the Template field a pointer, like so:

type ListGenerator struct {
    // +kubebuilder:validation:Optional
    Elements     []apiextensionsv1.JSON `json:"elements" protobuf:"bytes,1,name=elements"`
    Template     *ApplicationSetTemplate `json:"template,omitempty" protobuf:"bytes,2,name=template"` // <--- changed to pointer
    ElementsYaml string                 `json:"elementsYaml,omitempty" protobuf:"bytes,3,opt,name=elementsYaml"`
}

I also don't mind making a PR for it

exklamationmark commented 1 month ago

A quick attempt at this:

$> go build ./pkg/apis/application/v1alpha1/
# github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1
pkg/apis/application/v1alpha1/zz_generated.deepcopy.go:1644:27: cannot use &out.Template (value of type **ApplicationSetTemplate) as *ApplicationSetTemplate value in argument to in.Template.DeepCopyInto
pkg/apis/application/v1alpha1/zz_generated.deepcopy.go:1837:27: cannot use &out.Template (value of type **ApplicationSetTemplate) as *ApplicationSetTemplate value in argument to in.Template.DeepCopyInto
pkg/apis/application/v1alpha1/zz_generated.deepcopy.go:1994:27: cannot use &out.Template (value of type **ApplicationSetTemplate) as *ApplicationSetTemplate value in argument to in.Template.DeepCopyInto
pkg/apis/application/v1alpha1/zz_generated.deepcopy.go:2466:27: cannot use &out.Template (value of type **ApplicationSetTemplate) as *ApplicationSetTemplate value in argument to in.Template.DeepCopyInto
pkg/apis/application/v1alpha1/zz_generated.deepcopy.go:2520:27: cannot use &out.Template (value of type **ApplicationSetTemplate) as *ApplicationSetTemplate value in argument to in.Template.DeepCopyInto
pkg/apis/application/v1alpha1/zz_generated.deepcopy.go:2549:27: cannot use &out.Template (value of type **ApplicationSetTemplate) as *ApplicationSetTemplate value in argument to in.Template.DeepCopyInto
pkg/apis/application/v1alpha1/zz_generated.deepcopy.go:2834:27: cannot use &out.Template (value of type **ApplicationSetTemplate) as *ApplicationSetTemplate value in argument to in.Template.DeepCopyInto
pkg/apis/application/v1alpha1/zz_generated.deepcopy.go:2966:27: cannot use &out.Template (value of type **ApplicationSetTemplate) as *ApplicationSetTemplate value in argument to in.Template.DeepCopyInto
pkg/apis/application/v1alpha1/zz_generated.deepcopy.go:3824:27: cannot use &out.Template (value of type **ApplicationSetTemplate) as *ApplicationSetTemplate value in argument to in.Template.DeepCopyInto

I might be missing some steps in the codegen, but the script doesn't seem to pickup the fact that the type has changed.