diff --git a/pkg/controller/common/iri_secret_merger.go b/pkg/controller/common/iri_secret_merger.go index a5188dc5a8..3b590c1652 100644 --- a/pkg/controller/common/iri_secret_merger.go +++ b/pkg/controller/common/iri_secret_merger.go @@ -1,6 +1,7 @@ package common import ( + "crypto/subtle" "encoding/base64" "encoding/json" "errors" @@ -53,7 +54,7 @@ func NewIRISecretMerger( if err != nil { return "", "", fmt.Errorf("could not get ControllerConfig: %w", err) } - return extractIRICredentials(secret, cconfig) + return ExtractIRICredentials(secret, cconfig) }, } } @@ -72,7 +73,7 @@ func NewIRISecretMergerFromObjects( if !iri { return "", "", errIRIDisabled } - return extractIRICredentials(secret, cconfig) + return ExtractIRICredentials(secret, cconfig) }, } } @@ -91,7 +92,7 @@ func (m *IRISecretMerger) Merge(pullSecretRaw []byte) ([]byte, error) { if err != nil { return nil, err } - merged, changed, err := mergeIRIRegistryCredentialsIntoPullSecret(pullSecretRaw, password, baseDomain) + merged, changed, err := MergeIRIRegistryCredentialsIntoPullSecret(pullSecretRaw, password, baseDomain) if err != nil { return nil, err } @@ -101,9 +102,9 @@ func (m *IRISecretMerger) Merge(pullSecretRaw []byte) ([]byte, error) { return merged, nil } -// extractIRICredentials validates and extracts the password and baseDomain from +// ExtractIRICredentials validates and extracts the password and baseDomain from // the IRI credentials secret and ControllerConfig. -func extractIRICredentials(secret *corev1.Secret, cconfig *mcfgv1.ControllerConfig) (password, baseDomain string, err error) { +func ExtractIRICredentials(secret *corev1.Secret, cconfig *mcfgv1.ControllerConfig) (password, baseDomain string, err error) { if secret == nil { return "", "", fmt.Errorf("IRI registry credentials secret must not be nil") } @@ -124,13 +125,13 @@ func extractIRICredentials(secret *corev1.Secret, cconfig *mcfgv1.ControllerConf return string(pw), bd, nil } -// mergeIRIRegistryCredentialsIntoPullSecret merges IRI registry authentication +// MergeIRIRegistryCredentialsIntoPullSecret merges IRI registry authentication // credentials into a dockerconfigjson pull secret. It adds auth entries for // api-int.: (all nodes) and // localhost: (masters, where the registry runs locally). // Returns the merged bytes, a boolean indicating whether the pull secret was // changed, and any error. -func mergeIRIRegistryCredentialsIntoPullSecret(pullSecretRaw []byte, password, baseDomain string) ([]byte, bool, error) { +func MergeIRIRegistryCredentialsIntoPullSecret(pullSecretRaw []byte, password, baseDomain string) ([]byte, bool, error) { // The IRI registry is reachable via api-int on all nodes, and also via // localhost on master nodes where it runs locally. registries.conf mirror // rules on masters use localhost:22625, so credentials must be present for @@ -175,5 +176,5 @@ func mergeIRIRegistryCredentialsIntoPullSecret(pullSecretRaw []byte, password, b // matches expected. func pullSecretHasAuth(auths map[string]interface{}, host, expected string) bool { e, ok := auths[host].(map[string]interface{}) - return ok && e["auth"] == expected + return ok && subtle.ConstantTimeCompare([]byte(e["auth"].(string)), []byte(expected)) == 1 } diff --git a/pkg/controller/internalreleaseimage/internalreleaseimage_controller.go b/pkg/controller/internalreleaseimage/internalreleaseimage_controller.go index 44d915a523..f9e763c0b0 100644 --- a/pkg/controller/internalreleaseimage/internalreleaseimage_controller.go +++ b/pkg/controller/internalreleaseimage/internalreleaseimage_controller.go @@ -51,6 +51,7 @@ var updateBackoff = wait.Backoff{ // Controller defines the InternalReleaseImage controller. type Controller struct { client mcfgclientset.Interface + kubeClient clientset.Interface eventRecorder record.EventRecorder syncHandler func(mcp string) error @@ -101,6 +102,7 @@ func New( ctrl := &Controller{ client: mcfgClient, + kubeClient: kubeClient, eventRecorder: ctrlcommon.NamespacedEventRecorder(eventBroadcaster.NewRecorder(scheme.Scheme, corev1.EventSource{Component: "machineconfigcontroller-internalreleaseimagecontroller"})), queue: workqueue.NewTypedRateLimitingQueueWithConfig( workqueue.DefaultTypedControllerRateLimiter[string](), diff --git a/pkg/controller/template/template_controller.go b/pkg/controller/template/template_controller.go index 5b7588a402..6244decd30 100644 --- a/pkg/controller/template/template_controller.go +++ b/pkg/controller/template/template_controller.go @@ -158,9 +158,28 @@ func New( } func (ctrl *Controller) filterSecret(secret *corev1.Secret) { - if secret.Name == "pull-secret" || secret.Name == ctrlcommon.InternalReleaseImageAuthSecretName { + // Check if this is the IRI auth secret + if secret.Namespace == ctrlcommon.MCONamespace && secret.Name == ctrlcommon.InternalReleaseImageAuthSecretName { ctrl.enqueueController() - klog.Infof("Re-syncing ControllerConfig due to secret %s change", secret.Name) + klog.Infof("Re-syncing ControllerConfig due to secret %s/%s change", secret.Namespace, secret.Name) + return + } + + // Check if this is the configured global pull secret + cfg, err := ctrl.ccLister.Get(ctrlcommon.ControllerConfigName) + if err != nil { + // If we can't get the ControllerConfig, we can't determine if this secret + // is the pull secret, so skip the check. The controller will eventually + // sync when the ControllerConfig is available. + klog.V(4).Infof("Could not get ControllerConfig to check secret %s/%s: %v", secret.Namespace, secret.Name, err) + return + } + + if cfg.Spec.PullSecret != nil && + secret.Namespace == cfg.Spec.PullSecret.Namespace && + secret.Name == cfg.Spec.PullSecret.Name { + ctrl.enqueueController() + klog.Infof("Re-syncing ControllerConfig due to secret %s/%s change", secret.Namespace, secret.Name) } } @@ -170,7 +189,7 @@ func (ctrl *Controller) addSecret(obj interface{}) { ctrl.deleteSecret(secret) return } - klog.V(4).Infof("Add Secret %v", secret) + klog.V(4).Infof("Add Secret %s/%s", secret.Namespace, secret.Name) ctrl.filterSecret(secret) } @@ -178,7 +197,7 @@ func (ctrl *Controller) updateSecret(old, newObj interface{}) { oldSecret := old.(*corev1.Secret) newSecret := newObj.(*corev1.Secret) - klog.V(4).Infof("Update Secret %v", newSecret) + klog.V(4).Infof("Update Secret %s/%s", newSecret.Namespace, newSecret.Name) // Only trigger resync if the secret data actually changed // This prevents log spam from informer resyncs and watch reconnections @@ -189,7 +208,7 @@ func (ctrl *Controller) updateSecret(old, newObj interface{}) { func (ctrl *Controller) deleteSecret(obj interface{}) { secret, ok := obj.(*corev1.Secret) - klog.V(4).Infof("Delete Secret %v", secret) + klog.V(4).Infof("Delete Secret %s/%s", secret.Namespace, secret.Name) if !ok { tombstone, ok := obj.(cache.DeletedFinalStateUnknown) diff --git a/pkg/controller/template/template_controller_test.go b/pkg/controller/template/template_controller_test.go index d15b9a7501..3ea10d06ee 100644 --- a/pkg/controller/template/template_controller_test.go +++ b/pkg/controller/template/template_controller_test.go @@ -530,8 +530,8 @@ func TestKubeletAutoNodeSizingEnabled(t *testing.T) { } } -// TestMergesIRIRegistryCredentialsIntoPullSecret verifies that the template controller merges -// IRI registry credentials into the pull secret when rendering 00-master, so that +// TestMergesIRIRegistryCredentialsIntoPullSecret verifies that the template +// controller merges IRI registry credentials into the rendered pull secret so // nodes can authenticate to the IRI registry without writing to the user-controlled // global pull secret. func TestMergesIRIRegistryCredentialsIntoPullSecret(t *testing.T) { diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index 311c24c643..407b6687d3 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -290,6 +290,7 @@ func New( apiserverInformer.Informer(), moscInformer.Informer(), networkPolicyInformer.Informer(), + iriInformer.Informer(), } for _, i := range informers { i.AddEventHandler(optr.eventHandler()) diff --git a/pkg/operator/sync.go b/pkg/operator/sync.go index 4d330fa6a7..08dc05d782 100644 --- a/pkg/operator/sync.go +++ b/pkg/operator/sync.go @@ -2324,6 +2324,19 @@ func (optr *Operator) getImageRegistryPullSecrets() ([]byte, error) { return nil, fmt.Errorf("failed to marshal the merged pull secrets: %w", err) } + // Ensure nodes can authenticate to the internal release image (IRI) + // registry when pulling images during OS updates. The credentials + // added here become part of ControllerConfig.Spec.InternalRegistryPullSecret, + // which is the auth source the OS-update path (rpm-ostree/bootc) uses; + // that path is separate from the kubelet's pull secret, so IRI credentials + // must be supplied here in addition to the render-time merge. When IRI is + // not enabled on the cluster this merge makes no changes. + iriMerger := ctrlcommon.NewIRISecretMerger(optr.mcoSecretLister, optr.ccLister, optr.iriLister) + mergedPullSecrets, err = iriMerger.Merge(mergedPullSecrets) + if err != nil { + return nil, fmt.Errorf("failed to merge IRI registry credentials into image registry pull secrets: %w", err) + } + return mergedPullSecrets, nil } diff --git a/pkg/operator/sync_test.go b/pkg/operator/sync_test.go index 7c5ca80260..394e137bcc 100644 --- a/pkg/operator/sync_test.go +++ b/pkg/operator/sync_test.go @@ -2,6 +2,8 @@ package operator import ( "context" + "encoding/base64" + "encoding/json" "testing" configv1 "github.com/openshift/api/config/v1" @@ -14,6 +16,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" + corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" @@ -308,6 +311,136 @@ func TestMachineOSBuilderSecretReconciliation(t *testing.T) { } } +// newNamespacedIndexer returns an indexer configured with the namespace index, +// as required by the namespaced core listers (Secrets, ServiceAccounts). +func newNamespacedIndexer(objs ...interface{}) cache.Indexer { + idx := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) + for _, o := range objs { + idx.Add(o) + } + return idx +} + +// TestGetImageRegistryPullSecretsIRIMerge verifies that getImageRegistryPullSecrets +// merges InternalReleaseImage (IRI) registry credentials into the assembled +// image-registry pull secret when IRI is in use, and leaves the secret untouched +// when IRI is absent. This blob feeds ControllerConfig.Spec.InternalRegistryPullSecret, +// which the daemon writes to /etc/mco/internal-registry-pull-secret.json for the +// OS-update image-pull path. +func TestGetImageRegistryPullSecretsIRIMerge(t *testing.T) { + const ( + baseDomain = "example.com" + iriPassword = "s3cr3t" + ) + expectedIRIAuth := base64.StdEncoding.EncodeToString([]byte(ctrlcommon.IRIRegistryUsername + ":" + iriPassword)) + iriAPIIntHost := "api-int." + baseDomain + ":22625" + iriLocalHost := "localhost:22625" + + // Common fixtures independent of whether IRI is enabled. + imageRegistryCO := &configv1.ClusterOperator{ObjectMeta: metav1.ObjectMeta{Name: "image-registry"}} + clusterDNS := &configv1.DNS{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: configv1.DNSSpec{BaseDomain: baseDomain}, + } + // machine-os-puller SA with no image pull secrets; the cluster pull secret + // alone keeps the assembled "auths" map non-empty so the merge path runs. + machineOSPullerSA := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{Name: "machine-os-puller", Namespace: ctrlcommon.MCONamespace}, + } + populatedPullSecretContent := `{"auths":{"registry.example.com":{"auth":"` + + base64.StdEncoding.EncodeToString([]byte("user:pass")) + `"}}}` + + // IRI-specific fixtures. + iriInstance := &mcfgv1.InternalReleaseImage{ + ObjectMeta: metav1.ObjectMeta{Name: ctrlcommon.InternalReleaseImageInstanceName}, + } + iriAuthSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: ctrlcommon.InternalReleaseImageAuthSecretName, Namespace: ctrlcommon.MCONamespace}, + Data: map[string][]byte{"password": []byte(iriPassword)}, + } + controllerConfig := &mcfgv1.ControllerConfig{ + ObjectMeta: metav1.ObjectMeta{Name: ctrlcommon.ControllerConfigName}, + Spec: mcfgv1.ControllerConfigSpec{ + DNS: &configv1.DNS{Spec: configv1.DNSSpec{BaseDomain: baseDomain}}, + }, + } + + cases := []struct { + name string + // pullSecretContent is the raw ".dockerconfigjson" of the cluster pull secret. + pullSecretContent string + iriEnabled bool + // expectNil asserts getImageRegistryPullSecrets returns a nil secret, + // used for the empty-"auths" ("don't roll config") path. + expectNil bool + }{ + {name: "IRI absent - pull secret unchanged", pullSecretContent: populatedPullSecretContent, iriEnabled: false}, + {name: "IRI present - credentials merged", pullSecretContent: populatedPullSecretContent, iriEnabled: true}, + // With no image-pull secrets on the SA and an empty cluster pull secret, + // the assembled "auths" map is empty. Even with IRI enabled the function + // must return nil rather than emitting a secret carrying only IRI creds. + {name: "empty auths with IRI enabled - returns nil", pullSecretContent: "{}", iriEnabled: true, expectNil: true}, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + mcoSecretObjs := []interface{}{} + iriObjs := []interface{}{} + if tc.iriEnabled { + mcoSecretObjs = append(mcoSecretObjs, iriAuthSecret) + iriObjs = append(iriObjs, iriInstance) + } + + clusterPullSecret := helpers.NewDockerCfgJSONSecret(ctrlcommon.GlobalPullSecretName, ctrlcommon.OpenshiftConfigNamespace, tc.pullSecretContent) + + optr := &Operator{ + namespace: ctrlcommon.MCONamespace, + clusterOperatorLister: configlistersv1.NewClusterOperatorLister(newNamespacedIndexer(imageRegistryCO)), + dnsLister: configlistersv1.NewDNSLister(newNamespacedIndexer(clusterDNS)), + mcoSALister: corev1listers.NewServiceAccountLister(newNamespacedIndexer(machineOSPullerSA)), + mcoSecretLister: corev1listers.NewSecretLister(newNamespacedIndexer(mcoSecretObjs...)), + ocSecretLister: corev1listers.NewSecretLister(newNamespacedIndexer(clusterPullSecret)), + ccLister: mcplister.NewControllerConfigLister(newNamespacedIndexer(controllerConfig)), + iriLister: mcplister.NewInternalReleaseImageLister(newNamespacedIndexer(iriObjs...)), + } + + raw, err := optr.getImageRegistryPullSecrets() + assert.NoError(t, err) + + if tc.expectNil { + // Empty "auths": nothing to roll, so no secret is emitted (and + // IRI creds are not emitted on their own). + assert.Nil(t, raw) + return + } + assert.NotEmpty(t, raw) + + var parsed struct { + Auths map[string]struct { + Auth string `json:"auth"` + } `json:"auths"` + } + assert.NoError(t, json.Unmarshal(raw, &parsed)) + + // The original registry entry is always present. + assert.Contains(t, parsed.Auths, "registry.example.com") + + if tc.iriEnabled { + assert.Contains(t, parsed.Auths, iriAPIIntHost, "expected api-int IRI auth entry to be merged") + assert.Contains(t, parsed.Auths, iriLocalHost, "expected localhost IRI auth entry to be merged") + assert.Equal(t, expectedIRIAuth, parsed.Auths[iriAPIIntHost].Auth) + assert.Equal(t, expectedIRIAuth, parsed.Auths[iriLocalHost].Auth) + } else { + assert.NotContains(t, parsed.Auths, iriAPIIntHost, "IRI auth entry must not be present when IRI is absent") + assert.NotContains(t, parsed.Auths, iriLocalHost, "IRI auth entry must not be present when IRI is absent") + } + }) + } +} + func TestSyncMachineConfiguration(t *testing.T) { cases := []struct { name string