applicationset icon indicating copy to clipboard operation
applicationset copied to clipboard

Add a new untyped template field, to support passing JSON/YAML objects into ApplicationSet template fields

Open jgwest opened this issue 2 years ago • 12 comments

This issue is opened to discuss the passing of JSON/YAML objects into the template, rather than just individual string parameters.

This would be the ability, with the Git file generator, to pass a config.yaml like this (or equiv JSON):

cluster:
   helmParameters:
     - name: image.version
       value: 1.1.1

and then have an ApplicationSet with a template like this:

spec:
  # (...)
  template:
    spec:
      source:
         # (...)
        helm:
          parameters: {{ cluster.helmParameters}}

Such that when the Application is generated for this ApplicationSet, the YAML object (described by the config.json) will be inserted into the appropriate spot in the Helm parameters:

spec:
  # (...)
  template:
    spec:
      source:
        repoURL: https://github.com/argoproj/argocd-example-apps.git
        targetRevision: HEAD
        path: guestbook

        helm:
          parameters: 
            - name: image.version
              value: 1.1.1

However, (this is getting into implementation details) but I suspect it will require us to introduce a new field, as an alternative to template, like this:

// ApplicationSetSpec represents a class of application set state.
type ApplicationSetSpec struct {
	Generators []ApplicationSetGenerator `json:"generators"` // old
	Template   ApplicationSetTemplate    `json:"template"` // old
	UntypedTemplate apiextensionsv1.JSON `json:"templateUntyped"` // new hotness
	SyncPolicy *ApplicationSetSyncPolicy `json:"syncPolicy,omitempty"` // old
}

which would look like this:

spec:
  # (...)
  templateUntyped:
    spec:
      source:
       helm: # (...)

There's most definitely a better name for it than 'untyped template', but that's the idea :smile:. Ideally there would be a way to use the existing template field for this, but I don't see a way: I don't want to lose the typed nature of the existing template field, and yet we will need to break that structure with this feature.

jgwest avatar Sep 02 '21 15:09 jgwest

#323 just to relate them

rumstead avatar Sep 02 '21 18:09 rumstead

Idea is very good, can we also consider the situation where we would like to deploy same application in multiple namespaces in the given cluster.

parasappafluke avatar Sep 30 '21 05:09 parasappafluke

can we also add some functions that will allow us to have defaults for the template var?

mksha avatar Sep 30 '21 17:09 mksha

for example {{ myvar.myproperty | default "test" }}

mksha avatar Sep 30 '21 17:09 mksha

It will be very helpful, especially in the application set where we want to set the default chart version if not specified by user.

mksha avatar Sep 30 '21 17:09 mksha

we have a use case where we are deploying the same app in multiple namespaces, clusters in different regions. For each instance, we had to create a new confi.yaml. Can we add some feature that can avoid having breadcrumbs and just refer to a common config for diff instances of the same app?

mksha avatar Sep 30 '21 17:09 mksha

@chetan-rns:

Overview of work required

The high level overview of the work for this issue is this:

  • Add the templateUntyped field to ApplicationSet spec
    • This is like template, but represented in the CRD as a apiextensionsv1.JSON (and thus can theoretically contain any JSON value, but SHOULD still contain the same fields as template, BUT with support for JSON parameters)
  • Allow generators to return parameters that are JSON objects (rather than only supporting string parameters)
    • A generator can EITHER support only string parameters (this is the current behaviour for all generators), OR it can support BOTH string parameters and JSON parameters.
    • For this issue, we will only add JSON support to one generator.
  • Update the generator interface to allow generators to indicate that they support returning JSON objects.
    • A generator should return true for GeneratorSupportsJsonObjectParamValues() if it supports JSON objects, false otherwise.
  • Update the Git file generator to add support for generating JSON parameters
    • As part of this issue will we only be added support to the Git file generator.
    • We can use this generator as our test tube for this new functionality, making it our first generator to support both JSON/string values. Hopefully we can catch any initial issues here.
  • Update the logic that calls generators, to use this new feature
  • Add new tests

Update the generator interface, to allow generators to declare their support for returning json object parameters

Update the Generator interface:

  • Currently it is:
    • GenerateParams(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, applicationSetInfo *argoprojiov1alpha1.ApplicationSet) ([]map[string]string, error)
  • Add new functions:
    • New function which returns a []map[string]interface{}, rather than a []map[string]string:
      • GenerateJSONParams(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, applicationSetInfo *argoprojiov1alpha1.ApplicationSet) ([]map[string]interface{}, error)
    • New function which allows individiual generators to 'opt-in' to support for JSON parameters.
      • func GeneratorSupportsJsonObjectParamValues() bool
        • If a generator supports Json Object parameters, it should return true, otherwise false.

For a generator, the GenerateJSONParams function will only be called if the templateUntyped field is non-empty, and the template field is empty.

With the exception of the Git file generator, all existing generators should return false for GeneratorSupportsJsonObjectParamValues and GenerateJSONParams should return an error if called.

Add support for these new JSON param functions to the Git file generator

In the Git file generator:

  • The GenerateParams function will work as normal, and should require no changes.
  • In the GenerateJSONParams function, we will now parse JSON and/or YAML of files from Git, and return the full structure (rather than flattening that structure as we do for GenerateParams:

For example, the following JSON file:

{
  "aws_account": "123456",
  "asset_id": "11223344",
  "cluster": {
    "owner": "[email protected]",
    "name": "engineering-dev",
    "address": "https://1.2.3.4"
  },
}

would be converted into the following ApplicationSet parameters:

aws_account: "123456"
asset_id: "11223344"
cluster:   "cluster": {  "owner": "[email protected]", "name": "engineering-dev", "address": "https://1.2.3.4" }

(cluster is a single parameter containing the full JSON object)

Note this is different from the existing GenerateParams logic, which would create these (flattened) params:

aws_account: "123456"
asset_id: "11223344"
cluster.owner: "[email protected]"
cluster.name: "engineering-dev"
cluster.address: "https://1.2.3.4"

Update Transform in generator_spec_processor.go to support both generator modes

The line

params, err := g.GenerateParams(&requestedGenerator, appSet)

will now be

// If the ApplicationSet has the templateUntyped field defind, and the generator supports it, call the generator with json object support
if appSet.Spec.TemplateUntyped != nil && g.GeneratorSupportsJsonObjectParamValues() {
    params, err = g.GenerateJSONParams(/* */)    
} else {
    params, err = g.GenerateParams(/* */)
}

Such that GenerateJSONParams will now be called if the generator supports it, AND if the user requests it (by specifing a value in templateUntyped).

Update the Application CR, and Reconcile

Add a new field to type ApplicationSetSpec struct { }, templateUntyped of type apiextensionsv1.JSON

In Reconcile (or one of the validation methods), detect if both template and templateUntyped are non-empty; if so, this is an error: either one or the other should be specified, but not both.

If template field is specified in the ApplicationSet, then generateApplications function in applicationset_controller.go should work as normal. (this scenario doesn't change)

OTOH If templateUntyped field is specified, do the following:

In generateApplications, convert the templateUntyped to a string (JSON string).

Next, for each of the parameters generated (by the generator earlier in generateApplications function), convert them to JSON as follows:

  • parse as YAML; if succeeds, convert to JSON string
  • parse as JSON; if succeeds, no-op
  • otherwise, leave the parameter as a string

This should produce a []map[string]string, where the map's key is the parameter name, and the map's value is a valid JSON value (or just a normal string).

Create a new version of RenderTemplateParams

Create a new function, which is a new version of RenderTemplateParams (see util.go) with these parameters (notice the first parameter is different): func (r *Render) RenderTemplateParams(tmpl string, syncPolicy *argoprojiov1alpha1.ApplicationSetSyncPolicy, params map[string]string) (*argov1alpha1.Application, error) { /* ... */ } Where tmpl is the templateUntyped field converted to a JSON string, and params are the JSON string parameters from above.

This new function should otherwise work like the old function, but this new function does not need to tmplBytes, err := json.Marshal(tmpl), it can instead just use the tmpl string field.

The goal for this function is to render the JSON parameters (from params) in the JSON template (from tmpl), convert it back into an Argo CD Application object (to make sure the template is still a valid Application), then return it.

Call the new version of RenderTemplateParams in generateApplications

Finally, call the function in generateApplications:

	for _, a := range t {

		for _, p := range a.Params {
			app, err := r.Renderer.RenderTemplateParams(/* templateUntyped variable converted to a string */, applicationSetInfo.Spec.SyncPolicy, p)
			if err != nil {
				log.WithError(err).WithField("params", a.Params).WithField("generator", requestedGenerator).
					Error("error generating application from params")
				if firstError == nil {
					firstError = err
				}
				continue
			}
			res = append(res, *app)
		}
	}

Finally...

After all this, you should now:

  • Be able to specific an Application CR containing a templateUntyped field
  • The Git file generator should be able to process JSON/YAML files from Git repositories, and convert them into JSON objects (rather than strings)
  • You should be able to insert those JSON values into ApplicationSet, via the templateUntyped field.
  • The existing behaviour of ApplicationSet controller is unchanged, such that we don't break backwards compatibility. The new behavior defined here will only be 'activated' if templateUntyped field is defined.

re: implementation, if and when you get this far, post a PR and let me play with it, as I want to make sure it has the desired characteristics we are looking for. (I don't want y'all to spend some time writing tests, only for us to discover that a major change is needed which invalidates those tests!). So when the above is implemented, before writing any tests, let me know and I'll check out your branch.

jgwest avatar Nov 30 '21 04:11 jgwest

@jgwest Thanks for the detailed overview. I can work on this issue. Can you please assign it to me?

chetan-rns avatar Dec 02 '21 14:12 chetan-rns

@chetan-rns have you made any progress on this? I'm looking at a related PR dealing with functions in templates. Depending on the design choices, that work may overlap a bit with this.

crenshaw-dev avatar Feb 28 '22 17:02 crenshaw-dev

@crenshaw-dev I had started working on it a couple of weeks ago, but haven't reached far. So I'm okay if there will be any changes in the design choices. I will take a look at the PR.

chetan-rns avatar Mar 04 '22 16:03 chetan-rns

That sounds like a nice feature! With the suggested implementation, would it also be possible to have something like an all-parameters flag? It would allow to expand all parameters into the template, without needing to know a "root parameter". In the initial example the "root parameter" was cluster.helmParameters. I'd love, if it would be possible to remove that and directly injecting all generated parameters. When hjaving this possiblity, I could use the same config file in the generator for a helm Chart as well directly as a values-file for a manually deployed Chart.

An example config file could look like this:

name: image.version
value: 1.1.1
other:
  parameters:
    - foo: bar

and then have an ApplicationSet using a template like this:

spec:
  # (...)
  template:
    spec:
      source:
         # (...)
        helm:
          parameters: {{ ** }}

which should result in an Application like this:

spec:
  # (...)
  template:
    spec:
      source:
        repoURL: https://github.com/argoproj/argocd-example-apps.git
        targetRevision: HEAD
        path: guestbook

        helm:
          parameters: 
            - name: image.version
              value: 1.1.1
              other:
                 parameters:
                   - foo: bar

ffendt avatar Oct 14 '22 11:10 ffendt

@ffendt that should be possible with goTemplate: true (a 2.5 feature) and toJson from the sprig library.

crenshaw-dev avatar Oct 14 '22 14:10 crenshaw-dev