stefanprodan / timoni

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

Wrong cue type: {} when generating vendor crd #278

Closed Aoao54 closed 8 months ago

Aoao54 commented 8 months ago

Hi team, When generating vendor of crd.yaml resources with the x-kubernetes-preserve-unknown-fields like below:

spec:
  description: Specification of the desired behavior of the pod.
  type: object
  x-kubernetes-preserve-unknown-fields: true

the output is

// Specification of the desired behavior of the pod.
spec?: {}

But in the current version of cue, the {} means the empty struct instead of {...} arbitrary struct. So the correct output value is

// Specification of the desired behavior of the pod.
spec?: {...}

Then you can add anything you want of it. cc @yujunz

stefanprodan commented 8 months ago

Using latest Timoni release

timoni version 
api: timoni.sh/v1alpha1
client: 0.17.0
cue: 0.6.0

For a field like this:

              values:
                description: Values holds the values for this Helm release.
                x-kubernetes-preserve-unknown-fields: true

This is how it generates it:

    // Values holds the values for this Helm release.
    values?: _

Refs:

yujunz commented 8 months ago

@stefanprodan the issue is on type: object

Here are the steps to reproduce.

timoni mod vendor crds -f https://raw.githubusercontent.com/alexandrevilain/temporal-operator/main/config/crd/bases/temporal.io_temporalclusters.yaml
7:01PM INF schemas vendored: temporal.io/temporalcluster/v1beta1

The same schema type: object are imported differently

curl -s https://raw.githubusercontent.com/alexandrevilain/temporal-operator/main/config/crd/bases/temporal.io_temporalclusters.yaml | grep -1 "Specification of the desired behavior of the pod."
                                    spec:
                                      description: Specification of the desired behavior of the pod.
                                      type: object
--
                                        spec:
                                          description: Specification of the desired behavior of the pod.
                                          type: object
...

to

cat cue.mod/gen/temporal.io/temporalcluster/v1beta1/types_gen.cue | grep -2 'Specification of the desired behavior of the pod.'
                        }

                        // Specification of the desired behavior of the pod.
                        spec?: {
                            ...
--
                            }

                            // Specification of the desired behavior of the pod.
                            spec?: {}
                        }
...

Only the first one is imported as expected open struct. The rests are all closed struct but they are not supposed to be empty according to https://alexandrevilain.github.io/temporal-operator/api/v1beta1/

Kubernetes core/v1.PodSpec (Optional) Specification of the desired behavior of the pod

With spec?: {}, it becomes invalid with content

// Specification of the desired behavior of the Temporal cluster.
#TemporalClusterSpec: {
                            // Specification of the desired behavior of the pod.
                            spec?: {}
}

cluster: #TemporalClusterSpec & {spec: containers: []}

evals error

cluster.spec.containers: field not allowed:
    -:4:15
    -:7:10
    -:7:40
timoni version
api: timoni.sh/v1alpha1
client: 0.17.0
cue: 0.6.0
stefanprodan commented 8 months ago

There is a test for this here https://github.com/stefanprodan/timoni/blob/9c72f5d54419f21d922111802f697a5b64a708ab/internal/engine/importer_test.go#L194 but we do enforce closeness for the top spec prop. Why would you have a CRD that allows anything in their specification...

Aoao54 commented 8 months ago

oh.. We use this specification in overrides. As a option if user want to applying overrides to resources created by the operator. Here is a user case https://alexandrevilain.github.io/temporal-operator/features/overrides/

stefanprodan commented 8 months ago

Ah this is not the top level spec, something else is going on...