From 6a4d38dda5b6ecea964c3e6d11ce888e65438eb2 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Thu, 27 Jun 2024 19:50:18 +0200 Subject: [PATCH 1/5] feat: Split controller and node mode, add max volumes per node flag BREAKING CHANGE: switched flag library, renamed flags and introduced mode argument --- Makefile | 5 +- .../templates/csi-controller-deploy.yaml | 3 +- .../cloudstack-csi/templates/csi-node-ds.yaml | 5 +- cmd/cloudstack-csi-driver/main.go | 63 +++++++++++++------ deploy/k8s/controller-deployment.yaml | 9 +-- deploy/k8s/node-daemonset.yaml | 11 ++-- pkg/driver/constants.go | 32 ++++++++++ pkg/driver/driver.go | 60 ++++++++++++------ pkg/driver/node.go | 23 ++++--- pkg/driver/options.go | 52 +++++++++++++++ pkg/driver/server.go | 23 ++++--- pkg/driver/version.go | 60 ++++++++++++++++++ pkg/driver/version_test.go | 61 ++++++++++++++++++ 13 files changed, 337 insertions(+), 70 deletions(-) create mode 100644 pkg/driver/options.go create mode 100644 pkg/driver/version.go create mode 100644 pkg/driver/version_test.go diff --git a/Makefile b/Makefile index 6ffb13a..1752fd1 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,6 @@ CMDS=cloudstack-csi-driver cloudstack-csi-sc-syncer +PKG=github.com/leaseweb/cloudstack-csi-driver # Revision that gets built into each binary via the main.version # string. Uses the `git describe` output based on the most recent # version tag with a short revision suffix or, if nothing has been @@ -8,10 +9,12 @@ CMDS=cloudstack-csi-driver cloudstack-csi-sc-syncer # Beware that tags may also be missing in shallow clones as done by # some CI systems (like TravisCI, which pulls only 50 commits). REV=$(shell git describe --long --tags --match='v*' --dirty 2>/dev/null || git rev-list -n1 HEAD) +GIT_COMMIT?=$(shell git rev-parse HEAD) +BUILD_DATE?=$(shell date -u -Iseconds) DOCKER?=docker -IMPORTPATH_LDFLAGS = -X main.version=$(REV) +IMPORTPATH_LDFLAGS = -X ${PKG}/pkg/driver.driverVersion=$(REV) -X ${PKG}/pkg/driver.gitCommit=${GIT_COMMIT} -X ${PKG}/pkg/driver.buildDate=${BUILD_DATE} LDFLAGS = -s -w FULL_LDFLAGS = $(LDFLAGS) $(IMPORTPATH_LDFLAGS) diff --git a/charts/cloudstack-csi/templates/csi-controller-deploy.yaml b/charts/cloudstack-csi/templates/csi-controller-deploy.yaml index e7cd96f..5d751ad 100644 --- a/charts/cloudstack-csi/templates/csi-controller-deploy.yaml +++ b/charts/cloudstack-csi/templates/csi-controller-deploy.yaml @@ -43,8 +43,9 @@ spec: image: "{{ .Values.controller.csiDriverController.image.repository }}:{{ .Values.controller.csiDriverController.image.tag | default .Chart.AppVersion }}" imagePullPolicy: {{ .Values.controller.csiDriverController.image.pullPolicy }} args: + - "controller" - "--endpoint=$(CSI_ENDPOINT)" - - "--cloudstackconfig=$(CLOUD_CONFIG)" + - "--cloudstack-config=$(CLOUD_CONFIG)" - "--logging-format={{ .Values.logFormat }}" - "--v={{ .Values.logVerbosityLevel }}" {{- if .Values.controller.csiDriverController.extraArgs }} diff --git a/charts/cloudstack-csi/templates/csi-node-ds.yaml b/charts/cloudstack-csi/templates/csi-node-ds.yaml index 3a9a7e6..125e3ea 100644 --- a/charts/cloudstack-csi/templates/csi-node-ds.yaml +++ b/charts/cloudstack-csi/templates/csi-node-ds.yaml @@ -31,10 +31,11 @@ spec: image: "{{ .Values.node.csiDriver.image.repository }}:{{ .Values.node.csiDriver.image.tag | default .Chart.AppVersion }}" imagePullPolicy: {{ .Values.node.csiDriver.image.pullPolicy }} args: + - "node" - "--endpoint=$(CSI_ENDPOINT)" - - "--cloudstackconfig=$(CLOUD_CONFIG)" + - "--cloudstack-config=$(CLOUD_CONFIG)" - "--logging-format={{ .Values.logFormat }}" - - "--nodeName=$(NODE_NAME)" + - "--node-name=$(NODE_NAME)" - "--v={{ .Values.logVerbosityLevel }}" {{- if .Values.node.csiDriver.extraArgs }} {{- with .Values.node.csiDriver.extraArgs }} diff --git a/cmd/cloudstack-csi-driver/main.go b/cmd/cloudstack-csi-driver/main.go index 660c4cc..39793cc 100644 --- a/cmd/cloudstack-csi-driver/main.go +++ b/cmd/cloudstack-csi-driver/main.go @@ -7,11 +7,11 @@ package main import ( "context" - "flag" "fmt" "os" - "path" + "strings" + flag "github.com/spf13/pflag" "k8s.io/component-base/featuregate" "k8s.io/component-base/logs" logsapi "k8s.io/component-base/logs/api/v1" @@ -22,21 +22,18 @@ import ( "github.com/leaseweb/cloudstack-csi-driver/pkg/driver" ) -var ( - endpoint = flag.String("endpoint", "unix:///tmp/csi.sock", "CSI endpoint") - cloudstackconfig = flag.String("cloudstackconfig", "./cloud-config", "CloudStack configuration file") - nodeName = flag.String("nodeName", "", "Node name") - showVersion = flag.Bool("version", false, "Show version") - - // Version is set by the build process. - version = "" -) - func main() { + fs := flag.NewFlagSet("cloudstack-csi-driver", flag.ExitOnError) if err := logsapi.RegisterLogFormat(logsapi.JSONLogFormat, json.Factory{}, logsapi.LoggingBetaOptions); err != nil { klog.ErrorS(err, "failed to register JSON log format") } + var ( + showVersion = fs.Bool("version", false, "Show version") + args = os.Args[1:] + cmd = string(driver.AllMode) + options = driver.Options{} + ) fg := featuregate.NewFeatureGate() err := logsapi.AddFeatureGates(fg) if err != nil { @@ -44,8 +41,32 @@ func main() { } c := logsapi.NewLoggingConfiguration() - logsapi.AddGoFlags(c, flag.CommandLine) - flag.Parse() + logsapi.AddFlags(c, fs) + + if len(os.Args) > 1 && !strings.HasPrefix(os.Args[1], "-") { + cmd = os.Args[1] + args = os.Args[2:] + } + + switch cmd { + case string(driver.ControllerMode), string(driver.NodeMode), string(driver.AllMode): + options.Mode = driver.Mode(cmd) + default: + klog.Errorf("Unknown driver mode %s: Expected %s, %s, or %s", cmd, driver.ControllerMode, driver.NodeMode, driver.AllMode) + klog.FlushAndExit(klog.ExitFlushTimeout, 0) + } + + options.AddFlags(fs) + + if err = fs.Parse(args); err != nil { + klog.ErrorS(err, "Failed to parse options") + klog.FlushAndExit(klog.ExitFlushTimeout, 0) + } + if err = options.Validate(); err != nil { + klog.ErrorS(err, "Invalid options") + klog.FlushAndExit(klog.ExitFlushTimeout, 0) + } + logs.InitLogs() logger := klog.Background() if err = logsapi.ValidateAndApply(c, fg); err != nil { @@ -54,23 +75,27 @@ func main() { } if *showVersion { - baseName := path.Base(os.Args[0]) - fmt.Println(baseName, version) //nolint:forbidigo + versionInfo, versionErr := driver.GetVersionJSON() + if versionErr != nil { + logger.Error(err, "failed to get version") + klog.FlushAndExit(klog.ExitFlushTimeout, 1) + } + fmt.Println(versionInfo) //nolint:forbidigo os.Exit(0) } // Setup cloud connector. - config, err := cloud.ReadConfig(*cloudstackconfig) + config, err := cloud.ReadConfig(options.CloudStackConfig) if err != nil { logger.Error(err, "Cannot read CloudStack configuration") klog.FlushAndExit(klog.ExitFlushTimeout, 1) } - logger.Info("Successfully read CloudStack configuration", "cloudstackconfig", *cloudstackconfig) + logger.Info("Successfully read CloudStack configuration", "cloudstackconfig", options.CloudStackConfig) ctx := klog.NewContext(context.Background(), logger) csConnector := cloud.New(config) - d, err := driver.New(*endpoint, csConnector, nil, *nodeName, version) + d, err := driver.New(ctx, csConnector, &options, nil) if err != nil { logger.Error(err, "Failed to initialize driver") klog.FlushAndExit(klog.ExitFlushTimeout, 1) diff --git a/deploy/k8s/controller-deployment.yaml b/deploy/k8s/controller-deployment.yaml index 0887b70..f00d3f6 100644 --- a/deploy/k8s/controller-deployment.yaml +++ b/deploy/k8s/controller-deployment.yaml @@ -37,10 +37,11 @@ spec: image: cloudstack-csi-driver imagePullPolicy: Always args: - - "-endpoint=$(CSI_ENDPOINT)" - - "-cloudstackconfig=/etc/cloudstack-csi-driver/cloud-config" - - "-logging-format=text" - - "-v=4" + - "controller" + - "--endpoint=$(CSI_ENDPOINT)" + - "--cloudstack-config=/etc/cloudstack-csi-driver/cloud-config" + - "--logging-format=text" + - "--v=4" env: - name: CSI_ENDPOINT value: unix:///var/lib/csi/sockets/pluginproxy/csi.sock diff --git a/deploy/k8s/node-daemonset.yaml b/deploy/k8s/node-daemonset.yaml index ddfb2da..ddb649a 100644 --- a/deploy/k8s/node-daemonset.yaml +++ b/deploy/k8s/node-daemonset.yaml @@ -29,11 +29,12 @@ spec: image: cloudstack-csi-driver imagePullPolicy: Always args: - - "-endpoint=$(CSI_ENDPOINT)" - - "-cloudstackconfig=/etc/cloudstack-csi-driver/cloud-config" - - "-logging-format=text" - - "-nodeName=$(NODE_NAME)" - - "-v=4" + - "node" + - "--endpoint=$(CSI_ENDPOINT)" + - "--cloudstack-config=/etc/cloudstack-csi-driver/cloud-config" + - "--logging-format=text" + - "--node-name=$(NODE_NAME)" + - "--v=4" env: - name: CSI_ENDPOINT value: unix:///csi/csi.sock diff --git a/pkg/driver/constants.go b/pkg/driver/constants.go index f07dc5d..7775695 100644 --- a/pkg/driver/constants.go +++ b/pkg/driver/constants.go @@ -3,6 +3,38 @@ package driver // DriverName is the name of the CSI plugin. const DriverName = "csi.cloudstack.apache.org" +// Mode is the operating mode of the CSI driver. +type Mode string + +// Driver operating modes. +const ( + // ControllerMode is the mode that only starts the controller service. + ControllerMode Mode = "controller" + // NodeMode is the mode that only starts the node service. + NodeMode Mode = "node" + // AllMode is the mode that only starts both the controller and the node service. + AllMode Mode = "all" +) + +// constants for default command line flag values. +const ( + // DefaultCSIEndpoint is the default CSI endpoint for the driver. + DefaultCSIEndpoint = "unix://tmp/csi.sock" + DefaultMaxVolAttachLimit int64 = 256 +) + +// Filesystem types. +const ( + // FSTypeExt2 represents the ext2 filesystem type. + FSTypeExt2 = "ext2" + // FSTypeExt3 represents the ext3 filesystem type. + FSTypeExt3 = "ext3" + // FSTypeExt4 represents the ext4 filesystem type. + FSTypeExt4 = "ext4" + // FSTypeXfs represents the xfs filesystem type. + FSTypeXfs = "xfs" +) + // Topology keys. const ( ZoneKey = "topology." + DriverName + "/zone" diff --git a/pkg/driver/driver.go b/pkg/driver/driver.go index e5d8a14..35fefbc 100644 --- a/pkg/driver/driver.go +++ b/pkg/driver/driver.go @@ -5,6 +5,10 @@ package driver import ( "context" + "fmt" + + "github.com/container-storage-interface/spec/lib/go/csi" + "k8s.io/klog/v2" "github.com/leaseweb/cloudstack-csi-driver/pkg/cloud" "github.com/leaseweb/cloudstack-csi-driver/pkg/mount" @@ -17,29 +21,49 @@ type Interface interface { } type cloudstackDriver struct { - endpoint string - nodeName string - version string - - connector cloud.Interface - mounter mount.Interface + controller csi.ControllerServer + identity csi.IdentityServer + node csi.NodeServer + options *Options } // New instantiates a new CloudStack CSI driver. -func New(endpoint string, csConnector cloud.Interface, mounter mount.Interface, nodeName string, version string) (Interface, error) { - return &cloudstackDriver{ - endpoint: endpoint, - nodeName: nodeName, - version: version, - connector: csConnector, - mounter: mounter, - }, nil +func New(ctx context.Context, csConnector cloud.Interface, options *Options, mounter mount.Interface) (Interface, error) { + logger := klog.FromContext(ctx) + logger.Info("Driver starting", "Driver", DriverName, "Version", driverVersion) + + if err := validateMode(options.Mode); err != nil { + return nil, fmt.Errorf("invalid driver options: %w", err) + } + + driver := &cloudstackDriver{ + options: options, + } + + driver.identity = NewIdentityServer(driverVersion) + switch options.Mode { + case ControllerMode: + driver.controller = NewControllerServer(csConnector) + case NodeMode: + driver.node = NewNodeServer(csConnector, mounter, options) + case AllMode: + driver.controller = NewControllerServer(csConnector) + driver.node = NewNodeServer(csConnector, mounter, options) + default: + return nil, fmt.Errorf("unknown mode: %s", options.Mode) + } + + return driver, nil } func (cs *cloudstackDriver) Run(ctx context.Context) error { - ids := NewIdentityServer(cs.version) - ctrls := NewControllerServer(cs.connector) - ns := NewNodeServer(cs.connector, cs.mounter, cs.nodeName) + return cs.serve(ctx) +} + +func validateMode(mode Mode) error { + if mode != AllMode && mode != ControllerMode && mode != NodeMode { + return fmt.Errorf("mode is not supported (actual: %s, supported: %v)", mode, []Mode{AllMode, ControllerMode, NodeMode}) + } - return cs.serve(ctx, ids, ctrls, ns) + return nil } diff --git a/pkg/driver/node.go b/pkg/driver/node.go index fac21ab..d71801f 100644 --- a/pkg/driver/node.go +++ b/pkg/driver/node.go @@ -19,28 +19,30 @@ import ( const ( // default file system type to be used when it is not provided. - defaultFsType = "ext4" + defaultFsType = FSTypeExt4 ) type nodeServer struct { csi.UnimplementedNodeServer - connector cloud.Interface - mounter mount.Interface - nodeName string - volumeLocks *util.VolumeLocks + connector cloud.Interface + mounter mount.Interface + maxVolumesPerNode int64 + nodeName string + volumeLocks *util.VolumeLocks } // NewNodeServer creates a new Node gRPC server. -func NewNodeServer(connector cloud.Interface, mounter mount.Interface, nodeName string) csi.NodeServer { +func NewNodeServer(connector cloud.Interface, mounter mount.Interface, options *Options) csi.NodeServer { if mounter == nil { mounter = mount.New() } return &nodeServer{ - connector: connector, - mounter: mounter, - nodeName: nodeName, - volumeLocks: util.NewVolumeLocks(), + connector: connector, + mounter: mounter, + maxVolumesPerNode: options.VolumeAttachLimit, + nodeName: options.NodeName, + volumeLocks: util.NewVolumeLocks(), } } @@ -392,6 +394,7 @@ func (ns *nodeServer) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoReque return &csi.NodeGetInfoResponse{ NodeId: vm.ID, AccessibleTopology: topology.ToCSI(), + MaxVolumesPerNode: ns.maxVolumesPerNode, }, nil } diff --git a/pkg/driver/options.go b/pkg/driver/options.go new file mode 100644 index 0000000..f714f30 --- /dev/null +++ b/pkg/driver/options.go @@ -0,0 +1,52 @@ +package driver + +import ( + "errors" + + flag "github.com/spf13/pflag" +) + +// Options contains options and configuration settings for the driver. +type Options struct { + Mode Mode + + // #### Server options #### + + // Endpoint is the endpoint for the CSI driver server + Endpoint string + + // CloudStackConfig is the path to the CloudStack configuration file + CloudStackConfig string + + // #### Node options ##### + + // NodeName is used to retrieve the node instance ID in case metadata lookup fails. + NodeName string + + // VolumeAttachLimit specifies the value that shall be reported as "maximum number of attachable volumes" + // in CSINode objects. It is similar to https://kubernetes.io/docs/concepts/storage/storage-limits/#custom-limits + // which allowed administrators to specify custom volume limits by configuring the kube-scheduler. + VolumeAttachLimit int64 +} + +func (o *Options) AddFlags(f *flag.FlagSet) { + // Server options + f.StringVar(&o.Endpoint, "endpoint", DefaultCSIEndpoint, "Endpoint for the CSI driver server") + f.StringVar(&o.CloudStackConfig, "cloudstack-config", "./cloud-config", "Path to CloudStack configuration file") + + // Node options + if o.Mode == AllMode || o.Mode == NodeMode { + f.StringVar(&o.NodeName, "node-name", "", "Node name used to look up instance ID in case metadata lookup fails") + f.Int64Var(&o.VolumeAttachLimit, "volume-attach-limit", DefaultMaxVolAttachLimit, "Value for the maximum number of volumes attachable per node.") + } +} + +func (o *Options) Validate() error { + if o.Mode == AllMode || o.Mode == NodeMode { + if o.VolumeAttachLimit < 1 || o.VolumeAttachLimit > 256 { + return errors.New("invalid --volume-attach-limit specified, allowed range is 1 to 256") + } + } + + return nil +} diff --git a/pkg/driver/server.go b/pkg/driver/server.go index a46ab10..06aa440 100644 --- a/pkg/driver/server.go +++ b/pkg/driver/server.go @@ -12,9 +12,9 @@ import ( "k8s.io/klog/v2" ) -func (cs *cloudstackDriver) serve(ctx context.Context, ids csi.IdentityServer, ctrls csi.ControllerServer, ns csi.NodeServer) error { +func (cs *cloudstackDriver) serve(ctx context.Context) error { logger := klog.FromContext(ctx) - proto, addr, err := parseEndpoint(cs.endpoint) + proto, addr, err := parseEndpoint(cs.options.Endpoint) if err != nil { return err } @@ -46,14 +46,17 @@ func (cs *cloudstackDriver) serve(ctx context.Context, ids csi.IdentityServer, c } grpcServer := grpc.NewServer(opts...) - if ids != nil { - csi.RegisterIdentityServer(grpcServer, ids) - } - if ctrls != nil { - csi.RegisterControllerServer(grpcServer, ctrls) - } - if ns != nil { - csi.RegisterNodeServer(grpcServer, ns) + csi.RegisterIdentityServer(grpcServer, cs.identity) + switch cs.options.Mode { + case ControllerMode: + csi.RegisterControllerServer(grpcServer, cs.controller) + case NodeMode: + csi.RegisterNodeServer(grpcServer, cs.node) + case AllMode: + csi.RegisterControllerServer(grpcServer, cs.controller) + csi.RegisterNodeServer(grpcServer, cs.node) + default: + return fmt.Errorf("unknown mode: %s", cs.options.Mode) } logger.Info("Listening for connections", "address", listener.Addr()) diff --git a/pkg/driver/version.go b/pkg/driver/version.go new file mode 100644 index 0000000..a787ba4 --- /dev/null +++ b/pkg/driver/version.go @@ -0,0 +1,60 @@ +/* +Copyright 2024 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 driver + +import ( + "encoding/json" + "fmt" + "runtime" +) + +// These are set during build time via -ldflags. +var ( + driverVersion string + gitCommit string + buildDate string +) + +type VersionInfo struct { + DriverVersion string `json:"driverVersion"` + GitCommit string `json:"gitCommit"` + BuildDate string `json:"buildDate"` + GoVersion string `json:"goVersion"` + Compiler string `json:"compiler"` + Platform string `json:"platform"` +} + +func GetVersion() VersionInfo { + return VersionInfo{ + DriverVersion: driverVersion, + GitCommit: gitCommit, + BuildDate: buildDate, + GoVersion: runtime.Version(), + Compiler: runtime.Compiler, + Platform: fmt.Sprintf("%s/%s", runtime.GOOS, runtime.GOARCH), + } +} + +func GetVersionJSON() (string, error) { + info := GetVersion() + marshaled, err := json.MarshalIndent(&info, "", " ") + if err != nil { + return "", err + } + + return string(marshaled), nil +} diff --git a/pkg/driver/version_test.go b/pkg/driver/version_test.go new file mode 100644 index 0000000..327efcb --- /dev/null +++ b/pkg/driver/version_test.go @@ -0,0 +1,61 @@ +/* +Copyright 2024 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 driver + +import ( + "fmt" + "reflect" + "runtime" + "testing" +) + +func TestGetVersion(t *testing.T) { + version := GetVersion() + + expected := VersionInfo{ + DriverVersion: "", + GitCommit: "", + BuildDate: "", + GoVersion: runtime.Version(), + Compiler: runtime.Compiler, + Platform: fmt.Sprintf("%s/%s", runtime.GOOS, runtime.GOARCH), + } + + if !reflect.DeepEqual(version, expected) { + t.Fatalf("structs not equal\ngot:\n%+v\nexpected:\n%+v", version, expected) + } +} + +func TestGetVersionJSON(t *testing.T) { + version, err := GetVersionJSON() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expected := fmt.Sprintf(`{ + "driverVersion": "", + "gitCommit": "", + "buildDate": "", + "goVersion": "%s", + "compiler": "%s", + "platform": "%s" +}`, runtime.Version(), runtime.Compiler, fmt.Sprintf("%s/%s", runtime.GOOS, runtime.GOARCH)) + + if version != expected { + t.Fatalf("json not equal\ngot:\n%s\nexpected:\n%s", version, expected) + } +} From 791a8119cf77dd92ab57f605759334f6fcc8bbc6 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Thu, 27 Jun 2024 20:21:05 +0200 Subject: [PATCH 2/5] chore: add component-base to minor versions to ignore --- .github/dependabot.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 63d3559..27a0a20 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -40,6 +40,8 @@ updates: update-types: ["version-update:semver-minor"] - dependency-name: "k8s.io/apimachinery" update-types: ["version-update:semver-minor"] + - dependency-name: "k8s.io/component-base" + update-types: ["version-update:semver-minor"] - dependency-name: "k8s.io/client-go" update-types: ["version-update:semver-minor"] - dependency-name: "k8s.io/mount-utils" From b0d6c298418a949621a077569b9b45cf8cea988a Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Thu, 27 Jun 2024 21:20:25 +0200 Subject: [PATCH 3/5] refactor: identity server does not need version passed, get rid of serve move ParseEndpoint into util pkg --- pkg/driver/driver.go | 46 ++++++++++++++++++++++-- pkg/driver/identity.go | 26 +++----------- pkg/driver/server.go | 76 --------------------------------------- pkg/util/endpoint.go | 34 ++++++++++++++++++ pkg/util/endpoint_test.go | 75 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 156 insertions(+), 101 deletions(-) delete mode 100644 pkg/driver/server.go create mode 100644 pkg/util/endpoint.go create mode 100644 pkg/util/endpoint_test.go diff --git a/pkg/driver/driver.go b/pkg/driver/driver.go index 35fefbc..839574e 100644 --- a/pkg/driver/driver.go +++ b/pkg/driver/driver.go @@ -6,12 +6,15 @@ package driver import ( "context" "fmt" + "net" "github.com/container-storage-interface/spec/lib/go/csi" + "google.golang.org/grpc" "k8s.io/klog/v2" "github.com/leaseweb/cloudstack-csi-driver/pkg/cloud" "github.com/leaseweb/cloudstack-csi-driver/pkg/mount" + "github.com/leaseweb/cloudstack-csi-driver/pkg/util" ) // Interface is the CloudStack CSI driver interface. @@ -22,7 +25,6 @@ type Interface interface { type cloudstackDriver struct { controller csi.ControllerServer - identity csi.IdentityServer node csi.NodeServer options *Options } @@ -40,7 +42,6 @@ func New(ctx context.Context, csConnector cloud.Interface, options *Options, mou options: options, } - driver.identity = NewIdentityServer(driverVersion) switch options.Mode { case ControllerMode: driver.controller = NewControllerServer(csConnector) @@ -57,7 +58,46 @@ func New(ctx context.Context, csConnector cloud.Interface, options *Options, mou } func (cs *cloudstackDriver) Run(ctx context.Context) error { - return cs.serve(ctx) + logger := klog.FromContext(ctx) + scheme, addr, err := util.ParseEndpoint(cs.options.Endpoint) + if err != nil { + return err + } + + listener, err := net.Listen(scheme, addr) + if err != nil { + return fmt.Errorf("failed to listen: %w", err) + } + + // Log every request and payloads (request + response) + opts := []grpc.ServerOption{ + grpc.UnaryInterceptor(func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { + resp, err := handler(klog.NewContext(ctx, logger), req) + if err != nil { + logger.Error(err, "GRPC method failed", "method", info.FullMethod) + } + + return resp, err + }), + } + grpcServer := grpc.NewServer(opts...) + + csi.RegisterIdentityServer(grpcServer, cs) + switch cs.options.Mode { + case ControllerMode: + csi.RegisterControllerServer(grpcServer, cs.controller) + case NodeMode: + csi.RegisterNodeServer(grpcServer, cs.node) + case AllMode: + csi.RegisterControllerServer(grpcServer, cs.controller) + csi.RegisterNodeServer(grpcServer, cs.node) + default: + return fmt.Errorf("unknown mode: %s", cs.options.Mode) + } + + logger.Info("Listening for connections", "address", listener.Addr()) + + return grpcServer.Serve(listener) } func validateMode(mode Mode) error { diff --git a/pkg/driver/identity.go b/pkg/driver/identity.go index 60ade9e..e265275 100644 --- a/pkg/driver/identity.go +++ b/pkg/driver/identity.go @@ -4,46 +4,28 @@ import ( "context" "github.com/container-storage-interface/spec/lib/go/csi" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "k8s.io/klog/v2" ) -type identityServer struct { - csi.UnimplementedIdentityServer - version string -} - -// NewIdentityServer creates a new Identity gRPC server. -func NewIdentityServer(version string) csi.IdentityServer { - return &identityServer{ - version: version, - } -} - -func (ids *identityServer) GetPluginInfo(ctx context.Context, req *csi.GetPluginInfoRequest) (*csi.GetPluginInfoResponse, error) { +func (cs *cloudstackDriver) GetPluginInfo(ctx context.Context, req *csi.GetPluginInfoRequest) (*csi.GetPluginInfoResponse, error) { logger := klog.FromContext(ctx) logger.V(6).Info("GetPluginInfo: called", "args", *req) - if ids.version == "" { - return nil, status.Error(codes.Unavailable, "Driver is missing version") - } - resp := &csi.GetPluginInfoResponse{ Name: DriverName, - VendorVersion: ids.version, + VendorVersion: driverVersion, } return resp, nil } -func (ids *identityServer) Probe(ctx context.Context, req *csi.ProbeRequest) (*csi.ProbeResponse, error) { +func (cs *cloudstackDriver) Probe(ctx context.Context, req *csi.ProbeRequest) (*csi.ProbeResponse, error) { logger := klog.FromContext(ctx) logger.V(6).Info("Probe: called", "args", *req) return &csi.ProbeResponse{}, nil } -func (ids *identityServer) GetPluginCapabilities(ctx context.Context, req *csi.GetPluginCapabilitiesRequest) (*csi.GetPluginCapabilitiesResponse, error) { +func (cs *cloudstackDriver) GetPluginCapabilities(ctx context.Context, req *csi.GetPluginCapabilitiesRequest) (*csi.GetPluginCapabilitiesResponse, error) { logger := klog.FromContext(ctx) logger.V(6).Info("Probe: called", "args", *req) diff --git a/pkg/driver/server.go b/pkg/driver/server.go deleted file mode 100644 index 06aa440..0000000 --- a/pkg/driver/server.go +++ /dev/null @@ -1,76 +0,0 @@ -package driver - -import ( - "context" - "fmt" - "net" - "os" - "strings" - - "github.com/container-storage-interface/spec/lib/go/csi" - "google.golang.org/grpc" - "k8s.io/klog/v2" -) - -func (cs *cloudstackDriver) serve(ctx context.Context) error { - logger := klog.FromContext(ctx) - proto, addr, err := parseEndpoint(cs.options.Endpoint) - if err != nil { - return err - } - - if proto == "unix" { - if !strings.HasPrefix(addr, "/") { - addr = "/" + addr - } - if err := os.Remove(addr); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("failed to remove %s, error: %s", addr, err.Error()) - } - } - - listener, err := net.Listen(proto, addr) - if err != nil { - return fmt.Errorf("failed to listen: %w", err) - } - - // Log every request and payloads (request + response) - opts := []grpc.ServerOption{ - grpc.UnaryInterceptor(func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { - resp, err := handler(klog.NewContext(ctx, logger), req) - if err != nil { - logger.Error(err, "GRPC method failed", "method", info.FullMethod) - } - - return resp, err - }), - } - grpcServer := grpc.NewServer(opts...) - - csi.RegisterIdentityServer(grpcServer, cs.identity) - switch cs.options.Mode { - case ControllerMode: - csi.RegisterControllerServer(grpcServer, cs.controller) - case NodeMode: - csi.RegisterNodeServer(grpcServer, cs.node) - case AllMode: - csi.RegisterControllerServer(grpcServer, cs.controller) - csi.RegisterNodeServer(grpcServer, cs.node) - default: - return fmt.Errorf("unknown mode: %s", cs.options.Mode) - } - - logger.Info("Listening for connections", "address", listener.Addr()) - - return grpcServer.Serve(listener) -} - -func parseEndpoint(ep string) (string, string, error) { - if strings.HasPrefix(strings.ToLower(ep), "unix://") || strings.HasPrefix(strings.ToLower(ep), "tcp://") { - s := strings.SplitN(ep, "://", 2) - if s[1] != "" { - return s[0], s[1], nil - } - } - - return "", "", fmt.Errorf("invalid endpoint: %v", ep) -} diff --git a/pkg/util/endpoint.go b/pkg/util/endpoint.go new file mode 100644 index 0000000..cb46ddf --- /dev/null +++ b/pkg/util/endpoint.go @@ -0,0 +1,34 @@ +package util + +import ( + "fmt" + "net/url" + "os" + "path/filepath" + "strings" +) + +// ParseEndpoint parses the CSI socket endpoint and returns the components scheme and addr. +func ParseEndpoint(endpoint string) (string, string, error) { + u, err := url.Parse(endpoint) + if err != nil { + return "", "", fmt.Errorf("could not parse endpoint: %w", err) + } + + addr := filepath.Join(u.Host, filepath.FromSlash(u.Path)) + + scheme := strings.ToLower(u.Scheme) + switch scheme { + case "tcp": + case "unix": + addr = filepath.Join("/", addr) + // Remove the socket file if it already exists. + if err := os.Remove(addr); err != nil && !os.IsNotExist(err) { + return "", "", fmt.Errorf("could not remove unix domain socket %q: %w", addr, err) + } + default: + return "", "", fmt.Errorf("unsupported protocol: %s", scheme) + } + + return scheme, addr, nil +} diff --git a/pkg/util/endpoint_test.go b/pkg/util/endpoint_test.go new file mode 100644 index 0000000..93e2878 --- /dev/null +++ b/pkg/util/endpoint_test.go @@ -0,0 +1,75 @@ +package util + +import ( + "errors" + "testing" +) + +func TestParseEndpoint(t *testing.T) { + testCases := []struct { + name string + endpoint string + expScheme string + expAddr string + expErr error + }{ + { + name: "valid unix endpoint 1", + endpoint: "unix:///csi/csi.sock", + expScheme: "unix", + expAddr: "/csi/csi.sock", + }, + { + name: "valid unix endpoint 2", + endpoint: "unix://csi/csi.sock", + expScheme: "unix", + expAddr: "/csi/csi.sock", + }, + { + name: "valid unix endpoint 3", + endpoint: "unix:/csi/csi.sock", + expScheme: "unix", + expAddr: "/csi/csi.sock", + }, + { + name: "valid tcp endpoint", + endpoint: "tcp:///127.0.0.1/", + expScheme: "tcp", + expAddr: "/127.0.0.1", + }, + { + name: "valid tcp endpoint", + endpoint: "tcp:///127.0.0.1", + expScheme: "tcp", + expAddr: "/127.0.0.1", + }, + { + name: "invalid endpoint", + endpoint: "http://127.0.0.1", + expErr: errors.New("unsupported protocol: http"), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + scheme, addr, err := ParseEndpoint(tc.endpoint) + + if tc.expErr != nil { + if err.Error() != tc.expErr.Error() { + t.Fatalf("Expecting err: expected %v, got %v", tc.expErr, err) + } + } else { + if err != nil { + t.Fatalf("err is not nil. got: %v", err) + } + if scheme != tc.expScheme { + t.Fatalf("scheme mismatches: expected %v, got %v", tc.expScheme, scheme) + } + + if addr != tc.expAddr { + t.Fatalf("addr mismatches: expected %v, got %v", tc.expAddr, addr) + } + } + }) + } +} From 77d212ea273321a19bcd3b1f4137d71de0e2f693 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Thu, 27 Jun 2024 21:21:59 +0200 Subject: [PATCH 4/5] build: disable golangci-lint linters funlen and nestif on tests --- .golangci.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.golangci.yml b/.golangci.yml index b815473..a8b3ace 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -2,6 +2,11 @@ issues: exclude-use-default: true max-issues-per-linter: 50 max-same-issues: 0 # disable + exclude-rules: + - path: _test.go + linters: + - funlen + - nestif linters-settings: gci: From f1fd6b85c764d0aebc87ef5a9ea48aeb2348cac9 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Thu, 27 Jun 2024 21:48:17 +0200 Subject: [PATCH 5/5] test: Fix sanity test --- test/sanity/sanity_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/sanity/sanity_test.go b/test/sanity/sanity_test.go index 27eede4..ee1f4b8 100644 --- a/test/sanity/sanity_test.go +++ b/test/sanity/sanity_test.go @@ -39,7 +39,12 @@ func TestSanity(t *testing.T) { logger := klog.Background() ctx := klog.NewContext(context.Background(), logger) - csiDriver, err := driver.New(endpoint, fake.New(), mount.NewFake(), "node", "v0") + options := driver.Options{ + Mode: driver.AllMode, + Endpoint: endpoint, + NodeName: "node", + } + csiDriver, err := driver.New(ctx, fake.New(), &options, mount.NewFake()) if err != nil { t.Fatalf("error creating driver: %v", err) }