diff --git a/.github/workflows/helm.yaml b/.github/workflows/helm.yaml index 9b6a2f06..db06fc98 100644 --- a/.github/workflows/helm.yaml +++ b/.github/workflows/helm.yaml @@ -79,3 +79,46 @@ jobs: - name: Run chart-testing (install) run: ct install --config=.github/ci/ct.yaml + + # The pre-install guard calls Helm's lookup, which returns nothing during + # `helm template` and `--dry-run`, so the chart unit tests cannot cover it. + # Exercise it against the live kind cluster instead. + # + # The signal is the controller Deployment, not the CRD: Helm applies crds/ + # before rendering, so keying on the CRD would refuse every first install. + - name: Verify the duplicate-install pre-install check + run: | + set -euo pipefail + CHART=charts/node-readiness-controller + + echo "::group::a fresh install into an empty cluster succeeds with the guard ON" + # This is the path the CRD-based guard broke: crds/ is applied before the + # templates render, so the check must not trip on this install's own CRD. + helm install nrc-guard "$CHART" \ + --namespace nrc-guard --create-namespace --wait --timeout 5m + echo "::endgroup::" + + echo "::group::a second install is refused while the first is running" + if helm install nrc-guard-2 "$CHART" \ + --namespace nrc-guard-2 --create-namespace 2>guard.err; then + echo "expected the second install to be refused, but it succeeded" + exit 1 + fi + cat guard.err + grep -q "already installed in this cluster" guard.err + echo "::endgroup::" + + echo "::group::escape hatch allows a second install anyway" + helm install nrc-guard-2 "$CHART" \ + --namespace nrc-guard-2 --create-namespace \ + --set crds.preInstallCheck=false --wait --timeout 5m + echo "::endgroup::" + + echo "::group::upgrade of the first release is not blocked, with the guard enabled" + helm upgrade nrc-guard "$CHART" \ + --namespace nrc-guard --wait --timeout 5m + echo "::endgroup::" + + helm uninstall nrc-guard-2 --namespace nrc-guard-2 + helm uninstall nrc-guard --namespace nrc-guard + kubectl delete namespace nrc-guard nrc-guard-2 diff --git a/Makefile b/Makefile index 4f6e4953..6470b61b 100644 --- a/Makefile +++ b/Makefile @@ -146,6 +146,17 @@ vet: ## Run go vet against code. .PHONY: manifests manifests: $(CONTROLLER_GEN) ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects. $(CONTROLLER_GEN) rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases + $(MAKE) sync-chart-crds + +.PHONY: sync-chart-crds +sync-chart-crds: ## Regenerate the chart's CRD copy from config, stamping the Helm installer label. Run by `manifests`. + # The chart ships the CRD from crds/ (which Helm does not template), so its only + # difference from controller-gen output is the standard app.kubernetes.io/managed-by + # installer label: kustomize in config/, helm in the chart. Never hand-edit the + # chart CRD; `make manifests` regenerates it and verify-chart-drift.sh checks it in. + sed 's#\(app.kubernetes.io/managed-by:\) kustomize#\1 helm#' \ + config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml \ + > charts/node-readiness-controller/crds/nodereadinessrules.readiness.node.x-k8s.io.yaml .PHONY: generate generate: $(CONTROLLER_GEN) ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations. diff --git a/api/v1alpha1/nodereadinessrule_types.go b/api/v1alpha1/nodereadinessrule_types.go index 5bba7232..b51bef15 100644 --- a/api/v1alpha1/nodereadinessrule_types.go +++ b/api/v1alpha1/nodereadinessrule_types.go @@ -347,6 +347,14 @@ type DryRunResults struct { } // +kubebuilder:object:root=true +// Stamp the CRD with the standard app.kubernetes.io/managed-by label so the +// controlling installation is visible in clusters where the control plane is +// hosted and the controller itself is not. The value is the installer: this +// kustomize package stamps "kustomize" (matching every other resource under +// config/); the Helm chart ships "helm"; providers override it to their own name +// in their install pipeline. See docs/book/src/user-guide/installation.md. +// +// +kubebuilder:metadata:labels=app.kubernetes.io/managed-by=kustomize // +kubebuilder:subresource:status // +kubebuilder:resource:scope=Cluster,shortName=nrr // +kubebuilder:printcolumn:name="Mode",type=string,JSONPath=`.spec.enforcementMode`,description="The enforcement mode of the rule: bootstrap-only or continuous." diff --git a/charts/node-readiness-controller/README.md b/charts/node-readiness-controller/README.md index 62666970..cbe4c1a3 100644 --- a/charts/node-readiness-controller/README.md +++ b/charts/node-readiness-controller/README.md @@ -36,6 +36,36 @@ The command deploys the _node-readiness-controller_ on the Kubernetes cluster in Helm installs CRDs from the chart `crds/` directory during initial install, but Helm does not upgrade or delete CRDs from that directory during `helm upgrade` or `helm uninstall`. Before upgrading to a chart version that changes the `NodeReadinessRule` schema, apply the updated CRD from the release artifacts or from `charts/node-readiness-controller/crds`. +## Avoiding a duplicate installation + +The controller manages node taints. Two installations reconciling the same +`NodeReadinessRule` resources will both act on the same nodes, so only one should run +in a cluster. + +This is easy to do by accident on a managed cluster: the provider may run the +controller as part of a hosted control plane, where it is not visible to you. + +To make the existing installation visible, the CRD carries the standard +`app.kubernetes.io/managed-by` label naming the installer. The CRD is cluster scoped +and shared by every install flow, so its value is whichever installation created it: +`helm` from this chart, `kustomize` from the config manifests, or a provider's own name +when they override it. + +```console +$ kubectl get crd nodereadinessrules.readiness.node.x-k8s.io \ + -o jsonpath='{.metadata.labels.app\.kubernetes\.io/managed-by}' +helm +``` + +A fresh `helm install` refuses to proceed when a controller Deployment is already +running in the cluster, and names it in the error. (The guard keys on the running +workload rather than the CRD, because Helm installs the CRD from `crds/` before it +renders templates, so the CRD always exists by the time the check runs.) `helm upgrade` +is unaffected. Set `crds.preInstallCheck=false` to install anyway. + +Providers installing the controller should override the label value in their own +pipeline, so the manager shown above identifies them rather than the upstream default. + ## Uninstalling the Chart To uninstall/delete the `my-release` deployment: @@ -69,6 +99,7 @@ The following table lists the configurable parameters of the _node-readiness-con | `leaderElection.enabled` | Enable leader election to support multiple replicas | `true` | | `leaderElection.namespace` | Namespace for the leader election lease. Defaults to the release namespace when empty. | `""` | | `priorityClassName` | The name of the priority class to add to pods | `system-cluster-critical` | +| `crds.preInstallCheck` | Fail a fresh install when the `NodeReadinessRule` CRD already exists, so a second controller cannot be installed alongside one you cannot see. No effect on `helm upgrade`. | `true` | | `rbac.create` | If `true`, create & use RBAC resources | `true` | | `resources` | Node Readiness Controller container CPU and memory requests/limits | _see values.yaml_ | | `serviceAccount.create` | If `true`, create a service account | `true` | diff --git a/charts/node-readiness-controller/crds/nodereadinessrules.readiness.node.x-k8s.io.yaml b/charts/node-readiness-controller/crds/nodereadinessrules.readiness.node.x-k8s.io.yaml index ec6c49ad..c907404c 100644 --- a/charts/node-readiness-controller/crds/nodereadinessrules.readiness.node.x-k8s.io.yaml +++ b/charts/node-readiness-controller/crds/nodereadinessrules.readiness.node.x-k8s.io.yaml @@ -4,6 +4,8 @@ kind: CustomResourceDefinition metadata: annotations: controller-gen.kubebuilder.io/version: v0.19.0 + labels: + app.kubernetes.io/managed-by: helm name: nodereadinessrules.readiness.node.x-k8s.io spec: group: readiness.node.x-k8s.io diff --git a/charts/node-readiness-controller/templates/crd-ownership-check.yaml b/charts/node-readiness-controller/templates/crd-ownership-check.yaml new file mode 100644 index 00000000..cb11a70a --- /dev/null +++ b/charts/node-readiness-controller/templates/crd-ownership-check.yaml @@ -0,0 +1,59 @@ +{{- /* +Refuse to install a second node-readiness-controller into a cluster that already +has one. Two controllers reconciling the same NodeReadinessRules compete over the +same node taints. That is easy to do by accident where the control plane is hosted: +the provider's installation is not visible to the user, so nothing signals that the +controller is already running. + +Two independent checks run below; either one aborts the install. + +1. CRD ownership (value) check. The cluster-scoped NodeReadinessRule CRD carries the + standard app.kubernetes.io/managed-by label naming its installer. This chart's + crds/ copy stamps `helm` (crds.managedBy). Helm applies crds/ *before* it renders + templates, so on a self install the CRD this install just created already carries + our own value -- it matches and passes. A value that is present but DIFFERENT + belongs to another installer: `kustomize`, or a hosted provider's ``. + This catches an invisible provider install even when its workload is renamed or + hidden, which the Deployment check below cannot see. It also naturally allows a + `helm uninstall` -> `helm install` cycle, since Helm leaves crds/ in place and the + value still matches. + +2. Deployment (workload) check. Catches a duplicate that carries the SAME managed-by + value (for example a second Helm release of this chart), which the value check + cannot distinguish. We match any Deployment in any namespace carrying + app.kubernetes.io/name=node-readiness-controller, the label this chart stamps. + The Deployment is applied after rendering, so one found here belongs to a + different, already-running installation. Flows that rename the workload are not + seen here but are caught by check 1 via the CRD label; see the installation guide. + +Lookup foot-guns handled below: + * lookup with an empty name returns a *list* that is truthy even when it matched + nothing, so we range .items and count real matches. + * a Deployment (or CRD) may carry no labels at all, so we default the label map + before indexing to avoid a nil panic. + +lookup returns nothing during "helm template" and "--dry-run", so this only fires on +a real install. +*/ -}} +{{- if and .Values.crds.preInstallCheck .Release.IsInstall }} +{{- $expected := .Values.crds.managedBy }} +{{- /* 1. CRD ownership (value) check. */ -}} +{{- $crd := lookup "apiextensions.k8s.io/v1" "CustomResourceDefinition" "" "nodereadinessrules.readiness.node.x-k8s.io" }} +{{- if $crd }} +{{- $owner := get ($crd.metadata.labels | default dict) "app.kubernetes.io/managed-by" }} +{{- if and $owner (ne $owner $expected) }} +{{- fail (printf "node-readiness-controller is already installed in this cluster by a different installer.\n\nThe NodeReadinessRule CRD is labelled app.kubernetes.io/managed-by=%s, but this install claims %q.\n\nAnother installation -- often a hosted provider's, running in the control plane where\nit is not visible to you -- already owns the cluster-scoped CRD. Installing this chart\nwould make two controllers manage the same node taints.\n\nTo adopt the existing installation, run 'helm upgrade' against its release.\nTo install anyway, set crds.preInstallCheck=false." $owner $expected) }} +{{- end }} +{{- end }} +{{- /* 2. Deployment (workload) check. */ -}} +{{- $existing := "" }} +{{- range (lookup "apps/v1" "Deployment" "" "").items }} +{{- $labels := .metadata.labels | default dict }} +{{- if eq (get $labels "app.kubernetes.io/name") "node-readiness-controller" }} +{{- $existing = printf "%s/%s" .metadata.namespace .metadata.name }} +{{- end }} +{{- end }} +{{- if $existing }} +{{- fail (printf "node-readiness-controller is already installed in this cluster.\n\nThe Deployment %s is already running the controller.\n\nInstalling a second controller will make both of them manage the same node taints.\nIf this is a managed cluster, your provider may have installed the controller as\npart of the control plane, where it is not visible to you.\n\nTo upgrade the existing installation, run 'helm upgrade' against its release.\nTo install anyway, set crds.preInstallCheck=false." $existing) }} +{{- end }} +{{- end }} diff --git a/charts/node-readiness-controller/templates/nodereadinessrules.yaml b/charts/node-readiness-controller/templates/nodereadinessrules.yaml index 3427fd27..c3f2adf6 100644 --- a/charts/node-readiness-controller/templates/nodereadinessrules.yaml +++ b/charts/node-readiness-controller/templates/nodereadinessrules.yaml @@ -9,6 +9,8 @@ apiVersion: readiness.node.x-k8s.io/v1alpha1 kind: NodeReadinessRule metadata: name: {{ required "nodeReadinessRules[]: name is required" $rule.name | quote }} + labels: + {{- include "node-readiness-controller.labels" $ | nindent 4 }} spec: {{- toYaml $spec | nindent 2 }} {{- end }} diff --git a/charts/node-readiness-controller/tests/crd_ownership_check_test.yaml b/charts/node-readiness-controller/tests/crd_ownership_check_test.yaml new file mode 100644 index 00000000..40e6ceb0 --- /dev/null +++ b/charts/node-readiness-controller/tests/crd_ownership_check_test.yaml @@ -0,0 +1,43 @@ +suite: Test duplicate-install pre-install check + +templates: + - "*.yaml" + +release: + name: node-readiness-controller + +tests: + # The guard calls lookup, which returns nothing outside a live cluster. These + # specs therefore cover rendering only: that the template is well formed, that + # it emits no manifests of its own, and that toggling the value does not break + # the chart. Whether the guard actually allows a first install and refuses a + # second is covered in the Helm CI workflow, which installs against a real cluster. + - it: emits no manifests when the check passes + template: templates/crd-ownership-check.yaml + asserts: + - hasDocuments: + count: 0 + + - it: emits no manifests when the check is disabled + template: templates/crd-ownership-check.yaml + set: + crds.preInstallCheck: false + asserts: + - hasDocuments: + count: 0 + + - it: renders cleanly with a custom managed-by identity + template: templates/crd-ownership-check.yaml + set: + crds.managedBy: example-provider + asserts: + - hasDocuments: + count: 0 + + - it: does not affect the rest of the chart when disabled + template: templates/deployment.yaml + set: + crds.preInstallCheck: false + asserts: + - isKind: + of: Deployment diff --git a/charts/node-readiness-controller/values.yaml b/charts/node-readiness-controller/values.yaml index a15cfd61..19a3dd2a 100644 --- a/charts/node-readiness-controller/values.yaml +++ b/charts/node-readiness-controller/values.yaml @@ -74,6 +74,24 @@ controller: # -- Bind address for the pprof endpoint. Leave empty to disable. pprofBindAddress: "" +# CustomResourceDefinition handling. +# +# The CRD is installed from the chart's crds/ directory, which Helm does not +# template and does not upgrade. See the chart README for upgrade guidance. +crds: + # -- Fail a fresh install when another node-readiness-controller already owns the + # cluster: either the NodeReadinessRule CRD is labelled with a different + # `managed-by` installer, or a controller Deployment is already running. Two + # controllers reconciling the same rules compete over the same node taints, which + # is easy to do by accident on a managed cluster where the provider's installation + # is not visible. Has no effect on `helm upgrade`. + preInstallCheck: true + # -- The installer identity this chart claims, compared against the existing CRD's + # `app.kubernetes.io/managed-by` label by the pre-install check. Must match the + # value stamped in the chart's `crds/` CRD (`helm`); only change it if you also + # re-stamp that CRD (for example when repackaging the chart for a provider). + managedBy: helm + rbac: create: true diff --git a/config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml b/config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml index ec6c49ad..63ed5276 100644 --- a/config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml +++ b/config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml @@ -4,6 +4,8 @@ kind: CustomResourceDefinition metadata: annotations: controller-gen.kubebuilder.io/version: v0.19.0 + labels: + app.kubernetes.io/managed-by: kustomize name: nodereadinessrules.readiness.node.x-k8s.io spec: group: readiness.node.x-k8s.io diff --git a/docs/book/src/user-guide/installation.md b/docs/book/src/user-guide/installation.md index c608197a..9f96f426 100644 --- a/docs/book/src/user-guide/installation.md +++ b/docs/book/src/user-guide/installation.md @@ -99,6 +99,91 @@ example on deploying the controller as a static pod in a kind cluster. This is typically handled via a bootstrap script or post-install job in a `kubeadm` setup. --- +## Avoiding a Duplicate Installation + +The controller manages node taints, so only one installation should run in a cluster. +Two installations reconciling the same `NodeReadinessRule` resources will both act on +the same nodes. + +This is easy to do by accident on a managed cluster. Where the control plane is hosted, +the provider may already run the controller as part of it, and that installation is not +visible to you in the usual places. + +### Finding the existing installation + +The CRD is cluster-scoped and singleton, so it is the one artifact every installation +shares. It carries the standard `app.kubernetes.io/managed-by` label naming the +installer that owns it: + +```sh +kubectl get crd nodereadinessrules.readiness.node.x-k8s.io \ + -o jsonpath='{.metadata.labels.app\.kubernetes\.io/managed-by}' +``` + +The value is the installer: `helm` from the chart, `kustomize` from the config +manifests, or a provider's own name when they override it (see below). + +If the CRD is not present, the controller is not installed. + +### For providers + +Providers installing the controller should override the label value so it identifies +them rather than the `helm`/`kustomize` default, which makes the hosted installation +self-describing to users: + +```yaml +# kustomize patch +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: nodereadinessrules.readiness.node.x-k8s.io + labels: + app.kubernetes.io/managed-by: example-provider +``` + +### Helm guardrail + +A fresh `helm install` runs two checks and aborts if either one finds an existing +installation: + +1. **CRD ownership.** If the `NodeReadinessRule` CRD already carries a + `app.kubernetes.io/managed-by` value different from the one this chart claims + (`helm`, set by `crds.managedBy`), another installer owns it — a `kustomize` + install, or a provider's ``. This catches a hosted provider install + even when its workload is renamed or hidden. A self reinstall passes, because the + CRD Helm leaves behind still carries `helm`. +2. **Running workload.** If a controller Deployment labelled + `app.kubernetes.io/name=node-readiness-controller` is already running, the install + is refused and the Deployment is named. This catches a second Helm release, which + check 1 cannot distinguish because it carries the same `managed-by` value. + +The workload check names the offending Deployment in the error: + +```console +$ helm install nrc ./charts/node-readiness-controller +Error: node-readiness-controller is already installed in this cluster. + +The Deployment kube-system/node-readiness-controller-manager is already running the +controller. +... +``` + +The guard keys on the running controller Deployment, not on the CRD: Helm installs the +CRD from `crds/` before it renders templates, so the CRD always exists by the time the +check runs. The Deployment is applied after rendering, so one found during the check +belongs to a different, already-running installation. + +`helm upgrade` is unaffected. To install anyway, set `crds.preInstallCheck=false`. + +> [!NOTE] +> The check only runs during a real install. It is skipped by `helm template` and +> `helm install --dry-run`, which do not query the cluster. It matches Deployments +> labelled `app.kubernetes.io/name=node-readiness-controller`; an installation that +> renames the workload, or does not use this chart, is not detected. + +The other install paths -- `kubectl apply` of the release manifests, or Kustomize -- +do not have an equivalent guard. Check the label above before installing. + ## Verification After installation, verify that the controller is running successfully. diff --git a/hack/verify-chart-drift.sh b/hack/verify-chart-drift.sh index d43c28c0..94e092dc 100755 --- a/hack/verify-chart-drift.sh +++ b/hack/verify-chart-drift.sh @@ -23,8 +23,12 @@ set -o pipefail KUBE_ROOT="$(dirname "${BASH_SOURCE[0]}")/.." cd "${KUBE_ROOT}" +# `make manifests` regenerates config/crd/bases AND the chart's crds/ copy (via +# sync-chart-crds). So this check just runs it and asserts nothing changed: if the +# committed CRDs are stale, or the chart copy was hand-edited, regeneration moves +# them and git diff fails. Both files are generated; neither is edited by hand. make manifests -diff -u \ +git diff --exit-code -- \ config/crd/bases/readiness.node.x-k8s.io_nodereadinessrules.yaml \ charts/node-readiness-controller/crds/nodereadinessrules.readiness.node.x-k8s.io.yaml