From e7a1d2915f78feafff835460cfba1b1cf9ee5457 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 30 Oct 2026 08:41:45 +0000 Subject: [PATCH 01/14] e2e: Install the released chart in the upgrade test Since the move to Helm, TestOperatorUpgrade installed the released operator image with the chart from the current checkout. That only works while the current chart stays compatible with the released binary: a chart that passes a new controller flag or narrows the RBAC makes the released controller crash or fail to sync, and the test never gets to the upgrade. Install the released version from its published chart instead, the way users upgrading from it have it deployed, then upgrade to the local chart as before. The chart comes from RELEASED_OPERATOR_CHART, so downstream builds can point elsewhere, and its version is RELEASED_OPERATOR_TAG without the leading "v", matching how the release workflow publishes it. The released image is now the one the chart deploys, so RELEASED_OPERATOR_IMG is gone. On bink, push-released-operator-image reads that image from the chart and still copies it to the local registry, which keeps the test fast and avoids pulls from a remote registry timing out; the test overrides the chart image with that copy. The restore cleanup is now registered right after the uninstall: when the released operator failed to come up, the cleanup was never registered and every later test ran without an operator. Assisted-by: AI Signed-off-by: Alice Frosi --- Makefile | 30 +++++++++++-------- test/e2e/upgrade_test.go | 65 ++++++++++++++++++++++++++-------------- 2 files changed, 60 insertions(+), 35 deletions(-) diff --git a/Makefile b/Makefile index 52605716..648af029 100644 --- a/Makefile +++ b/Makefile @@ -17,10 +17,12 @@ BINK_NODE_DISK_IMAGE ?= ghcr.io/bootc-dev/bink/node:v$(DEFAULT_KUBE_MINOR)-fedor BINK_LOCAL_REGISTRY_NODE_IMAGE ?= registry.cluster.local:5000/node E2E_REGISTRY_USER ?= e2e-user E2E_REGISTRY_PASSWORD ?= e2e-password -# Released operator for upgrade testing (TestOperatorUpgrade). -# Override RELEASED_OPERATOR_IMG for downstream images. +# Released operator for upgrade testing (TestOperatorUpgrade): the published +# chart and the release tag, whose version without the leading "v" is the +# chart version. The released image is the one the chart deploys. Override +# RELEASED_OPERATOR_CHART for downstream charts. RELEASED_OPERATOR_TAG ?= v0.2.2 -RELEASED_OPERATOR_IMG ?= ghcr.io/bootc-dev/bootc-operator:$(RELEASED_OPERATOR_TAG) +RELEASED_OPERATOR_CHART ?= oci://ghcr.io/bootc-dev/bootc-operator/charts/bootc-operator IMG_BINK_RELEASED ?= registry.cluster.local:5000/bootc-operator-released:latest # YEAR defines the year value used for substituting the YEAR placeholder in the boilerplate header. YEAR ?= $(shell date +%Y) @@ -120,7 +122,8 @@ e2e: ## Run e2e tests (requires: make deploy-bink). V=1 for verbose. RUN= E2E_NODE_IMAGE_UPDATE2_DIGEST=$$(skopeo inspect --tls-verify=false docker://localhost:5000/node:update2 | jq -r '.Digest') \ E2E_REGISTRY_USER=$(E2E_REGISTRY_USER) E2E_REGISTRY_PASSWORD=$(E2E_REGISTRY_PASSWORD) \ $(if $(RELEASED_OPERATOR_TAG),E2E_OPERATOR_RELEASE_TAG=$(RELEASED_OPERATOR_TAG)) \ - $(if $(RELEASED_OPERATOR_IMG),E2E_OPERATOR_RELEASED_IMG=$(IMG_BINK_RELEASED)) \ + $(if $(RELEASED_OPERATOR_TAG),E2E_OPERATOR_RELEASED_IMG=$(IMG_BINK_RELEASED)) \ + $(if $(RELEASED_OPERATOR_CHART),E2E_OPERATOR_RELEASED_CHART=$(RELEASED_OPERATOR_CHART)) \ go test -timeout 90m -count=1 -parallel 2 $(if $(V),-v) $(if $(RUN),-run '$(RUN)') . # EKS e2e settings @@ -152,7 +155,7 @@ e2e-eks: ## Run e2e tests against EKS. V=1 for verbose. RUN= to filter. E2E_REGISTRY_USER="$(EKS_REGISTRY_USER)" \ E2E_REGISTRY_PASSWORD="$(EKS_REGISTRY_PASSWORD)" \ $(if $(RELEASED_OPERATOR_TAG),E2E_OPERATOR_RELEASE_TAG=$(RELEASED_OPERATOR_TAG)) \ - $(if $(RELEASED_OPERATOR_IMG),E2E_OPERATOR_RELEASED_IMG=$(RELEASED_OPERATOR_IMG)) \ + $(if $(RELEASED_OPERATOR_CHART),E2E_OPERATOR_RELEASED_CHART=$(RELEASED_OPERATOR_CHART)) \ $(if $(EKS_IMAGE_PULL_SECRET),E2E_IMAGE_PULL_SECRET=$(EKS_IMAGE_PULL_SECRET)) \ E2E_NODE_PULL_SECRET="$(EKS_NODE_PULL_SECRET)" \ ARTIFACTS="$(ARTIFACTS)" \ @@ -192,13 +195,14 @@ build-update-image: ## Build derived node images for update testing and push to podman push --tls-verify=false localhost:5000/node:update2 .PHONY: push-released-operator-image -push-released-operator-image: ## Pull released operator image and push to bink registry for upgrade testing. - @if [ -z "$(RELEASED_OPERATOR_IMG)" ]; then \ - echo "Error: RELEASED_OPERATOR_IMG must be set (e.g. ghcr.io/bootc-dev/bootc-operator:v0.1.0)"; \ - exit 1; \ - fi - podman pull $(RELEASED_OPERATOR_IMG) - podman push --tls-verify=false $(RELEASED_OPERATOR_IMG) localhost:5000/bootc-operator-released:latest +push-released-operator-image: yq ## Copy the image of the released chart to the bink registry for upgrade testing. + img=$$(helm template bootc-operator $(RELEASED_OPERATOR_CHART) \ + --version $(RELEASED_OPERATOR_TAG:v%=%) \ + --kube-version $(DEFAULT_KUBE_MINOR).0 | \ + $(YQ) 'select(.kind == "Deployment") | .spec.template.spec.containers[] | select(.name == "manager") | .image') && \ + test -n "$$img" && \ + podman pull "$$img" && \ + podman push --tls-verify=false "$$img" localhost:5000/bootc-operator-released:latest ##@ Deployment @@ -256,7 +260,7 @@ start-bink: seed-node-image ## Start a bink cluster (idempotent). kubectl --kubeconfig $(KUBECONFIG_BINK) wait --for=condition=Ready node/controller --timeout=5m .PHONY: deploy-bink -deploy-bink: start-bink build-update-image $(if $(RELEASED_OPERATOR_IMG),push-released-operator-image) helm ## Deploy to a bink cluster (requires: buildimg). +deploy-bink: start-bink build-update-image $(if $(RELEASED_OPERATOR_TAG),push-released-operator-image) helm ## Deploy to a bink cluster (requires: buildimg). podman push --tls-verify=false $(IMG) localhost:5000/bootc-operator-e2e:latest helm dependency build ./chart/bootc-operator img="$(IMG_BINK)" && \ diff --git a/test/e2e/upgrade_test.go b/test/e2e/upgrade_test.go index da7ff675..957539b1 100644 --- a/test/e2e/upgrade_test.go +++ b/test/e2e/upgrade_test.go @@ -32,9 +32,9 @@ var operatorCRDNames = []string{ } // TestOperatorUpgrade installs the released version of the operator -// via Helm, verifies basic functionality, then upgrades by running -// helm upgrade with the current image and verifies the operator still -// works. +// from its published Helm chart, verifies basic functionality, then +// upgrades by running helm upgrade with the current chart and image and +// verifies the operator still works. func TestOperatorUpgrade(t *testing.T) { e2eutil.Providers(t, "bink", "eks") @@ -42,10 +42,14 @@ func TestOperatorUpgrade(t *testing.T) { if releaseTag == "" { t.Skip("E2E_OPERATOR_RELEASE_TAG not set") } + // Optional: overrides the image the released chart deploys, e.g. + // with a copy in a local registry. releasedImg := os.Getenv("E2E_OPERATOR_RELEASED_IMG") - if releasedImg == "" { - t.Skip("E2E_OPERATOR_RELEASED_IMG not set") + releasedChart := os.Getenv("E2E_OPERATOR_RELEASED_CHART") + if releasedChart == "" { + t.Skip("E2E_OPERATOR_RELEASED_CHART not set") } + releasedChartVersion := strings.TrimPrefix(releaseTag, "v") g := NewWithT(t) g.SetDefaultEventuallyTimeout(pollTimeout) @@ -64,16 +68,17 @@ func TestOperatorUpgrade(t *testing.T) { currentImg := containerImage(t, origDeploy.Spec.Template.Spec.Containers, "manager") extraArgs := controllerExtraArgs(t, origDeploy.Spec.Template.Spec.Containers) t.Logf("Current operator image: %s", currentImg) - t.Logf("Released operator tag: %s, image: %s", releaseTag, releasedImg) + t.Logf("Released operator tag: %s, image: %s, chart: %s", + releaseTag, releasedImg, releasedChart) - // Phase 1: Uninstall the current operator and re-install with - // the released image via Helm. + // Phase 1: Uninstall the current operator and re-install the + // released version from its published chart, the way users + // upgrading from it have it deployed. The current chart may not work + // with the released image. uninstallOperator(t, g, ctx, env) - helmInstallOperator(t, chartPath, releasedImg, extraArgs) - waitForOperatorReady(t, g, ctx, env.Client) - - t.Logf("Released operator %s installed via Helm", releaseTag) + // Restore the current operator even if the released one fails to + // install, so the tests that run after this one have an operator. t.Cleanup(func() { t.Logf("Restoring operator to current version...") helmUpgradeOperator(t, chartPath, currentImg, extraArgs) @@ -81,6 +86,12 @@ func TestOperatorUpgrade(t *testing.T) { t.Logf("Operator restored") }) + helmInstallOperator(t, releasedChart, releasedChartVersion, + releasedImg, extraArgs) + waitForOperatorReady(t, g, ctx, env.Client) + + t.Logf("Released operator %s installed via Helm", releaseTag) + // Phase 2: Verify the released version works. nodeName := env.AddNode(t) @@ -181,12 +192,17 @@ func uninstallOperator( t.Logf("Operator uninstalled") } -// helmInstallOperator installs the operator via helm install. -func helmInstallOperator(t *testing.T, chartPath, img string, extraArgs []string) { +// helmInstallOperator installs the given version of a published chart +// via helm install. +func helmInstallOperator( + t *testing.T, + chartRef, chartVersion, img string, + extraArgs []string, +) { t.Helper() - helmRun(t, "dependency", "build", chartPath) args := []string{ - "install", helmReleaseName, chartPath, + "install", helmReleaseName, chartRef, + "--version", chartVersion, "--namespace", testutil.OperatorNamespaceName, "--create-namespace", } @@ -194,9 +210,11 @@ func helmInstallOperator(t *testing.T, chartPath, img string, extraArgs []string helmRun(t, args...) } -// helmUpgradeOperator upgrades the operator via helm upgrade. +// helmUpgradeOperator upgrades the operator to the local chart via helm +// upgrade. func helmUpgradeOperator(t *testing.T, chartPath, img string, extraArgs []string) { t.Helper() + helmRun(t, "dependency", "build", chartPath) args := []string{ "upgrade", "--install", helmReleaseName, chartPath, "--namespace", testutil.OperatorNamespaceName, @@ -206,13 +224,16 @@ func helmUpgradeOperator(t *testing.T, chartPath, img string, extraArgs []string } // helmImageArgs returns the --set flags for image and controller args. +// An empty img keeps the image of the chart. func helmImageArgs(t *testing.T, img string, extraArgs []string) []string { t.Helper() - repo, tag := splitImage(img) - args := []string{ - "--set-string", "image.repository=" + repo, - "--set-string", "image.tag=" + tag, - "--set-string", "image.pullPolicy=Always", + args := []string{"--set-string", "image.pullPolicy=Always"} + if img != "" { + repo, tag := splitImage(img) + args = append(args, + "--set-string", "image.repository="+repo, + "--set-string", "image.tag="+tag, + ) } if secret := os.Getenv("E2E_IMAGE_PULL_SECRET"); secret != "" { args = append(args, "--set", "imagePullSecrets[0].name="+secret) From 670fc77b9286a8155958c428b5ace10fd0332aff Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Thu, 29 Oct 2026 15:07:50 +0000 Subject: [PATCH 02/14] controller: Add pull secret namespace list parser Prep for a --pull-secret-namespaces flag. The operator namespace is always included so that setting the flag only adds namespaces, and the parser rejects invalid names up front: a namespaced Secret informer for one could never sync, which would leave the controller stuck at startup with an unclear error. Assisted-by: AI Signed-off-by: Alice Frosi --- internal/controller/pullsecret.go | 31 ++++++++++++++++++++++++++ internal/controller/pullsecret_test.go | 29 ++++++++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 internal/controller/pullsecret.go diff --git a/internal/controller/pullsecret.go b/internal/controller/pullsecret.go new file mode 100644 index 00000000..1d2a8b8c --- /dev/null +++ b/internal/controller/pullsecret.go @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "fmt" + "slices" + "strings" + + "k8s.io/apimachinery/pkg/util/validation" +) + +// ParsePullSecretNamespaces parses a comma-separated namespace list and +// prepends operatorNamespace (when non-empty), which is always allowed. +// Empty entries and surrounding whitespace are dropped and duplicates +// removed. Invalid namespace names are rejected, since the Secret informer +// for them could never sync. +func ParsePullSecretNamespaces(value, operatorNamespace string) ([]string, error) { + var namespaces []string + for _, ns := range append([]string{operatorNamespace}, strings.Split(value, ",")...) { + ns = strings.TrimSpace(ns) + if ns == "" || slices.Contains(namespaces, ns) { + continue + } + if errs := validation.IsDNS1123Label(ns); len(errs) > 0 { + return nil, fmt.Errorf("invalid namespace %q: %s", ns, strings.Join(errs, "; ")) + } + namespaces = append(namespaces, ns) + } + return namespaces, nil +} diff --git a/internal/controller/pullsecret_test.go b/internal/controller/pullsecret_test.go index 27b6e1f3..a6ce5167 100644 --- a/internal/controller/pullsecret_test.go +++ b/internal/controller/pullsecret_test.go @@ -156,3 +156,32 @@ func TestPullSecretRemovalClearsRef(t *testing.T) { return bn.Spec.PullSecretRef, err }).Should(BeNil()) } + +func TestParsePullSecretNamespaces(t *testing.T) { + tests := []struct { + name string + value string + opNS string + want []string + wantErr bool + }{ + {"empty", "", "op", []string{"op"}, false}, + {"only separators", " , ,", "op", []string{"op"}, false}, + {"unknown operator namespace", "", "", nil, false}, + {"operator namespace prepended", "team-a", "op", []string{"op", "team-a"}, false}, + {"trims and dedupes", " team-a ,,op,team-a", "op", []string{"op", "team-a"}, false}, + {"invalid name", "Team-A", "op", nil, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + got, err := ParsePullSecretNamespaces(tt.value, tt.opNS) + if tt.wantErr { + g.Expect(err).To(MatchError(ContainSubstring(`invalid namespace "Team-A"`))) + return + } + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(got).To(Equal(tt.want)) + }) + } +} From a57b90ea681295b89a485f94c0a71cffc7af0558 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Thu, 29 Oct 2026 15:09:10 +0000 Subject: [PATCH 03/14] controller: Reject pull secrets outside allowed namespaces Anyone who can create or edit a BootcNodePool could point pullSecretRef at a Secret in any namespace and have the operator use those credentials and ship them to managed nodes. Pools may now only reference pull secrets in the namespaces given by the new --pull-secret-namespaces flag, plus the operator namespace, which is always allowed. Other pools become Degraded with reason SecretError and a message naming the namespace and the allowed ones, and no nodes are created or updated for them. This is a behavior change for pools that use a secret outside the operator namespace: after upgrading they must move the secret or have its namespace added to the flag. Assisted-by: AI Signed-off-by: Alice Frosi --- cmd/controller/main.go | 27 +++++- .../controller/bootcnodepool_controller.go | 15 ++- internal/controller/pullsecret_test.go | 97 +++++++++++++++++-- internal/controller/suite_test.go | 9 +- 4 files changed, 135 insertions(+), 13 deletions(-) diff --git a/cmd/controller/main.go b/cmd/controller/main.go index f15223d3..fb376913 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -49,6 +49,7 @@ func main() { var probeAddr string var tagResolutionInterval time.Duration var allowInsecureRegistry bool + var pullSecretNamespacesFlag string flag.StringVar( &probeAddr, "health-probe-bind-address", @@ -67,6 +68,13 @@ func main() { false, "Allow falling back to HTTP when resolving tag-based image refs against registries that do not serve TLS.", ) + flag.StringVar( + &pullSecretNamespacesFlag, + "pull-secret-namespaces", + "", + "Comma-separated extra namespaces that pools may reference pull secrets from. "+ + "The operator namespace is always allowed.", + ) flag.BoolVar(&enableLeaderElection, "leader-elect", false, "Enable leader election for controller manager. "+ "Enabling this will ensure there is only one active controller manager.") @@ -88,6 +96,22 @@ func main() { version.GitCommit, ) + operatorNamespace := detectNamespace() + pullSecretNamespaces, err := controller.ParsePullSecretNamespaces( + pullSecretNamespacesFlag, + operatorNamespace, + ) + if err != nil { + setupLog.Error(err, "Invalid --pull-secret-namespaces") + os.Exit(1) + } + if len(pullSecretNamespaces) == 0 { + setupLog.Error(nil, "No pull secret namespaces configured and operator namespace unknown; "+ + "set --pull-secret-namespaces or POD_NAMESPACE") + os.Exit(1) + } + setupLog.Info("Pull secret namespaces", "namespaces", pullSecretNamespaces) + mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ Scheme: scheme, HealthProbeBindAddress: probeAddr, @@ -109,9 +133,10 @@ func main() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), KubeClient: kubeClient, - EventNamespace: detectNamespace(), + EventNamespace: operatorNamespace, TagResolver: ®istry.GGCRResolver{AllowInsecure: allowInsecureRegistry}, TagResolutionInterval: tagResolutionInterval, + PullSecretNamespaces: pullSecretNamespaces, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "Failed to create controller", "controller", "bootcnodepool") os.Exit(1) diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index 6f9dee32..4a646518 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -67,6 +67,10 @@ type BootcNodePoolReconciler struct { TagResolver TagResolver TagResolutionInterval time.Duration + // PullSecretNamespaces is the allowlist of namespaces pools may + // reference pull secrets from. + PullSecretNamespaces []string + // drainCh receives events from drain goroutines to re-enqueue the // owning pool after a drain completes. drainCh chan event.GenericEvent @@ -701,7 +705,8 @@ func (r *BootcNodePoolReconciler) listAllBootcNodes( } // fetchPullSecretData fetches the .dockerconfigjson data from the pull -// secret referenced by the pool. Returns nil if no pull secret is set. +// secret referenced by the pool. Returns nil if no pull secret is set, and +// an error if the secret's namespace is not in PullSecretNamespaces. func (r *BootcNodePoolReconciler) fetchPullSecretData( ctx context.Context, pool *bootcv1alpha1.BootcNodePool, @@ -710,6 +715,14 @@ func (r *BootcNodePoolReconciler) fetchPullSecretData( return nil, nil } + if !slices.Contains(r.PullSecretNamespaces, pool.Spec.PullSecretRef.Namespace) { + return nil, fmt.Errorf( + "pull secret namespace %q is not allowed (allowed: %s)", + pool.Spec.PullSecretRef.Namespace, + strings.Join(r.PullSecretNamespaces, ", "), + ) + } + key := types.NamespacedName{ Name: pool.Spec.PullSecretRef.Name, Namespace: pool.Spec.PullSecretRef.Namespace, diff --git a/internal/controller/pullsecret_test.go b/internal/controller/pullsecret_test.go index a6ce5167..c117ca08 100644 --- a/internal/controller/pullsecret_test.go +++ b/internal/controller/pullsecret_test.go @@ -36,7 +36,7 @@ func TestPullSecretRefPropagation(t *testing.T) { secretData := []byte(`{"auths":{"registry.example.com":{}}}`) - secret := newDockerConfigSecret("ps-prop-secret", "default", secretData) + secret := newDockerConfigSecret("ps-prop-secret", testutil.OperatorNamespaceName, secretData) g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) @@ -46,7 +46,7 @@ func TestPullSecretRefPropagation(t *testing.T) { pool := testutil.NewPool("ps-prop-pool", testImageDigestRefA, testutil.WithWorkerSelector(), - testutil.WithPullSecret("ps-prop-secret", "default"), + testutil.WithPullSecret("ps-prop-secret", testutil.OperatorNamespaceName), ) g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) t.Cleanup(func() { _ = k8sClient.Delete(ctx, pool) }) @@ -56,7 +56,7 @@ func TestPullSecretRefPropagation(t *testing.T) { err := k8sClient.Get(ctx, client.ObjectKey{Name: "ps-prop-node"}, &bn) return bn.Spec.PullSecretRef, err }).Should(Equal(&bootcv1alpha1.PullSecretRef{ - Name: "ps-prop-secret", Namespace: "default", + Name: "ps-prop-secret", Namespace: testutil.OperatorNamespaceName, })) } @@ -72,7 +72,7 @@ func TestPullSecretMissingDegradedAndRecovery(t *testing.T) { pool := testutil.NewPool("ps-missing-pool", testImageDigestRefA, testutil.WithWorkerSelector(), - testutil.WithPullSecret("ps-missing-secret", "default"), + testutil.WithPullSecret("ps-missing-secret", testutil.OperatorNamespaceName), ) g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) t.Cleanup(func() { _ = k8sClient.Delete(ctx, pool) }) @@ -99,7 +99,7 @@ func TestPullSecretMissingDegradedAndRecovery(t *testing.T) { // Create the missing secret and verify recovery. secretData := []byte(`{"auths":{"recovered":{}}}`) - secret := newDockerConfigSecret("ps-missing-secret", "default", secretData) + secret := newDockerConfigSecret("ps-missing-secret", testutil.OperatorNamespaceName, secretData) g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) @@ -121,7 +121,7 @@ func TestPullSecretRemovalClearsRef(t *testing.T) { ctx := context.Background() secretData := []byte(`{"auths":{"remove":{}}}`) - secret := newDockerConfigSecret("ps-remove-secret", "default", secretData) + secret := newDockerConfigSecret("ps-remove-secret", testutil.OperatorNamespaceName, secretData) g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) @@ -131,7 +131,7 @@ func TestPullSecretRemovalClearsRef(t *testing.T) { pool := testutil.NewPool("ps-remove-pool", testImageDigestRefA, testutil.WithWorkerSelector(), - testutil.WithPullSecret("ps-remove-secret", "default"), + testutil.WithPullSecret("ps-remove-secret", testutil.OperatorNamespaceName), ) g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) t.Cleanup(func() { _ = k8sClient.Delete(ctx, pool) }) @@ -157,6 +157,89 @@ func TestPullSecretRemovalClearsRef(t *testing.T) { }).Should(BeNil()) } +func TestPullSecretNamespaceDeniedAndRecovery(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + deniedNS := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ps-denied-ns"}} + g.Expect(k8sClient.Create(ctx, deniedNS)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, deniedNS) }) + + secretData := []byte(`{"auths":{"denied":{}}}`) + deniedSecret := newDockerConfigSecret("ps-denied-secret", deniedNS.Name, secretData) + g.Expect(k8sClient.Create(ctx, deniedSecret)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, deniedSecret) }) + + node := testutil.NewK8sNode("ps-denied-node", testutil.WorkerLabels()) + g.Expect(k8sClient.Create(ctx, node)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, node) }) + + pool := testutil.NewPool("ps-denied-pool", testImageDigestRefA, + testutil.WithWorkerSelector(), + testutil.WithPullSecret(deniedSecret.Name, deniedSecret.Namespace), + ) + g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, pool) }) + + // The secret exists, but its namespace is not in the allowlist. + g.Eventually(func() ([]metav1.Condition, error) { + var p bootcv1alpha1.BootcNodePool + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(pool), &p) + return p.Status.Conditions, err + }).Should(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolDegraded), + HaveField("Status", metav1.ConditionTrue), + HaveField("Reason", bootcv1alpha1.PoolSecretError), + HaveField("Message", ContainSubstring(`"ps-denied-ns"`)), + ))) + + g.Consistently(func() error { + return k8sClient.Get( + ctx, + client.ObjectKey{Name: "ps-denied-node"}, + &bootcv1alpha1.BootcNode{}, + ) + }, "2s", pollInterval).ShouldNot(Succeed()) + + // Move the secret to the operator namespace and point the pool at it. + allowedSecret := newDockerConfigSecret( + "ps-denied-secret", + testutil.OperatorNamespaceName, + secretData, + ) + g.Expect(k8sClient.Create(ctx, allowedSecret)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, allowedSecret) }) + + g.Eventually(func() error { + var p bootcv1alpha1.BootcNodePool + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(pool), &p); err != nil { + return err + } + p.Spec.PullSecretRef.Namespace = testutil.OperatorNamespaceName + return k8sClient.Update(ctx, &p) + }).Should(Succeed()) + + g.Eventually(func() ([]metav1.Condition, error) { + var p bootcv1alpha1.BootcNodePool + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(pool), &p) + return p.Status.Conditions, err + }).Should(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolDegraded), + HaveField("Status", metav1.ConditionFalse), + HaveField("Reason", bootcv1alpha1.PoolHealthy), + ))) + + g.Eventually(func() (*bootcv1alpha1.PullSecretRef, error) { + var bn bootcv1alpha1.BootcNode + err := k8sClient.Get(ctx, client.ObjectKey{Name: "ps-denied-node"}, &bn) + return bn.Spec.PullSecretRef, err + }).Should(Equal(&bootcv1alpha1.PullSecretRef{ + Name: "ps-denied-secret", Namespace: testutil.OperatorNamespaceName, + })) +} + func TestParsePullSecretNamespaces(t *testing.T) { tests := []struct { name string diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index a85cb868..1f1c64c5 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -84,10 +84,11 @@ func TestMain(m *testing.M) { } testReconciler = &BootcNodePoolReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - KubeClient: kubeClient, - EventNamespace: testutil.OperatorNamespaceName, + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + KubeClient: kubeClient, + EventNamespace: testutil.OperatorNamespaceName, + PullSecretNamespaces: []string{testutil.OperatorNamespaceName}, } if err := testReconciler.SetupWithManager(mgr); err != nil { fmt.Fprintf(os.Stderr, "Failed to setup reconciler: %v\n", err) From 58078c7af8a5033fea0b779ab1720f578104f02a Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Thu, 29 Oct 2026 15:09:27 +0000 Subject: [PATCH 04/14] controller: Restrict the Secret cache to allowed namespaces Watching Secrets made the informer cache every Secret in the cluster; secretTypePredicate only filters which events trigger a reconcile, not what is stored. Now that pools can only use secrets from the allowed namespaces, there is no reason to cache anything else. This is also what lets the controller drop its cluster-wide Secret RBAC: the informer no longer lists or watches Secrets outside the allowlist. Assisted-by: AI Signed-off-by: Alice Frosi --- cmd/controller/main.go | 4 ++++ internal/controller/bootcnodepool_controller.go | 3 ++- internal/controller/pullsecret.go | 16 ++++++++++++++++ internal/controller/suite_test.go | 7 ++++++- 4 files changed, 28 insertions(+), 2 deletions(-) diff --git a/cmd/controller/main.go b/cmd/controller/main.go index fb376913..6fb2971f 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -13,6 +13,7 @@ import ( "k8s.io/client-go/kubernetes" clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -117,6 +118,9 @@ func main() { HealthProbeBindAddress: probeAddr, LeaderElection: enableLeaderElection, LeaderElectionID: "bootc-operator.bootc.dev", + Cache: cache.Options{ + ByObject: controller.SecretCacheByObject(pullSecretNamespaces), + }, }) if err != nil { setupLog.Error(err, "Failed to start manager") diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index 4a646518..2fe91baa 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -68,7 +68,8 @@ type BootcNodePoolReconciler struct { TagResolutionInterval time.Duration // PullSecretNamespaces is the allowlist of namespaces pools may - // reference pull secrets from. + // reference pull secrets from. The manager's Secret cache must be + // restricted to the same namespaces. PullSecretNamespaces []string // drainCh receives events from drain goroutines to re-enqueue the diff --git a/internal/controller/pullsecret.go b/internal/controller/pullsecret.go index 1d2a8b8c..795d82a1 100644 --- a/internal/controller/pullsecret.go +++ b/internal/controller/pullsecret.go @@ -7,7 +7,10 @@ import ( "slices" "strings" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/validation" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" ) // ParsePullSecretNamespaces parses a comma-separated namespace list and @@ -29,3 +32,16 @@ func ParsePullSecretNamespaces(value, operatorNamespace string) ([]string, error } return namespaces, nil } + +// SecretCacheByObject returns manager cache settings that restrict the +// Secret informer to the given namespaces, so the controller never caches +// (or needs RBAC for) Secrets elsewhere in the cluster. +func SecretCacheByObject(namespaces []string) map[client.Object]cache.ByObject { + config := make(map[string]cache.Config, len(namespaces)) + for _, ns := range namespaces { + config[ns] = cache.Config{} + } + return map[client.Object]cache.ByObject{ + &corev1.Secret{}: {Namespaces: config}, + } +} diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 1f1c64c5..4dd5ec35 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -14,6 +14,7 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -60,11 +61,15 @@ func TestMain(m *testing.M) { // Start a manager with the reconciler so controller logic runs // against the envtest API server. + pullSecretNamespaces := []string{testutil.OperatorNamespaceName} mgr, err := ctrl.NewManager(cfg, ctrl.Options{ Scheme: scheme.Scheme, Metrics: metricsserver.Options{ BindAddress: "0", // disable metrics server in tests }, + Cache: cache.Options{ + ByObject: SecretCacheByObject(pullSecretNamespaces), + }, }) if err != nil { fmt.Fprintf(os.Stderr, "Failed to create manager: %v\n", err) @@ -88,7 +93,7 @@ func TestMain(m *testing.M) { Scheme: mgr.GetScheme(), KubeClient: kubeClient, EventNamespace: testutil.OperatorNamespaceName, - PullSecretNamespaces: []string{testutil.OperatorNamespaceName}, + PullSecretNamespaces: pullSecretNamespaces, } if err := testReconciler.SetupWithManager(mgr); err != nil { fmt.Fprintf(os.Stderr, "Failed to setup reconciler: %v\n", err) From a8bd81fc9306fa79eca537a26a10a272a4ea6d3e Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Thu, 29 Oct 2026 15:09:53 +0000 Subject: [PATCH 05/14] rbac: Grant secrets access through a namespaced Role The controller's ClusterRole let it read every Secret in the cluster, broader than ARCHITECTURE.md describes. With the Secret cache now limited to the allowed namespaces, cluster-wide access is no longer needed. Secret read access moves from the ClusterRole to a Role in the operator namespace, bound to the controller ServiceAccount, in both the Kustomize manifests and the generated Helm chart. Extra namespaces need their own Role, which the Helm chart grows next. controller-gen now emits role.yaml as two documents, a ClusterRole and a Role. sync-helm.sh read .rules from the whole file, which would merge them and put the secrets rule back into the Helm ClusterRole, so it now selects the document by kind. It also templates the namespace of the new RoleBinding, whose source carries the Kustomize "system" placeholder. Assisted-by: AI Signed-off-by: Alice Frosi --- .gitignore | 2 ++ config/rbac/kustomization.yaml | 1 + config/rbac/namespaced_role_binding.yaml | 16 +++++++++++++ config/rbac/role.yaml | 23 ++++++++++++------- hack/sync-helm.sh | 13 +++++++++-- .../controller/bootcnodepool_controller.go | 2 +- 6 files changed, 46 insertions(+), 11 deletions(-) create mode 100644 config/rbac/namespaced_role_binding.yaml diff --git a/.gitignore b/.gitignore index 01dc05d8..c1d5cf8b 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,8 @@ chart/bootc-operator/charts/ chart/bootc-operator/Chart.lock chart/bootc-operator-crds/templates/ chart/bootc-operator/templates/controller-clusterrole.yaml +chart/bootc-operator/templates/controller-role.yaml +chart/bootc-operator/templates/controller-rolebinding.yaml chart/bootc-operator/templates/daemon-clusterrole.yaml chart/bootc-operator/templates/leader-election-role.yaml chart/bootc-operator/templates/controller-deployment.yaml diff --git a/config/rbac/kustomization.yaml b/config/rbac/kustomization.yaml index 809aa028..066d954c 100644 --- a/config/rbac/kustomization.yaml +++ b/config/rbac/kustomization.yaml @@ -2,6 +2,7 @@ resources: - controller_service_account.yaml - role.yaml - role_binding.yaml +- namespaced_role_binding.yaml - leader_election_role.yaml - leader_election_role_binding.yaml - daemon_service_account.yaml diff --git a/config/rbac/namespaced_role_binding.yaml b/config/rbac/namespaced_role_binding.yaml new file mode 100644 index 00000000..a4b2f20b --- /dev/null +++ b/config/rbac/namespaced_role_binding.yaml @@ -0,0 +1,16 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + labels: + app.kubernetes.io/name: bootc-operator + app.kubernetes.io/managed-by: kustomize + name: manager-rolebinding + namespace: system +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: manager-role +subjects: +- kind: ServiceAccount + name: controller-manager + namespace: system diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index c25de146..a1e0f340 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -27,14 +27,6 @@ rules: - pods/eviction verbs: - create -- apiGroups: - - "" - resources: - - secrets - verbs: - - get - - list - - watch - apiGroups: - "" - events.k8s.io @@ -82,3 +74,18 @@ rules: - bootcnodes/status verbs: - get +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: manager-role + namespace: system +rules: +- apiGroups: + - "" + resources: + - secrets + verbs: + - get + - list + - watch diff --git a/hack/sync-helm.sh b/hack/sync-helm.sh index f55df976..17d28a11 100755 --- a/hack/sync-helm.sh +++ b/hack/sync-helm.sh @@ -20,10 +20,13 @@ echo "Synced CRDs" # --- RBAC --- # gen_rbac OUTPUT_FILE SOURCE_FILE KIND NAME_SUFFIX COMPONENT [NAMESPACE] +# Takes the rules from the KIND document in SOURCE_FILE, which may hold +# several (controller-gen emits namespaced rules as a separate Role). gen_rbac() { local output="$1" source="$2" kind="$3" suffix="$4" component="$5" ns="${6:-}" local rules - rules=$("$YQ" -I2 '.rules' "$source") + rules=$("$YQ" -I2 "select(.kind == \"${kind}\") | .rules" "$source") + [ -n "$rules" ] || { echo "error: no ${kind} rules in ${source}" >&2; exit 1; } { echo "apiVersion: rbac.authorization.k8s.io/v1" echo "kind: ${kind}" @@ -40,6 +43,8 @@ gen_rbac() { gen_rbac "$CHART_DIR/templates/controller-clusterrole.yaml" "$CONFIG_DIR/rbac/role.yaml" \ ClusterRole controller controller +gen_rbac "$CHART_DIR/templates/controller-role.yaml" "$CONFIG_DIR/rbac/role.yaml" \ + Role controller controller "{{ .Release.Namespace }}" gen_rbac "$CHART_DIR/templates/daemon-clusterrole.yaml" "$CONFIG_DIR/rbac/daemon_role.yaml" \ ClusterRole daemon daemon gen_rbac "$CHART_DIR/templates/leader-election-role.yaml" "$CONFIG_DIR/rbac/leader_election_role.yaml" \ @@ -112,7 +117,8 @@ gen_binding() { .metadata.labels.HELM_LABELS_PLACEHOLDER = true | .roleRef.name = \"HELM_FULLNAME-${role_suffix}\" | .subjects[0].name = \"HELM_FULLNAME-${sa_suffix}\" | - .subjects[0].namespace = \"HELM_NAMESPACE\" + .subjects[0].namespace = \"HELM_NAMESPACE\" | + with(select(.metadata.namespace != null); .metadata.namespace = \"HELM_NAMESPACE\") " "$source" | helm_sed | sed \ -e '/HELM_LABELS_PLACEHOLDER/c\ {{- include "bootc-operator.labels" . | nindent 4 }}' \ > "$output" @@ -121,6 +127,9 @@ gen_binding() { gen_binding "$CONFIG_DIR/rbac/role_binding.yaml" \ "$CHART_DIR/templates/controller-clusterrolebinding.yaml" \ controller ClusterRole controller controller +gen_binding "$CONFIG_DIR/rbac/namespaced_role_binding.yaml" \ + "$CHART_DIR/templates/controller-rolebinding.yaml" \ + controller Role controller controller gen_binding "$CONFIG_DIR/rbac/daemon_role_binding.yaml" \ "$CHART_DIR/templates/daemon-clusterrolebinding.yaml" \ daemon ClusterRole daemon daemon diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index 2fe91baa..e23be865 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -90,7 +90,7 @@ type BootcNodePoolReconciler struct { // +kubebuilder:rbac:groups=node.bootc.dev,resources=bootcnodes,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=node.bootc.dev,resources=bootcnodes/status,verbs=get // +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch;update;patch -// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch +// +kubebuilder:rbac:groups="",namespace=system,resources=secrets,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=pods,verbs=get;list // +kubebuilder:rbac:groups="",resources=pods/eviction,verbs=create // +kubebuilder:rbac:groups=apps,resources=daemonsets,verbs=get From a8f7b4ae1a377a9cc4c0547f72aae5025237e013 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Thu, 29 Oct 2026 15:10:09 +0000 Subject: [PATCH 06/14] helm: Add pullSecretNamespaces value Lets pools use pull secrets from namespaces other than the release namespace, e.g. ones owned by other teams, set at install or upgrade time: helm upgrade ... --set 'pullSecretNamespaces={team-a,team-b}' The one value drives both the controller's --pull-secret-namespaces flag (always including the release namespace) and a Role and RoleBinding in each listed namespace, so the allowlist and the permissions cannot drift. The namespaces must already exist; the chart does not create them. A values schema rejects anything that is not a list of valid namespace names, as the controller would otherwise fail to start. Assisted-by: AI Signed-off-by: Alice Frosi --- chart/bootc-operator/templates/_helpers.tpl | 4 ++ .../pull-secret-namespaces-rbac.yaml | 37 +++++++++++++++++++ chart/bootc-operator/values.schema.json | 14 +++++++ chart/bootc-operator/values.yaml | 4 ++ hack/sync-helm.sh | 3 +- test/e2e/upgrade_test.go | 3 +- 6 files changed, 63 insertions(+), 2 deletions(-) create mode 100644 chart/bootc-operator/templates/pull-secret-namespaces-rbac.yaml create mode 100644 chart/bootc-operator/values.schema.json diff --git a/chart/bootc-operator/templates/_helpers.tpl b/chart/bootc-operator/templates/_helpers.tpl index e16553e1..090b6cc8 100644 --- a/chart/bootc-operator/templates/_helpers.tpl +++ b/chart/bootc-operator/templates/_helpers.tpl @@ -23,6 +23,10 @@ app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} app.kubernetes.io/managed-by: {{ .Release.Service }} {{- end }} +{{- define "bootc-operator.pullSecretNamespaces" -}} +{{- prepend .Values.pullSecretNamespaces .Release.Namespace | uniq | join "," }} +{{- end }} + {{- define "bootc-operator.image" -}} {{- $tag := default .Chart.AppVersion .Values.image.tag -}} {{ printf "%s:%s" .Values.image.repository $tag }} diff --git a/chart/bootc-operator/templates/pull-secret-namespaces-rbac.yaml b/chart/bootc-operator/templates/pull-secret-namespaces-rbac.yaml new file mode 100644 index 00000000..8b3fc74a --- /dev/null +++ b/chart/bootc-operator/templates/pull-secret-namespaces-rbac.yaml @@ -0,0 +1,37 @@ +{{- range without (.Values.pullSecretNamespaces | uniq) $.Release.Namespace }} +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: {{ include "bootc-operator.fullname" $ }}-pull-secrets + namespace: {{ . }} + labels: + {{- include "bootc-operator.labels" $ | nindent 4 }} + app.kubernetes.io/component: controller +rules: +- apiGroups: + - "" + resources: + - secrets + verbs: + - get + - list + - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: {{ include "bootc-operator.fullname" $ }}-pull-secrets + namespace: {{ . }} + labels: + {{- include "bootc-operator.labels" $ | nindent 4 }} + app.kubernetes.io/component: controller +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: {{ include "bootc-operator.fullname" $ }}-pull-secrets +subjects: +- kind: ServiceAccount + name: {{ include "bootc-operator.fullname" $ }}-controller + namespace: {{ $.Release.Namespace }} +{{- end }} diff --git a/chart/bootc-operator/values.schema.json b/chart/bootc-operator/values.schema.json new file mode 100644 index 00000000..7c4847ef --- /dev/null +++ b/chart/bootc-operator/values.schema.json @@ -0,0 +1,14 @@ +{ + "$schema": "https://json-schema.org/draft-07/schema#", + "type": "object", + "properties": { + "pullSecretNamespaces": { + "type": "array", + "items": { + "type": "string", + "maxLength": 63, + "pattern": "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$" + } + } + } +} diff --git a/chart/bootc-operator/values.yaml b/chart/bootc-operator/values.yaml index 383c3705..a91f91eb 100644 --- a/chart/bootc-operator/values.yaml +++ b/chart/bootc-operator/values.yaml @@ -10,5 +10,9 @@ image: imagePullSecrets: [] +# Extra namespaces that BootcNodePool pullSecretRefs may point to. The release +# namespace is always allowed. The namespaces must already exist. +pullSecretNamespaces: [] + controller: extraArgs: [] diff --git a/hack/sync-helm.sh b/hack/sync-helm.sh index 17d28a11..434b3102 100755 --- a/hack/sync-helm.sh +++ b/hack/sync-helm.sh @@ -89,8 +89,9 @@ gen_workload() { gen_workload "$CONFIG_DIR/manager/manager.yaml" Deployment controller \ "$CHART_DIR/templates/controller-deployment.yaml" -# Inject controller.extraArgs into the args list +# Inject --pull-secret-namespaces and controller.extraArgs into the args list sed -i '/^ image: {{ include "bootc-operator\.image" \. }}/i\ + - {{ printf "--pull-secret-namespaces=%s" (include "bootc-operator.pullSecretNamespaces" .) | quote }}\ {{- range .Values.controller.extraArgs }}\ - {{ . | quote }}\ {{- end }}' "$CHART_DIR/templates/controller-deployment.yaml" diff --git a/test/e2e/upgrade_test.go b/test/e2e/upgrade_test.go index 957539b1..1bea96cf 100644 --- a/test/e2e/upgrade_test.go +++ b/test/e2e/upgrade_test.go @@ -309,7 +309,8 @@ func controllerExtraArgs(t *testing.T, containers []corev1.Container) []string { for _, arg := range c.Args { switch { case arg == "--leader-elect", - strings.HasPrefix(arg, "--health-probe-bind-address="): + strings.HasPrefix(arg, "--health-probe-bind-address="), + strings.HasPrefix(arg, "--pull-secret-namespaces="): default: extra = append(extra, arg) } From 61e36a7083cda34214e8bab9779ef88d61459827 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Thu, 29 Oct 2026 15:10:24 +0000 Subject: [PATCH 07/14] helm: Add chart template tests The chart's RBAC and controller args are generated or templated from several sources, and nothing checked the rendered result, so a mistake in sync-helm.sh or the templates would only show up on a real install. make helm-test renders the chart with and without extra pullSecretNamespaces and checks the controller flag, that each namespace gets one secrets Role bound to the controller, that the controller ClusterRole has no secrets access, and that the values schema rejects bad input. helm is installed with go install like the other tools. The unit CI job runs the target. Assisted-by: AI Signed-off-by: Alice Frosi --- .github/workflows/ci.yaml | 3 ++ Makefile | 11 +++++++ hack/test-helm.sh | 64 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+) create mode 100755 hack/test-helm.sh diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 403af343..3e370b4d 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -42,6 +42,9 @@ jobs: - name: Unit tests run: make unit + - name: Helm template tests + run: make helm-test + - name: Vet run: make vet diff --git a/Makefile b/Makefile index 648af029..b8a04a84 100644 --- a/Makefile +++ b/Makefile @@ -64,6 +64,10 @@ manifests: controller-gen ## Generate CRD and RBAC manifests. helm: manifests yq ## Regenerate Helm chart from Kustomize manifests. YQ="$(YQ)" ./hack/sync-helm.sh +.PHONY: helm-test +helm-test: helm helm-tool ## Render the Helm chart and check the output. + HELM="$(HELM)" YQ="$(YQ)" ./hack/test-helm.sh + .PHONY: generate generate: controller-gen ## Generate DeepCopy method implementations. "$(CONTROLLER_GEN)" object:headerFile="hack/boilerplate.go.txt",year=$(YEAR) paths="./..." @@ -298,12 +302,14 @@ GOLANGCI_LINT = $(LOCALBIN)/golangci-lint GOLINES_BIN = $(LOCALBIN)/golines GOVULNCHECK = $(LOCALBIN)/govulncheck YQ ?= $(LOCALBIN)/yq +HELM ?= $(LOCALBIN)/helm KUSTOMIZE_VERSION ?= v5.8.1 CONTROLLER_TOOLS_VERSION ?= v0.20.1 GOLANGCI_LINT_VERSION ?= v2.11.4 GOVULNCHECK_VERSION ?= v1.9.0 YQ_VERSION ?= v4.53.2 +HELM_VERSION ?= v3.19.0 # ENVTEST_VERSION is the version of controller-runtime release branch to fetch the envtest setup script (i.e. release-0.20) ENVTEST_VERSION ?= $(shell v='$(call gomodver,sigs.k8s.io/controller-runtime)'; \ @@ -357,6 +363,11 @@ yq: $(YQ) ## Download yq locally if necessary. $(YQ): $(LOCALBIN) $(call go-install-tool,$(YQ),github.com/mikefarah/yq/v4,$(YQ_VERSION)) +.PHONY: helm-tool +helm-tool: $(HELM) ## Download helm locally if necessary. +$(HELM): $(LOCALBIN) + $(call go-install-tool,$(HELM),helm.sh/helm/v3/cmd/helm,$(HELM_VERSION)) + # go-install-tool will 'go install' any package with custom target and name of binary, if it doesn't exist # $1 - target path with name of binary # $2 - package url which can be installed diff --git a/hack/test-helm.sh b/hack/test-helm.sh new file mode 100755 index 00000000..c77d6e87 --- /dev/null +++ b/hack/test-helm.sh @@ -0,0 +1,64 @@ +#!/usr/bin/env bash +# Render the Helm chart with representative values and check the output. +# Run: make helm-test + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" +CHART_DIR="$REPO_ROOT/chart/bootc-operator" +HELM="${HELM:-helm}" +YQ="${YQ:-yq}" +NS=bootc-operator +SA=test-bootc-operator-controller + +fail() { echo "FAIL: $*" >&2; exit 1; } + +# A go-installed helm lacks build-time version info and assumes Kubernetes +# v1.20, which the chart's kubeVersion constraint rejects. +render() { + "$HELM" template test "$CHART_DIR" --namespace "$NS" --kube-version 1.34.0 "$@" +} + +# check MANIFESTS FLAG_VALUE NAMESPACES... +# Asserts that the controller flag is FLAG_VALUE, that each namespace has +# exactly one secret-reading Role bound to the controller ServiceAccount, +# and that the controller ClusterRole does not grant access to secrets. +check() { + local manifests="$1" flag="$2"; shift 2 + local want got + want=$(printf '%s\n' "$@" | sort) + + got=$(echo "$manifests" | "$YQ" -N 'select(.kind == "Deployment") | + .spec.template.spec.containers[0].args[] | select(test("^--pull-secret-namespaces="))') + [ "$got" = "--pull-secret-namespaces=$flag" ] || fail "controller flag is [$got]" + + roles=$(echo "$manifests" | "$YQ" -N 'select(.kind == "Role") | + select(.rules | any_c(.resources | any_c(. == "secrets"))) | + .metadata.namespace + "/" + .metadata.name' | sort) + [ "$(echo "$roles" | cut -d/ -f1)" = "$want" ] || fail "secret Roles: [$roles]" + + got=$(echo "$manifests" | "$YQ" -N 'select(.kind == "RoleBinding") | + select(.subjects | any_c(.name == "'"$SA"'" and .namespace == "'"$NS"'")) | + .metadata.namespace + "/" + .roleRef.name' | sort) + # The leader election binding has no namespace in the template. + got=$(echo "$got" | grep -v '^/') + [ "$got" = "$roles" ] || fail "controller RoleBindings [$got], want [$roles]" + + got=$(echo "$manifests" | "$YQ" -N 'select(.kind == "ClusterRole") | + select(.metadata.name == "'"$SA"'") | + select(.rules | any_c(.resources | any_c(. == "secrets"))) | .metadata.name') + [ -z "$got" ] || fail "controller ClusterRole grants secrets access" +} + +"$HELM" dependency build "$CHART_DIR" >/dev/null + +check "$(render)" "$NS" "$NS" +# Duplicates and the release namespace itself must not produce extra RBAC. +check "$(render --set "pullSecretNamespaces={team-a,$NS,team-b,team-a}")" \ + "$NS,team-a,team-b" "$NS" team-a team-b + +for bad in "pullSecretNamespaces=team-a" "pullSecretNamespaces={team-a,}" \ + "pullSecretNamespaces={Team-A}"; do + if render --set "$bad" >/dev/null 2>&1; then fail "accepted --set $bad"; fi +done +echo "ok" From d2c6f9e9b178c9cb2625bc39d3021c2b587ced50 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Thu, 29 Oct 2026 16:49:57 +0000 Subject: [PATCH 08/14] e2e: Keep pullSecretNamespaces in the operator upgrade test TestOperatorUpgrade uninstalls the operator and reinstalls it with Helm, passing back the image and controller.extraArgs but no other values. On a cluster deployed with pullSecretNamespaces, the extra namespaces were dropped, so pools using secrets there broke for the rest of the run. It now reads the allowed namespaces from the controller's --pull-secret-namespaces flag before uninstalling and sets them again whenever it moves to the current chart. The released chart does not know the value, so the install of the released version does not pass it. Assisted-by: AI Signed-off-by: Alice Frosi --- test/e2e/upgrade_test.go | 46 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/test/e2e/upgrade_test.go b/test/e2e/upgrade_test.go index 1bea96cf..95f94b60 100644 --- a/test/e2e/upgrade_test.go +++ b/test/e2e/upgrade_test.go @@ -67,6 +67,7 @@ func TestOperatorUpgrade(t *testing.T) { g.Expect(env.Client.Get(ctx, operatorDeployKey(), &origDeploy)).To(Succeed()) currentImg := containerImage(t, origDeploy.Spec.Template.Spec.Containers, "manager") extraArgs := controllerExtraArgs(t, origDeploy.Spec.Template.Spec.Containers) + pullSecretNamespaces := controllerPullSecretNamespaces(t, g, ctx, env.Client) t.Logf("Current operator image: %s", currentImg) t.Logf("Released operator tag: %s, image: %s, chart: %s", releaseTag, releasedImg, releasedChart) @@ -81,7 +82,7 @@ func TestOperatorUpgrade(t *testing.T) { // install, so the tests that run after this one have an operator. t.Cleanup(func() { t.Logf("Restoring operator to current version...") - helmUpgradeOperator(t, chartPath, currentImg, extraArgs) + helmUpgradeOperator(t, chartPath, currentImg, extraArgs, pullSecretNamespaces) waitForOperatorReady(t, g, ctx, env.Client) t.Logf("Operator restored") }) @@ -103,7 +104,7 @@ func TestOperatorUpgrade(t *testing.T) { t.Logf("Node %q is Idle with released operator", nodeName) // Phase 3: Upgrade via helm upgrade to the current version. - helmUpgradeOperator(t, chartPath, currentImg, extraArgs) + helmUpgradeOperator(t, chartPath, currentImg, extraArgs, pullSecretNamespaces) waitForOperatorReady(t, g, ctx, env.Client) t.Logf("Upgraded operator to current version via Helm") @@ -211,8 +212,13 @@ func helmInstallOperator( } // helmUpgradeOperator upgrades the operator to the local chart via helm -// upgrade. -func helmUpgradeOperator(t *testing.T, chartPath, img string, extraArgs []string) { +// upgrade. The release namespace is always allowed, so including it in +// pullSecretNamespaces is harmless. +func helmUpgradeOperator( + t *testing.T, + chartPath, img string, + extraArgs, pullSecretNamespaces []string, +) { t.Helper() helmRun(t, "dependency", "build", chartPath) args := []string{ @@ -220,6 +226,13 @@ func helmUpgradeOperator(t *testing.T, chartPath, img string, extraArgs []string "--namespace", testutil.OperatorNamespaceName, } args = append(args, helmImageArgs(t, img, extraArgs)...) + if len(pullSecretNamespaces) > 0 { + j, err := json.Marshal(pullSecretNamespaces) + if err != nil { + t.Fatalf("marshaling pullSecretNamespaces: %v", err) + } + args = append(args, "--set-json", "pullSecretNamespaces="+string(j)) + } helmRun(t, args...) } @@ -321,6 +334,31 @@ func controllerExtraArgs(t *testing.T, containers []corev1.Container) []string { return nil } +// controllerPullSecretNamespaces returns the namespaces the running +// controller allows pull secrets from, as given by its +// --pull-secret-namespaces flag. +func controllerPullSecretNamespaces( + t *testing.T, + g Gomega, + ctx context.Context, + c client.Client, +) []string { + t.Helper() + var d appsv1.Deployment + g.Expect(c.Get(ctx, operatorDeployKey(), &d)).To(Succeed()) + for _, cont := range d.Spec.Template.Spec.Containers { + if cont.Name != "manager" { + continue + } + for _, arg := range cont.Args { + if v, ok := strings.CutPrefix(arg, "--pull-secret-namespaces="); ok { + return strings.Split(v, ",") + } + } + } + return nil +} + // waitForOperatorReady polls until the operator deployment has one available and up-to-date replica. func waitForOperatorReady( t *testing.T, From 1046667e7143e710b88933ea5ae18765325a3553 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Thu, 29 Oct 2026 16:49:57 +0000 Subject: [PATCH 09/14] e2e: Test pull secrets from other namespaces deploy-bink now creates an extra namespace and adds it to the release's pullSecretNamespaces, and passes its name to the tests. Configuring it at deploy time, rather than from the test, avoids restarting the controller during the run, so the new test can run in parallel. In TestPullSecretNamespaces, a pool referencing a secret in a namespace the test creates must be Degraded with SecretError and claim no node; switched to a secret in the allowed namespace, its node must update through the authenticated registry. The test is skipped if the operator does not allow the namespace, e.g. when it was deployed some other way. Assisted-by: AI Signed-off-by: Alice Frosi --- Makefile | 6 ++ test/e2e/pullsecret_namespace_test.go | 122 ++++++++++++++++++++++++++ 2 files changed, 128 insertions(+) create mode 100644 test/e2e/pullsecret_namespace_test.go diff --git a/Makefile b/Makefile index b8a04a84..f6a9f1a9 100644 --- a/Makefile +++ b/Makefile @@ -17,6 +17,8 @@ BINK_NODE_DISK_IMAGE ?= ghcr.io/bootc-dev/bink/node:v$(DEFAULT_KUBE_MINOR)-fedor BINK_LOCAL_REGISTRY_NODE_IMAGE ?= registry.cluster.local:5000/node E2E_REGISTRY_USER ?= e2e-user E2E_REGISTRY_PASSWORD ?= e2e-password +# Extra namespace deploy-bink allows pull secrets from (TestPullSecretNamespaces). +E2E_PULL_SECRET_NAMESPACE ?= bootc-e2e-pull-secrets # Released operator for upgrade testing (TestOperatorUpgrade): the published # chart and the release tag, whose version without the leading "v" is the # chart version. The released image is the one the chart deploys. Override @@ -125,6 +127,7 @@ e2e: ## Run e2e tests (requires: make deploy-bink). V=1 for verbose. RUN= E2E_NODE_IMAGE_UPDATE_DIGEST=$$(skopeo inspect --tls-verify=false docker://localhost:5000/node:update | jq -r '.Digest') \ E2E_NODE_IMAGE_UPDATE2_DIGEST=$$(skopeo inspect --tls-verify=false docker://localhost:5000/node:update2 | jq -r '.Digest') \ E2E_REGISTRY_USER=$(E2E_REGISTRY_USER) E2E_REGISTRY_PASSWORD=$(E2E_REGISTRY_PASSWORD) \ + E2E_PULL_SECRET_NAMESPACE=$(E2E_PULL_SECRET_NAMESPACE) \ $(if $(RELEASED_OPERATOR_TAG),E2E_OPERATOR_RELEASE_TAG=$(RELEASED_OPERATOR_TAG)) \ $(if $(RELEASED_OPERATOR_TAG),E2E_OPERATOR_RELEASED_IMG=$(IMG_BINK_RELEASED)) \ $(if $(RELEASED_OPERATOR_CHART),E2E_OPERATOR_RELEASED_CHART=$(RELEASED_OPERATOR_CHART)) \ @@ -267,6 +270,8 @@ start-bink: seed-node-image ## Start a bink cluster (idempotent). deploy-bink: start-bink build-update-image $(if $(RELEASED_OPERATOR_TAG),push-released-operator-image) helm ## Deploy to a bink cluster (requires: buildimg). podman push --tls-verify=false $(IMG) localhost:5000/bootc-operator-e2e:latest helm dependency build ./chart/bootc-operator + kubectl --kubeconfig $(KUBECONFIG_BINK) create namespace $(E2E_PULL_SECRET_NAMESPACE) \ + --dry-run=client -o yaml | kubectl --kubeconfig $(KUBECONFIG_BINK) apply -f - img="$(IMG_BINK)" && \ helm upgrade --install bootc-operator ./chart/bootc-operator \ --kubeconfig $(abspath $(KUBECONFIG_BINK)) \ @@ -274,6 +279,7 @@ deploy-bink: start-bink build-update-image $(if $(RELEASED_OPERATOR_TAG),push-re --set-string image.repository="$${img%:*}" \ --set-string image.tag="$${img##*:}" \ --set-string image.pullPolicy=Always \ + --set-json 'pullSecretNamespaces=["$(E2E_PULL_SECRET_NAMESPACE)"]' \ --set-json 'controller.extraArgs=["--allow-insecure-registry","--tag-resolution-interval=10s"]' kubectl --kubeconfig $(KUBECONFIG_BINK) -n bootc-operator rollout restart deployment/bootc-operator-controller kubectl --kubeconfig $(KUBECONFIG_BINK) -n bootc-operator rollout status deployment/bootc-operator-controller --timeout=3m diff --git a/test/e2e/pullsecret_namespace_test.go b/test/e2e/pullsecret_namespace_test.go new file mode 100644 index 00000000..eae7c901 --- /dev/null +++ b/test/e2e/pullsecret_namespace_test.go @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "context" + "encoding/base64" + "fmt" + "os" + "slices" + "testing" + "time" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" + "github.com/bootc-dev/bootc-operator/test/e2e/e2eutil" + testutil "github.com/bootc-dev/bootc-operator/test/util" +) + +// TestPullSecretNamespaces verifies that pools can only use pull secrets +// from the namespaces the operator is configured to read from. The +// operator must already allow E2E_PULL_SECRET_NAMESPACE (make deploy-bink +// does). A pool referencing a secret in a namespace the test creates must +// be Degraded without claiming its node; switched to a secret in the +// allowed namespace, its node must update through the authenticated +// registry. +func TestPullSecretNamespaces(t *testing.T) { + e2eutil.Providers(t, "bink", "eks") + t.Parallel() + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + + env := e2eutil.New(t) + if env.RegistryUser() == "" || env.RegistryPassword() == "" { + t.Skip("E2E_REGISTRY_USER / E2E_REGISTRY_PASSWORD not set") + } + allowedNS := os.Getenv("E2E_PULL_SECRET_NAMESPACE") + if allowedNS == "" { + t.Skip("E2E_PULL_SECRET_NAMESPACE not set") + } + + ctx := context.Background() + if !slices.Contains(controllerPullSecretNamespaces(t, g, ctx, env.Client), allowedNS) { + t.Skipf("operator does not allow pull secrets from %q", allowedNS) + } + + deniedNS := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: env.TestID() + "-denied"}} + g.Expect(env.Client.Create(ctx, deniedNS)).To(Succeed()) + t.Cleanup(func() { _ = env.Client.Delete(context.Background(), deniedNS) }) + + nodeName := env.AddNode(t) + + deniedSecret := registrySecret(env, env.TestID()+"-pull-secret", deniedNS.Name) + g.Expect(env.Client.Create(ctx, deniedSecret)).To(Succeed()) + allowedSecret := registrySecret(env, env.TestID()+"-pull-secret", allowedNS) + g.Expect(env.Client.Create(ctx, allowedSecret)).To(Succeed()) + t.Cleanup(func() { _ = env.Client.Delete(context.Background(), allowedSecret) }) + + authImageRef := env.AuthImageRef() + pool := env.NewPool("psns", authImageRef, + testutil.WithPullSecret(deniedSecret.Name, deniedSecret.Namespace), + ) + g.Expect(env.Client.Create(ctx, pool)).To(Succeed()) + + // Phase 1: the secret exists, but its namespace is not allowed. + g.Eventually(func() ([]metav1.Condition, error) { + var p bootcv1alpha1.BootcNodePool + err := env.Client.Get(ctx, client.ObjectKeyFromObject(pool), &p) + return p.Status.Conditions, err + }).Should(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolDegraded), + HaveField("Status", metav1.ConditionTrue), + HaveField("Reason", bootcv1alpha1.PoolSecretError), + HaveField("Message", ContainSubstring(fmt.Sprintf("%q", deniedNS.Name))), + ))) + g.Consistently(func() bool { + var bn bootcv1alpha1.BootcNode + err := env.Client.Get(ctx, client.ObjectKey{Name: nodeName}, &bn) + return apierrors.IsNotFound(err) + }).WithTimeout(10*time.Second).Should(BeTrue(), + "no BootcNode should be created for a pool with a denied secret") + + t.Logf("Pool %q is Degraded for secret in namespace %q", pool.Name, deniedNS.Name) + + // Phase 2: a secret in the allowed namespace is used to pull the image. + modified := pool.DeepCopy() + modified.Spec.PullSecretRef = &bootcv1alpha1.PullSecretRef{ + Name: allowedSecret.Name, Namespace: allowedSecret.Namespace, + } + g.Expect(env.Client.Patch(ctx, modified, client.MergeFrom(pool))).To(Succeed()) + *pool = *modified + + testutil.WaitForNodeIdle(t, g, ctx, env.Client, nodeName, 8*time.Minute, + HaveField("Booted", HaveField("Image", Equal(authImageRef))), + ) + g.Eventually(fetchPoolStatus(ctx, env.Client, pool)). + Should(poolAllUpdated(1, env.NodeImageUpdateDigest())) + + t.Logf("Node %q updated with a pull secret from namespace %q", nodeName, allowedNS) +} + +// registrySecret returns a dockerconfigjson Secret with the credentials of +// the authenticated e2e registry. +func registrySecret(env *e2eutil.Env, name, namespace string) *corev1.Secret { + auth := base64.StdEncoding.EncodeToString( + []byte(env.RegistryUser() + ":" + env.RegistryPassword()), + ) + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{ + corev1.DockerConfigJsonKey: fmt.Appendf(nil, + `{"auths":{%q:{"auth":%q}}}`, env.RegistryHost(), auth), + }, + } +} From c453ba18dc2e6eef3e1232d83ca398ce6114134d Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 30 Oct 2026 11:50:31 +0000 Subject: [PATCH 10/14] pullsecret: Add aggregated pull secret key The daemon will read pull secrets from a single Secret maintained by the controller and mounted into the DaemonSet, with one key per source secret. The new internal/pullsecret package holds the Secret name, the daemon mount directory and the key function, "_", so the controller and the daemon cannot drift. A namespace and a Secret name together can exceed the 253-character Secret key limit, and one invalid key would make the API server reject the whole aggregated Secret. Key returns an error in that case, and fetchPullSecretData reports it on the pool as SecretError. Assisted-by: AI Signed-off-by: Alice Frosi --- .../controller/bootcnodepool_controller.go | 11 +++- internal/controller/pullsecret_test.go | 34 ++++++++++ internal/pullsecret/pullsecret.go | 55 ++++++++++++++++ internal/pullsecret/pullsecret_test.go | 62 +++++++++++++++++++ 4 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 internal/pullsecret/pullsecret.go create mode 100644 internal/pullsecret/pullsecret_test.go diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index e23be865..064fe24b 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -37,6 +37,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/source" bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" + "github.com/bootc-dev/bootc-operator/internal/pullsecret" ) // drainStatus tracks an in-progress drain goroutine for a single node. @@ -707,7 +708,8 @@ func (r *BootcNodePoolReconciler) listAllBootcNodes( // fetchPullSecretData fetches the .dockerconfigjson data from the pull // secret referenced by the pool. Returns nil if no pull secret is set, and -// an error if the secret's namespace is not in PullSecretNamespaces. +// an error if the secret's namespace is not in PullSecretNamespaces or the +// reference cannot be stored in the aggregated pull secret. func (r *BootcNodePoolReconciler) fetchPullSecretData( ctx context.Context, pool *bootcv1alpha1.BootcNodePool, @@ -724,6 +726,13 @@ func (r *BootcNodePoolReconciler) fetchPullSecretData( ) } + if _, err := pullsecret.Key( + pool.Spec.PullSecretRef.Namespace, + pool.Spec.PullSecretRef.Name, + ); err != nil { + return nil, err + } + key := types.NamespacedName{ Name: pool.Spec.PullSecretRef.Name, Namespace: pool.Spec.PullSecretRef.Namespace, diff --git a/internal/controller/pullsecret_test.go b/internal/controller/pullsecret_test.go index c117ca08..70c4764d 100644 --- a/internal/controller/pullsecret_test.go +++ b/internal/controller/pullsecret_test.go @@ -4,6 +4,7 @@ package controller import ( "context" + "strings" "testing" . "github.com/onsi/gomega" @@ -240,6 +241,39 @@ func TestPullSecretNamespaceDeniedAndRecovery(t *testing.T) { })) } +func TestPullSecretNameTooLong(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + // A valid Secret name whose aggregated key "_" exceeds + // the 253-character Secret key limit. + secretName := strings.Repeat("a", 240) + secret := newDockerConfigSecret(secretName, testutil.OperatorNamespaceName, + []byte(`{"auths":{"long":{}}}`)) + g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) + + pool := testutil.NewPool("ps-too-long-pool", testImageDigestRefA, + testutil.WithWorkerSelector(), + testutil.WithPullSecret(secretName, testutil.OperatorNamespaceName), + ) + g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, pool) }) + + g.Eventually(func() ([]metav1.Condition, error) { + var p bootcv1alpha1.BootcNodePool + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(pool), &p) + return p.Status.Conditions, err + }).Should(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolDegraded), + HaveField("Status", metav1.ConditionTrue), + HaveField("Reason", bootcv1alpha1.PoolSecretError), + HaveField("Message", ContainSubstring("name too long")), + ))) +} + func TestParsePullSecretNamespaces(t *testing.T) { tests := []struct { name string diff --git a/internal/pullsecret/pullsecret.go b/internal/pullsecret/pullsecret.go new file mode 100644 index 00000000..a11af102 --- /dev/null +++ b/internal/pullsecret/pullsecret.go @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package pullsecret defines the aggregated pull secret shared by the +// controller, which writes it, and the daemon, which reads it from a +// volume mount. +package pullsecret + +import ( + "errors" + "fmt" + "strings" + + "k8s.io/apimachinery/pkg/util/validation" +) + +const ( + // SecretName is the name of the aggregated Secret in the operator + // namespace. + SecretName = "bootc-pull-secrets" //nolint:gosec // G101: a name, not a credential + + // MountDir is where the daemon mounts the aggregated Secret. + MountDir = "/var/run/bootc-pull-secrets" +) + +// ErrKeyTooLong is returned by Key when the key exceeds the Secret key length +// limit. +var ErrKeyTooLong = errors.New("name too long") + +// Key returns the aggregated Secret key for the source secret +// namespace/name. "_" cannot appear in namespace or Secret names, so the key +// is unambiguous. An error is returned if namespace or name is not a valid +// object name, since the daemon uses the key as a file name, or if the key +// exceeds the Secret key length limit, since a single invalid key makes the +// API server reject the whole aggregated Secret. +func Key(namespace, name string) (string, error) { + if errs := validation.IsDNS1123Label(namespace); len(errs) > 0 { + return "", fmt.Errorf("pull secret %s/%s: invalid namespace: %s", + namespace, name, strings.Join(errs, "; ")) + } + if errs := validation.IsDNS1123Subdomain(name); len(errs) > 0 { + return "", fmt.Errorf("pull secret %s/%s: invalid name: %s", + namespace, name, strings.Join(errs, "; ")) + } + key := namespace + "_" + name + if len(key) > validation.DNS1123SubdomainMaxLength { + return "", fmt.Errorf( + "pull secret %s/%s: %w (namespace and name must not exceed %d characters combined)", + namespace, + name, + ErrKeyTooLong, + validation.DNS1123SubdomainMaxLength-1, + ) + } + return key, nil +} diff --git a/internal/pullsecret/pullsecret_test.go b/internal/pullsecret/pullsecret_test.go new file mode 100644 index 00000000..56d0e3e0 --- /dev/null +++ b/internal/pullsecret/pullsecret_test.go @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pullsecret + +import ( + "strings" + "testing" + + . "github.com/onsi/gomega" +) + +func TestKey(t *testing.T) { + tests := []struct { + name string + namespace string + secret string + want string + wantErr bool + }{ + { + "namespace and name", + "bootc-operator", + "my-pull-secret", + "bootc-operator_my-pull-secret", + false, + }, + {"dotted name", "team-a", "registry.example.com", "team-a_registry.example.com", false}, + {"at limit", "ns", strings.Repeat("a", 250), "ns_" + strings.Repeat("a", 250), false}, + {"over limit", "ns", strings.Repeat("a", 251), "", true}, + {"long namespace and name", strings.Repeat("n", 63), strings.Repeat("a", 253), "", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + got, err := Key(tt.namespace, tt.secret) + if tt.wantErr { + g.Expect(err).To(MatchError(ErrKeyTooLong)) + return + } + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(got).To(Equal(tt.want)) + }) + } +} + +func TestKeyRejectsInvalidNames(t *testing.T) { + for _, tt := range []struct{ namespace, name string }{ + {"", "secret"}, + {"ns", ""}, + {"..", "secret"}, + {"ns", "../../etc/shadow"}, + {"ns", "a/b"}, + {"ns", "a_b"}, + {"Team-A", "secret"}, + } { + t.Run(tt.namespace+"/"+tt.name, func(t *testing.T) { + g := NewWithT(t) + _, err := Key(tt.namespace, tt.name) + g.Expect(err).To(MatchError(ContainSubstring("invalid"))) + }) + } +} From 5dfb9e9b05f4f7013fea0275ad4ddfdc3c07af75 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 30 Oct 2026 11:57:39 +0000 Subject: [PATCH 11/14] controller: Aggregate pull secrets into one Secret The daemon will stop reading pull secrets through the API and read them from a Secret mounted into the DaemonSet instead. The new PullSecretAggregator maintains that Secret, bootc-pull-secrets in the operator namespace, with one key per source secret referenced by a pool holding the secret's .dockerconfigjson. It is the Secret's only writer, so pool reconciles cannot conflict on it. Pool, source secret and bootc-pull-secrets events all map to one request, and each reconcile recomputes the data from scratch, so stale keys disappear and a deleted or edited Secret is repaired. References the pool reconciler reports as SecretError are skipped. The aggregator also reconciles once at startup, so the Secret exists whenever the controller runs; it is kept, with no keys, when no pool references a pull secret. The controller needs create on secrets in its namespace, and update and patch on bootc-pull-secrets only. It now requires the operator namespace to be known, since that is where the Secret lives. Assisted-by: AI Signed-off-by: Alice Frosi --- cmd/controller/main.go | 18 +- config/rbac/role.yaml | 10 + internal/controller/pullsecret_aggregator.go | 179 +++++++++++++++++ .../controller/pullsecret_aggregator_test.go | 190 ++++++++++++++++++ internal/controller/suite_test.go | 9 + 5 files changed, 401 insertions(+), 5 deletions(-) create mode 100644 internal/controller/pullsecret_aggregator.go create mode 100644 internal/controller/pullsecret_aggregator_test.go diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 6fb2971f..f00f6931 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -98,6 +98,10 @@ func main() { ) operatorNamespace := detectNamespace() + if operatorNamespace == "" { + setupLog.Error(nil, "Operator namespace unknown; set POD_NAMESPACE") + os.Exit(1) + } pullSecretNamespaces, err := controller.ParsePullSecretNamespaces( pullSecretNamespacesFlag, operatorNamespace, @@ -106,11 +110,6 @@ func main() { setupLog.Error(err, "Invalid --pull-secret-namespaces") os.Exit(1) } - if len(pullSecretNamespaces) == 0 { - setupLog.Error(nil, "No pull secret namespaces configured and operator namespace unknown; "+ - "set --pull-secret-namespaces or POD_NAMESPACE") - os.Exit(1) - } setupLog.Info("Pull secret namespaces", "namespaces", pullSecretNamespaces) mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ @@ -146,6 +145,15 @@ func main() { os.Exit(1) } + if err := (&controller.PullSecretAggregator{ + Client: mgr.GetClient(), + Namespace: operatorNamespace, + PullSecretNamespaces: pullSecretNamespaces, + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "Failed to create controller", "controller", "pullsecretaggregator") + os.Exit(1) + } + // +kubebuilder:scaffold:builder if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index a1e0f340..f6f17c53 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -86,6 +86,16 @@ rules: resources: - secrets verbs: + - create - get - list - watch +- apiGroups: + - "" + resourceNames: + - bootc-pull-secrets + resources: + - secrets + verbs: + - patch + - update diff --git a/internal/controller/pullsecret_aggregator.go b/internal/controller/pullsecret_aggregator.go new file mode 100644 index 00000000..d4a822fc --- /dev/null +++ b/internal/controller/pullsecret_aggregator.go @@ -0,0 +1,179 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "bytes" + "context" + "fmt" + "maps" + "slices" + "time" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + "sigs.k8s.io/controller-runtime/pkg/source" + + bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" + "github.com/bootc-dev/bootc-operator/internal/pullsecret" +) + +// PullSecretAggregator maintains the aggregated pull secret mounted into the +// daemon: one key per source secret referenced by a pool, holding that +// secret's .dockerconfigjson. It is the Secret's only writer and recomputes +// it from scratch on every reconcile. +type PullSecretAggregator struct { + client.Client + + // Namespace is the operator namespace, which holds the aggregated + // Secret. + Namespace string + + // PullSecretNamespaces is the allowlist of namespaces pools may + // reference pull secrets from. + PullSecretNamespaces []string +} + +// +kubebuilder:rbac:groups="",namespace=system,resources=secrets,verbs=get;list;watch;create +// +kubebuilder:rbac:groups="",namespace=system,resources=secrets,resourceNames=bootc-pull-secrets,verbs=update;patch + +// SetupWithManager sets up the aggregator with the Manager. All events map to +// the single aggregated Secret. +func (r *PullSecretAggregator) SetupWithManager(mgr ctrl.Manager) error { + enqueue := handler.EnqueueRequestsFromMapFunc( + func(context.Context, client.Object) []reconcile.Request { + return []reconcile.Request{{NamespacedName: r.secretKey()}} + }, + ) + // Reconcile once at startup, so the Secret exists even when no pool or + // source secret generates an event. + startup := make(chan event.GenericEvent, 1) + startup <- event.GenericEvent{Object: &corev1.Secret{}} + return ctrl.NewControllerManagedBy(mgr). + Named("pullsecretaggregator"). + WatchesRawSource(source.Channel(startup, enqueue)). + Watches(&bootcv1alpha1.BootcNodePool{}, enqueue, + builder.WithPredicates(predicate.GenerationChangedPredicate{})). + Watches(&corev1.Secret{}, enqueue, + builder.WithPredicates(r.secretPredicate())). + Complete(r) +} + +func (r *PullSecretAggregator) secretKey() types.NamespacedName { + return types.NamespacedName{Namespace: r.Namespace, Name: pullsecret.SecretName} +} + +// secretPredicate accepts source secrets (dockerconfigjson) and the +// aggregated Secret itself, so it is repaired if deleted or edited. +func (r *PullSecretAggregator) secretPredicate() predicate.Predicate { + return predicate.NewPredicateFuncs(func(obj client.Object) bool { + secret, ok := obj.(*corev1.Secret) + if !ok { + return false + } + return secret.Type == corev1.SecretTypeDockerConfigJson || + client.ObjectKeyFromObject(secret) == r.secretKey() + }) +} + +// Reconcile creates or updates the aggregated Secret to match the source +// secrets currently referenced by pools. +func (r *PullSecretAggregator) Reconcile( + ctx context.Context, + _ ctrl.Request, +) (ctrl.Result, error) { + log := logf.FromContext(ctx) + + desired, err := r.desiredData(ctx) + if err != nil { + return ctrl.Result{}, err + } + + var secret corev1.Secret + err = r.Get(ctx, r.secretKey(), &secret) + if apierrors.IsNotFound(err) { + secret = corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: pullsecret.SecretName, + Namespace: r.Namespace, + }, + Type: corev1.SecretTypeOpaque, + Data: desired, + } + if err := r.Create(ctx, &secret); err != nil { + return requeueIfStale(ctx, fmt.Errorf("creating aggregated pull secret: %w", err)) + } + log.Info("Created aggregated pull secret", "keys", len(desired)) + return ctrl.Result{}, nil + } + if err != nil { + return ctrl.Result{}, fmt.Errorf("fetching aggregated pull secret: %w", err) + } + + if maps.EqualFunc(secret.Data, desired, bytes.Equal) { + return ctrl.Result{}, nil + } + secret.Data = desired + if err := r.Update(ctx, &secret); err != nil { + return requeueIfStale(ctx, fmt.Errorf("updating aggregated pull secret: %w", err)) + } + log.Info("Updated aggregated pull secret", "keys", len(desired)) + return ctrl.Result{}, nil +} + +// requeueIfStale requeues without reporting an error when a write failed +// because the cache was behind (typically right after the aggregator's own +// write); the retry recomputes the Secret from scratch. +func requeueIfStale(ctx context.Context, err error) (ctrl.Result, error) { + if apierrors.IsConflict(err) || apierrors.IsAlreadyExists(err) { + logf.FromContext(ctx). + V(1). + Info("Aggregated pull secret changed, retrying", "error", err.Error()) + return ctrl.Result{RequeueAfter: time.Second}, nil + } + return ctrl.Result{}, err +} + +// desiredData returns the aggregated Secret data for all pools. References +// the pool reconciler reports as SecretError (namespace not allowed, name +// too long, secret missing or without .dockerconfigjson) are skipped. +func (r *PullSecretAggregator) desiredData(ctx context.Context) (map[string][]byte, error) { + var pools bootcv1alpha1.BootcNodePoolList + if err := r.List(ctx, &pools); err != nil { + return nil, fmt.Errorf("listing BootcNodePools: %w", err) + } + + desired := map[string][]byte{} + for i := range pools.Items { + ref := pools.Items[i].Spec.PullSecretRef + if ref == nil || !slices.Contains(r.PullSecretNamespaces, ref.Namespace) { + continue + } + key, err := pullsecret.Key(ref.Namespace, ref.Name) + if err != nil { + continue + } + var secret corev1.Secret + err = r.Get(ctx, types.NamespacedName{Namespace: ref.Namespace, Name: ref.Name}, &secret) + if apierrors.IsNotFound(err) { + continue + } + if err != nil { + return nil, fmt.Errorf("fetching pull secret %s/%s: %w", ref.Namespace, ref.Name, err) + } + if data, ok := secret.Data[corev1.DockerConfigJsonKey]; ok { + desired[key] = data + } + } + return desired, nil +} diff --git a/internal/controller/pullsecret_aggregator_test.go b/internal/controller/pullsecret_aggregator_test.go new file mode 100644 index 00000000..471337d2 --- /dev/null +++ b/internal/controller/pullsecret_aggregator_test.go @@ -0,0 +1,190 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "strings" + "testing" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" + "github.com/bootc-dev/bootc-operator/internal/pullsecret" + testutil "github.com/bootc-dev/bootc-operator/test/util" +) + +// aggregatedData returns the data of the aggregated pull secret. +func aggregatedData(ctx context.Context) (map[string][]byte, error) { + var secret corev1.Secret + err := k8sClient.Get(ctx, client.ObjectKey{ + Namespace: testutil.OperatorNamespaceName, + Name: pullsecret.SecretName, + }, &secret) + return secret.Data, err +} + +// newAggregatorPool returns a pool that references the given pull secret and +// selects no node, so only the aggregator acts on it. +func newAggregatorPool(name, secretName, secretNamespace string) *bootcv1alpha1.BootcNodePool { + return testutil.NewPool(name, testImageDigestRefA, + testutil.WithNodeSelector(map[string]string{"bootc.dev/aggregator-test": name}), + testutil.WithPullSecret(secretName, secretNamespace), + ) +} + +func TestPullSecretAggregatorAddsKey(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + data := []byte(`{"auths":{"add.example.com":{}}}`) + secret := newDockerConfigSecret("agg-add-secret", testutil.OperatorNamespaceName, data) + g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) + + pool := newAggregatorPool("agg-add-pool", secret.Name, secret.Namespace) + g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, pool) }) + + g.Eventually(aggregatedData).WithArguments(ctx). + Should(HaveKeyWithValue(testutil.OperatorNamespaceName+"_agg-add-secret", data)) +} + +func TestPullSecretAggregatorUpdatesAndRemovesKey(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + key := testutil.OperatorNamespaceName + "_agg-update-secret" + + secret := newDockerConfigSecret("agg-update-secret", testutil.OperatorNamespaceName, + []byte(`{"auths":{"old.example.com":{}}}`)) + g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) + + // Two pools share one source secret, and so one key. + pool1 := newAggregatorPool("agg-update-pool1", secret.Name, secret.Namespace) + g.Expect(k8sClient.Create(ctx, pool1)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, pool1) }) + pool2 := newAggregatorPool("agg-update-pool2", secret.Name, secret.Namespace) + g.Expect(k8sClient.Create(ctx, pool2)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, pool2) }) + + g.Eventually(aggregatedData).WithArguments(ctx).Should(HaveKey(key)) + + // Rotating the source secret updates the key. + rotated := []byte(`{"auths":{"new.example.com":{}}}`) + secret.Data[corev1.DockerConfigJsonKey] = rotated + g.Expect(k8sClient.Update(ctx, secret)).To(Succeed()) + g.Eventually(aggregatedData).WithArguments(ctx).Should(HaveKeyWithValue(key, rotated)) + + // The key stays while any pool references the secret. + g.Expect(k8sClient.Delete(ctx, pool1)).To(Succeed()) + g.Eventually(func() error { + return k8sClient.Get(ctx, client.ObjectKeyFromObject(pool1), &bootcv1alpha1.BootcNodePool{}) + }).Should(Satisfy(apierrors.IsNotFound)) + g.Consistently(aggregatedData, "2s", pollInterval).WithArguments(ctx).Should(HaveKey(key)) + + // Dropping the last reference removes the key; the Secret is kept. + g.Eventually(func() error { + var p bootcv1alpha1.BootcNodePool + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(pool2), &p); err != nil { + return err + } + p.Spec.PullSecretRef = nil + return k8sClient.Update(ctx, &p) + }).Should(Succeed()) + g.Eventually(aggregatedData).WithArguments(ctx).ShouldNot(HaveKey(key)) +} + +func TestPullSecretAggregatorSkipsInvalidReferences(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + longName := strings.Repeat("a", 240) + long := newDockerConfigSecret(longName, testutil.OperatorNamespaceName, []byte(`{}`)) + g.Expect(k8sClient.Create(ctx, long)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, long) }) + + noData := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "agg-nodata-secret", + Namespace: testutil.OperatorNamespaceName, + }, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{"other": []byte("x")}, + } + g.Expect(k8sClient.Create(ctx, noData)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, noData) }) + + valid := newDockerConfigSecret("agg-valid-secret", testutil.OperatorNamespaceName, []byte(`{}`)) + g.Expect(k8sClient.Create(ctx, valid)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, valid) }) + + for _, p := range []*bootcv1alpha1.BootcNodePool{ + // The namespace is not allowed, hence not cached: reading from it + // would fail the reconcile and block the valid key below. + newAggregatorPool("agg-skip-denied", "agg-denied-secret", "agg-denied-ns"), + newAggregatorPool("agg-skip-long", long.Name, long.Namespace), + newAggregatorPool("agg-skip-nodata", noData.Name, noData.Namespace), + newAggregatorPool("agg-skip-missing", "agg-missing-secret", testutil.OperatorNamespaceName), + newAggregatorPool("agg-skip-valid", valid.Name, valid.Namespace), + } { + g.Expect(k8sClient.Create(ctx, p)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, p) }) + } + + // Invalid references are skipped without blocking the valid one. + g.Eventually(aggregatedData).WithArguments(ctx). + Should(HaveKey(testutil.OperatorNamespaceName + "_agg-valid-secret")) + g.Expect(aggregatedData(ctx)).To(And( + Not(HaveKey("agg-denied-ns_agg-denied-secret")), + Not(HaveKey(testutil.OperatorNamespaceName+"_"+longName)), + Not(HaveKey(testutil.OperatorNamespaceName+"_agg-nodata-secret")), + Not(HaveKey(testutil.OperatorNamespaceName+"_agg-missing-secret")), + )) +} + +func TestPullSecretAggregatorRepairsSecret(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + key := testutil.OperatorNamespaceName + "_agg-repair-secret" + + data := []byte(`{"auths":{"repair.example.com":{}}}`) + secret := newDockerConfigSecret("agg-repair-secret", testutil.OperatorNamespaceName, data) + g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) + + pool := newAggregatorPool("agg-repair-pool", secret.Name, secret.Namespace) + g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, pool) }) + + g.Eventually(aggregatedData).WithArguments(ctx).Should(HaveKeyWithValue(key, data)) + + // A manual edit is reverted. + var aggregated corev1.Secret + g.Expect(k8sClient.Get(ctx, client.ObjectKey{ + Namespace: testutil.OperatorNamespaceName, Name: pullsecret.SecretName, + }, &aggregated)).To(Succeed()) + aggregated.Data[key] = []byte("tampered") + aggregated.Data["stray"] = []byte("x") + g.Expect(k8sClient.Update(ctx, &aggregated)).To(Succeed()) + g.Eventually(aggregatedData).WithArguments(ctx).Should(And( + HaveKeyWithValue(key, data), + Not(HaveKey("stray")), + )) + + // A deleted Secret is recreated. + g.Expect(k8sClient.Delete(ctx, &aggregated)).To(Succeed()) + g.Eventually(aggregatedData).WithArguments(ctx).Should(HaveKeyWithValue(key, data)) +} diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 4dd5ec35..f98878d0 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -100,6 +100,15 @@ func TestMain(m *testing.M) { os.Exit(1) } + if err := (&PullSecretAggregator{ + Client: mgr.GetClient(), + Namespace: testutil.OperatorNamespaceName, + PullSecretNamespaces: pullSecretNamespaces, + }).SetupWithManager(mgr); err != nil { + fmt.Fprintf(os.Stderr, "Failed to setup pull secret aggregator: %v\n", err) + os.Exit(1) + } + mgrCtx, mgrCancel := context.WithCancel(context.Background()) mgrDone := make(chan struct{}) From de8d7ad392570149969de2145abe680edeaf592e Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 30 Oct 2026 12:04:39 +0000 Subject: [PATCH 12/14] daemon: Read the pull secret from the mounted Secret The DaemonSet now mounts bootc-pull-secrets, maintained by the controller, at /var/run/bootc-pull-secrets. The volume is optional, as the daemon may start before the Secret exists, and uses no subPath, so the kubelet keeps it up to date. While an update is pending, before staging, syncPullSecret copies the entry for the node's pullSecretRef to /run/ostree/auth.json instead of fetching the Secret through the API. Without a reference or a readable entry, it now removes the auth file instead of leaving stale credentials behind, and bootc falls back to /etc or /usr/lib. A missing entry needs no special handling: the kubelet may take a minute to sync a new or rotated Secret, and a failed stage is retried with backoff and re-reads the file. The daemon tests now use temporary directories for the host root and the mount, so they never touch the real /run/ostree. Assisted-by: AI --- cmd/daemon/main.go | 2 + config/daemon/daemon.yaml | 11 +++ internal/daemon/pullsecret_test.go | 151 +++++++++++++++++++++++++++++ internal/daemon/reconciler.go | 89 +++++++++++------ internal/daemon/reconciler_test.go | 20 ++-- internal/daemon/suite_test.go | 41 ++++++-- 6 files changed, 269 insertions(+), 45 deletions(-) create mode 100644 internal/daemon/pullsecret_test.go diff --git a/cmd/daemon/main.go b/cmd/daemon/main.go index 829142f4..9a381db4 100644 --- a/cmd/daemon/main.go +++ b/cmd/daemon/main.go @@ -21,6 +21,7 @@ import ( bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" "github.com/bootc-dev/bootc-operator/internal/bootc" "github.com/bootc-dev/bootc-operator/internal/daemon" + "github.com/bootc-dev/bootc-operator/internal/pullsecret" "github.com/bootc-dev/bootc-operator/internal/version" ) @@ -106,6 +107,7 @@ func main() { Scheme: mgr.GetScheme(), NodeName: nodeName, HostRoot: "/proc/1/root", + PullSecretDir: pullsecret.MountDir, Executor: executor, StatusWatcher: watcher, APIReader: mgr.GetAPIReader(), diff --git a/config/daemon/daemon.yaml b/config/daemon/daemon.yaml index 7b8c8bc9..f701fdc3 100644 --- a/config/daemon/daemon.yaml +++ b/config/daemon/daemon.yaml @@ -42,6 +42,17 @@ spec: requests: cpu: 10m memory: 64Mi + volumeMounts: + # No subPath: subPath mounts do not receive Secret updates. + - name: pull-secrets + mountPath: /var/run/bootc-pull-secrets + readOnly: true + volumes: + - name: pull-secrets + secret: + secretName: bootc-pull-secrets + # The daemon must start before the controller creates the Secret. + optional: true tolerations: - operator: Exists serviceAccountName: daemon diff --git a/internal/daemon/pullsecret_test.go b/internal/daemon/pullsecret_test.go new file mode 100644 index 00000000..43bb25fc --- /dev/null +++ b/internal/daemon/pullsecret_test.go @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: Apache-2.0 + +package daemon + +import ( + "context" + "errors" + "os" + "path/filepath" + "slices" + "sync" + "testing" + + . "github.com/onsi/gomega" + + testutil "github.com/bootc-dev/bootc-operator/test/util" +) + +// authAtStage records the content of /run/ostree/auth.json under the test +// host root each time the fake executor stages, or "" if missing. +type authAtStage struct { + mu sync.Mutex + seen []string +} + +func recordAuthAtStage(fake *fakeExecutor) *authAtStage { + a := &authAtStage{} + fake.setStageHook(func() { + content := "" + if data, err := os.ReadFile(testAuthFile()); err == nil { + content = string(data) + } + a.mu.Lock() + defer a.mu.Unlock() + a.seen = append(a.seen, content) + }) + return a +} + +func (a *authAtStage) get() []string { + a.mu.Lock() + defer a.mu.Unlock() + return slices.Clone(a.seen) +} + +func testAuthFile() string { + return filepath.Join(testHostRoot, "run", "ostree", "auth.json") +} + +func writeMountedPullSecret(t *testing.T, name, namespace, content string) { + t.Helper() + path := filepath.Join(testPullSecretDir, namespace+"_"+name) + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } +} + +func TestStageUsesMountedPullSecret(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + fake := newTestEnv(t) + fake.status = newBootcStatus(testutil.DigestA) + auth := recordAuthAtStage(fake) + + dockerConfig := `{"auths":{"registry.example.com":{}}}` + writeMountedPullSecret(t, "my-secret", "my-ns", dockerConfig) + + bn := testutil.NewNode(testNodeName, testutil.ImageDigestRefB, + testutil.WithNodePullSecret("my-secret", "my-ns")) + g.Expect(k8sClient.Create(ctx, bn)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, bn) }) + + g.Eventually(auth.get).Should(Equal([]string{dockerConfig})) +} + +func writeStaleAuthFile(t *testing.T) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(testAuthFile()), 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(testAuthFile(), []byte("stale"), 0o600); err != nil { + t.Fatal(err) + } +} + +func TestStageRemovesAuthFileWithoutMountedEntry(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + fake := newTestEnv(t) + fake.status = newBootcStatus(testutil.DigestA) + auth := recordAuthAtStage(fake) + writeStaleAuthFile(t) + + // The kubelet has not synced the entry yet, or it was never written. + bn := testutil.NewNode(testNodeName, testutil.ImageDigestRefB, + testutil.WithNodePullSecret("not-mounted", "my-ns")) + g.Expect(k8sClient.Create(ctx, bn)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, bn) }) + + g.Eventually(auth.get).Should(Equal([]string{""})) +} + +func TestStageRemovesAuthFileWithoutPullSecretRef(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + fake := newTestEnv(t) + fake.status = newBootcStatus(testutil.DigestA) + auth := recordAuthAtStage(fake) + writeStaleAuthFile(t) + + bn := testutil.NewNode(testNodeName, testutil.ImageDigestRefB) + g.Expect(k8sClient.Create(ctx, bn)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, bn) }) + + g.Eventually(auth.get).Should(Equal([]string{""})) +} + +func TestStageRetryUsesRotatedPullSecret(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + fake := newTestEnv(t) + fake.status = newBootcStatus(testutil.DigestA) + fake.setStageErr(errors.New("unauthorized")) + auth := recordAuthAtStage(fake) + + writeMountedPullSecret(t, "my-secret", "my-ns", "old") + + bn := testutil.NewNode(testNodeName, testutil.ImageDigestRefB, + testutil.WithNodePullSecret("my-secret", "my-ns")) + g.Expect(k8sClient.Create(ctx, bn)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, bn) }) + + g.Eventually(auth.get).Should(Equal([]string{"old"})) + + // The kubelet syncs the rotated secret; the next attempt uses it. + writeMountedPullSecret(t, "my-secret", "my-ns", "new") + fake.setStageErr(nil) + g.Eventually(auth.get).Should(Equal([]string{"old", "new"})) +} diff --git a/internal/daemon/reconciler.go b/internal/daemon/reconciler.go index 7a873336..b632f568 100644 --- a/internal/daemon/reconciler.go +++ b/internal/daemon/reconciler.go @@ -6,7 +6,9 @@ package daemon import ( "context" + "errors" "fmt" + "io/fs" "os" "path/filepath" "reflect" @@ -16,12 +18,10 @@ import ( "github.com/distribution/reference" "github.com/go-logr/logr" - corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -34,6 +34,7 @@ import ( bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" "github.com/bootc-dev/bootc-operator/internal/bootc" "github.com/bootc-dev/bootc-operator/internal/image" + "github.com/bootc-dev/bootc-operator/internal/pullsecret" ) const ( @@ -60,9 +61,11 @@ type stageOp struct { // mismatches, and drives updates via bootc stage. type BootcNodeReconciler struct { client.Client - Scheme *runtime.Scheme - NodeName string - HostRoot string + Scheme *runtime.Scheme + NodeName string + HostRoot string + // PullSecretDir is where the aggregated pull secret is mounted. + PullSecretDir string Executor bootc.Executor StatusWatcher *StatusWatcher APIReader client.Reader @@ -222,7 +225,7 @@ func (r *BootcNodeReconciler) reconcileBootcNode( }, nil } - r.syncPullSecret(ctx, log, bn) + r.syncPullSecret(log, bn) desiredImage := desiredRef.String() @@ -409,47 +412,75 @@ func (r *BootcNodeReconciler) classifyAction( return actionReboot } -// syncPullSecret fetches the pull secret referenced by the BootcNode -// and writes it to the host filesystem so bootc can authenticate. +// syncPullSecret makes the host's auth file mirror the mounted pull secret +// entry for the BootcNode's pullSecretRef, so bootc authenticates with it. +// Without a reference or a readable mounted entry, the auth file is removed +// and bootc falls back to credentials in /etc or /usr/lib. A missing entry is +// not an error: the kubelet may not have synced it yet, and a failed stage is +// retried with backoff. func (r *BootcNodeReconciler) syncPullSecret( - ctx context.Context, log logr.Logger, bn *bootcv1alpha1.BootcNode, ) { - if bn.Spec.PullSecretRef == nil { - return - } - - key := types.NamespacedName{ - Name: bn.Spec.PullSecretRef.Name, - Namespace: bn.Spec.PullSecretRef.Namespace, - } - var secret corev1.Secret - if err := r.APIReader.Get(ctx, key, &secret); err != nil { - log.Error(err, "Failed to fetch pull secret, continuing", "secret", key) - return + data, err := r.mountedPullSecret(bn.Spec.PullSecretRef) + if err != nil { + log.Error(err, "Failed to read mounted pull secret, staging without it") } - data, ok := secret.Data[corev1.DockerConfigJsonKey] - if !ok { - log.Info("Pull secret missing .dockerconfigjson key, continuing", "secret", key) + if data == nil { + if err := r.removeAuthFile(); err != nil { + log.Error(err, "Failed to remove auth file, continuing") + } return } - if err := r.writeAuthFile(data); err != nil { log.Error(err, "Failed to write auth file, continuing") } } -// writeAuthFile writes dockerconfigjson data to the host filesystem -// atomically (temp file + rename). -func (r *BootcNodeReconciler) writeAuthFile(data []byte) error { +// mountedPullSecret returns the mounted pull secret entry for ref, or nil if +// ref is nil or the entry does not exist. +func (r *BootcNodeReconciler) mountedPullSecret(ref *bootcv1alpha1.PullSecretRef) ([]byte, error) { + if ref == nil { + return nil, nil + } + key, err := pullsecret.Key(ref.Namespace, ref.Name) + if err != nil { + return nil, err + } + // Key validates the names, so key cannot contain "/" or be "..". + data, err := os.ReadFile(filepath.Join(r.PullSecretDir, key)) //nolint:gosec // G304 + if errors.Is(err, fs.ErrNotExist) { + return nil, nil + } + if err != nil { + return nil, err + } + return data, nil +} + +// authDir returns the host directory holding bootc's runtime auth file. +func (r *BootcNodeReconciler) authDir() string { hostRoot := r.HostRoot if hostRoot == "" { hostRoot = "/proc/1/root" } + return filepath.Join(hostRoot, "run", "ostree") +} - dir := filepath.Join(hostRoot, "run", "ostree") +// removeAuthFile removes the host's auth file if present. +func (r *BootcNodeReconciler) removeAuthFile() error { + err := os.Remove(filepath.Join(r.authDir(), "auth.json")) + if errors.Is(err, fs.ErrNotExist) { + return nil + } + return err +} + +// writeAuthFile writes dockerconfigjson data to the host filesystem +// atomically (temp file + rename). +func (r *BootcNodeReconciler) writeAuthFile(data []byte) error { + dir := r.authDir() // Standard mode for a host system directory under /run; it only // applies if the host has not created the directory already. if err := os.MkdirAll(dir, 0755); err != nil { //nolint:gosec // G301 diff --git a/internal/daemon/reconciler_test.go b/internal/daemon/reconciler_test.go index 3ea02aec..55932e32 100644 --- a/internal/daemon/reconciler_test.go +++ b/internal/daemon/reconciler_test.go @@ -32,7 +32,7 @@ func TestReconcilePopulatesStatus(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) v1 := "v1" v2 := "v2" @@ -113,7 +113,7 @@ func TestReconcileBootcStatusError(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) fake.setStatusErr(errors.New(bootcStatusErrMsg)) bn := testutil.NewNode(testNodeName, testutil.ImageDigestRefA) @@ -145,7 +145,7 @@ func TestStagingTriggered(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) fake.status = newBootcStatus(testutil.DigestA) bn := testutil.NewNode(testNodeName, testutil.ImageDigestRefB) @@ -186,7 +186,7 @@ func TestStagingError(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) fake.status = newBootcStatus(testutil.DigestA) fake.setStageErr(errors.New(stageErrMsg)) @@ -221,7 +221,7 @@ func TestAlreadyStaged(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) fake.status = newBootcStatus(testutil.DigestA) fake.status.Status.Staged = newBootEntry(testutil.ImageDigestRefB, testutil.DigestB) @@ -250,7 +250,7 @@ func TestRebootingSet(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) fake.status = newBootcStatus(testutil.DigestA) fake.status.Status.Staged = newBootEntry(testutil.ImageDigestRefB, testutil.DigestB) @@ -283,7 +283,7 @@ func TestSoftReboot(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) fake.status = newBootcStatus(testutil.DigestA) fake.status.Status.Staged = newBootEntry(testutil.ImageDigestRefB, testutil.DigestB) @@ -319,7 +319,7 @@ func TestRebootOnlyPolicy(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) fake.status = newBootcStatus(testutil.DigestA) fake.status.Status.Staged = newBootEntry(testutil.ImageDigestRefB, testutil.DigestB) @@ -354,7 +354,7 @@ func TestRollback(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) fake.status = newBootcStatus(testutil.DigestA) fake.status.Status.Staged = newBootEntry(testutil.ImageDigestRefB, testutil.DigestB) @@ -389,7 +389,7 @@ func TestCancelInflightStage(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) fake.status = newBootcStatus(testutil.DigestA) firstBlock := make(chan struct{}) diff --git a/internal/daemon/suite_test.go b/internal/daemon/suite_test.go index a8393c93..b249c44b 100644 --- a/internal/daemon/suite_test.go +++ b/internal/daemon/suite_test.go @@ -26,6 +26,11 @@ var ( testEnv *envtest.Environment k8sClient client.Client testReconciler *BootcNodeReconciler + + // testHostRoot and testPullSecretDir stand in for the host filesystem + // and the pull secret mount, so tests never touch the real host. + testHostRoot string + testPullSecretDir string ) func TestMain(m *testing.M) { @@ -64,12 +69,22 @@ func TestMain(m *testing.M) { os.Exit(1) } + tmpDir, err := os.MkdirTemp("", "bootc-daemon-test-") + if err != nil { + fmt.Fprintf(os.Stderr, "Failed to create temp dir: %v\n", err) + os.Exit(1) + } + testHostRoot = filepath.Join(tmpDir, "host") + testPullSecretDir = filepath.Join(tmpDir, "pull-secrets") + fake := &fakeExecutor{} testReconciler = &BootcNodeReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - NodeName: testNodeName, - Executor: fake, + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + NodeName: testNodeName, + HostRoot: testHostRoot, + PullSecretDir: testPullSecretDir, + Executor: fake, StatusWatcher: &StatusWatcher{ Events: make(chan event.GenericEvent, 1), NodeName: testNodeName, @@ -104,13 +119,27 @@ func TestMain(m *testing.M) { if err := testEnv.Stop(); err != nil { fmt.Fprintf(os.Stderr, "Failed to stop envtest: %v\n", err) } + if err := os.RemoveAll(tmpDir); err != nil { + fmt.Fprintf(os.Stderr, "Failed to remove temp dir: %v\n", err) + } os.Exit(code) } // newTestEnv creates a fresh fakeExecutor and StatusWatcher for a test, -// wiring them into the shared reconciler. Each test gets its own cache. -func newTestEnv() *fakeExecutor { +// wiring them into the shared reconciler. Each test gets its own cache and +// an empty host root and pull secret mount. +func newTestEnv(t *testing.T) *fakeExecutor { + t.Helper() + for _, dir := range []string{testHostRoot, testPullSecretDir} { + if err := os.RemoveAll(dir); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatal(err) + } + } + fake := &fakeExecutor{} testReconciler.Executor = fake testReconciler.rebootIssued = false From a769d5cc959e81427e0208624c5383ce24cf2eca Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 30 Oct 2026 12:07:26 +0000 Subject: [PATCH 13/14] rbac: Drop the daemon's access to secrets The daemon reads pull secrets from the mounted bootc-pull-secrets Secret, so it no longer needs to get Secrets through the API. Remove the rule from its ClusterRole, which allowed every node to read any Secret in the cluster, and the now unused APIReader. The kubelet still reads the mounted Secret: the node authorizer allows a node to read Secrets used by pods scheduled on it. make helm-test now checks that no ClusterRole, the daemon's included, grants access to secrets. Assisted-by: AI Signed-off-by: Alice Frosi --- cmd/daemon/main.go | 1 - config/rbac/daemon_role.yaml | 6 ------ hack/test-helm.sh | 5 ++--- internal/daemon/reconciler.go | 1 - 4 files changed, 2 insertions(+), 11 deletions(-) diff --git a/cmd/daemon/main.go b/cmd/daemon/main.go index 9a381db4..6d39660d 100644 --- a/cmd/daemon/main.go +++ b/cmd/daemon/main.go @@ -110,7 +110,6 @@ func main() { PullSecretDir: pullsecret.MountDir, Executor: executor, StatusWatcher: watcher, - APIReader: mgr.GetAPIReader(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "Failed to create controller", "controller", "bootcnode") os.Exit(1) diff --git a/config/rbac/daemon_role.yaml b/config/rbac/daemon_role.yaml index f026a8eb..7e87c360 100644 --- a/config/rbac/daemon_role.yaml +++ b/config/rbac/daemon_role.yaml @@ -6,12 +6,6 @@ metadata: app.kubernetes.io/managed-by: kustomize name: daemon-role rules: -- apiGroups: - - "" - resources: - - secrets - verbs: - - get - apiGroups: - node.bootc.dev resources: diff --git a/hack/test-helm.sh b/hack/test-helm.sh index c77d6e87..c436eb69 100755 --- a/hack/test-helm.sh +++ b/hack/test-helm.sh @@ -22,7 +22,7 @@ render() { # check MANIFESTS FLAG_VALUE NAMESPACES... # Asserts that the controller flag is FLAG_VALUE, that each namespace has # exactly one secret-reading Role bound to the controller ServiceAccount, -# and that the controller ClusterRole does not grant access to secrets. +# and that no ClusterRole (controller or daemon) grants access to secrets. check() { local manifests="$1" flag="$2"; shift 2 local want got @@ -45,9 +45,8 @@ check() { [ "$got" = "$roles" ] || fail "controller RoleBindings [$got], want [$roles]" got=$(echo "$manifests" | "$YQ" -N 'select(.kind == "ClusterRole") | - select(.metadata.name == "'"$SA"'") | select(.rules | any_c(.resources | any_c(. == "secrets"))) | .metadata.name') - [ -z "$got" ] || fail "controller ClusterRole grants secrets access" + [ -z "$got" ] || fail "ClusterRoles grant secrets access: [$got]" } "$HELM" dependency build "$CHART_DIR" >/dev/null diff --git a/internal/daemon/reconciler.go b/internal/daemon/reconciler.go index b632f568..e0338d1a 100644 --- a/internal/daemon/reconciler.go +++ b/internal/daemon/reconciler.go @@ -68,7 +68,6 @@ type BootcNodeReconciler struct { PullSecretDir string Executor bootc.Executor StatusWatcher *StatusWatcher - APIReader client.Reader inflight stageOp stageDone chan event.GenericEvent From 7c63f4b3d50f4371954eeaa06e36b49df519d62e Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 30 Oct 2026 12:09:00 +0000 Subject: [PATCH 14/14] docs: Explain how to remove the aggregated pull secret The controller now creates bootc-pull-secrets in the release namespace. Helm does not know about it, and helm uninstall keeps the namespace created by --create-namespace, so the Secret is left behind. Document deleting the namespace to remove it. Assisted-by: AI Signed-off-by: Alice Frosi --- docs/src/helm.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/src/helm.md b/docs/src/helm.md index 91a73e49..141bb8c1 100644 --- a/docs/src/helm.md +++ b/docs/src/helm.md @@ -121,6 +121,15 @@ helm uninstall bootc-operator --namespace bootc-operator > **Warning**: uninstalling the chart also removes the CRDs and all > their instances cluster-wide. +The controller creates the `bootc-pull-secrets` Secret in the release +namespace, which Helm does not track, and `helm uninstall` does not +delete the namespace created by `--create-namespace`. Delete the namespace +to remove that Secret, along with any pull secrets created there: + +```bash +kubectl delete namespace bootc-operator +``` + ## Regenerating the chart Most Helm templates are generated from the kustomize manifests. After