From e5bd25da1d4e145f9e567276b8c790bcf3cb5da4 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 6 Nov 2026 09:05:46 +0000 Subject: [PATCH 1/8] registry: Normalize registry keys when looking up credentials The tag resolver looked up credentials with an exact match of the registry host in the auths map. Entries written by kubectl create secret docker-registry and docker login use other forms, most commonly https://index.docker.io/v1/ for Docker Hub. Those missed, the resolver silently fell back to anonymous access and tag resolution failed with 401, while the node's container runtime accepted the same secret. Compare keys the way containers/image does for auth files: strip the http(s):// scheme, drop the path when a scheme was present, and map docker.io and registry-1.docker.io to index.docker.io. An exact match still wins, and normalized keys are tried in sorted order so the result is deterministic. Scoped keys without a scheme, such as quay.io/org, are still not matched; the resolver looks up registries only. Assisted-by: AI Signed-off-by: Alice Frosi --- internal/registry/resolver.go | 43 ++++++++++- internal/registry/resolver_test.go | 110 +++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 1 deletion(-) diff --git a/internal/registry/resolver.go b/internal/registry/resolver.go index 0c8631ee..a7599b5a 100644 --- a/internal/registry/resolver.go +++ b/internal/registry/resolver.go @@ -8,6 +8,8 @@ import ( "errors" "fmt" "net/http" + "slices" + "strings" "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/name" @@ -60,13 +62,52 @@ type dockerConfigJSON struct { Auths map[string]authn.AuthConfig `json:"auths"` } +// Resolve returns the credentials for the target's registry. An exact key +// match wins; otherwise keys are compared after normalization, so that +// entries such as "https://index.docker.io/v1/", which kubectl and docker +// write, match the registry host like they do for the node's container +// runtime. func (k *dockerConfigJSON) Resolve(target authn.Resource) (authn.Authenticator, error) { - if cfg, ok := k.Auths[target.RegistryStr()]; ok { + registry := target.RegistryStr() + if cfg, ok := k.Auths[registry]; ok { return authn.FromConfig(cfg), nil } + want := normalizeRegistry(registry) + keys := make([]string, 0, len(k.Auths)) + for key := range k.Auths { + keys = append(keys, key) + } + slices.Sort(keys) + for _, key := range keys { + if normalizeAuthKey(key) == want { + return authn.FromConfig(k.Auths[key]), nil + } + } return authn.Anonymous, nil } +// normalizeAuthKey reduces an auths key to a registry host the way +// containers/image does for auth files: the scheme is stripped and, when +// there was one, so is the path. +func normalizeAuthKey(key string) string { + stripped := strings.TrimPrefix(key, "http://") + stripped = strings.TrimPrefix(stripped, "https://") + if stripped != key { + stripped, _, _ = strings.Cut(stripped, "/") + } + return normalizeRegistry(stripped) +} + +// normalizeRegistry maps the Docker Hub host names to the one +// go-containerregistry uses. +func normalizeRegistry(registry string) string { + switch registry { + case "docker.io", "registry-1.docker.io": + return name.DefaultRegistry + } + return registry +} + func keychainFromDockerConfig(data []byte) (authn.Keychain, error) { var cfg dockerConfigJSON if err := json.Unmarshal(data, &cfg); err != nil { diff --git a/internal/registry/resolver_test.go b/internal/registry/resolver_test.go index a060e403..8042e818 100644 --- a/internal/registry/resolver_test.go +++ b/internal/registry/resolver_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "testing" + "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/name" "github.com/google/go-containerregistry/pkg/registry" "github.com/google/go-containerregistry/pkg/v1/empty" @@ -74,3 +75,112 @@ func TestResolveUnreachableRegistry(t *testing.T) { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring("fetching manifest")) } + +func TestDockerConfigResolve(t *testing.T) { + tests := []struct { + name string + auths []string + ref string + wantUser string + }{ + {"exact host", []string{"quay.io"}, "quay.io/org/img:v1", "quay.io"}, + { + "host with port", + []string{"reg.example.com:5000"}, + "reg.example.com:5000/img", + "reg.example.com:5000", + }, + {"https scheme", []string{"https://quay.io"}, "quay.io/org/img", "https://quay.io"}, + {"http scheme", []string{"http://quay.io"}, "quay.io/org/img", "http://quay.io"}, + { + "scheme and path", + []string{"https://quay.io/v2/"}, + "quay.io/org/img", + "https://quay.io/v2/", + }, + { + "hub v1 url", + []string{"https://index.docker.io/v1/"}, + "docker.io/library/busybox", + "https://index.docker.io/v1/", + }, + { + "hub v1 url, implicit registry", + []string{"https://index.docker.io/v1/"}, + "busybox", + "https://index.docker.io/v1/", + }, + { + "hub v1 url, registry-1 ref", + []string{"https://index.docker.io/v1/"}, + "registry-1.docker.io/library/busybox", + "https://index.docker.io/v1/", + }, + {"hub docker.io key", []string{"docker.io"}, "busybox", "docker.io"}, + {"hub registry-1 key", []string{"registry-1.docker.io"}, "busybox", "registry-1.docker.io"}, + { + "hub registry-1 url", + []string{"https://registry-1.docker.io/v2/"}, + "busybox", + "https://registry-1.docker.io/v2/", + }, + { + "exact wins over normalized", + []string{"https://quay.io/v2/", "quay.io"}, + "quay.io/img", + "quay.io", + }, + { + "normalized ties go to the first sorted key", + []string{"http://quay.io", "https://quay.io/a"}, + "quay.io/img", + "http://quay.io", + }, + {"other registry", []string{"https://index.docker.io/v1/"}, "quay.io/org/img", ""}, + {"path without scheme is not a host", []string{"quay.io/org"}, "quay.io/org/img", ""}, + {"no entries", nil, "quay.io/org/img", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + cfg := &dockerConfigJSON{Auths: map[string]authn.AuthConfig{}} + for _, key := range tt.auths { + cfg.Auths[key] = authn.AuthConfig{Username: key, Password: "pw-" + key} + } + ref, err := name.ParseReference(tt.ref) + g.Expect(err).NotTo(HaveOccurred()) + + a, err := cfg.Resolve(ref.Context()) + g.Expect(err).NotTo(HaveOccurred()) + got, err := a.Authorization() + g.Expect(err).NotTo(HaveOccurred()) + if tt.wantUser == "" { + g.Expect(*got).To(Equal(authn.AuthConfig{})) + return + } + g.Expect(*got).To(Equal(authn.AuthConfig{ + Username: tt.wantUser, + Password: "pw-" + tt.wantUser, + })) + }) + } +} + +func TestKeychainFromDockerConfigHubURL(t *testing.T) { + g := NewWithT(t) + + // The entry kubectl create secret docker-registry writes by default. + kc, err := keychainFromDockerConfig([]byte( + `{"auths":{"https://index.docker.io/v1/":{"username":"u","password":"p"}}}`)) + g.Expect(err).NotTo(HaveOccurred()) + ref, err := name.ParseReference("docker.io/library/busybox") + g.Expect(err).NotTo(HaveOccurred()) + + a, err := kc.Resolve(ref.Context()) + g.Expect(err).NotTo(HaveOccurred()) + got, err := a.Authorization() + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(got.Username).To(Equal("u")) + g.Expect(got.Password).To(Equal("p")) +} From 23ea0541e11a810eebfc5dabbb80c08578d9ee6a Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 30 Oct 2026 11:50:31 +0000 Subject: [PATCH 2/8] 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 3a0ff86a..7b95db41 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -39,6 +39,7 @@ import ( bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" "github.com/bootc-dev/bootc-operator/internal/karpenter" + "github.com/bootc-dev/bootc-operator/internal/pullsecret" ) // drainStatus tracks an in-progress drain goroutine for a single node. @@ -828,7 +829,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, @@ -845,6 +847,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 761470e5..36bff9fd 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" @@ -251,6 +252,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("key 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..bb416725 --- /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" +) + +// ErrKeyNameTooLong is returned by Key when the key name exceeds the Secret key +// length limit. +var ErrKeyNameTooLong = errors.New("key 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, + ErrKeyNameTooLong, + 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..5bdc8354 --- /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(ErrKeyNameTooLong)) + 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 4399f6caa6d21850f6bbb8ff7853aad1d81f5f07 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 6 Nov 2026 09:07:33 +0000 Subject: [PATCH 3/8] pullsecret: Normalize legacy .dockercfg pull secrets The pool reconciler required a .dockerconfigjson key and passed its bytes unchanged to the tag resolver and, via the aggregated Secret, to bootc. Both expect the {"auths": ...} wrapper. A legacy .dockercfg Secret, which holds the bare auths map and which OpenShift generates for service accounts, was rejected as missing a key; had it been passed through, it would have meant silently no credentials. Add pullsecret.AuthData, which returns the credentials in .dockerconfigjson format. It goes by the Secret's keys rather than its type, so an Opaque Secret works too. .dockerconfigjson is returned as is and wins when both keys are set. .dockercfg is wrapped in {"auths": ...} without re-encoding its entries, so fields such as identitytoken and the credential bytes are preserved. Data that is not a JSON object, including an empty .dockerconfigjson, is reported with the key name but never with its content. fetchPullSecretData uses it, so the reconciler normalizes once and the daemon keeps copying bytes verbatim. Assisted-by: AI Signed-off-by: Alice Frosi --- .../controller/bootcnodepool_controller.go | 16 +-- internal/controller/pullsecret_test.go | 81 ++++++++++++ internal/pullsecret/pullsecret.go | 36 ++++++ internal/pullsecret/pullsecret_test.go | 118 ++++++++++++++++++ 4 files changed, 241 insertions(+), 10 deletions(-) diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index 7b95db41..c999e8f7 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -827,8 +827,9 @@ func (r *BootcNodePoolReconciler) listAllBootcNodes( return all, nil } -// fetchPullSecretData fetches the .dockerconfigjson data from the pull -// secret referenced by the pool. Returns nil if no pull secret is set, and +// fetchPullSecretData fetches the credentials of the pull secret referenced +// by the pool, normalized to .dockerconfigjson format (see +// pullsecret.AuthData). Returns nil if no pull secret is set, and // 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( @@ -863,14 +864,9 @@ func (r *BootcNodePoolReconciler) fetchPullSecretData( return nil, fmt.Errorf("fetching pull secret %s/%s: %w", key.Namespace, key.Name, err) } - data, ok := secret.Data[corev1.DockerConfigJsonKey] - if !ok { - return nil, fmt.Errorf( - "pull secret %s/%s missing %s key", - key.Namespace, - key.Name, - corev1.DockerConfigJsonKey, - ) + data, err := pullsecret.AuthData(&secret) + if err != nil { + return nil, fmt.Errorf("pull secret %s/%s: %w", key.Namespace, key.Name, err) } return data, nil diff --git a/internal/controller/pullsecret_test.go b/internal/controller/pullsecret_test.go index 36bff9fd..6bddd6d8 100644 --- a/internal/controller/pullsecret_test.go +++ b/internal/controller/pullsecret_test.go @@ -10,7 +10,9 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" testutil "github.com/bootc-dev/bootc-operator/test/util" @@ -313,3 +315,82 @@ func TestParsePullSecretNamespaces(t *testing.T) { }) } } + +func TestFetchPullSecretDataNormalizesFormat(t *testing.T) { + const ( + current = `{"auths":{"quay.io":{"auth":"Zm9vOmJhcg=="}}}` + legacy = `{"quay.io":{"auth":"Zm9vOmJhcg==","identitytoken":"tok"}}` + ) + tests := []struct { + name string + secret corev1.Secret + want string + wantErr string + }{ + { + name: "dockerconfigjson", + secret: corev1.Secret{ + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{corev1.DockerConfigJsonKey: []byte(current)}, + }, + want: current, + }, + { + name: "legacy dockercfg", + secret: corev1.Secret{ + Type: corev1.SecretTypeDockercfg, + Data: map[string][]byte{corev1.DockerConfigKey: []byte(legacy)}, + }, + want: `{"auths":` + legacy + `}`, + }, + { + name: "opaque with dockerconfigjson", + secret: corev1.Secret{ + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{corev1.DockerConfigJsonKey: []byte(current)}, + }, + want: current, + }, + { + name: "no credentials key", + secret: corev1.Secret{ + Data: map[string][]byte{"token": []byte("x")}, + }, + wantErr: "pull secret ns/ps: missing .dockerconfigjson or .dockercfg key", + }, + { + name: "malformed", + secret: corev1.Secret{ + Data: map[string][]byte{corev1.DockerConfigKey: []byte("nope")}, + }, + wantErr: "pull secret ns/ps: .dockercfg is not a JSON object", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + secret := tt.secret.DeepCopy() + secret.Name, secret.Namespace = "ps", "ns" + r := &BootcNodePoolReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithObjects(secret). + Build(), + PullSecretNamespaces: []string{"ns"}, + } + pool := testutil.NewPool( + "pool", + testutil.ImageDigestRefA, + testutil.WithPullSecret("ps", "ns"), + ) + + got, err := r.fetchPullSecretData(context.Background(), pool) + if tt.wantErr != "" { + g.Expect(err).To(MatchError(ContainSubstring(tt.wantErr))) + return + } + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(string(got)).To(MatchJSON(tt.want)) + }) + } +} diff --git a/internal/pullsecret/pullsecret.go b/internal/pullsecret/pullsecret.go index bb416725..32b0a990 100644 --- a/internal/pullsecret/pullsecret.go +++ b/internal/pullsecret/pullsecret.go @@ -6,10 +6,12 @@ package pullsecret import ( + "encoding/json" "errors" "fmt" "strings" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/validation" ) @@ -53,3 +55,37 @@ func Key(namespace, name string) (string, error) { } return key, nil } + +// AuthData returns the pull secret's credentials in .dockerconfigjson format, +// the one the tag resolver and bootc read, whichever of the two Kubernetes +// pull secret formats the Secret holds. It goes by the Secret's keys rather +// than its type, since an Opaque Secret works as well. +// +// .dockerconfigjson is returned as is. The legacy .dockercfg, which is the +// auths map without the wrapper (OpenShift generates such secrets for service +// accounts), is wrapped in {"auths": ...} without decoding its entries, so +// that fields like identitytoken survive; the bytes are not re-encoded. If both keys are set, +// .dockerconfigjson wins. The error names the keys but never includes +// secret data. +func AuthData(secret *corev1.Secret) ([]byte, error) { + if data, ok := secret.Data[corev1.DockerConfigJsonKey]; ok { + if !isJSONObject(data) { + return nil, fmt.Errorf("%s is not a JSON object", corev1.DockerConfigJsonKey) + } + return data, nil + } + if data, ok := secret.Data[corev1.DockerConfigKey]; ok { + if !isJSONObject(data) { + return nil, fmt.Errorf("%s is not a JSON object", corev1.DockerConfigKey) + } + wrapped := append([]byte(`{"auths":`), data...) + return append(wrapped, '}'), nil + } + return nil, fmt.Errorf("missing %s or %s key", + corev1.DockerConfigJsonKey, corev1.DockerConfigKey) +} + +func isJSONObject(data []byte) bool { + var obj map[string]json.RawMessage + return json.Unmarshal(data, &obj) == nil && obj != nil +} diff --git a/internal/pullsecret/pullsecret_test.go b/internal/pullsecret/pullsecret_test.go index 5bdc8354..6b3b34f8 100644 --- a/internal/pullsecret/pullsecret_test.go +++ b/internal/pullsecret/pullsecret_test.go @@ -7,6 +7,7 @@ import ( "testing" . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" ) func TestKey(t *testing.T) { @@ -60,3 +61,120 @@ func TestKeyRejectsInvalidNames(t *testing.T) { }) } } + +func TestAuthData(t *testing.T) { + const ( + current = `{"auths":{"quay.io":{"auth":"Zm9vOmJhcg=="}}}` + legacy = `{"quay.io":{"auth":"Zm9vOmJhcg==","identitytoken":"tok"}}` + ) + tests := []struct { + name string + data map[string]string + want string + wantErr string + }{ + { + name: "dockerconfigjson as is", + data: map[string]string{corev1.DockerConfigJsonKey: current}, + want: current, + }, + { + name: "dockerconfigjson is not rewrapped", + data: map[string]string{corev1.DockerConfigJsonKey: legacy}, + want: legacy, + }, + { + name: "legacy dockercfg wrapped, entries untouched", + data: map[string]string{corev1.DockerConfigKey: legacy}, + want: `{"auths":` + legacy + `}`, + }, + { + name: "legacy dockercfg empty", + data: map[string]string{corev1.DockerConfigKey: `{}`}, + want: `{"auths":{}}`, + }, + { + name: "dockerconfigjson preferred when both set", + data: map[string]string{ + corev1.DockerConfigJsonKey: current, + corev1.DockerConfigKey: legacy, + }, + want: current, + }, + { + name: "malformed dockerconfigjson is not replaced by dockercfg", + data: map[string]string{ + corev1.DockerConfigJsonKey: `{"auths":`, + corev1.DockerConfigKey: legacy, + }, + wantErr: corev1.DockerConfigJsonKey + " is not a JSON object", + }, + { + name: "malformed dockerconfigjson", + data: map[string]string{corev1.DockerConfigJsonKey: `{"auths":`}, + wantErr: corev1.DockerConfigJsonKey + " is not a JSON object", + }, + { + name: "dockerconfigjson not an object", + data: map[string]string{corev1.DockerConfigJsonKey: `["a"]`}, + wantErr: corev1.DockerConfigJsonKey + " is not a JSON object", + }, + { + name: "dockerconfigjson null", + data: map[string]string{corev1.DockerConfigJsonKey: `null`}, + wantErr: corev1.DockerConfigJsonKey + " is not a JSON object", + }, + { + name: "malformed dockercfg", + data: map[string]string{corev1.DockerConfigKey: `secret-token`}, + wantErr: corev1.DockerConfigKey + " is not a JSON object", + }, + { + name: "neither key", + data: map[string]string{"token": "x"}, + wantErr: "missing " + corev1.DockerConfigJsonKey + " or " + corev1.DockerConfigKey, + }, + { + name: "no data", + wantErr: "missing " + corev1.DockerConfigJsonKey + " or " + corev1.DockerConfigKey, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + secret := &corev1.Secret{Data: map[string][]byte{}} + for k, v := range tt.data { + secret.Data[k] = []byte(v) + } + got, err := AuthData(secret) + if tt.wantErr != "" { + g.Expect(err).To(MatchError(ContainSubstring(tt.wantErr))) + return + } + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(string(got)).To(MatchJSON(tt.want)) + }) + } +} + +func TestAuthDataKeepsLegacyBytes(t *testing.T) { + g := NewWithT(t) + // Characters that a JSON re-encoder would escape or strip. + legacy := `{ "quay.io" : {"auth":"a&b"} }` + secret := &corev1.Secret{Data: map[string][]byte{ + corev1.DockerConfigKey: []byte(legacy), + }} + got, err := AuthData(secret) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(string(got)).To(Equal(`{"auths":` + legacy + `}`)) +} + +func TestAuthDataErrorOmitsSecretData(t *testing.T) { + g := NewWithT(t) + secret := &corev1.Secret{Data: map[string][]byte{ + corev1.DockerConfigKey: []byte("hunter2-not-json"), + }} + _, err := AuthData(secret) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).NotTo(ContainSubstring("hunter2")) +} From e66518c532169c136357f10daea6d2aaaa043ff7 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 6 Nov 2026 09:11:07 +0000 Subject: [PATCH 4/8] controller: Map pull secret events via a pool index The pool reconciler decided which Secret events to act on by type, accepting only kubernetes.io/dockerconfigjson. That ignored events for the other forms of pull secret that fetchPullSecretData accepts, such as legacy .dockercfg and Opaque Secrets carrying a .dockerconfigjson key, so rotating them was never noticed. Matching on type is also not meaningful for a Secret that is not referenced by any pool, and the mapper listed every pool on each Secret event to find out. Index BootcNodePool by spec.pullSecretRef and map a Secret event to the pools in that index. A referenced Secret now triggers a reconcile whatever its type or content, so data that becomes valid or invalid is always seen, and Secrets no pool references enqueue nothing. The Secret type is immutable, so type changes need no handling. The field index is registered once per manager, in main and in the envtest suite, since a manager rejects duplicate registrations. Assisted-by: AI Signed-off-by: Alice Frosi --- cmd/controller/main.go | 10 ++ .../controller/bootcnodepool_controller.go | 47 ++---- internal/controller/pullsecret_test.go | 151 ++++++++++++++++++ internal/controller/suite_test.go | 9 ++ internal/pullsecret/index.go | 45 ++++++ 5 files changed, 228 insertions(+), 34 deletions(-) create mode 100644 internal/pullsecret/index.go diff --git a/cmd/controller/main.go b/cmd/controller/main.go index e1ffd5f0..f7367e96 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -3,6 +3,7 @@ package main import ( + "context" "flag" "fmt" "os" @@ -21,6 +22,7 @@ import ( bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" "github.com/bootc-dev/bootc-operator/internal/config" "github.com/bootc-dev/bootc-operator/internal/controller" + "github.com/bootc-dev/bootc-operator/internal/pullsecret" "github.com/bootc-dev/bootc-operator/internal/registry" "github.com/bootc-dev/bootc-operator/internal/version" ) @@ -156,6 +158,14 @@ func main() { os.Exit(1) } + if err := pullsecret.IndexPoolPullSecretRef( + context.Background(), + mgr.GetFieldIndexer(), + ); err != nil { + setupLog.Error(err, "Failed to index pools by pull secret") + os.Exit(1) + } + kubeClient, err := kubernetes.NewForConfig(mgr.GetConfig()) if err != nil { setupLog.Error(err, "Failed to create Kubernetes clientset") diff --git a/internal/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index c999e8f7..687939b3 100644 --- a/internal/controller/bootcnodepool_controller.go +++ b/internal/controller/bootcnodepool_controller.go @@ -113,7 +113,7 @@ func (r *BootcNodePoolReconciler) SetupWithManager(mgr ctrl.Manager) error { For(&bootcv1alpha1.BootcNodePool{}). Owns(&bootcv1alpha1.BootcNode{}). Watches(&corev1.Node{}, handler.EnqueueRequestsFromMapFunc(r.mapNodeToPoolRequests), builder.WithPredicates(nodePredicates())). - Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.mapSecretToPoolRequests), builder.WithPredicates(secretTypePredicate())). + Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.mapSecretToPoolRequests)). WatchesRawSource(source.Channel(r.drainCh, &handler.EnqueueRequestForObject{})) // Karpenter is optional. Only register the NodeClaim watch when its CRD is @@ -270,35 +270,27 @@ func (r *BootcNodePoolReconciler) mapNodeToPoolRequests( } // mapSecretToPoolRequests maps a Secret event to the BootcNodePool(s) that -// should be reconciled. It enqueues pools whose pullSecretRef matches the -// changed Secret's name and namespace. +// should be reconciled: the pools whose pullSecretRef names the Secret, +// whatever its type or content, so that a Secret gaining, changing or losing +// its credentials is always noticed. It needs the pullsecret.PoolRefIndex +// field index. func (r *BootcNodePoolReconciler) mapSecretToPoolRequests( ctx context.Context, obj client.Object, ) []reconcile.Request { - secret, ok := obj.(*corev1.Secret) - if !ok { - logf.FromContext(ctx).Error(nil, "Unexpected object type in secret mapper", "object", obj) - return nil - } - var pools bootcv1alpha1.BootcNodePoolList - if err := r.List(ctx, &pools); err != nil { - log := logf.FromContext(ctx) - log.Error(err, "Failed to list BootcNodePools in secret mapper") + if err := r.List(ctx, &pools, client.MatchingFields{ + pullsecret.PoolRefIndex: pullsecret.PoolRefIndexValue(obj.GetNamespace(), obj.GetName()), + }); err != nil { + logf.FromContext(ctx).Error(err, "Failed to list BootcNodePools in secret mapper") return nil } - var requests []reconcile.Request + requests := make([]reconcile.Request, 0, len(pools.Items)) for i := range pools.Items { - pool := &pools.Items[i] - if pool.Spec.PullSecretRef != nil && - pool.Spec.PullSecretRef.Name == secret.Name && - pool.Spec.PullSecretRef.Namespace == secret.Namespace { - requests = append(requests, reconcile.Request{ - NamespacedName: types.NamespacedName{Name: pool.Name}, - }) - } + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{Name: pools.Items[i].Name}, + }) } return requests } @@ -381,19 +373,6 @@ func hasKarpenterRebootTaint(node *corev1.Node) bool { return false } -// secretTypePredicate returns a predicate that accepts only Secrets of -// type kubernetes.io/dockerconfigjson. -func secretTypePredicate() predicate.Predicate { - return predicate.NewPredicateFuncs(func(obj client.Object) bool { - secret, ok := obj.(*corev1.Secret) - if !ok { - logf.Log.Error(nil, "Unexpected object type in secret predicate", "object", obj) - return false - } - return secret.Type == corev1.SecretTypeDockerConfigJson - }) -} - // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. func (r *BootcNodePoolReconciler) Reconcile( diff --git a/internal/controller/pullsecret_test.go b/internal/controller/pullsecret_test.go index 6bddd6d8..f708039f 100644 --- a/internal/controller/pullsecret_test.go +++ b/internal/controller/pullsecret_test.go @@ -4,6 +4,7 @@ package controller import ( "context" + "fmt" "strings" "testing" @@ -13,8 +14,10 @@ import ( "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" 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" ) @@ -117,6 +120,154 @@ func TestPullSecretMissingDegradedAndRecovery(t *testing.T) { ))) } +// TestPullSecretChangesPropagate checks that a referenced Secret appearing or +// changing reaches the pool whatever the Secret's type, since the pool goes by +// the Secret's keys. The pool is created first and must report SecretError, +// then recover once the Secret is valid, with no other event to trigger it. +func TestPullSecretChangesPropagate(t *testing.T) { + tests := []struct { + name string + secretType corev1.SecretType + key string + good string + // bad is the data of a Secret present from the start, or empty if + // the Secret is created only once the pool reports it missing. + bad string + }{ + { + "opaque created", + corev1.SecretTypeOpaque, + corev1.DockerConfigJsonKey, + `{"auths":{"registry.example.com":{}}}`, + "", + }, + { + "opaque rotated", + corev1.SecretTypeOpaque, + corev1.DockerConfigJsonKey, + `{"auths":{"registry.example.com":{}}}`, + "not json", + }, + { + "dockercfg created", + corev1.SecretTypeDockercfg, + corev1.DockerConfigKey, + `{"registry.example.com":{}}`, + "", + }, + } + for i, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + + name := fmt.Sprintf("ps-change-%d", i) + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testutil.OperatorNamespaceName, + }, + Type: tt.secretType, + Data: map[string][]byte{tt.key: []byte(tt.bad)}, + } + if tt.bad != "" { + g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + } + t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) + + pool := testutil.NewPool(name, testImageDigestRefA, + testutil.WithWorkerSelector(), + testutil.WithPullSecret(name, testutil.OperatorNamespaceName), + ) + g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, pool) }) + + conditions := func() ([]metav1.Condition, error) { + var p bootcv1alpha1.BootcNodePool + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(pool), &p) + return p.Status.Conditions, err + } + g.Eventually(conditions).Should(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolDegraded), + HaveField("Status", metav1.ConditionTrue), + HaveField("Reason", bootcv1alpha1.PoolSecretError), + ))) + + secret.Data = map[string][]byte{tt.key: []byte(tt.good)} + if tt.bad == "" { + g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + } else { + g.Expect(k8sClient.Update(ctx, secret)).To(Succeed()) + } + + g.Eventually(conditions).Should(ContainElement(And( + HaveField("Type", bootcv1alpha1.PoolDegraded), + HaveField("Status", metav1.ConditionFalse), + HaveField("Reason", bootcv1alpha1.PoolHealthy), + ))) + }) + } +} + +func TestMapSecretToPoolRequests(t *testing.T) { + poolA := testutil.NewPool( + "pool-a", + testutil.ImageDigestRefA, + testutil.WithPullSecret("ps", "ns"), + ) + poolB := testutil.NewPool( + "pool-b", + testutil.ImageDigestRefA, + testutil.WithPullSecret("ps", "ns"), + ) + other := testutil.NewPool( + "other", + testutil.ImageDigestRefA, + testutil.WithPullSecret("ps", "other-ns"), + ) + none := testutil.NewPool("none", testutil.ImageDigestRefA) + r := &BootcNodePoolReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithObjects(poolA, poolB, other, none). + WithIndex(&bootcv1alpha1.BootcNodePool{}, pullsecret.PoolRefIndex, pullsecret.PoolRefIndexFunc). + Build(), + } + secret := func(namespace, name string) *corev1.Secret { + return &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}} + } + + request := func(pool *bootcv1alpha1.BootcNodePool) reconcile.Request { + return reconcile.Request{NamespacedName: client.ObjectKeyFromObject(pool)} + } + tests := []struct { + name string + secret *corev1.Secret + want []reconcile.Request + }{ + { + "shared by two pools", + secret("ns", "ps"), + []reconcile.Request{request(poolA), request(poolB)}, + }, + { + "same name in another namespace", + secret("other-ns", "ps"), + []reconcile.Request{request(other)}, + }, + {"unreferenced", secret("ns", "unreferenced"), nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + got := r.mapSecretToPoolRequests(context.Background(), tt.secret) + g.Expect(got).To(ConsistOf(tt.want)) + }) + } +} + func TestPullSecretRemovalClearsRef(t *testing.T) { g := NewWithT(t) g.SetDefaultEventuallyTimeout(pollTimeout) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index c6d2c194..bc35fb42 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -22,6 +22,7 @@ import ( metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" 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" ) @@ -78,6 +79,14 @@ func TestMain(m *testing.M) { os.Exit(1) } + if err := pullsecret.IndexPoolPullSecretRef( + context.Background(), + mgr.GetFieldIndexer(), + ); err != nil { + fmt.Fprintf(os.Stderr, "Failed to index pools by pull secret: %v\n", err) + os.Exit(1) + } + kubeClient, err := kubernetes.NewForConfig(cfg) if err != nil { fmt.Fprintf(os.Stderr, "Failed to create Kubernetes clientset: %v\n", err) diff --git a/internal/pullsecret/index.go b/internal/pullsecret/index.go new file mode 100644 index 00000000..5e7c1313 --- /dev/null +++ b/internal/pullsecret/index.go @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pullsecret + +import ( + "context" + "fmt" + + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" +) + +// PoolRefIndex is the field index on BootcNodePool that maps a pull secret +// to the pools referencing it. Use it with client.MatchingFields and +// PoolRefIndexValue on a cache-backed client. +const PoolRefIndex = "spec.pullSecretRef" + +// PoolRefIndexValue returns the PoolRefIndex value for the secret +// namespace/name. +func PoolRefIndexValue(namespace, name string) string { + return types.NamespacedName{Namespace: namespace, Name: name}.String() +} + +// PoolRefIndexFunc extracts the PoolRefIndex values of a BootcNodePool. +func PoolRefIndexFunc(obj client.Object) []string { + pool, ok := obj.(*bootcv1alpha1.BootcNodePool) + if !ok || pool.Spec.PullSecretRef == nil { + return nil + } + ref := pool.Spec.PullSecretRef + return []string{PoolRefIndexValue(ref.Namespace, ref.Name)} +} + +// IndexPoolPullSecretRef registers PoolRefIndex with the manager's cache. A +// manager accepts the registration once, so call it once per manager, before +// starting it. +func IndexPoolPullSecretRef(ctx context.Context, indexer client.FieldIndexer) error { + err := indexer.IndexField(ctx, &bootcv1alpha1.BootcNodePool{}, PoolRefIndex, PoolRefIndexFunc) + if err != nil { + return fmt.Errorf("indexing BootcNodePool %s: %w", PoolRefIndex, err) + } + return nil +} From 08c4625b1ffed23fa8f77775b98f65467c802ecc Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 30 Oct 2026 11:57:39 +0000 Subject: [PATCH 5/8] 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 its credentials normalized to .dockerconfigjson format. It lives in internal/pullsecret, next to the key function and the constants it shares with the daemon, so the reconciler and the key format cannot drift apart. It is the Secret's only writer, so pool reconciles cannot conflict on it. Pool, referenced source secret and bootc-pull-secrets events all map to one request, found through the pool index, 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_test.go | 37 ++-- internal/pullsecret/aggregator.go | 188 ++++++++++++++++++ internal/pullsecret/aggregator_test.go | 263 +++++++++++++++++++++++++ internal/pullsecret/index.go | 3 +- internal/pullsecret/pullsecret.go | 6 +- internal/pullsecret/suite_test.go | 124 ++++++++++++ test/util/builders.go | 15 ++ 9 files changed, 636 insertions(+), 28 deletions(-) create mode 100644 internal/pullsecret/aggregator.go create mode 100644 internal/pullsecret/aggregator_test.go create mode 100644 internal/pullsecret/suite_test.go diff --git a/cmd/controller/main.go b/cmd/controller/main.go index f7367e96..d5efa207 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -120,6 +120,10 @@ func main() { os.Exit(1) } operatorNamespace := detectNamespace() + if operatorNamespace == "" { + setupLog.Error(nil, "Operator namespace unknown; set POD_NAMESPACE") + os.Exit(1) + } pullSecretNamespaces, err := controller.ParsePullSecretNamespaces( pullSecretNamespacesFlag, operatorNamespace, @@ -128,11 +132,6 @@ func main() { setupLog.Error(err, "Invalid pull secret namespace configuration") os.Exit(1) } - if len(pullSecretNamespaces) == 0 { - setupLog.Error(nil, "No pull secret namespaces configured and operator namespace unknown; "+ - "set spec.controller.pullSecretNamespaces, --pull-secret-namespaces, or POD_NAMESPACE") - os.Exit(1) - } setupLog.Info("Pull secret namespaces", "namespaces", pullSecretNamespaces) config.LogSource( @@ -185,6 +184,15 @@ func main() { os.Exit(1) } + if err := (&pullsecret.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 9396faf9..0c639952 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -101,6 +101,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_test.go b/internal/controller/pullsecret_test.go index f708039f..cdfbeea4 100644 --- a/internal/controller/pullsecret_test.go +++ b/internal/controller/pullsecret_test.go @@ -21,19 +21,6 @@ import ( testutil "github.com/bootc-dev/bootc-operator/test/util" ) -func newDockerConfigSecret(name, namespace string, dockerCfg []byte) *corev1.Secret { - return &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - }, - Type: corev1.SecretTypeDockerConfigJson, - Data: map[string][]byte{ - corev1.DockerConfigJsonKey: dockerCfg, - }, - } -} - func TestPullSecretRefPropagation(t *testing.T) { g := NewWithT(t) g.SetDefaultEventuallyTimeout(pollTimeout) @@ -42,7 +29,11 @@ func TestPullSecretRefPropagation(t *testing.T) { secretData := []byte(`{"auths":{"registry.example.com":{}}}`) - secret := newDockerConfigSecret("ps-prop-secret", testutil.OperatorNamespaceName, secretData) + secret := testutil.NewDockerConfigSecret( + "ps-prop-secret", + testutil.OperatorNamespaceName, + secretData, + ) g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) @@ -105,7 +96,11 @@ func TestPullSecretMissingDegradedAndRecovery(t *testing.T) { // Create the missing secret and verify recovery. secretData := []byte(`{"auths":{"recovered":{}}}`) - secret := newDockerConfigSecret("ps-missing-secret", testutil.OperatorNamespaceName, secretData) + secret := testutil.NewDockerConfigSecret( + "ps-missing-secret", + testutil.OperatorNamespaceName, + secretData, + ) g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) @@ -275,7 +270,11 @@ func TestPullSecretRemovalClearsRef(t *testing.T) { ctx := context.Background() secretData := []byte(`{"auths":{"remove":{}}}`) - secret := newDockerConfigSecret("ps-remove-secret", testutil.OperatorNamespaceName, secretData) + secret := testutil.NewDockerConfigSecret( + "ps-remove-secret", + testutil.OperatorNamespaceName, + secretData, + ) g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) @@ -333,7 +332,7 @@ func TestPullSecretNamespaceDeniedAndRecovery(t *testing.T) { t.Cleanup(func() { _ = k8sClient.Delete(ctx, deniedNS) }) secretData := []byte(`{"auths":{"denied":{}}}`) - deniedSecret := newDockerConfigSecret("ps-denied-secret", deniedNS.Name, secretData) + deniedSecret := testutil.NewDockerConfigSecret("ps-denied-secret", deniedNS.Name, secretData) g.Expect(k8sClient.Create(ctx, deniedSecret)).To(Succeed()) t.Cleanup(func() { _ = k8sClient.Delete(ctx, deniedSecret) }) @@ -369,7 +368,7 @@ func TestPullSecretNamespaceDeniedAndRecovery(t *testing.T) { }, "2s", pollInterval).ShouldNot(Succeed()) // Move the secret to the operator namespace and point the pool at it. - allowedSecret := newDockerConfigSecret( + allowedSecret := testutil.NewDockerConfigSecret( "ps-denied-secret", testutil.OperatorNamespaceName, secretData, @@ -414,7 +413,7 @@ func TestPullSecretNameTooLong(t *testing.T) { // A valid Secret name whose aggregated key "_" exceeds // the 253-character Secret key limit. secretName := strings.Repeat("a", 240) - secret := newDockerConfigSecret(secretName, testutil.OperatorNamespaceName, + secret := testutil.NewDockerConfigSecret(secretName, testutil.OperatorNamespaceName, []byte(`{"auths":{"long":{}}}`)) g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) diff --git a/internal/pullsecret/aggregator.go b/internal/pullsecret/aggregator.go new file mode 100644 index 00000000..61798bc2 --- /dev/null +++ b/internal/pullsecret/aggregator.go @@ -0,0 +1,188 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pullsecret + +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" +) + +// PullSecretAggregator maintains the aggregated pull secret mounted into the +// daemon: one key per source secret referenced by a pool, holding that +// secret's credentials in .dockerconfigjson format (see AuthData). 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. It needs the PoolRefIndex field index. +func (r *PullSecretAggregator) SetupWithManager(mgr ctrl.Manager) error { + enqueueAll := 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, handler.EnqueueRequestsFromMapFunc(enqueueAll))). + Watches(&bootcv1alpha1.BootcNodePool{}, handler.EnqueueRequestsFromMapFunc(enqueueAll), + builder.WithPredicates(predicate.GenerationChangedPredicate{})). + Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.mapSecret)). + Complete(r) +} + +func (r *PullSecretAggregator) secretKey() types.NamespacedName { + return types.NamespacedName{Namespace: r.Namespace, Name: SecretName} +} + +// mapSecret enqueues the aggregated Secret for events on the aggregated +// Secret itself, so it is repaired if deleted or edited, and on source +// secrets a pool references, whatever their type or content. Other Secrets +// do not matter. +func (r *PullSecretAggregator) mapSecret( + ctx context.Context, + obj client.Object, +) []reconcile.Request { + request := []reconcile.Request{{NamespacedName: r.secretKey()}} + if client.ObjectKeyFromObject(obj) == r.secretKey() { + return request + } + + var pools bootcv1alpha1.BootcNodePoolList + if err := r.List(ctx, &pools, client.MatchingFields{ + PoolRefIndex: PoolRefIndexValue(obj.GetNamespace(), obj.GetName()), + }); err != nil { + logf.FromContext(ctx).Error(err, "Failed to list BootcNodePools in secret mapper") + return request + } + if len(pools.Items) == 0 { + return nil + } + return request +} + +// 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: 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 valid credentials) 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 := 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, err := AuthData(&secret); err == nil { + desired[key] = data + } + } + return desired, nil +} diff --git a/internal/pullsecret/aggregator_test.go b/internal/pullsecret/aggregator_test.go new file mode 100644 index 00000000..d1f0d359 --- /dev/null +++ b/internal/pullsecret/aggregator_test.go @@ -0,0 +1,263 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pullsecret + +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" + 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: 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, testutil.ImageDigestRefA, + 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 := testutil.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 TestPullSecretAggregatorNormalizesLegacySecret(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + key := testutil.OperatorNamespaceName + "_agg-legacy-secret" + + legacy := `{"legacy.example.com":{"auth":"Zm9vOmJhcg==","identitytoken":"tok"}}` + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "agg-legacy-secret", + Namespace: testutil.OperatorNamespaceName, + }, + Type: corev1.SecretTypeDockercfg, + Data: map[string][]byte{corev1.DockerConfigKey: []byte(legacy)}, + } + g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) + + pool := newAggregatorPool("agg-legacy-pool", secret.Name, secret.Namespace) + g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, pool) }) + + // The aggregate holds the .dockerconfigjson form, entries intact. + g.Eventually(aggregatedData).WithArguments(ctx). + Should(HaveKeyWithValue(key, []byte(`{"auths":`+legacy+`}`))) + + // A rotation of the legacy secret is picked up. + rotated := `{"rotated.example.com":{"auth":"Zm9vOmJhcg=="}}` + secret.Data[corev1.DockerConfigKey] = []byte(rotated) + g.Expect(k8sClient.Update(ctx, secret)).To(Succeed()) + g.Eventually(aggregatedData).WithArguments(ctx). + Should(HaveKeyWithValue(key, []byte(`{"auths":`+rotated+`}`))) +} + +func TestPullSecretAggregatorFollowsOpaqueSecret(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(pollTimeout) + g.SetDefaultEventuallyPollingInterval(pollInterval) + ctx := context.Background() + key := testutil.OperatorNamespaceName + "_agg-opaque-secret" + + // An Opaque Secret without credentials is skipped until it gains them. + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "agg-opaque-secret", + Namespace: testutil.OperatorNamespaceName, + }, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{"other": []byte("x")}, + } + g.Expect(k8sClient.Create(ctx, secret)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, secret) }) + + pool := newAggregatorPool("agg-opaque-pool", secret.Name, secret.Namespace) + g.Expect(k8sClient.Create(ctx, pool)).To(Succeed()) + t.Cleanup(func() { _ = k8sClient.Delete(ctx, pool) }) + + g.Consistently(aggregatedData, "2s", pollInterval).WithArguments(ctx).ShouldNot(HaveKey(key)) + + data := []byte(`{"auths":{"opaque.example.com":{}}}`) + secret.Data[corev1.DockerConfigJsonKey] = data + g.Expect(k8sClient.Update(ctx, secret)).To(Succeed()) + g.Eventually(aggregatedData).WithArguments(ctx).Should(HaveKeyWithValue(key, 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 := testutil.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 := testutil.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 := testutil.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 := testutil.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: 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/pullsecret/index.go b/internal/pullsecret/index.go index 5e7c1313..52dd1420 100644 --- a/internal/pullsecret/index.go +++ b/internal/pullsecret/index.go @@ -35,7 +35,8 @@ func PoolRefIndexFunc(obj client.Object) []string { // IndexPoolPullSecretRef registers PoolRefIndex with the manager's cache. A // manager accepts the registration once, so call it once per manager, before -// starting it. +// starting it, not from the controllers' SetupWithManager. The pool reconciler +// and the PullSecretAggregator both rely on it. func IndexPoolPullSecretRef(ctx context.Context, indexer client.FieldIndexer) error { err := indexer.IndexField(ctx, &bootcv1alpha1.BootcNodePool{}, PoolRefIndex, PoolRefIndexFunc) if err != nil { diff --git a/internal/pullsecret/pullsecret.go b/internal/pullsecret/pullsecret.go index 32b0a990..6afebc97 100644 --- a/internal/pullsecret/pullsecret.go +++ b/internal/pullsecret/pullsecret.go @@ -1,8 +1,8 @@ // 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 defines the aggregated pull secret and the controller +// that maintains it. The PullSecretAggregator writes the Secret; the daemon +// reads it from a volume mount. package pullsecret import ( diff --git a/internal/pullsecret/suite_test.go b/internal/pullsecret/suite_test.go new file mode 100644 index 00000000..5dbad892 --- /dev/null +++ b/internal/pullsecret/suite_test.go @@ -0,0 +1,124 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pullsecret + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "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" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + + bootcv1alpha1 "github.com/bootc-dev/bootc-operator/api/v1alpha1" + testutil "github.com/bootc-dev/bootc-operator/test/util" +) + +const ( + pollInterval = 200 * time.Millisecond + pollTimeout = 10 * time.Second +) + +var k8sClient client.Client + +func TestMain(m *testing.M) { + ctrl.SetLogger(zap.New(zap.UseDevMode(true))) + + if err := bootcv1alpha1.AddToScheme(scheme.Scheme); err != nil { + fmt.Fprintf(os.Stderr, "Failed to add scheme: %v\n", err) + os.Exit(1) + } + + testEnv := &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, + ErrorIfCRDPathMissing: true, + } + + cfg, err := testEnv.Start() + if err != nil { + fmt.Fprintf(os.Stderr, "Failed to start envtest: %v\n", err) + os.Exit(1) + } + + k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) + if err != nil { + fmt.Fprintf(os.Stderr, "Failed to create client: %v\n", err) + os.Exit(1) + } + + // Mirror the manager the operator runs: the Secret informer is + // restricted to the allowed pull secret namespaces. + pullSecretNamespaces := []string{testutil.OperatorNamespaceName} + secretNamespaces := make(map[string]cache.Config, len(pullSecretNamespaces)) + for _, ns := range pullSecretNamespaces { + secretNamespaces[ns] = cache.Config{} + } + mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + Scheme: scheme.Scheme, + Metrics: metricsserver.Options{ + BindAddress: "0", // disable metrics server in tests + }, + Cache: cache.Options{ + ByObject: map[client.Object]cache.ByObject{ + &corev1.Secret{}: {Namespaces: secretNamespaces}, + }, + }, + }) + if err != nil { + fmt.Fprintf(os.Stderr, "Failed to create manager: %v\n", err) + os.Exit(1) + } + + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: testutil.OperatorNamespaceName}} + if err := k8sClient.Create(context.Background(), ns); err != nil { + fmt.Fprintf(os.Stderr, "Failed to create operator namespace: %v\n", err) + os.Exit(1) + } + + if err := IndexPoolPullSecretRef(context.Background(), mgr.GetFieldIndexer()); err != nil { + fmt.Fprintf(os.Stderr, "Failed to index pools by pull secret: %v\n", err) + 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{}) + go func() { + defer close(mgrDone) + if err := mgr.Start(mgrCtx); err != nil { + fmt.Fprintf(os.Stderr, "Manager exited with error: %v\n", err) + os.Exit(1) + } + }() + + code := m.Run() + + // Stop the manager; this implicitly tests that it can shut down cleanly. + mgrCancel() + <-mgrDone + + if err := testEnv.Stop(); err != nil { + fmt.Fprintf(os.Stderr, "Failed to stop envtest: %v\n", err) + } + + os.Exit(code) +} diff --git a/test/util/builders.go b/test/util/builders.go index bfd416a7..d82da51d 100644 --- a/test/util/builders.go +++ b/test/util/builders.go @@ -34,6 +34,21 @@ const ( OperatorNamespaceName = "bootc-operator" ) +// NewDockerConfigSecret creates a kubernetes.io/dockerconfigjson Secret +// holding the given config. +func NewDockerConfigSecret(name, namespace string, dockerCfg []byte) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: map[string][]byte{ + corev1.DockerConfigJsonKey: dockerCfg, + }, + } +} + // PoolOption configures a BootcNodePool. type PoolOption func(*bootcv1alpha1.BootcNodePool) From 45aac6ac1c8b897e3f1ef46d34f8d661553b821f Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 30 Oct 2026 12:04:39 +0000 Subject: [PATCH 6/8] 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 Signed-off-by: Alice Frosi --- cmd/daemon/main.go | 2 + config/daemon/daemon.yaml | 11 ++ internal/daemon/pullsecret_test.go | 155 +++++++++++++++++++++++++++++ internal/daemon/reconciler.go | 89 +++++++++++------ internal/daemon/reconciler_test.go | 22 ++-- internal/daemon/suite_test.go | 41 ++++++-- 6 files changed, 274 insertions(+), 46 deletions(-) create mode 100644 internal/daemon/pullsecret_test.go diff --git a/cmd/daemon/main.go b/cmd/daemon/main.go index dcdd4868..be3e4114 100644 --- a/cmd/daemon/main.go +++ b/cmd/daemon/main.go @@ -22,6 +22,7 @@ import ( "github.com/bootc-dev/bootc-operator/internal/bootc" "github.com/bootc-dev/bootc-operator/internal/config" "github.com/bootc-dev/bootc-operator/internal/daemon" + "github.com/bootc-dev/bootc-operator/internal/pullsecret" "github.com/bootc-dev/bootc-operator/internal/version" ) @@ -127,6 +128,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..d61aa014 --- /dev/null +++ b/internal/daemon/pullsecret_test.go @@ -0,0 +1,155 @@ +// 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.NewBootcNode(testNodeName, testutil.ImageDigestRefB, + testutil.WithBootcNodePullSecret("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.NewBootcNode(testNodeName, testutil.ImageDigestRefB, + testutil.WithBootcNodePullSecret("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.NewBootcNode(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.NewBootcNode(testNodeName, testutil.ImageDigestRefB, + testutil.WithBootcNodePullSecret("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. The + // retry waits for the stage backoff, which can exceed pollTimeout on + // slow runners, so allow for more than one backoff period. + writeMountedPullSecret(t, "my-secret", "my-ns", "new") + fake.setStageErr(nil) + g.Eventually(auth.get). + WithTimeout(2*stageBackoffMin + pollTimeout). + Should(Equal([]string{"old", "new"})) +} diff --git a/internal/daemon/reconciler.go b/internal/daemon/reconciler.go index 7c88147a..8ae995b6 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() @@ -418,47 +421,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 4ac80c6b..9ca4af5f 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" @@ -114,7 +114,7 @@ func TestReconcileBootcVersionErrorNotDegraded(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) fake.status = newBootcStatus(testutil.DigestA) fake.versionErr = errors.New("bootc --version failed") @@ -144,7 +144,7 @@ func TestReconcileBootcStatusError(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) fake.setStatusErr(errors.New(bootcStatusErrMsg)) bn := testutil.NewBootcNode(testNodeName, testutil.ImageDigestRefA) @@ -176,7 +176,7 @@ func TestStagingTriggered(t *testing.T) { g.SetDefaultEventuallyPollingInterval(pollInterval) ctx := context.Background() - fake := newTestEnv() + fake := newTestEnv(t) fake.status = newBootcStatus(testutil.DigestA) bn := testutil.NewBootcNode(testNodeName, testutil.ImageDigestRefB) @@ -217,7 +217,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)) @@ -252,7 +252,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) @@ -281,7 +281,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) @@ -314,7 +314,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) @@ -350,7 +350,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) @@ -385,7 +385,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) @@ -420,7 +420,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 629b9c36..3c09dfb2 100644 --- a/internal/daemon/suite_test.go +++ b/internal/daemon/suite_test.go @@ -30,6 +30,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) { @@ -68,12 +73,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{version: testBootcVersionOutput} 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, @@ -108,13 +123,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{version: testBootcVersionOutput} testReconciler.Executor = fake testReconciler.rebootIssued = false From ff8d61205186a68a5911228f453e353c7f63c862 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 30 Oct 2026 12:07:26 +0000 Subject: [PATCH 7/8] 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 be3e4114..acac4642 100644 --- a/cmd/daemon/main.go +++ b/cmd/daemon/main.go @@ -131,7 +131,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 16dcdd4c..688f5a8e 100644 --- a/config/rbac/daemon_role.yaml +++ b/config/rbac/daemon_role.yaml @@ -12,12 +12,6 @@ rules: - bootcoperatorconfigs verbs: - list -- apiGroups: - - "" - resources: - - secrets - verbs: - - get - apiGroups: - node.bootc.dev resources: diff --git a/hack/test-helm.sh b/hack/test-helm.sh index 30d84ac6..f58f11d3 100755 --- a/hack/test-helm.sh +++ b/hack/test-helm.sh @@ -22,7 +22,7 @@ render() { # check MANIFESTS FLAG_ARGUMENTS NAMESPACES... # Asserts the namespace flag arguments in order (empty means absent), 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 roles @@ -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 8ae995b6..06d60495 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 c557e5fb06592cb9a47b40a8ab81597ef1031430 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 30 Oct 2026 12:09:00 +0000 Subject: [PATCH 8/8] 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 154f4b19..026b0ae8 100644 --- a/docs/src/helm.md +++ b/docs/src/helm.md @@ -201,6 +201,15 @@ helm uninstall bootc-operator --namespace bootc-operator > preserves the CRDs and configuration. Uninstalling `bootc-operator-crds` > deletes the CRDs and all their instances. +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