Improve debug HTTP server usage

- Rename the -address parameter to -debug-addr.
- Only start the HTTP server on the Controller service: the Node service
  does not have a token configured and thus would never be able to
  respond successfully to a request.
- Only start the debug server if -debug-addr is given, ensuring that
  existing users won't be surprised by the new functionality. This also
  allows us to test a dev version of the CSI plugin on a cluster where
  the port might already be used.
- Remove explicit HTTP listener which is encapsulated by HTTP server.
- Bound graceful HTTP server shutdown by 10 seconds.

CHANGELOG.md

@@ -1,5 +1,8 @@
 ## unreleased
 
+* Improve debug HTTP server usage
+  [[GH-281]](https://github.com/digitalocean/csi-digitalocean/pull/281)
+
 ## v1.2.0 - 2020.01.15
 
 * Update csi-snapshotter to v1.2.2

cmd/do-csi-plugin/main.go

@@ -35,7 +35,7 @@ func main() {
 		url        = flag.String("url", "https://api.digitalocean.com/", "DigitalOcean API URL")
 		doTag      = flag.String("do-tag", "", "Tag DigitalOcean volumes on Create/Attach")
 		driverName = flag.String("driver-name", driver.DefaultDriverName, "Name for the driver")
-		address    = flag.String("address", driver.DefaultAddress, "Address to serve on")
+		debugAddr  = flag.String("debug-addr", "", "Address to serve the HTTP debug server on")
 		version    = flag.Bool("version", false, "Print the version and exit.")
 	)
 	flag.Parse()
@@ -45,7 +45,7 @@ func main() {
 		os.Exit(0)
 	}
 
-	drv, err := driver.NewDriver(*endpoint, *token, *url, *doTag, *driverName, *address)
+	drv, err := driver.NewDriver(*endpoint, *token, *url, *doTag, *driverName, *debugAddr)
 	if err != nil {
 		log.Fatalln(err)
 	}

driver/driver.go

@@ -41,10 +41,7 @@ import (
 const (
 	// DefaultDriverName defines the name that is used in Kubernetes and the CSI
 	// system for the canonical, official name of this plugin
-	DefaultDriverName = "dobs.csi.digitalocean.com"
-	// DefaultAddress is the default address that the csi plugin will serve its
-	// http handler on.
-	DefaultAddress           = "127.0.0.1:12302"
+	DefaultDriverName        = "dobs.csi.digitalocean.com"
 	defaultWaitActionTimeout = 1 * time.Minute
 )
 
@@ -67,15 +64,15 @@ type Driver struct {
 	publishInfoVolumeName string
 
 	endpoint          string
-	address           string
+	debugAddr         string
 	hostID            string
 	region            string
 	doTag             string
 	isController      bool
 	waitActionTimeout time.Duration
 
 	srv     *grpc.Server
-	httpSrv http.Server
+	httpSrv *http.Server
 	log     *logrus.Entry
 	mounter Mounter
 
@@ -97,7 +94,7 @@ type Driver struct {
 // NewDriver returns a CSI plugin that contains the necessary gRPC
 // interfaces to interact with Kubernetes over unix domain sockets for
 // managaing DigitalOcean Block Storage
-func NewDriver(ep, token, url, doTag, driverName, address string) (*Driver, error) {
+func NewDriver(ep, token, url, doTag, driverName, debugAddr string) (*Driver, error) {
 	if driverName == "" {
 		driverName = DefaultDriverName
 	}
@@ -140,13 +137,13 @@ func NewDriver(ep, token, url, doTag, driverName, address string) (*Driver, erro
 		name:                  driverName,
 		publishInfoVolumeName: driverName + "/volume-name",
 
-		doTag:    doTag,
-		endpoint: ep,
-		address:  address,
-		hostID:   hostID,
-		region:   region,
-		mounter:  newMounter(log),
-		log:      log,
+		doTag:     doTag,
+		endpoint:  ep,
+		debugAddr: debugAddr,
+		hostID:    hostID,
+		region:    region,
+		mounter:   newMounter(log),
+		log:       log,
 		// for now we're assuming only the controller has a non-empty token. In
 		// the future we should pass an explicit flag to the driver.
 		isController:      token != "",
@@ -215,41 +212,51 @@ func (d *Driver) Run(ctx context.Context) error {
 				"num_volumes": details.numVolumes,
 			}).Warn("CSI plugin will not function correctly, please resolve volume limit")
 		}
+
+		if d.debugAddr != "" {
+			mux := http.NewServeMux()
+			mux.HandleFunc("/health", func(w http.ResponseWriter, r *http.Request) {
+				err := d.healthChecker.Check(r.Context())
+				if err != nil {
+					w.WriteHeader(http.StatusInternalServerError)
+					return
+				}
+				w.WriteHeader(http.StatusOK)
+			})
+			d.httpSrv = &http.Server{
+				Addr:    d.debugAddr,
+				Handler: mux,
+			}
+		}
 	}
 
 	d.srv = grpc.NewServer(grpc.UnaryInterceptor(errHandler))
 	csi.RegisterIdentityServer(d.srv, d)
 	csi.RegisterControllerServer(d.srv, d)
 	csi.RegisterNodeServer(d.srv, d)
 
-	httpListener, err := net.Listen("tcp", d.address)
-	if err != nil {
-		return fmt.Errorf("failed to listen: %v", err)
-	}
-	mux := http.NewServeMux()
-	mux.HandleFunc("/health", func(w http.ResponseWriter, r *http.Request) {
-		err := d.healthChecker.Check(r.Context())
-		if err != nil {
-			w.WriteHeader(http.StatusInternalServerError)
-			return
-		}
-		w.WriteHeader(http.StatusOK)
-	})
-	d.httpSrv = http.Server{
-		Handler: mux,
-	}
-
 	d.ready = true // we're now ready to go!
 	d.log.WithFields(logrus.Fields{
 		"grpc_addr": grpcAddr,
-		"http_addr": d.address,
+		"http_addr": d.debugAddr,
 	}).Info("starting server")
 
 	var eg errgroup.Group
-	eg.Go(func() error {
-		<-ctx.Done()
-		return d.httpSrv.Shutdown(context.Background())
-	})
+	if d.httpSrv != nil {
+		eg.Go(func() error {
+			<-ctx.Done()
+			ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+			defer cancel()
+			return d.httpSrv.Shutdown(ctx)
+		})
+		eg.Go(func() error {
+			err := d.httpSrv.ListenAndServe()
+			if err == http.ErrServerClosed {
+				return nil
+			}
+			return err
+		})
+	}
 	eg.Go(func() error {
 		go func() {
 			<-ctx.Done()
@@ -261,13 +268,6 @@ func (d *Driver) Run(ctx context.Context) error {
 		}()
 		return d.srv.Serve(grpcListener)
 	})
-	eg.Go(func() error {
-		err := d.httpSrv.Serve(httpListener)
-		if err == http.ErrServerClosed {
-			return nil
-		}
-		return err
-	})
 
 	return eg.Wait()
 }