feat(chart): make CRD installation configurable - #346
Conversation
3ce1c8c to
1038d55
Compare
The chart shipped its CRDs in `crds/`, which Helm never templates, so the documented `installCRDs` value had no effect and there was no way to opt out of installing the shared `wgpolicyk8s.io` CRDs. That made Netchecks hard to run alongside anything else that produces PolicyReports, and risked silently overwriting a newer schema with the copy vendored here. Move the CRDs into `templates/` and gate them: crds.install master switch (honours legacy `installCRDs`) crds.keep annotate with `helm.sh/resource-policy: keep` crds.groups.netchecks NetworkAssertion (`netchecks.io`) crds.groups.wgpolicyk8s PolicyReport/ClusterPolicyReport (`wgpolicyk8s.io`) `crds.keep` defaults to true so that uninstalling the release leaves the CRDs and their resources in place, matching the previous `crds/` behaviour. Also drop a duplicate `policyreports.wgpolicyk8s.io` definition that was carried in `crds/clusterpolicyreports.yaml` alongside the identical one in `crds/policyreports.yaml`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1038d55 to
026a8dc
Compare
hardbyte
left a comment
There was a problem hiding this comment.
Thanks! Looks good to me. Mind seeing why it failed ci?
The operator writes reports to `wgpolicyk8s.io/v1beta1` specifically. The CRDs shipped by this chart serve v1alpha1, v1alpha2 and v1beta1, but other owners of the same API group may serve fewer — Kyverno's chart serves only v1alpha2. Handing ownership to such a component leaves the operator unable to write reports, and NetworkAssertions fail to reconcile with `404 page not found`. Document the constraint next to the toggle so it is not read as a free choice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed 688f422 after finding that the toggle this PR adds is not a free choice. The operator writes to The README and the values comment now state the constraint and give the command to check served versions before disabling. Opened #347 for the underlying fix — runtime version discovery in the operator, after which the toggle really would be unconstrained. |
https://github.com/hardbyte/netchecks/actions/runs/32362264967/job/96409014469?pr=346#step:7:499 #27 CANCELED
------
> exporting to image:
------
ERROR: failed to build: failed to solve: failed to push ghcr.io/hardbyte/netchecks:pr-346: denied: installation not allowed to Write organization package |
Fixes #345.
What
The chart shipped its CRDs in
crds/, which Helm never templates. Two consequences:installCRDsvalue invalues.yamlwas never referenced by anything — it had no effect.wgpolicyk8s.ioCRDs, so Netchecks conflicts with any other component that installs them (Kyverno, the Trivy PolicyReport adapters, Kubescape). Since the copy vendored here is generated with controller-gen v0.9.0 and Kyverno's with v0.20.0, continuous reconciliation can silently replace the newer schema with the older one.--skip-crdswas not a workaround, because it also dropsnetworkassertions.yaml, without which the operator cannot run.The toggle is not a free choice, and the docs say so. The operator writes to
wgpolicyk8s.io/v1beta1specifically (reconciler.rs). The copy shipped here servesv1alpha1,v1alpha2andv1beta1; Kyverno's serves onlyv1alpha2. So this chart's copy is a superset that satisfies both tools, while the reverse is not true — handing ownership to Kyverno breaks reconciliation with404 page not found. The README and the values comment spell that out, with the command to check the served versions first. See #347 for the underlying fix.This moves the CRDs into
templates/and gates them through values:Defaults keep today's behaviour: everything installed, nothing removed on uninstall.
crds.keepaddresses the one real downside of leavingcrds/— templates follow the release lifecycle and would otherwise be deleted byhelm uninstall, taking existingNetworkAssertionandPolicyReportresources with them. With the annotation set by default, uninstall leaves them alone, ascrds/did.installCRDsis honoured as an alias, soinstallCRDs: falsenow does what its comment always claimed.Also in here
Duplicate CRD removed.
crds/clusterpolicyreports.yamlcarried a second, byte-identical copy ofpolicyreports.wgpolicyk8s.io, which is already defined incrds/policyreports.yaml. Dropped.Upgrade note
This is the one behaviour change that needs calling out in the release notes. Objects installed from
crds/carry no Helm ownership metadata, so when they start being rendered fromtemplates/,helm upgraderefuses to adopt them (pkg/action/upgrade.go→checkOwnership) and fails withinvalid ownership metadata.Documented in the chart README with the one-time adoption command:
Users who would rather not adopt them can set
crds.install: falseand manage the CRDs outside the chart.Chart version bumped 0.3.1 → 0.4.0.
Testing
helm lintpasses. Render matrix, all output parsed as YAML:keepannotationscrds.keep=falsecrds.groups.wgpolicyk8s=falsecrds.groups.netchecks=falsecrds.install=falseinstallCRDs=falseinstallCRDs=trueexamples/kind-installation/values.yamlalready contained acrds.install: truekey that had no effect; it now resolves against the new schema, so the kind-based integration tests keep getting the CRDs.I have not run the kind integration suite locally — no cluster to hand here.
Two things deliberately left out of scope
This PR only touches the chart. Both of these are downstream of the move and are yours to decide on:
manifests/deploy.yamlis not regenerated here. Worth knowing that it currently contains no CRDs at all, becausecreate-static-manifests.shuseshelm template, which skipscrds/— while the installation docs present that file as installing "the NetworkAssertion CRDs and the Netchecks operator". After this change, regenerating it picks up all three, which fixes that mismatch.create-static-manifests.shmay want--set crds.keep=false. Itssed -i.bak '/helm.sh/d'strips thehelm.sh/resource-policy: keepline, which leaves a bareannotations:key on the NetworkAssertion CRD in the generated file. Valid YAML and accepted by Kubernetes, but untidy; rendering withcrds.keep=falseavoids it at the source.Unrelated observation
.github/workflows/helm-chart.yamlrunsct lint --chart-dirs chartsfrom the repo root and triggers onpaths: charts/**, but the chart lives atoperator/charts/netchecks. As far as I can tell that job finds no charts and the path filter never matches, so chart linting isn't actually running. Happy to open a separate issue.