diff --git a/cmd/controller/main.go b/cmd/controller/main.go index e1ffd5f0..d5efa207 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" ) @@ -118,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, @@ -126,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( @@ -156,6 +157,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") @@ -175,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/cmd/daemon/main.go b/cmd/daemon/main.go index dcdd4868..acac4642 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,9 +128,9 @@ func main() { Scheme: mgr.GetScheme(), NodeName: nodeName, HostRoot: "/proc/1/root", + 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/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/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/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/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 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/controller/bootcnodepool_controller.go b/internal/controller/bootcnodepool_controller.go index 3a0ff86a..687939b3 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. @@ -112,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 @@ -269,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 } @@ -380,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( @@ -826,9 +806,11 @@ 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 -// an error if the secret's namespace is not in PullSecretNamespaces. +// 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( ctx context.Context, pool *bootcv1alpha1.BootcNodePool, @@ -845,6 +827,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, @@ -854,14 +843,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 761470e5..cdfbeea4 100644 --- a/internal/controller/pullsecret_test.go +++ b/internal/controller/pullsecret_test.go @@ -4,30 +4,23 @@ package controller import ( "context" + "fmt" + "strings" "testing" . "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" + "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" ) -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) @@ -36,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) }) @@ -99,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) }) @@ -114,6 +115,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) @@ -121,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) }) @@ -179,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) }) @@ -215,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, @@ -251,6 +404,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 := testutil.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 @@ -279,3 +465,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/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/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..06d60495 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,12 +61,13 @@ 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 inflight stageOp stageDone chan event.GenericEvent @@ -222,7 +224,7 @@ func (r *BootcNodeReconciler) reconcileBootcNode( }, nil } - r.syncPullSecret(ctx, log, bn) + r.syncPullSecret(log, bn) desiredImage := desiredRef.String() @@ -418,47 +420,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 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 new file mode 100644 index 00000000..52dd1420 --- /dev/null +++ b/internal/pullsecret/index.go @@ -0,0 +1,46 @@ +// 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, 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 { + return fmt.Errorf("indexing BootcNodePool %s: %w", PoolRefIndex, err) + } + return nil +} diff --git a/internal/pullsecret/pullsecret.go b/internal/pullsecret/pullsecret.go new file mode 100644 index 00000000..6afebc97 --- /dev/null +++ b/internal/pullsecret/pullsecret.go @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: Apache-2.0 + +// 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 ( + "encoding/json" + "errors" + "fmt" + "strings" + + corev1 "k8s.io/api/core/v1" + "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 +} + +// 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 new file mode 100644 index 00000000..6b3b34f8 --- /dev/null +++ b/internal/pullsecret/pullsecret_test.go @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pullsecret + +import ( + "strings" + "testing" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" +) + +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"))) + }) + } +} + +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")) +} 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/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")) +} 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)