diff --git a/internal/controllers/mock_nmc_reconciler.go b/internal/controllers/mock_nmc_reconciler.go index d1fba7345..6f77c9c4d 100644 --- a/internal/controllers/mock_nmc_reconciler.go +++ b/internal/controllers/mock_nmc_reconciler.go @@ -3,7 +3,7 @@ // // Generated by this command: // -// mockgen -source=nmc_reconciler.go -package=controllers -destination=mock_nmc_reconciler.go pullSecretHelper +// mockgen -source=nmc_reconciler.go -package=controllers -destination=mock_nmc_reconciler.go podManager // // Package controllers is a generated GoMock package. package controllers @@ -372,42 +372,3 @@ func (mr *MockpodManagerMockRecorder) UnloaderPodTemplate(ctx, nmc, nms any) *go mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UnloaderPodTemplate", reflect.TypeOf((*MockpodManager)(nil).UnloaderPodTemplate), ctx, nmc, nms) } - -// MockpullSecretHelper is a mock of pullSecretHelper interface. -type MockpullSecretHelper struct { - ctrl *gomock.Controller - recorder *MockpullSecretHelperMockRecorder -} - -// MockpullSecretHelperMockRecorder is the mock recorder for MockpullSecretHelper. -type MockpullSecretHelperMockRecorder struct { - mock *MockpullSecretHelper -} - -// NewMockpullSecretHelper creates a new mock instance. -func NewMockpullSecretHelper(ctrl *gomock.Controller) *MockpullSecretHelper { - mock := &MockpullSecretHelper{ctrl: ctrl} - mock.recorder = &MockpullSecretHelperMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockpullSecretHelper) EXPECT() *MockpullSecretHelperMockRecorder { - return m.recorder -} - -// VolumesAndVolumeMounts mocks base method. -func (m *MockpullSecretHelper) VolumesAndVolumeMounts(ctx context.Context, nms *v1beta1.ModuleItem) ([]v1.Volume, []v1.VolumeMount, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "VolumesAndVolumeMounts", ctx, nms) - ret0, _ := ret[0].([]v1.Volume) - ret1, _ := ret[1].([]v1.VolumeMount) - ret2, _ := ret[2].(error) - return ret0, ret1, ret2 -} - -// VolumesAndVolumeMounts indicates an expected call of VolumesAndVolumeMounts. -func (mr *MockpullSecretHelperMockRecorder) VolumesAndVolumeMounts(ctx, nms any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "VolumesAndVolumeMounts", reflect.TypeOf((*MockpullSecretHelper)(nil).VolumesAndVolumeMounts), ctx, nms) -} diff --git a/internal/controllers/nmc_reconciler.go b/internal/controllers/nmc_reconciler.go index 8399a8659..55dec0970 100644 --- a/internal/controllers/nmc_reconciler.go +++ b/internal/controllers/nmc_reconciler.go @@ -28,7 +28,6 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/tools/record" "k8s.io/kubectl/pkg/cmd/util/podcmd" - "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -749,7 +748,6 @@ type podManager interface { type podManagerImpl struct { client client.Client - psh pullSecretHelper scheme *runtime.Scheme workerCfg *config.Worker workerImage string @@ -758,7 +756,6 @@ type podManagerImpl struct { func newPodManager(client client.Client, workerImage string, scheme *runtime.Scheme, workerCfg *config.Worker) podManager { return &podManagerImpl{ client: client, - psh: &pullSecretHelperImpl{client: client}, scheme: scheme, workerCfg: workerCfg, workerImage: workerImage, @@ -982,11 +979,6 @@ func (p *podManagerImpl) baseWorkerPod(ctx context.Context, nmc client.Object, i hostPathDirectory := v1.HostPathDirectory - psv, psvm, err := p.psh.VolumesAndVolumeMounts(ctx, item) - if err != nil { - return nil, fmt.Errorf("could not list pull secrets for worker Pod: %v", err) - } - volumes := []v1.Volume{ { Name: volumeNameConfig, @@ -1088,7 +1080,7 @@ func (p *podManagerImpl) baseWorkerPod(ctx context.Context, nmc client.Object, i { Name: workerContainerName, Image: p.workerImage, - VolumeMounts: append(volumeMounts, psvm...), + VolumeMounts: volumeMounts, Resources: v1.ResourceRequirements{ Requests: requests, Limits: limits, @@ -1098,17 +1090,21 @@ func (p *podManagerImpl) baseWorkerPod(ctx context.Context, nmc client.Object, i NodeName: nodeName, RestartPolicy: v1.RestartPolicyOnFailure, ServiceAccountName: item.ServiceAccountName, - Volumes: append(volumes, psv...), + Volumes: volumes, }, } - if err = ctrl.SetControllerReference(nmc, &pod, p.scheme); err != nil { + if item.ImageRepoSecret != nil { + pod.Spec.ImagePullSecrets = []v1.LocalObjectReference{*item.ImageRepoSecret} + } + + if err := ctrl.SetControllerReference(nmc, &pod, p.scheme); err != nil { return nil, fmt.Errorf("could not set the owner as controller: %v", err) } kmodsPathContainerImg := filepath.Join(moduleConfig.Modprobe.DirName, "lib", "modules", moduleConfig.KernelVersion) kmodsPathWorkerImg := filepath.Join(sharedFilesDir, moduleConfig.Modprobe.DirName, "lib", "modules") - if err = addCopyCommand(&pod, kmodsPathContainerImg, kmodsPathWorkerImg); err != nil { + if err := addCopyCommand(&pod, kmodsPathContainerImg, kmodsPathWorkerImg); err != nil { return nil, fmt.Errorf("could not add the copy command to the init container: %v", err) } @@ -1260,97 +1256,3 @@ func getModulesOrderAnnotationValue(modulesNames []string) string { } return softDepData.String() } - -//go:generate mockgen -source=nmc_reconciler.go -package=controllers -destination=mock_nmc_reconciler.go pullSecretHelper - -type pullSecretHelper interface { - VolumesAndVolumeMounts(ctx context.Context, nms *kmmv1beta1.ModuleItem) ([]v1.Volume, []v1.VolumeMount, error) -} - -type pullSecretHelperImpl struct { - client client.Client -} - -func (p *pullSecretHelperImpl) VolumesAndVolumeMounts(ctx context.Context, item *kmmv1beta1.ModuleItem) ([]v1.Volume, []v1.VolumeMount, error) { - logger := ctrl.LoggerFrom(ctx) - - secretNames := sets.New[string]() - - type pullSecret struct { - secretName string - volumeName string - optional bool - } - - pullSecrets := make([]pullSecret, 0) - - if irs := item.ImageRepoSecret; irs != nil { - secretNames.Insert(irs.Name) - - ps := pullSecret{ - secretName: irs.Name, - volumeName: volNameImageRepoSecret, - } - - pullSecrets = append(pullSecrets, ps) - } - - if san := item.ServiceAccountName; san != "" { - sa := v1.ServiceAccount{} - nsn := types.NamespacedName{Namespace: item.Namespace, Name: san} - - logger.V(1).Info("Getting service account", "name", nsn) - - if err := p.client.Get(ctx, nsn, &sa); err != nil { - return nil, nil, fmt.Errorf("could not get ServiceAccount %s: %v", nsn, err) - } - - for _, s := range sa.ImagePullSecrets { - if secretNames.Has(s.Name) { - continue - } - - secretNames.Insert(s.Name) - - hashValue, err := hashstructure.Hash(s.Name, hashstructure.FormatV2, nil) - if err != nil { - return nil, nil, fmt.Errorf("failed to hash secret %s: %v", s.Name, err) - } - - ps := pullSecret{ - secretName: s.Name, - volumeName: fmt.Sprintf("pull-secret-%d", hashValue), - optional: true, // to match the node's container runtime behaviour - } - - pullSecrets = append(pullSecrets, ps) - } - } - - volumes := make([]v1.Volume, 0, len(pullSecrets)) - volumeMounts := make([]v1.VolumeMount, 0, len(pullSecrets)) - - for _, s := range pullSecrets { - v := v1.Volume{ - Name: s.volumeName, - VolumeSource: v1.VolumeSource{ - Secret: &v1.SecretVolumeSource{ - SecretName: s.secretName, - Optional: ptr.To(s.optional), - }, - }, - } - - volumes = append(volumes, v) - - vm := v1.VolumeMount{ - Name: s.volumeName, - ReadOnly: true, - MountPath: filepath.Join(worker.PullSecretsDir, s.secretName), - } - - volumeMounts = append(volumeMounts, vm) - } - - return volumes, volumeMounts, nil -} diff --git a/internal/controllers/nmc_reconciler_test.go b/internal/controllers/nmc_reconciler_test.go index 5997b57d9..21e28b861 100644 --- a/internal/controllers/nmc_reconciler_test.go +++ b/internal/controllers/nmc_reconciler_test.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "path/filepath" "reflect" "strings" "time" @@ -19,7 +18,6 @@ import ( "github.com/kubernetes-sigs/kernel-module-management/internal/constants" "github.com/kubernetes-sigs/kernel-module-management/internal/nmc" "github.com/kubernetes-sigs/kernel-module-management/internal/utils" - "github.com/kubernetes-sigs/kernel-module-management/internal/worker" "github.com/mitchellh/hashstructure/v2" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -1591,7 +1589,6 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() { var ( ctrl *gomock.Controller client *testclient.MockClient - psh *MockpullSecretHelper nmc *kmmv1beta1.NodeModulesConfig mi kmmv1beta1.ModuleItem @@ -1604,7 +1601,6 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() { ctrl = gomock.NewController(GinkgoT()) client = testclient.NewMockClient(ctrl) - psh = NewMockpullSecretHelper(ctrl) nmc = &kmmv1beta1.NodeModulesConfig{ ObjectMeta: metav1.ObjectMeta{Name: nmcName}, @@ -1630,11 +1626,8 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() { Config: moduleConfigToUse, } - psh.EXPECT().VolumesAndVolumeMounts(ctx, &mi) - pm := &podManagerImpl{ client: client, - psh: psh, scheme: scheme, workerImage: workerImage, workerCfg: workerCfg, @@ -1660,7 +1653,7 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() { Config: moduleConfigToUse, } - expected := getBaseWorkerPod("load", nmc, firmwareHostPath, withFirmwareLoading, true) + expected := getBaseWorkerPod("load", nmc, firmwareHostPath, withFirmwareLoading, true, mi.ImageRepoSecret) Expect( controllerutil.SetControllerReference(nmc, expected, scheme), @@ -1693,7 +1686,6 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() { expected.Annotations[hashAnnotationKey] = fmt.Sprintf("%d", hash) gomock.InOrder( - psh.EXPECT().VolumesAndVolumeMounts(ctx, &mi), client.EXPECT().Create(ctx, cmpmock.DiffEq(expected)), ) @@ -1702,7 +1694,6 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() { pm := &podManagerImpl{ client: client, - psh: psh, scheme: scheme, workerImage: workerImage, workerCfg: &workerCfg, @@ -1728,10 +1719,8 @@ var _ = Describe("podManagerImpl_CreateUnloaderPod", func() { var ( ctrl *gomock.Controller client *testclient.MockClient - psh *MockpullSecretHelper - - nmc *kmmv1beta1.NodeModulesConfig - mi kmmv1beta1.ModuleItem + nmc *kmmv1beta1.NodeModulesConfig + mi kmmv1beta1.ModuleItem ctx context.Context moduleConfigToUse kmmv1beta1.ModuleConfig @@ -1742,7 +1731,6 @@ var _ = Describe("podManagerImpl_CreateUnloaderPod", func() { ctrl = gomock.NewController(GinkgoT()) client = testclient.NewMockClient(ctrl) - psh = NewMockpullSecretHelper(ctrl) nmc = &kmmv1beta1.NodeModulesConfig{ ObjectMeta: metav1.ObjectMeta{Name: nmcName}, @@ -1766,10 +1754,7 @@ var _ = Describe("podManagerImpl_CreateUnloaderPod", func() { It("it should fail if firmwareClassPath was not set but firmware loading was", func() { - psh.EXPECT().VolumesAndVolumeMounts(ctx, &mi) - pm := newPodManager(client, workerImage, scheme, workerCfg) - pm.(*podManagerImpl).psh = psh Expect( pm.CreateUnloaderPod(ctx, nmc, status), @@ -1780,7 +1765,7 @@ var _ = Describe("podManagerImpl_CreateUnloaderPod", func() { It("should work as expected", func() { - expected := getBaseWorkerPod("unload", nmc, ptr.To("/lib/firmware"), true, false) + expected := getBaseWorkerPod("unload", nmc, ptr.To("/lib/firmware"), true, false, mi.ImageRepoSecret) container, _ := podcmd.FindContainerByName(expected, "worker") Expect(container).NotTo(BeNil()) @@ -1798,16 +1783,12 @@ var _ = Describe("podManagerImpl_CreateUnloaderPod", func() { expected.Annotations[hashAnnotationKey] = fmt.Sprintf("%d", hash) - gomock.InOrder( - psh.EXPECT().VolumesAndVolumeMounts(ctx, &mi), - client.EXPECT().Create(ctx, cmpmock.DiffEq(expected)), - ) + client.EXPECT().Create(ctx, cmpmock.DiffEq(expected)) workerCfg := *workerCfg workerCfg.FirmwareHostPath = ptr.To("/lib/firmware") pm := newPodManager(client, workerImage, scheme, &workerCfg) - pm.(*podManagerImpl).psh = psh Expect( pm.CreateUnloaderPod(ctx, nmc, status), @@ -1908,7 +1889,7 @@ var _ = Describe("podManagerImpl_ListWorkerPodsOnNode", func() { }) func getBaseWorkerPod(subcommand string, owner ctrlclient.Object, firmwareHostPath *string, - withFirmware, isLoaderPod bool) *v1.Pod { + withFirmware, isLoaderPod bool, imagePullSecret *v1.LocalObjectReference) *v1.Pod { GinkgoHelper() const ( @@ -2119,6 +2100,10 @@ cp -R /firmware-path/* /tmp/firmware-path; } + if imagePullSecret != nil { + pod.Spec.ImagePullSecrets = []v1.LocalObjectReference{*imagePullSecret} + } + Expect( controllerutil.SetControllerReference(owner, &pod, scheme), ).NotTo( @@ -2130,98 +2115,6 @@ cp -R /firmware-path/* /tmp/firmware-path; return &pod } -var _ = Describe("pullSecretHelperImpl_VolumesAndVolumeMounts", func() { - It("should work as expected", func() { - ctrl := gomock.NewController(GinkgoT()) - kubeClient := testclient.NewMockClient(ctrl) - psh := pullSecretHelperImpl{client: kubeClient} - - const ( - irs = "pull-secret-0" - saPullSecret1 = "pull-secret-1" - saPullSecret2 = "pull-secret-2" - ) - - saPullSecret1Hash, err := hashstructure.Hash(saPullSecret1, hashstructure.FormatV2, nil) - Expect(err).To(BeNil()) - saPullSecret2Hash, err := hashstructure.Hash(saPullSecret2, hashstructure.FormatV2, nil) - Expect(err).To(BeNil()) - - item := kmmv1beta1.ModuleItem{ - ImageRepoSecret: &v1.LocalObjectReference{Name: irs}, - Namespace: namespace, - ServiceAccountName: serviceAccountName, - } - - ctx := context.TODO() - nsn := types.NamespacedName{Namespace: namespace, Name: serviceAccountName} - - kubeClient. - EXPECT(). - Get(ctx, nsn, &v1.ServiceAccount{}). - Do(func(_ context.Context, _ types.NamespacedName, sa *v1.ServiceAccount, _ ...ctrlclient.GetOption) { - sa.ImagePullSecrets = []v1.LocalObjectReference{ - {Name: saPullSecret1}, - {Name: saPullSecret1}, // intentional duplicate, should not be in the volume list - {Name: saPullSecret2}, - } - }) - - vols := []v1.Volume{ - { - Name: volNameImageRepoSecret, - VolumeSource: v1.VolumeSource{ - Secret: &v1.SecretVolumeSource{ - SecretName: irs, - Optional: ptr.To(false), - }, - }, - }, - { - Name: fmt.Sprintf("pull-secret-%d", saPullSecret1Hash), - VolumeSource: v1.VolumeSource{ - Secret: &v1.SecretVolumeSource{ - SecretName: saPullSecret1, - Optional: ptr.To(true), - }, - }, - }, - { - Name: fmt.Sprintf("pull-secret-%d", saPullSecret2Hash), - VolumeSource: v1.VolumeSource{ - Secret: &v1.SecretVolumeSource{ - SecretName: saPullSecret2, - Optional: ptr.To(true), - }, - }, - }, - } - - volMounts := []v1.VolumeMount{ - { - Name: volNameImageRepoSecret, - ReadOnly: true, - MountPath: filepath.Join(worker.PullSecretsDir, irs), - }, - { - Name: fmt.Sprintf("pull-secret-%d", saPullSecret1Hash), - ReadOnly: true, - MountPath: filepath.Join(worker.PullSecretsDir, saPullSecret1), - }, - { - Name: fmt.Sprintf("pull-secret-%d", saPullSecret2Hash), - ReadOnly: true, - MountPath: filepath.Join(worker.PullSecretsDir, saPullSecret2), - }, - } - - resVols, resVolMounts, err := psh.VolumesAndVolumeMounts(ctx, &item) - Expect(err).NotTo(HaveOccurred()) - Expect(resVols).To(BeComparableTo(vols)) - Expect(resVolMounts).To(BeComparableTo(volMounts)) - }) -}) - var _ = Describe("getKernelModuleReadyLabels", func() { lph := newLabelPreparationHelper()