From fcb8d3cda4df0a554bc428e09b2c7fd7184c7db5 Mon Sep 17 00:00:00 2001 From: Markus Lehtonen Date: Fri, 5 Apr 2024 13:24:54 +0300 Subject: [PATCH] nfd-master: implement opts for modifying NfdMaster instance This provides a more controlled way for setting up the NfdMaster instance for testing. --- cmd/nfd-master/main.go | 2 +- pkg/nfd-master/nfd-master-internal_test.go | 71 ++++++++++++---------- pkg/nfd-master/nfd-master.go | 50 +++++++++++---- pkg/nfd-master/nfd-master_test.go | 8 +-- pkg/nfd-master/node-updater-pool_test.go | 6 +- pkg/nfd-worker/nfd-worker_test.go | 2 +- 6 files changed, 88 insertions(+), 51 deletions(-) diff --git a/cmd/nfd-master/main.go b/cmd/nfd-master/main.go index 40dba05884..08a35324f9 100644 --- a/cmd/nfd-master/main.go +++ b/cmd/nfd-master/main.go @@ -109,7 +109,7 @@ func main() { // Get new NfdMaster instance args.GrpcHealthPort = GrpcHealthPort - instance, err := master.NewNfdMaster(args) + instance, err := master.NewNfdMaster(master.WithArgs(args)) if err != nil { klog.ErrorS(err, "failed to initialize NfdMaster instance") os.Exit(1) diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go index 5d2e1642a4..96fb7eb2e6 100644 --- a/pkg/nfd-master/nfd-master-internal_test.go +++ b/pkg/nfd-master/nfd-master-internal_test.go @@ -36,7 +36,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" - k8sclient "k8s.io/client-go/kubernetes" fakeclient "k8s.io/client-go/kubernetes/fake" fakecorev1client "k8s.io/client-go/kubernetes/typed/core/v1/fake" clienttesting "k8s.io/client-go/testing" @@ -104,12 +103,24 @@ func newFakeNfdAPIController(client *fakenfdclient.Clientset) *nfdController { return c } -func newFakeMaster(cli k8sclient.Interface) *nfdMaster { - return &nfdMaster{ - nodeName: testNodeName, - config: &NFDConfig{}, - k8sClient: cli, +func withNodeName(nodeName string) NfdMasterOption { + return &nfdMasterOpt{f: func(n *nfdMaster) { n.nodeName = nodeName }} +} + +func withConfig(config *NFDConfig) NfdMasterOption { + return &nfdMasterOpt{f: func(n *nfdMaster) { n.config = config }} +} + +func newFakeMaster(opts ...NfdMasterOption) *nfdMaster { + defaultOpts := []NfdMasterOption{ + withNodeName(testNodeName), + withConfig(&NFDConfig{}), + } + m, err := NewNfdMaster(append(defaultOpts, opts...)...) + if err != nil { + panic(err) } + return m.(*nfdMaster) } func TestUpdateNodeObject(t *testing.T) { @@ -153,7 +164,7 @@ func TestUpdateNodeObject(t *testing.T) { // Create fake api client and initialize NfdMaster instance fakeCli := fakeclient.NewSimpleClientset(testNode) - fakeMaster := newFakeMaster(fakeCli) + fakeMaster := newFakeMaster(WithKubernetesClient(fakeCli)) Convey("When I successfully update the node with feature labels", func() { err := fakeMaster.updateNodeObject(testNode, featureLabels, featureAnnotations, featureExtResources, nil) @@ -205,7 +216,7 @@ func TestUpdateMasterNode(t *testing.T) { Convey("When update operation succeeds", func() { fakeCli := fakeclient.NewSimpleClientset(testNode) - fakeMaster := newFakeMaster(fakeCli) + fakeMaster := newFakeMaster(WithKubernetesClient(fakeCli)) err := fakeMaster.updateMasterNode() Convey("No error should be returned", func() { So(err, ShouldBeNil) @@ -220,7 +231,7 @@ func TestUpdateMasterNode(t *testing.T) { Convey("When getting API node object fails", func() { fakeCli := fakeclient.NewSimpleClientset(testNode) - fakeMaster := newFakeMaster(fakeCli) + fakeMaster := newFakeMaster(WithKubernetesClient(fakeCli)) fakeMaster.nodeName = "does-not-exist" err := fakeMaster.updateMasterNode() @@ -235,7 +246,7 @@ func TestUpdateMasterNode(t *testing.T) { fakeCli.CoreV1().(*fakecorev1client.FakeCoreV1).PrependReactor("patch", "nodes", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { return true, &v1.Node{}, fakeErr }) - fakeMaster := newFakeMaster(fakeCli) + fakeMaster := newFakeMaster(WithKubernetesClient(fakeCli)) err := fakeMaster.updateMasterNode() Convey("An error should be returned", func() { @@ -247,7 +258,7 @@ func TestUpdateMasterNode(t *testing.T) { func TestAddingExtResources(t *testing.T) { Convey("When adding extended resources", t, func() { - fakeMaster := newFakeMaster(nil) + fakeMaster := newFakeMaster() Convey("When there are no matching labels", func() { testNode := newTestNode() resourceLabels := ExtendedResources{} @@ -290,7 +301,7 @@ func TestAddingExtResources(t *testing.T) { func TestRemovingExtResources(t *testing.T) { Convey("When removing extended resources", t, func() { - fakeMaster := newFakeMaster(nil) + fakeMaster := newFakeMaster() Convey("When none are removed", func() { testNode := newTestNode() resourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-1": "1", nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"} @@ -339,7 +350,7 @@ func TestSetLabels(t *testing.T) { Convey("When node update succeeds", func() { fakeCli := fakeclient.NewSimpleClientset(testNode) - fakeMaster := newFakeMaster(fakeCli) + fakeMaster := newFakeMaster(WithKubernetesClient(fakeCli)) _, err := fakeMaster.SetLabels(ctx, req) Convey("No error should be returned", func() { @@ -354,11 +365,13 @@ func TestSetLabels(t *testing.T) { Convey("When -resource-labels is specified", func() { fakeCli := fakeclient.NewSimpleClientset(testNode) - fakeMaster := newFakeMaster(fakeCli) - fakeMaster.config.ResourceLabels = map[string]struct{}{ - "feature.node.kubernetes.io/feature-3": {}, - "feature-1": {}, - } + fakeMaster := newFakeMaster( + WithKubernetesClient(fakeCli), + withConfig(&NFDConfig{ + ResourceLabels: map[string]struct{}{ + "feature.node.kubernetes.io/feature-3": {}, + "feature-1": {}}, + })) _, err := fakeMaster.SetLabels(ctx, req) Convey("Error is nil", func() { @@ -374,7 +387,7 @@ func TestSetLabels(t *testing.T) { Convey("When node update fails", func() { fakeCli := fakeclient.NewSimpleClientset(testNode) - fakeMaster := newFakeMaster(fakeCli) + fakeMaster := newFakeMaster(WithKubernetesClient(fakeCli)) fakeErr := errors.New("Fake error when patching node") fakeCli.CoreV1().(*fakecorev1client.FakeCoreV1).PrependReactor("patch", "nodes", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) { @@ -388,7 +401,7 @@ func TestSetLabels(t *testing.T) { Convey("With '-no-publish'", func() { fakeCli := fakeclient.NewSimpleClientset(testNode) - fakeMaster := newFakeMaster(fakeCli) + fakeMaster := newFakeMaster(WithKubernetesClient(fakeCli)) fakeMaster.config.NoPublish = true _, err := fakeMaster.SetLabels(ctx, req) @@ -400,7 +413,7 @@ func TestSetLabels(t *testing.T) { } func TestFilterLabels(t *testing.T) { - fakeMaster := newFakeMaster(nil) + fakeMaster := newFakeMaster() fakeMaster.config.ExtraLabelNs = map[string]struct{}{"example.io": {}} fakeMaster.deniedNs = deniedNs{ normal: map[string]struct{}{"": struct{}{}, "kubernetes.io": struct{}{}, "denied.ns": struct{}{}}, @@ -571,9 +584,7 @@ func TestRemoveLabelsWithPrefix(t *testing.T) { func TestConfigParse(t *testing.T) { Convey("When parsing configuration", t, func() { - m, err := NewNfdMaster(&Args{}) - So(err, ShouldBeNil) - master := m.(*nfdMaster) + master := newFakeMaster() overrides := `{"noPublish": true, "enableTaints": true, "extraLabelNs": ["added.ns.io","added.kubernetes.io"], "denyLabelNs": ["denied.ns.io","denied.kubernetes.io"], "resourceLabels": ["vendor-1.com/feature-1","vendor-2.io/feature-2"], "labelWhiteList": "foo"}` Convey("and no core cmdline flags have been specified", func() { @@ -684,18 +695,16 @@ extraLabelNs: ["added.ns.io"] os.Exit(1) } _ = features.NFDMutableFeatureGate.OverrideDefault(features.NodeFeatureAPI, false) - m, err := NewNfdMaster(&Args{ + master := newFakeMaster(WithArgs(&Args{ ConfigFile: configFile, Overrides: ConfigOverrideArgs{ NoPublish: &noPublish, }, - }) - So(err, ShouldBeNil) - master := m.(*nfdMaster) + })) Convey("config file updates should take effect", func() { - go func() { _ = m.Run() }() - defer m.Stop() + go func() { _ = master.Run() }() + defer master.Stop() // Check initial config time.Sleep(10 * time.Second) So(func() interface{} { return master.config.ExtraLabelNs }, @@ -759,7 +768,7 @@ func newTestNodeList() *corev1.NodeList { func BenchmarkNfdAPIUpdateAllNodes(b *testing.B) { fakeCli := fakeclient.NewSimpleClientset(newTestNodeList()) - fakeMaster := newFakeMaster(fakeCli) + fakeMaster := newFakeMaster(WithKubernetesClient(fakeCli)) fakeMaster.nfdController = newFakeNfdAPIController(fakenfdclient.NewSimpleClientset()) nodeUpdaterPool := newNodeUpdaterPool(fakeMaster) diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go index 6c45fcc68a..ede51a2103 100644 --- a/pkg/nfd-master/nfd-master.go +++ b/pkg/nfd-master/nfd-master.go @@ -157,37 +157,41 @@ type nfdMaster struct { } // NewNfdMaster creates a new NfdMaster server instance. -func NewNfdMaster(args *Args) (NfdMaster, error) { - nfd := &nfdMaster{args: *args, +func NewNfdMaster(opts ...NfdMasterOption) (NfdMaster, error) { + nfd := &nfdMaster{ nodeName: utils.NodeName(), namespace: utils.GetKubernetesNamespace(), ready: make(chan struct{}), stop: make(chan struct{}), } - if args.Instance != "" { - if ok, _ := regexp.MatchString(`^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$`, args.Instance); !ok { + for _, o := range opts { + o.apply(nfd) + } + + if nfd.args.Instance != "" { + if ok, _ := regexp.MatchString(`^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$`, nfd.args.Instance); !ok { return nfd, fmt.Errorf("invalid -instance %q: instance name "+ "must start and end with an alphanumeric character and may only contain "+ - "alphanumerics, `-`, `_` or `.`", args.Instance) + "alphanumerics, `-`, `_` or `.`", nfd.args.Instance) } } // Check TLS related args - if args.CertFile != "" || args.KeyFile != "" || args.CaFile != "" { - if args.CertFile == "" { + if nfd.args.CertFile != "" || nfd.args.KeyFile != "" || nfd.args.CaFile != "" { + if nfd.args.CertFile == "" { return nfd, fmt.Errorf("-cert-file needs to be specified alongside -key-file and -ca-file") } - if args.KeyFile == "" { + if nfd.args.KeyFile == "" { return nfd, fmt.Errorf("-key-file needs to be specified alongside -cert-file and -ca-file") } - if args.CaFile == "" { + if nfd.args.CaFile == "" { return nfd, fmt.Errorf("-ca-file needs to be specified alongside -cert-file and -key-file") } } - if args.ConfigFile != "" { - nfd.configFilePath = filepath.Clean(args.ConfigFile) + if nfd.args.ConfigFile != "" { + nfd.configFilePath = filepath.Clean(nfd.args.ConfigFile) } nfd.nodeUpdaterPool = newNodeUpdaterPool(nfd) @@ -195,6 +199,30 @@ func NewNfdMaster(args *Args) (NfdMaster, error) { return nfd, nil } +// NfdMasterOption sets properties of the NfdMaster instance. +type NfdMasterOption interface { + apply(*nfdMaster) +} + +// WithArgs is used for passing settings from command line arguments. +func WithArgs(args *Args) NfdMasterOption { + return &nfdMasterOpt{f: func(n *nfdMaster) { n.args = *args }} +} + +// WithKuberneteClient forces to use the given kubernetes client, without +// initializing one from kubeconfig. +func WithKubernetesClient(cli k8sclient.Interface) NfdMasterOption { + return &nfdMasterOpt{f: func(n *nfdMaster) { n.k8sClient = cli }} +} + +type nfdMasterOpt struct { + f func(*nfdMaster) +} + +func (f *nfdMasterOpt) apply(n *nfdMaster) { + f.f(n) +} + func newDefaultConfig() *NFDConfig { return &NFDConfig{ DenyLabelNs: utils.StringSetVal{}, diff --git a/pkg/nfd-master/nfd-master_test.go b/pkg/nfd-master/nfd-master_test.go index b15721601a..e1ac515e31 100644 --- a/pkg/nfd-master/nfd-master_test.go +++ b/pkg/nfd-master/nfd-master_test.go @@ -26,9 +26,9 @@ import ( func TestNewNfdMaster(t *testing.T) { Convey("When initializing new NfdMaster instance", t, func() { Convey("When one of -cert-file, -key-file or -ca-file is missing", func() { - _, err := m.NewNfdMaster(&m.Args{CertFile: "crt", KeyFile: "key"}) - _, err2 := m.NewNfdMaster(&m.Args{KeyFile: "key", CaFile: "ca"}) - _, err3 := m.NewNfdMaster(&m.Args{CertFile: "crt", CaFile: "ca"}) + _, err := m.NewNfdMaster(m.WithArgs(&m.Args{CertFile: "crt", KeyFile: "key"})) + _, err2 := m.NewNfdMaster(m.WithArgs(&m.Args{KeyFile: "key", CaFile: "ca"})) + _, err3 := m.NewNfdMaster(m.WithArgs(&m.Args{CertFile: "crt", CaFile: "ca"})) Convey("An error should be returned", func() { So(err, ShouldNotBeNil) So(err2, ShouldNotBeNil) @@ -36,7 +36,7 @@ func TestNewNfdMaster(t *testing.T) { }) }) Convey("When -config is supplied", func() { - _, err := m.NewNfdMaster(&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"})) Convey("An error should not be returned", func() { So(err, ShouldBeNil) }) diff --git a/pkg/nfd-master/node-updater-pool_test.go b/pkg/nfd-master/node-updater-pool_test.go index 5c6924bf66..3a21654f4d 100644 --- a/pkg/nfd-master/node-updater-pool_test.go +++ b/pkg/nfd-master/node-updater-pool_test.go @@ -34,7 +34,7 @@ func newFakeNodeUpdaterPool(nfdMaster *nfdMaster) *nodeUpdaterPool { } func TestNodeUpdaterStart(t *testing.T) { - fakeMaster := newFakeMaster(nil) + fakeMaster := newFakeMaster() nodeUpdaterPool := newFakeNodeUpdaterPool(fakeMaster) Convey("When starting the node updater pool", t, func() { @@ -53,7 +53,7 @@ func TestNodeUpdaterStart(t *testing.T) { } func TestNodeUpdaterStop(t *testing.T) { - fakeMaster := newFakeMaster(nil) + fakeMaster := newFakeMaster() nodeUpdaterPool := newFakeNodeUpdaterPool(fakeMaster) nodeUpdaterPool.start(10) @@ -70,7 +70,7 @@ func TestNodeUpdaterStop(t *testing.T) { } func TestRunNodeUpdater(t *testing.T) { - fakeMaster := newFakeMaster(fakek8sclient.NewSimpleClientset()) + fakeMaster := newFakeMaster(WithKubernetesClient(fakek8sclient.NewSimpleClientset())) fakeMaster.nfdController = newFakeNfdAPIController(fakenfdclient.NewSimpleClientset()) nodeUpdaterPool := newFakeNodeUpdaterPool(fakeMaster) diff --git a/pkg/nfd-worker/nfd-worker_test.go b/pkg/nfd-worker/nfd-worker_test.go index 4df97e7dbc..dc525ca46f 100644 --- a/pkg/nfd-worker/nfd-worker_test.go +++ b/pkg/nfd-worker/nfd-worker_test.go @@ -52,7 +52,7 @@ func setupTest(args *master.Args) testContext { os.Exit(1) } _ = features.NFDMutableFeatureGate.OverrideDefault(features.NodeFeatureAPI, false) - m, err := master.NewNfdMaster(args) + m, err := master.NewNfdMaster(master.WithArgs(args)) if err != nil { fmt.Printf("Test setup failed: %v\n", err) os.Exit(1)