Repository navigation
Conversation
kubeconform validated every custom resource against the datreeio CRDs catalog, which is generated from whichever CRD version it was last refreshed with. A chart that deploys a newer operator then fails on a field its own CRD allows: postgres-operator 1.15.1 sets aws_or_gcp.enable_ebs_gp3_migration on its OperatorConfiguration, and the catalog schema for acid.zalan.do/v1 declares additionalProperties: false over a property set that predates it. eval-charts.sh now extracts the openAPIV3Schema of every CRD version a chart ships - its own crds/ and those of its dependencies, unpacked from the archives helm dependency build leaves behind - and gives kubeconform that schema location ahead of the catalog, which still covers the kinds a chart defines no CRD for. Only a crds/ directory that sits directly in a chart is read, the way Helm reads them: opentelemetry-operator keeps Helm templates in conf/crds, which are not parseable YAML.
|
Please apply exactly one of the following labels to the PR: 'patch', 'minor', 'major'. |
vramperez
left a comment
vramperez
left a comment
There was a problem hiding this comment.
Nice approach overall; a few points on strictness and error handling of the extracted CRD schemas.
| mkdir -p "$schema_dir/$group" | ||
| "$YQ" e -N -o=json \ | ||
| "select(documentIndex == $doc) | .spec.versions[$index].schema.openAPIV3Schema" \ | ||
| "$crd" > "$schema_dir/$group/${kind,,}_$version.json" || return 1 |
There was a problem hiding this comment.
kubeconform's -strict only switches the schema suffix and rejects duplicate keys; it doesn't add additionalProperties: false, which the catalog schemas carry but raw CRD schemas don't. So a typo in a CR that used to fail against the catalog (e.g. spec.numberOfInstance on a zalando postgresql) now passes. Could we set additionalProperties: false on every object with properties (skipping x-kubernetes-preserve-unknown-fields) when writing the schema, like the catalog's openapi2jsonschema does?
| "$YQ" e -N -o=json \ | ||
| "select(documentIndex == $doc) | .spec.versions[$index].schema.openAPIV3Schema" \ | ||
| "$crd" > "$schema_dir/$group/${kind,,}_$version.json" || return 1 | ||
| done < <("$YQ" e -N "$versions_query" "$crd") |
There was a problem hiding this comment.
The exit status of the yq inside < <(...) is lost, so an unparseable CRD file (or a missing yq) leaves no schemas and silently falls back to the catalog. Capture the query output first and return 1 on failure, and maybe check command -v "$YQ" up front.
| ($crd + [.key, .value.name]) | join(" ")' | ||
|
|
||
| mkdir -p "$unpack_dir" | ||
| for archive in "$chart"/charts/*.tgz; do |
There was a problem hiding this comment.
Only the top-level charts/*.tgz are unpacked, but a packaged dependency carries its own subcharts as archives in its charts/, and Helm installs their CRDs too. An umbrella whose dependency bundles e.g. postgres-operator would still hit the catalog issue; unpack the archives recursively.
| [ "$status" -eq 1 ] | ||
| } | ||
|
|
||
| @test "a property the chart's CRD adds is accepted, where the catalog alone would reject it" { |
There was a problem hiding this comment.
This test passes even without the colour property (the raw CRD schema allows unknown fields, and example.test isn't in the catalog), so it proves neither precedence over the catalog nor strictness. Worth adding a case where an unknown property must fail once the schema is made strict.
vramperez
left a comment
vramperez
left a comment
There was a problem hiding this comment.
Checked against decentralized-iam#43, whose chart-lint fails since the CRDs catalog moved acid.zalan.do to the postgres-operator 2.0.3 schema: with this script and yq 4.54.1 the chart validates (34 of 34 resources, the OperatorConfiguration included), so it fixes the failure, also for a CRD nested two charts down (decentralized-iam → vc-authentication → postgres-operator). Two comments on how it fails when the CRDs cannot be read.
| "$YQ" e -N -o=json \ | ||
| "select(documentIndex == $doc) | .spec.versions[$index].schema.openAPIV3Schema" \ | ||
| "$crd" > "$schema_dir/$group/${kind,,}_$version.json" || return 1 | ||
| done < <("$YQ" e -N "$versions_query" "$crd") |
There was a problem hiding this comment.
If this yq query fails, the error is lost: the exit status of a process substitution is not checked, so write_crd_schemas returns 0, no schema is written, and kubeconform falls back to the catalog - with the very failure this PR fixes and no hint why.
That happens with yq 4.35.2 (the Alpine package), where the query fails with Error: bad expression, please check expression syntax; with 4.54.1 it works. Reading the output into a variable first would make it fail where the problem is, e.g.:
local entries
entries=$("$YQ" e -N "$versions_query" "$crd") || return 1
while read -r doc group kind index version; do
...
done <<< "$entries"| # CHARTS_DIR directory that holds one directory per chart (default: charts) | ||
| # HELM helm binary (default: helm) | ||
| # KUBECONFORM kubeconform binary (default: kubeconform) | ||
| # YQ yq binary (mikefarah, v4; default: yq) |
There was a problem hiding this comment.
The script now depends on the yq version: the query above needs a newer one than 4.35.2. The GitHub runners have a recent one, which is why CI passes, but a runner image change or a local run with an older yq would silently go back to the catalog.
Worth stating the minimum version here, and installing a pinned yq in chart-lint.yml next to Helm and kubeconform, so that the result does not depend on the runner image.
kubeconform validated every custom resource against the datreeio CRDs catalog, which is generated from whichever CRD version it was last refreshed with. A chart that deploys a newer operator then fails on a field its own CRD allows: postgres-operator 1.15.1 sets aws_or_gcp.enable_ebs_gp3_migration on its OperatorConfiguration, and the catalog schema for acid.zalan.do/v1 declares additionalProperties: false over a property set that predates it.
eval-charts.sh now extracts the openAPIV3Schema of every CRD version a chart ships - its own crds/ and those of its dependencies, unpacked from the archives helm dependency build leaves behind - and gives kubeconform that schema location ahead of the catalog, which still covers the kinds a chart defines no CRD for.
Only a crds/ directory that sits directly in a chart is read, the way Helm reads them: opentelemetry-operator keeps Helm templates in conf/crds, which are not parseable YAML.