stefanprodan / timoni

Timoni is a package manager for Kubernetes, powered by CUE and inspired by Helm.
https://timoni.sh
Apache License 2.0
1.54k stars 68 forks source link

Nested packages in templates is difficult #273

Closed jmgilman closed 9 months ago

jmgilman commented 10 months ago

The issue is a bit hard to explain but it is mostly relevant to large packages which produce a lot of resources. Given the default Timoni structure:

Let's say that the templates folder has grown very large (>10 files at the root). The templates package is also starting to become overcrowded. So, we introduce a new subpackage:

Now, here we run into a problem:

  1. Each of the files contains structures that rely on #Config being passed in. This is normal and is used in all of the Timoni examples.
  2. The #Instance structure in config.cue also relies on these individual files, as they declare the resources being generated by the module. For example, #Instance might use subpkg.#Deployment.
  3. So, for example, config.cue imports timoni.sh/my_module/subpkg to access the resource structures and file3.cue imports timoni.sh/my_module/templates to access the #Config structure. This creates a cyclic dependency.

The only reason this appears to be a problem is because #Instance is included with #Config in the same package. If, for example, we moved #Instance to another package, it could have a dependency on #Config and all of the resources in the subpkg package without creating a cyclic dependency.

It's very possible I'm missing something obvious here, or there is another way to solve this issue, but my limited CUE knowledge isn't really showing me anything. From what I'm seeing, it's impossible to easily introduce additional packages containing resource structures without running into a cyclic dependency every time.

jmgilman commented 10 months ago
flowchart LR
    A[config.cue]-- #Instance depends on #Service--->C[subpkg/file3.cue #Service]
    C-- #Service depends on #Config-->A

This creates a cyclic import:

flowchart LR
    A[config.cue]-- import 'timoni.sh/mod/templates/subpkg' --->C[subpkg/file3.cue #Service]
    C-- import 'timoni.sh/mod/templates' --->A

If we, for example, move #Instance to the root-level, we resolve the circular dependency:

flowchart LR
    A[instance.cue]-- import 'timoni.sh/mod/templates/subpkg' --->C[subpkg/file3.cue #Service]
    A[instance.cue]-- import 'timoni.sh/mod/templates' --->B[config.cue #Config]
    C-- import 'timoni.sh/mod/templates' --->B
jmgilman commented 10 months ago

I managed to move #Config to a different package (config/config.cue), which resolved the cyclic import error. However, now the entire build fails with errors stating there are no concrete values anywhere. For example, I have:

import (
    conf "timoni.sh/jormungandr/config"
    node "timoni.sh/jormungandr/templates/node"
)

#Instance: {
    config: conf.#Config

    objects:
        {
            "nodeLeader1": #NodeLeader & {
                _config: config
            }
                     "nodeLeader2": node.#NodeLeader & {
                _config: config
            }
        }
}               

In the above case, #NodeLeader and node.#NodeLeader are identical (except they are in a different package). The nodeLeader1 entry works just fine. The nodeLeader2 entry complains there are no concrete values anywhere:

timoni.instance.objects.nodeLeader2.spec.template.spec.containers.0.image: cannot reference optional field: image:
    ./templates/node/node_leader.cue:40:32

It just seems like Timoni doesn't want to play nicely with sub-packages in general 🀣

b4nst commented 10 months ago

You probably have an issue with your structure, because Timoni does nothing on top of CUE wrt package management. See this example where I moved config to a subpackage, it works smoothly. Maybe you forgot to modify timoni.cue file?

dummy.tar.gz

stefanprodan commented 9 months ago

@b4nst To close this, we'll need an example with another subpackage besides config, let's say tests where the Job template it.

b4nst commented 9 months ago

Ok I got to the point where I can reproduce the "cannot reference optional field". However I'm still convinced it has nothing to do with timoni itself, because I can reproduce it with bare cue command. Strangely enough, the def of an object from root looks slightly different from the one inside the tests package:

// cue def -i . -t name=my-app -t namespace=default -t mv=devel -t kv=1.26.0 -e 'timoni.instance.objects.sa'

import (
    corev1 "k8s.io/api/core/v1"
    conf "timoni.sh/my-app/templates/config"
)

_#def
_#def: corev1.#ServiceAccount & {
    _config:    conf.#Config
    apiVersion: "v1"
    kind:       "ServiceAccount"
    metadata:   _config.metadata
} & {
    _config: conf.#Config & (conf.#Config & {
        message: "Hello World"
        image: {
            repository: "cgr.dev/chainguard/nginx"
            digest:     "sha256:3dd8fa303f77d7eb6ce541cb05009a5e8723bd7e3778b95131ab4a2d12fadb8f"
            tag:        "1.25.3"
        }
        test: image: {
            repository: "cgr.dev/chainguard/curl"
            digest:     ""
            tag:        "latest"
        }
    }) & {
        metadata: {
            name:      "my-app"  @tag(name)
            namespace: "default" @tag(namespace)
        }
        moduleVersion: "devel"  @tag(mv, var=moduleVersion)
        kubeVersion:   "1.26.0" @tag(kv, var=kubeVersion)
    }
}
cue def -i . -t name=my-app -t namespace=default -t mv=devel -t kv=1.26.0 -e 'timoni.instance.tests."test-svc"'
import (
    T "timoni.sh/my-app/templates/tests"
    conf "timoni.sh/my-app/templates/config"
)

_#def
_#def: T.#TestJob & {
    _config: conf.#Config & (conf.#Config & {
        message: "Hello World"
        image: {
            repository: "cgr.dev/chainguard/nginx"
            digest:     "sha256:3dd8fa303f77d7eb6ce541cb05009a5e8723bd7e3778b95131ab4a2d12fadb8f"
            tag:        "1.25.3"
        }
        test: image: {
            repository: "cgr.dev/chainguard/curl"
            digest:     ""
            tag:        "latest"
        }
    }) & {
        metadata: {
            name:      "my-app"  @tag(name)
            namespace: "default" @tag(namespace)
        }
        moduleVersion: "devel"  @tag(mv, var=moduleVersion)
        kubeVersion:   "1.26.0" @tag(kv, var=kubeVersion)
    }
}

And you can see the output of what timoni is complaining about by running:

cue eval -i . -t name=my-app -t namespace=default -t mv=devel -t kv=1.26.0 -e 'timoni.instance.tests."test-svc"'

I'm not quite sure if it comes from a CUE bug or a the file structure being wrong, but definitely not from timoni. I'll keep looking. Here's my module if you wanna give it a look @stefanprodan. I have a brown-bag live session with Paul and Marcel in 2 weeks, I'll show them the module if we fail finding the root cause.

my-app.tar.gz

b4nst commented 9 months ago

Well an easy fix is to replace the hidden _config field by a definition #config inside objects definition.

With my previous example, this goes something like

package tests

import (
    "encoding/yaml"
    "uuid"

    corev1 "k8s.io/api/core/v1"
    batchv1 "k8s.io/api/batch/v1"
    timoniv1 "timoni.sh/core/v1alpha1"
    conf "timoni.sh/my-app/templates/config"
)

#TestJob: batchv1.#Job & {
//      ↓ here the hidden field becomes a definition.
    #config:    conf.#Config 

    apiVersion: "batch/v1"
    kind:       "Job"
    metadata: timoniv1.#MetaComponent & {
        #Meta:      #config.metadata // <-- then replace everywhere
        #Component: "test"
    }
    metadata: annotations: timoniv1.Action.Force
    spec: batchv1.#JobSpec & {
        template: corev1.#PodTemplateSpec & {
            let _checksum = uuid.SHA1(uuid.ns.DNS, yaml.Marshal(#config))
            metadata: annotations: "timoni.sh/checksum": "\(_checksum)"
            spec: {
                containers: [{
                    name:            "curl"
                    image:           #config.test.image.reference
                    imagePullPolicy: #config.test.image.pullPolicy
                    command: [
                        "curl",
                        "-v",
                        "-m",
                        "5",
                        "\(#config.metadata.name):\(#config.service.port)",
                    ]
                }]
                restartPolicy: "Never"
                if #config.podSecurityContext != _|_ {
                    securityContext: #config.podSecurityContext
                }
                if #config.topologySpreadConstraints != _|_ {
                    topologySpreadConstraints: #config.topologySpreadConstraints
                }
                if #config.affinity != _|_ {
                    affinity: #config.affinity
                }
                if #config.tolerations != _|_ {
                    tolerations: #config.tolerations
                }
                if #config.imagePullSecrets != _|_ {
                    imagePullSecrets: #config.imagePullSecrets
                }
            }
        }
        backoffLimit: 1
    }
}

for templates/tests/job.cue and

    tests: {
        "test-svc": T.#TestJob & {#config: config} // <-- and don't forget here too
    }

inside templates/instance.cue.

I randomly tried ☝️ because I recall having such issue with hidden field across different packages. Might be a good idea to replace it everywhere inside blueprints and examples, what do you think @stefanprodan? I don't think using a def instead of an hidden field cause any issue. Now that I see it, I actually like it better. Makes it clear it's a configuration stuff, and not a temporary field you're using only locally.

Now wrt this particular issue I think we can close it, and maybe open something in the CUE repo itself (if not already). I'll try to come with a minimal reproducing structure if I don't find any related issue.

stefanprodan commented 9 months ago

@b4nst yes πŸ’― a definition makes way more sense than a hidden field. I will change in the blueprint and close this issue with that PR. Thanks so much for getting to the bottom of this.

b4nst commented 9 months ago

Would that be a good time to structure the templates package? Maybe something like

templates
β”œβ”€β”€ config.cue
β”œβ”€β”€ instance.cue
β”œβ”€β”€ objects
β”‚  β”œβ”€β”€ deploy.cue
β”‚  β”œβ”€β”€ sa.cue
β”‚  └── svc.cue
└── tests
   └── job.cue

Or maybe even keeping templates/ only for objects definitions (#Deployment, #Service, ...) and moving instance and config to their own package. End of the day everyone is free to come up with whatever structure they like, but having best practices to share could be a good idea. Given the flexibility (especially with the blueprint feature) I'm not strongly opinionated on any structure, but it might help new comers.

stefanprodan commented 9 months ago

@b4nst for that structure to work, you would still need a dedicated package for #Config to avoid the circular dependency in #Instance.

@jmgilman I have refactored the redis example module in #302, please take a look at let me know if it helps. Looks like this:

β”œβ”€β”€ templates
β”‚Β Β  β”œβ”€β”€ instance.cue
β”‚Β Β  β”œβ”€β”€ config
β”‚Β Β  β”‚Β Β  └── config.cue
β”‚Β Β  β”œβ”€β”€ master
β”‚Β Β  β”‚Β Β  β”œβ”€β”€ configmap.cue
β”‚Β Β  β”‚Β Β  β”œβ”€β”€ deployment.cue
β”‚Β Β  β”‚Β Β  β”œβ”€β”€ pvc.cue
β”‚Β Β  β”‚Β Β  β”œβ”€β”€ service.cue
β”‚Β Β  β”‚Β Β  β”œβ”€β”€ serviceaccount.cue
β”‚Β Β  β”‚Β Β  └── test.job.cue
β”‚Β Β  └── replica
β”‚Β Β      β”œβ”€β”€ deployment.cue
β”‚Β Β      └── service.cue
β”œβ”€β”€ timoni.cue
β”œβ”€β”€ timoni.ignore
└── values.cue
jmgilman commented 9 months ago

Thanks for taking the time to look into this. If I understand this correctly, the solution was changing the _config to #config due to an unknown bug in Cue?

As for the structure, I still wonder if it's not better to have instance.cue be moved out of templates. The only thing that imports it is timoni.cue, which would now have direct access to it as they would be in the same scope.

b4nst commented 9 months ago

Maybe

β”œβ”€β”€ templates
β”‚   β”œβ”€β”€ config.cue
β”‚   β”œβ”€β”€ master
β”‚   β”‚   β”œβ”€β”€ configmap.cue
β”‚   β”‚   β”œβ”€β”€ deployment.cue
β”‚   β”‚   β”œβ”€β”€ pvc.cue
β”‚   β”‚   β”œβ”€β”€ service.cue
β”‚   β”‚   β”œβ”€β”€ serviceaccount.cue
β”‚   β”‚   └── test.job.cue
β”‚   └── replica
β”‚       β”œβ”€β”€ deployment.cue
β”‚       └── service.cue
β”œβ”€β”€ instance.cue
β”œβ”€β”€ timoni.cue
β”œβ”€β”€ timoni.ignore
└── values.cue
stefanprodan commented 9 months ago

I'm not sure if instance is Ok to be in root. From a testability and encapsulation POV is better to have it in templates, so that package has a well defined input (config values) and output (map of objects). Another aspect is the cognitive load when making changes, if you want to add a new object, the number of files you need to edit, are now only 2, you add the template then you edit config.cue to add the options and the object to the output.

salotz commented 6 months ago

Did you try an alias on the hidden field? I had similar issues even with definitions. Discussing here: https://github.com/stefanprodan/timoni/discussions/372

Not sure its a bug in Cue, but rather a fairly obtuse by product of the evaluation semantics: https://cuelang.org/docs/concept/alias-and-reference-scopes/