nfd-master: implement opts for modifying NfdMaster instance

This provides a more controlled way for setting up the NfdMaster
instance for testing.

Author: marquiz

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)

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)

pkg/nfd-master/nfd-master.go

@@ -157,44 +157,72 @@ 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)
 
 	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{},

pkg/nfd-master/nfd-master_test.go

@@ -26,17 +26,17 @@ 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)
 				So(err3, ShouldNotBeNil)
 			})
 		})
 		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)
 			})

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)
 

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)