From 47033db9c1ac86f6201ba61786e4b68d1544e434 Mon Sep 17 00:00:00 2001 From: Markus Lehtonen Date: Fri, 19 Feb 2021 07:38:55 +0200 Subject: [PATCH] nfd-master: use flag for command line parsing --- cmd/nfd-master/main.go | 145 ++++++++------------- cmd/nfd-master/main_test.go | 55 -------- docs/advanced/developer-guide.md | 65 ++++----- pkg/nfd-master/nfd-master-internal_test.go | 9 +- pkg/nfd-master/nfd-master.go | 24 ++-- pkg/nfd-master/nfd-master_test.go | 8 +- pkg/nfd-worker/nfd-worker_test.go | 8 +- pkg/utils/flags.go | 62 +++++++++ 8 files changed, 171 insertions(+), 205 deletions(-) delete mode 100644 cmd/nfd-master/main_test.go create mode 100644 pkg/utils/flags.go diff --git a/cmd/nfd-master/main.go b/cmd/nfd-master/main.go index 9f66a37c3..6611d5ccd 100644 --- a/cmd/nfd-master/main.go +++ b/cmd/nfd-master/main.go @@ -1,5 +1,5 @@ /* -Copyright 2019 The Kubernetes Authors. +Copyright 2019-2021 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -17,14 +17,14 @@ limitations under the License. package main import ( + "flag" "fmt" "log" + "os" "regexp" - "strconv" - "strings" - "github.com/docopt/docopt-go" master "sigs.k8s.io/node-feature-discovery/pkg/nfd-master" + "sigs.k8s.io/node-feature-discovery/pkg/utils" "sigs.k8s.io/node-feature-discovery/pkg/version" ) @@ -34,17 +34,29 @@ const ( ) func main() { + flags := flag.NewFlagSet(ProgramName, flag.ExitOnError) + + printVersion := flags.Bool("version", false, "Print version and exit.") + + args := initFlags(flags) + + _ = flags.Parse(os.Args[1:]) + if len(flags.Args()) > 0 { + fmt.Printf("unknown command line argument: %s\n", flags.Args()[0]) + flags.Usage() + os.Exit(2) + } + + if *printVersion { + fmt.Println(ProgramName, version.Get()) + os.Exit(0) + } + // Assert that the version is known if version.Undefined() { log.Print("WARNING: version not set! Set -ldflags \"-X sigs.k8s.io/node-feature-discovery/pkg/version.version=`git describe --tags --dirty --always`\" during build or run.") } - // Parse command-line arguments. - args, err := argsParse(nil) - if err != nil { - log.Fatalf("failed to parse command line: %v", err) - } - // Get new NfdMaster instance instance, err := master.NewNfdMaster(args) if err != nil { @@ -56,88 +68,37 @@ func main() { } } -// argsParse parses the command line arguments passed to the program. -// The argument argv is passed only for testing purposes. -func argsParse(argv []string) (master.Args, error) { - args := master.Args{} - usage := fmt.Sprintf(`%s. - - Usage: - %s [--prune] [--no-publish] [--label-whitelist=] [--port=] - [--ca-file=] [--cert-file=] [--key-file=] - [--verify-node-name] [--extra-label-ns=] [--resource-labels=] - [--kubeconfig=] [--instance=] - %s -h | --help - %s --version - - Options: - -h --help Show this screen. - --version Output version and exit. - --prune Prune all NFD related attributes from all nodes - of the cluster and exit. - --instance= Instance name. Used to separate annotation - namespaces for multiple parallel deployments. - [Default: ] - --kubeconfig= Kubeconfig to use [Default: ] - of the cluster and exit. - --port= Port on which to listen for connections. - [Default: 8080] - --ca-file= Root certificate for verifying connections - [Default: ] - --cert-file= Certificate used for authenticating connections - [Default: ] - --key-file= Private key matching --cert-file - [Default: ] - --verify-node-name Verify worker node name against CN from the TLS - certificate. Only has effect when TLS authentication - has been enabled. - --no-publish Do not publish feature labels - --label-whitelist= Regular expression to filter label names to - publish to the Kubernetes API server. - NB: the label namespace is omitted i.e. the filter - is only applied to the name part after '/'. - [Default: ] - --extra-label-ns= Comma separated list of allowed extra label namespaces - [Default: ] - --resource-labels= Comma separated list of labels to be exposed as extended resources. - [Default: ]`, - ProgramName, - ProgramName, - ProgramName, - ProgramName, - ) - - arguments, _ := docopt.ParseArgs(usage, argv, - fmt.Sprintf("%s %s", ProgramName, version.Get())) - - // Parse argument values as usable types. - var err error - args.Instance = arguments["--instance"].(string) - if ok, _ := regexp.MatchString(`^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$`, args.Instance); args.Instance != "" && !ok { - return args, fmt.Errorf("invalid --instance %q: instance name "+ - "must start and end with an alphanumeric character and may only contain "+ - "alphanumerics, `-`, `_` or `.`", args.Instance) +func initFlags(flagset *flag.FlagSet) *master.Args { + args := &master.Args{ + LabelWhiteList: utils.RegexpVal{Regexp: *regexp.MustCompile("")}, } - args.CaFile = arguments["--ca-file"].(string) - args.CertFile = arguments["--cert-file"].(string) - args.KeyFile = arguments["--key-file"].(string) - args.NoPublish = arguments["--no-publish"].(bool) - args.Port, err = strconv.Atoi(arguments["--port"].(string)) - if err != nil { - return args, fmt.Errorf("invalid --port defined: %s", err) - } - args.LabelWhiteList, err = regexp.Compile(arguments["--label-whitelist"].(string)) - if err != nil { - return args, fmt.Errorf("error parsing whitelist regex (%s): %s", arguments["--label-whitelist"], err) - } - args.VerifyNodeName = arguments["--verify-node-name"].(bool) - args.ExtraLabelNs = map[string]struct{}{} - for _, n := range strings.Split(arguments["--extra-label-ns"].(string), ",") { - args.ExtraLabelNs[n] = struct{}{} - } - args.ResourceLabels = strings.Split(arguments["--resource-labels"].(string), ",") - args.Prune = arguments["--prune"].(bool) - args.Kubeconfig = arguments["--kubeconfig"].(string) - return args, nil + flagset.StringVar(&args.CaFile, "ca-file", "", + "Root certificate for verifying connections") + flagset.StringVar(&args.CertFile, "cert-file", "", + "Certificate used for authenticating connections") + flagset.Var(&args.ExtraLabelNs, "extra-label-ns", + "Comma separated list of allowed extra label namespaces") + flagset.StringVar(&args.Instance, "instance", "", + "Instance name. Used to separate annotation namespaces for multiple parallel deployments.") + flagset.StringVar(&args.KeyFile, "key-file", "", + "Private key matching -cert-file") + flagset.StringVar(&args.Kubeconfig, "kubeconfig", "", + "Kubeconfig to use") + flagset.Var(&args.LabelWhiteList, "label-whitelist", + "Regular expression to filter label names to publish to the Kubernetes API server. "+ + "NB: the label namespace is omitted i.e. the filter is only applied to the name part after '/'.") + flagset.BoolVar(&args.NoPublish, "no-publish", false, + "Do not publish feature labels") + flagset.IntVar(&args.Port, "port", 8080, + "Port on which to listen for connections.") + flagset.BoolVar(&args.Prune, "prune", false, + "Prune all NFD related attributes from all nodes of the cluaster and exit.") + flagset.Var(&args.ResourceLabels, "resource-labels", + "Comma separated list of labels to be exposed as extended resources.") + flagset.BoolVar(&args.VerifyNodeName, "verify-node-name", false, + "Verify worker node name against CN from the TLS certificate. "+ + "Only takes effect when TLS authentication has been enabled.") + + return args } diff --git a/cmd/nfd-master/main_test.go b/cmd/nfd-master/main_test.go deleted file mode 100644 index 57ab6bf4f..000000000 --- a/cmd/nfd-master/main_test.go +++ /dev/null @@ -1,55 +0,0 @@ -/* -Copyright 2019 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package main - -import ( - "testing" - - . "github.com/smartystreets/goconvey/convey" -) - -func TestArgsParse(t *testing.T) { - Convey("When parsing command line arguments", t, func() { - Convey("When --no-publish and --oneshot flags are passed", func() { - args, err := argsParse([]string{"--no-publish"}) - Convey("noPublish is set and args.sources is set to the default value", func() { - So(args.NoPublish, ShouldBeTrue) - So(len(args.LabelWhiteList.String()), ShouldEqual, 0) - So(err, ShouldBeNil) - }) - }) - - Convey("When valid args are specified", func() { - args, err := argsParse([]string{"--label-whitelist=.*rdt.*", "--port=1234", "--cert-file=crt", "--key-file=key", "--ca-file=ca"}) - Convey("Argument parsing should succeed and args set to correct values", func() { - So(args.NoPublish, ShouldBeFalse) - So(args.Port, ShouldEqual, 1234) - So(args.CertFile, ShouldEqual, "crt") - So(args.KeyFile, ShouldEqual, "key") - So(args.CaFile, ShouldEqual, "ca") - So(args.LabelWhiteList.String(), ShouldResemble, ".*rdt.*") - So(err, ShouldBeNil) - }) - }) - Convey("When invalid --port is defined", func() { - _, err := argsParse([]string{"--port=123a"}) - Convey("argsParse should fail", func() { - So(err, ShouldNotBeNil) - }) - }) - }) -} diff --git a/docs/advanced/developer-guide.md b/docs/advanced/developer-guide.md index 80268bcf1..b6d143989 100644 --- a/docs/advanced/developer-guide.md +++ b/docs/advanced/developer-guide.md @@ -164,43 +164,34 @@ $ docker run --rm --name=nfd-test ${NFD_CONTAINER_IMAGE} nfd-master --no-publish Command line flags of nfd-master: ```bash -$ docker run --rm ${NFD_CONTAINER_IMAGE} nfd-master --help -... -Usage: - nfd-master [--prune] [--no-publish] [--label-whitelist=] [--port=] - [--ca-file=] [--cert-file=] [--key-file=] - [--verify-node-name] [--extra-label-ns=] [--resource-labels=] - [--kubeconfig=] - nfd-master -h | --help - nfd-master --version - - Options: - -h --help Show this screen. - --version Output version and exit. - --prune Prune all NFD related attributes from all nodes - of the cluster and exit. - --kubeconfig= Kubeconfig to use [Default: ] - --port= Port on which to listen for connections. - [Default: 8080] - --ca-file= Root certificate for verifying connections - [Default: ] - --cert-file= Certificate used for authenticating connections - [Default: ] - --key-file= Private key matching --cert-file - [Default: ] - --verify-node-name Verify worker node name against CN from the TLS - certificate. Only has effect when TLS authentication - has been enabled. - --no-publish Do not publish feature labels - --label-whitelist= Regular expression to filter label names to - publish to the Kubernetes API server. - NB: the label namespace is omitted i.e. the filter - is only applied to the name part after '/'. - [Default: ] - --extra-label-ns= Comma separated list of allowed extra label namespaces - [Default: ] - --resource-labels= Comma separated list of labels to be exposed as extended resources. - [Default: ] +$ docker run --rm ${NFD_CONTAINER_IMAGE} nfd-master -help +Usage of nfd-master: + -ca-file string + Root certificate for verifying connections + -cert-file string + Certificate used for authenticating connections + -extra-label-ns value + Comma separated list of allowed extra label namespaces + -instance string + Instance name. Used to separate annotation namespaces for multiple parallel deployments. + -key-file string + Private key matching -cert-file + -kubeconfig string + Kubeconfig to use + -label-whitelist value + Regular expression to filter label names to publish to the Kubernetes API server. NB: the label namespace is omitted i.e. the filter is only applied to the name part after '/'. + -no-publish + Do not publish feature labels + -port int + Port on which to listen for connections. (default 8080) + -prune + Prune all NFD related attributes from all nodes of the cluaster and exit. + -resource-labels value + Comma separated list of labels to be exposed as extended resources. + -verify-node-name + Verify worker node name against CN from the TLS certificate. Only takes effect when TLS authentication has been enabled. + -version + Print version and exit. ``` ### NFD-Worker diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go index 45a1d239e..3accdaaa6 100644 --- a/pkg/nfd-master/nfd-master-internal_test.go +++ b/pkg/nfd-master/nfd-master-internal_test.go @@ -1,5 +1,5 @@ /* -Copyright 2019 The Kubernetes Authors. +Copyright 2019-2021 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -34,6 +34,7 @@ import ( k8sclient "k8s.io/client-go/kubernetes" "sigs.k8s.io/node-feature-discovery/pkg/apihelper" "sigs.k8s.io/node-feature-discovery/pkg/labeler" + "sigs.k8s.io/node-feature-discovery/pkg/utils" "sigs.k8s.io/node-feature-discovery/pkg/version" "sigs.k8s.io/yaml" ) @@ -55,7 +56,7 @@ func newMockMaster(apihelper apihelper.APIHelpers) *nfdMaster { return &nfdMaster{ nodeName: mockNodeName, annotationNs: AnnotationNsBase, - args: Args{LabelWhiteList: regexp.MustCompile("")}, + args: Args{LabelWhiteList: utils.RegexpVal{Regexp: *regexp.MustCompile("")}}, apihelper: apihelper, } } @@ -339,7 +340,7 @@ func TestSetLabels(t *testing.T) { apihelper.NewJsonPatch("add", "/metadata/labels", LabelNs+"/feature-2", mockLabels["feature-2"]), } - mockMaster.args.LabelWhiteList = regexp.MustCompile("^f.*2$") + mockMaster.args.LabelWhiteList.Regexp = *regexp.MustCompile("^f.*2$") mockHelper.On("GetClient").Return(mockClient, nil) mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil) mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) @@ -390,7 +391,7 @@ func TestSetLabels(t *testing.T) { apihelper.NewJsonPatch("add", "/status/capacity", LabelNs+"/feature-3", mockLabels["feature-3"]), } - mockMaster.args.ResourceLabels = []string{"feature-3", "feature-1"} + mockMaster.args.ResourceLabels = map[string]struct{}{"feature-3": struct{}{}, "feature-1": struct{}{}} mockHelper.On("GetClient").Return(mockClient, nil) mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil) mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go index 6a3ac39c5..f1e9ed378 100644 --- a/pkg/nfd-master/nfd-master.go +++ b/pkg/nfd-master/nfd-master.go @@ -1,5 +1,5 @@ /* -Copyright 2019 The Kubernetes Authors. +Copyright 2019-2021 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -38,6 +38,7 @@ import ( api "k8s.io/api/core/v1" "sigs.k8s.io/node-feature-discovery/pkg/apihelper" pb "sigs.k8s.io/node-feature-discovery/pkg/labeler" + "sigs.k8s.io/node-feature-discovery/pkg/utils" "sigs.k8s.io/node-feature-discovery/pkg/version" ) @@ -74,16 +75,16 @@ type Annotations map[string]string type Args struct { CaFile string CertFile string - ExtraLabelNs map[string]struct{} + ExtraLabelNs utils.StringSetVal Instance string KeyFile string Kubeconfig string - LabelWhiteList *regexp.Regexp + LabelWhiteList utils.RegexpVal NoPublish bool Port int Prune bool VerifyNodeName bool - ResourceLabels []string + ResourceLabels utils.StringSetVal } type NfdMaster interface { @@ -102,8 +103,8 @@ type nfdMaster struct { } // Create new NfdMaster server instance. -func NewNfdMaster(args Args) (NfdMaster, error) { - nfd := &nfdMaster{args: args, +func NewNfdMaster(args *Args) (NfdMaster, error) { + nfd := &nfdMaster{args: *args, nodeName: os.Getenv("NODE_NAME"), ready: make(chan bool, 1), } @@ -111,6 +112,11 @@ func NewNfdMaster(args Args) (NfdMaster, error) { if args.Instance == "" { nfd.annotationNs = AnnotationNsBase } else { + if ok, _ := regexp.MatchString(`^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$`, 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) + } nfd.annotationNs = args.Instance + "." + AnnotationNsBase } @@ -283,7 +289,7 @@ func (m *nfdMaster) updateMasterNode() error { // into extended resources. This function also handles proper namespacing of // labels and ERs, i.e. adds the possibly missing default namespace for labels // arriving through the gRPC API. -func filterFeatureLabels(labels Labels, extraLabelNs map[string]struct{}, labelWhiteList *regexp.Regexp, extendedResourceNames []string) (Labels, ExtendedResources) { +func filterFeatureLabels(labels Labels, extraLabelNs map[string]struct{}, labelWhiteList regexp.Regexp, extendedResourceNames map[string]struct{}) (Labels, ExtendedResources) { outLabels := Labels{} for label, value := range labels { @@ -310,7 +316,7 @@ func filterFeatureLabels(labels Labels, extraLabelNs map[string]struct{}, labelW // Remove labels which are intended to be extended resources extendedResources := ExtendedResources{} - for _, extendedResourceName := range extendedResourceNames { + for extendedResourceName := range extendedResourceNames { // Add possibly missing default ns extendedResourceName = addNs(extendedResourceName, LabelNs) if value, ok := outLabels[extendedResourceName]; ok { @@ -354,7 +360,7 @@ func (m *nfdMaster) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*pb.Se } stdoutLogger.Printf("REQUEST Node: %s NFD-version: %s Labels: %s", r.NodeName, r.NfdVersion, r.Labels) - labels, extendedResources := filterFeatureLabels(r.Labels, m.args.ExtraLabelNs, m.args.LabelWhiteList, m.args.ResourceLabels) + labels, extendedResources := filterFeatureLabels(r.Labels, m.args.ExtraLabelNs, m.args.LabelWhiteList.Regexp, m.args.ResourceLabels) if !m.args.NoPublish { // Advertise NFD worker version as an annotation diff --git a/pkg/nfd-master/nfd-master_test.go b/pkg/nfd-master/nfd-master_test.go index d3d5e085c..43b0b60a6 100644 --- a/pkg/nfd-master/nfd-master_test.go +++ b/pkg/nfd-master/nfd-master_test.go @@ -1,5 +1,5 @@ /* -Copyright 2019 The Kubernetes Authors. +Copyright 2019-2021 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -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.Args{CertFile: "crt", KeyFile: "key"}) + _, err2 := m.NewNfdMaster(&m.Args{KeyFile: "key", CaFile: "ca"}) + _, err3 := m.NewNfdMaster(&m.Args{CertFile: "crt", CaFile: "ca"}) Convey("An error should be returned", func() { So(err, ShouldNotBeNil) So(err2, ShouldNotBeNil) diff --git a/pkg/nfd-worker/nfd-worker_test.go b/pkg/nfd-worker/nfd-worker_test.go index 2707a98bb..b8c5dab52 100644 --- a/pkg/nfd-worker/nfd-worker_test.go +++ b/pkg/nfd-worker/nfd-worker_test.go @@ -34,11 +34,11 @@ type testContext struct { errs chan error } -func setupTest(args nfdmaster.Args) testContext { +func setupTest(args *nfdmaster.Args) testContext { // Fixed port and no-publish, for convenience args.NoPublish = true args.Port = 8192 - args.LabelWhiteList = regexp.MustCompile("") + args.LabelWhiteList.Regexp = *regexp.MustCompile("") m, err := nfdmaster.NewNfdMaster(args) if err != nil { fmt.Printf("Test setup failed: %v\n", err) @@ -86,7 +86,7 @@ func TestNewNfdWorker(t *testing.T) { } func TestRun(t *testing.T) { - ctx := setupTest(nfdmaster.Args{}) + ctx := setupTest(&nfdmaster.Args{}) defer teardownTest(ctx) Convey("When running nfd-worker against nfd-master", t, func() { Convey("When publishing features from fake source", func() { @@ -100,7 +100,7 @@ func TestRun(t *testing.T) { } func TestRunTls(t *testing.T) { - masterArgs := nfdmaster.Args{ + masterArgs := &nfdmaster.Args{ CaFile: data.FilePath("ca.crt"), CertFile: data.FilePath("nfd-test-master.crt"), KeyFile: data.FilePath("nfd-test-master.key"), diff --git a/pkg/utils/flags.go b/pkg/utils/flags.go new file mode 100644 index 000000000..81f33e9df --- /dev/null +++ b/pkg/utils/flags.go @@ -0,0 +1,62 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package utils + +import ( + "regexp" + "sort" + "strings" +) + +// RegexpVal is a wrapper for regexp command line flags +type RegexpVal struct { + regexp.Regexp +} + +// Set implements the flag.Value interface +func (a *RegexpVal) Set(val string) error { + r, err := regexp.Compile(val) + a.Regexp = *r + return err +} + +// StringSetVal is a Value encapsulating a set of comma-separated strings +type StringSetVal map[string]struct{} + +// Set implements the flag.Value interface +func (a *StringSetVal) Set(val string) error { + m := map[string]struct{}{} + for _, n := range strings.Split(val, ",") { + m[n] = struct{}{} + } + *a = m + return nil +} + +// String implements the flag.Value interface +func (a *StringSetVal) String() string { + if *a == nil { + return "" + } + + vals := make([]string, len(*a), 0) + for val := range *a { + vals = append(vals, val) + } + sort.Strings(vals) + return strings.Join(vals, ",") +}