From 8709cccf719e79e376b00c70b4e9d5444cefc4c3 Mon Sep 17 00:00:00 2001 From: Markus Lehtonen Date: Thu, 4 Apr 2024 17:48:47 +0300 Subject: [PATCH] nfd-master: parse kubeconfig even with NoPublish set Don't try to be too smart when kubeconfig is needed. In practice, the nfd-master really doesn't work anymore (with the NodeFeature API enabled) without a kubeconfig set. This patch fixes crashes happening when NoPublish is enabled, e.g. in listing all nodes in the nfd api handler and in getting single node objects in the node updater pool. This patch changes the kubeconfig parsing to happen at the creation of the nfd-master instance. We don't need to do that at reconfigure time as none of the dynamic config options affect it. Unit tests are adjusted, accordingly. --- pkg/nfd-master/nfd-master-internal_test.go | 33 +++++++++++---------- pkg/nfd-master/nfd-master.go | 34 ++++++++++++---------- pkg/nfd-master/nfd-master_test.go | 9 +++++- pkg/nfd-worker/nfd-worker_test.go | 5 +++- 4 files changed, 47 insertions(+), 34 deletions(-) diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go index 96fb7eb2e6..4a13e84b6c 100644 --- a/pkg/nfd-master/nfd-master-internal_test.go +++ b/pkg/nfd-master/nfd-master-internal_test.go @@ -40,7 +40,6 @@ import ( fakecorev1client "k8s.io/client-go/kubernetes/typed/core/v1/fake" clienttesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/cache" - "k8s.io/klog/v2" nfdv1alpha1 "sigs.k8s.io/node-feature-discovery/pkg/apis/nfd/v1alpha1" "sigs.k8s.io/node-feature-discovery/pkg/features" @@ -115,6 +114,7 @@ func newFakeMaster(opts ...NfdMasterOption) *nfdMaster { defaultOpts := []NfdMasterOption{ withNodeName(testNodeName), withConfig(&NFDConfig{}), + WithKubernetesClient(fakeclient.NewSimpleClientset()), } m, err := NewNfdMaster(append(defaultOpts, opts...)...) if err != nil { @@ -664,6 +664,10 @@ leaderElection: func TestDynamicConfig(t *testing.T) { Convey("When running nfd-master", t, func() { + // Add feature gates as running nfd-master depends on that + err := features.NFDMutableFeatureGate.Add(features.DefaultNFDFeatureGates) + So(err, ShouldBeNil) + tmpDir, err := os.MkdirTemp("", "*.nfd-test") So(err, ShouldBeNil) defer os.RemoveAll(tmpDir) @@ -674,7 +678,7 @@ func TestDynamicConfig(t *testing.T) { So(err, ShouldBeNil) // Create config file - configFile := filepath.Join(configDir, "master.conf") + configFile := filepath.Clean(filepath.Join(configDir, "master.conf")) writeConfig := func(data string) { f, err := os.Create(configFile) @@ -685,25 +689,22 @@ func TestDynamicConfig(t *testing.T) { So(err, ShouldBeNil) } writeConfig(` +klog: + v: "4" extraLabelNs: ["added.ns.io"] `) - noPublish := true - // Add FeatureGates flag - if err := features.NFDMutableFeatureGate.Add(features.DefaultNFDFeatureGates); err != nil { - klog.ErrorS(err, "failed to add default feature gates") - os.Exit(1) - } - _ = features.NFDMutableFeatureGate.OverrideDefault(features.NodeFeatureAPI, false) - master := newFakeMaster(WithArgs(&Args{ - ConfigFile: configFile, - Overrides: ConfigOverrideArgs{ - NoPublish: &noPublish, - }, - })) + master := newFakeMaster( + WithArgs(&Args{ConfigFile: configFile}), + WithKubernetesClient(fakeclient.NewSimpleClientset(newTestNode()))) Convey("config file updates should take effect", func() { - go func() { _ = master.Run() }() + go func() { + Convey("nfd-master should exit gracefully", t, func() { + err = master.Run() + So(err, ShouldBeNil) + }) + }() defer master.Stop() // Check initial config time.Sleep(10 * time.Second) diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go index ede51a2103..c964b2a7f3 100644 --- a/pkg/nfd-master/nfd-master.go +++ b/pkg/nfd-master/nfd-master.go @@ -194,6 +194,19 @@ func NewNfdMaster(opts ...NfdMasterOption) (NfdMaster, error) { nfd.configFilePath = filepath.Clean(nfd.args.ConfigFile) } + // k8sClient might've been set via opts by tests + if nfd.k8sClient == nil { + kubeconfig, err := utils.GetKubeconfig(nfd.args.Kubeconfig) + if err != nil { + return nfd, err + } + cli, err := k8sclient.NewForConfig(kubeconfig) + if err != nil { + return nfd, err + } + nfd.k8sClient = cli + } + nfd.nodeUpdaterPool = newNodeUpdaterPool(nfd) return nfd, nil @@ -770,10 +783,6 @@ func (m *nfdMaster) nfdAPIUpdateOneNode(node *corev1.Node) error { return objs[i].Namespace < objs[j].Namespace }) - if m.config.NoPublish { - return nil - } - klog.V(1).InfoS("processing of node initiated by NodeFeature API", "nodeName", node.Name) features := nfdv1alpha1.NewNodeFeatureSpec() @@ -875,6 +884,11 @@ func (m *nfdMaster) refreshNodeFeatures(node *corev1.Node, labels map[string]str taints = filterTaints(crTaints) } + if m.config.NoPublish { + klog.V(1).InfoS("node update skipped, NoPublish=true", "nodeName", node.Name) + return nil + } + err := m.updateNodeObject(node, labels, annotations, extendedResources, taints) if err != nil { klog.ErrorS(err, "failed to update node", "nodeName", node.Name) @@ -1250,18 +1264,6 @@ func (m *nfdMaster) configure(filepath string, overrides string) error { return err } - if !c.NoPublish { - kubeconfig, err := utils.GetKubeconfig(m.args.Kubeconfig) - if err != nil { - return err - } - cli, err := k8sclient.NewForConfig(kubeconfig) - if err != nil { - return err - } - m.k8sClient = cli - } - // Pre-process DenyLabelNS into 2 lists: one for normal ns, and the other for wildcard ns normalDeniedNs, wildcardDeniedNs := preProcessDeniedNamespaces(c.DenyLabelNs) m.deniedNs.normal = normalDeniedNs diff --git a/pkg/nfd-master/nfd-master_test.go b/pkg/nfd-master/nfd-master_test.go index e1ac515e31..412a8a2331 100644 --- a/pkg/nfd-master/nfd-master_test.go +++ b/pkg/nfd-master/nfd-master_test.go @@ -20,6 +20,7 @@ import ( "testing" . "github.com/smartystreets/goconvey/convey" + fakeclient "k8s.io/client-go/kubernetes/fake" m "sigs.k8s.io/node-feature-discovery/pkg/nfd-master" ) @@ -36,7 +37,13 @@ func TestNewNfdMaster(t *testing.T) { }) }) Convey("When -config is supplied", func() { - _, err := m.NewNfdMaster(m.WithArgs(&m.Args{CertFile: "crt", KeyFile: "key", CaFile: "ca", ConfigFile: "master-config.yaml"})) + _, err := m.NewNfdMaster( + m.WithArgs(&m.Args{ + CertFile: "crt", + KeyFile: "key", + CaFile: "ca", + ConfigFile: "master-config.yaml"}), + m.WithKubernetesClient(fakeclient.NewSimpleClientset())) Convey("An error should not be returned", func() { So(err, ShouldBeNil) }) diff --git a/pkg/nfd-worker/nfd-worker_test.go b/pkg/nfd-worker/nfd-worker_test.go index dc525ca46f..219098d0e0 100644 --- a/pkg/nfd-worker/nfd-worker_test.go +++ b/pkg/nfd-worker/nfd-worker_test.go @@ -24,6 +24,7 @@ import ( "time" . "github.com/smartystreets/goconvey/convey" + fakeclient "k8s.io/client-go/kubernetes/fake" "k8s.io/klog/v2" "sigs.k8s.io/node-feature-discovery/pkg/features" @@ -52,7 +53,9 @@ func setupTest(args *master.Args) testContext { os.Exit(1) } _ = features.NFDMutableFeatureGate.OverrideDefault(features.NodeFeatureAPI, false) - m, err := master.NewNfdMaster(master.WithArgs(args)) + m, err := master.NewNfdMaster( + master.WithArgs(args), + master.WithKubernetesClient(fakeclient.NewSimpleClientset())) if err != nil { fmt.Printf("Test setup failed: %v\n", err) os.Exit(1)