Repository: kubernetes-csi/external-attacher (go)

Issue:
external-attacher should not decide default filesystem type
When filesystem type (FSType) is empty, the csi-attacher sidecar changes it to "ext4" before calling the CSI driver's ControllerPublishVolume().

It should not. While the default for many types of volume is "ext4", no default is specified for a CSI volume, and that is correct: it is the particular CSI driver that must decide what the default filesystem is when none is specified, not the sidecar container.

There appears to be no simple workaround: a driver using external-attacher has no way of knowing whether the "ext4" it sees in a ControllerPublishVolume is there because it was specifically requested or because the csi-attacher has exceeded its authority.

Therefore remove `defaultFSType` and the associated code from external-attacher.

After this fix, some existing CSI drivers may need to change to preserve their existing response to a PVC with an unspecified filesystem. If that is a significant concern then consider replacing defaultFSType with a command-line argument with a default value of "ext4" but which can at least be set to "" to disable the current interference. However, simple removal of defaultFSType is the preferable fix.

/kind bug

Pull request description:
allow setting default fs type through command line argument
The external attacher was using a hardcoded default ext4 filesystem which was applied when PV spec did not contain fstype.

As a result attacher always reported ext4 as fs_type volume capability creating interference with some CSI drivers.

This patch adds a flag to csi-attacher binary which provides users with a workaround to set their own default filesystem or simply disable the interference by using an empty string:

`
./bin/csi-attacher --default-fstype ""
`

When the flag is not used  we still use ext4, next major release will allow behavior change and we can default to "" instead preserving the argument in case it's ever needed.

/kind bug

**Which issue(s) this PR fixes**:
Fixes #287 

**Does this PR introduce a user-facing change?**:
```release-note
There is a new --default-fstype flag available in this release that defaults to ext4 and can be used to set any value a driver may need. In the next major release ext4 as the default fsType will be deprecated and replaced by ""
```


Patch:
diff --git a/README.md b/README.md
index 9478e4c0..40a6b4be 100644
--- a/README.md
+++ b/README.md
@@ -83,6 +83,8 @@ Note that the external-attacher does not scale with more replicas. Only one exte
 
 * `--leader-election-retry-period <duration>`: Duration, in seconds, the LeaderElector clients should wait between tries of actions. Defaults to 5 seconds.
 
+* `--default-fstype <type>`: The default filesystem type of the volume to publish. Defaults to `ext4`.
+
 #### Other recognized arguments
 
 * `--kubeconfig <path>`: Path to Kubernetes client configuration that the external-attacher uses to connect to Kubernetes API server. When omitted, default token provided by Kubernetes will be used. This option is useful only when the external-attacher does not run as a Kubernetes pod, e.g. for debugging.
diff --git a/cmd/csi-attacher/main.go b/cmd/csi-attacher/main.go
index 83fc9524..fd8c1589 100644
--- a/cmd/csi-attacher/main.go
+++ b/cmd/csi-attacher/main.go
@@ -66,6 +66,8 @@ var (
 	leaderElectionRenewDeadline = flag.Duration("leader-election-renew-deadline", 10*time.Second, "Duration, in seconds, that the acting leader will retry refreshing leadership before giving up. Defaults to 10 seconds.")
 	leaderElectionRetryPeriod   = flag.Duration("leader-election-retry-period", 5*time.Second, "Duration, in seconds, the LeaderElector clients should wait between tries of actions. Defaults to 5 seconds.")
 
+	defaultFSType = flag.String("default-fstype", "ext4", "The default filesystem type of the volume to publish.")
+
 	reconcileSync = flag.Duration("reconcile-sync", 1*time.Minute, "Resync interval of the VolumeAttachment reconciler.")
 
 	metricsAddress = flag.String("metrics-address", "", "(deprecated) The TCP network address where the prometheus metrics endpoint will listen (example: `:8080`). The default is empty string, which means metrics endpoint is disabled. Only one of `--metrics-address` and `--http-endpoint` can be set.")
@@ -219,6 +221,7 @@ func main() {
 				supportsReadOnly,
 				supportsSingleNodeMultiWriter,
 				csitrans.New(),
+				*defaultFSType,
 			)
 			klog.V(2).Infof("CSI driver supports ControllerPublishUnpublish, using real CSI handler")
 		} else {
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
index 953f8bf3..819237ee 100644
--- a/pkg/controller/controller.go
+++ b/pkg/controller/controller.go
@@ -61,6 +61,8 @@ type CSIAttachController struct {
 	shouldReconcileVolumeAttachment bool
 	reconcileSync                   time.Duration
 	translator                      AttacherCSITranslator
+
+	defaultFSType string
 }
 
 // Handler is responsible for handling VolumeAttachment events from informer.
diff --git a/pkg/controller/csi_handler.go b/pkg/controller/csi_handler.go
index 00c3ab3b..ed3c6c92 100644
--- a/pkg/controller/csi_handler.go
+++ b/pkg/controller/csi_handler.go
@@ -72,6 +72,7 @@ type csiHandler struct {
 	supportsPublishReadOnly       bool
 	supportsSingleNodeMultiWriter bool
 	translator                    AttacherCSITranslator
+	defaultFSType                 string
 }
 
 var _ Handler = &csiHandler{}
@@ -88,7 +89,8 @@ func NewCSIHandler(
 	timeout *time.Duration,
 	supportsPublishReadOnly bool,
 	supportsSingleNodeMultiWriter bool,
-	translator AttacherCSITranslator) Handler {
+	translator AttacherCSITranslator,
+	defaultFSType string) Handler {
 
 	return &csiHandler{
 		client:                        client,
@@ -104,6 +106,7 @@ func NewCSIHandler(
 		translator:                    translator,
 		forceSync:                     map[string]bool{},
 		forceSyncMux:                  sync.Mutex{},
+		defaultFSType:                 defaultFSType,
 	}
 }
 
@@ -482,7 +485,7 @@ func (h *csiHandler) csiAttach(va *storage.VolumeAttachment) (*storage.VolumeAtt
 		readOnly = false
 	}
 
-	volumeCapabilities, err := GetVolumeCapabilities(pvSpec, h.supportsSingleNodeMultiWriter)
+	volumeCapabilities, err := GetVolumeCapabilities(pvSpec, h.supportsSingleNodeMultiWriter, h.defaultFSType)
 	if err != nil {
 		return va, nil, err
 	}
diff --git a/pkg/controller/util.go b/pkg/controller/util.go
index 70c5e6eb..31e60539 100644
--- a/pkg/controller/util.go
+++ b/pkg/controller/util.go
@@ -108,7 +108,6 @@ func markAsDetached(client kubernetes.Interface, va *storage.VolumeAttachment) (
 }
 
 const (
-	defaultFSType      = "ext4"
 	vaNodeIDAnnotation = "csi.alpha.kubernetes.io/node-id"
 )
 
@@ -140,7 +139,7 @@ func GetNodeIDFromCSINode(driver string, csiNode *storage.CSINode) (string, bool
 
 // GetVolumeCapabilities returns a VolumeCapability from the PV spec. Which access mode will be set depends if the driver supports the
 // SINGLE_NODE_MULTI_WRITER capability.
-func GetVolumeCapabilities(pvSpec *v1.PersistentVolumeSpec, singleNodeMultiWriterCapable bool) (*csi.VolumeCapability, error) {
+func GetVolumeCapabilities(pvSpec *v1.PersistentVolumeSpec, singleNodeMultiWriterCapable bool, defaultFSType string) (*csi.VolumeCapability, error) {
 	if pvSpec.CSI == nil {
 		return nil, errors.New("CSI volume source was nil")
 	}
@@ -158,6 +157,7 @@ func GetVolumeCapabilities(pvSpec *v1.PersistentVolumeSpec, singleNodeMultiWrite
 		fsType := pvSpec.CSI.FSType
 		if len(fsType) == 0 {
 			fsType = defaultFSType
+			klog.V(4).Infof("Filesystem type not found in PV spec. Using defaultFSType: %v.", fsType)
 		}
 
 		cap = &csi.VolumeCapability{


Test patch:
diff --git a/pkg/controller/csi_handler_test.go b/pkg/controller/csi_handler_test.go
index 0a8526bb..0e70b19a 100644
--- a/pkg/controller/csi_handler_test.go
+++ b/pkg/controller/csi_handler_test.go
@@ -43,6 +43,8 @@ import (
 const (
 	// Finalizer value
 	fin = "external-attacher/csi-test"
+
+	defaultFSType = "ext4"
 )
 
 var (
@@ -66,6 +68,7 @@ func csiHandlerFactory(client kubernetes.Interface, informerFactory informers.Sh
 		true,  /* supports PUBLISH_READONLY */
 		false, /* does not support SINGLE_NODE_MULTI_WRITER access mode */
 		csitranslator.New(),
+		defaultFSType,
 	)
 }
 
@@ -82,6 +85,7 @@ func csiHandlerFactoryNoReadOnly(client kubernetes.Interface, informerFactory in
 		false, /* does not support PUBLISH_READONLY */
 		false, /* does not support SINGLE_NODE_MULTI_WRITER access mode */
 		csitranslator.New(),
+		defaultFSType,
 	)
 }
 
diff --git a/pkg/controller/util_test.go b/pkg/controller/util_test.go
index 072eb758..fa9c4802 100644
--- a/pkg/controller/util_test.go
+++ b/pkg/controller/util_test.go
@@ -51,6 +51,7 @@ func createMountCapability(fsType string, mode csi.VolumeCapability_AccessMode_M
 func TestGetVolumeCapabilities(t *testing.T) {
 	blockVolumeMode := v1.PersistentVolumeMode(v1.PersistentVolumeBlock)
 	filesystemVolumeMode := v1.PersistentVolumeMode(v1.PersistentVolumeFilesystem)
+	defaultFSType := "ext4"
 
 	tests := []struct {
 		name                          string
@@ -205,7 +206,7 @@ func TestGetVolumeCapabilities(t *testing.T) {
 				},
 			},
 		}
-		cap, err := GetVolumeCapabilities(&pv.Spec, test.supportsSingleNodeMultiWriter)
+		cap, err := GetVolumeCapabilities(&pv.Spec, test.supportsSingleNodeMultiWriter, defaultFSType)
 
 		if err == nil && test.expectError {
 			t.Errorf("test %s: expected error, got none", test.name)