Validation PVs for mount options

We are going to move the validation in its own package
and we will be calling validation for individual volume types
as needed.

hack/.linted_packages

@@ -262,6 +262,7 @@ pkg/volume/util/nestedpendingoperations
 pkg/volume/util/operationexecutor
 pkg/volume/util/types
 pkg/volume/util/volumehelper
+pkg/volume/validation
 pkg/watch
 pkg/watch/json
 pkg/watch/versioned

pkg/api/validation/BUILD

@@ -15,7 +15,6 @@ go_library(
         "events.go",
         "schema.go",
         "validation.go",
-        "volume_plugins.go",
     ],
     tags = ["automanaged"],
     deps = [
@@ -27,30 +26,6 @@ go_library(
         "//pkg/capabilities:go_default_library",
         "//pkg/features:go_default_library",
         "//pkg/security/apparmor:go_default_library",
-        "//pkg/volume:go_default_library",
-        "//pkg/volume/aws_ebs:go_default_library",
-        "//pkg/volume/azure_dd:go_default_library",
-        "//pkg/volume/azure_file:go_default_library",
-        "//pkg/volume/cephfs:go_default_library",
-        "//pkg/volume/cinder:go_default_library",
-        "//pkg/volume/configmap:go_default_library",
-        "//pkg/volume/downwardapi:go_default_library",
-        "//pkg/volume/empty_dir:go_default_library",
-        "//pkg/volume/fc:go_default_library",
-        "//pkg/volume/flexvolume:go_default_library",
-        "//pkg/volume/flocker:go_default_library",
-        "//pkg/volume/gce_pd:go_default_library",
-        "//pkg/volume/git_repo:go_default_library",
-        "//pkg/volume/glusterfs:go_default_library",
-        "//pkg/volume/host_path:go_default_library",
-        "//pkg/volume/iscsi:go_default_library",
-        "//pkg/volume/nfs:go_default_library",
-        "//pkg/volume/photon_pd:go_default_library",
-        "//pkg/volume/projected:go_default_library",
-        "//pkg/volume/quobyte:go_default_library",
-        "//pkg/volume/rbd:go_default_library",
-        "//pkg/volume/secret:go_default_library",
-        "//pkg/volume/vsphere_volume:go_default_library",
         "//vendor:github.com/emicklei/go-restful/swagger",
         "//vendor:github.com/exponent-io/jsonpath",
         "//vendor:github.com/golang/glog",
@@ -100,7 +75,6 @@ go_test(
         "//pkg/apis/extensions/v1beta1:go_default_library",
         "//pkg/capabilities:go_default_library",
         "//pkg/security/apparmor:go_default_library",
-        "//pkg/volume:go_default_library",
         "//vendor:github.com/ghodss/yaml",
         "//vendor:k8s.io/apimachinery/pkg/api/resource",
         "//vendor:k8s.io/apimachinery/pkg/api/testing",

pkg/api/validation/validation.go

@@ -47,7 +47,6 @@ import (
 	"k8s.io/kubernetes/pkg/capabilities"
 	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/security/apparmor"
-	"k8s.io/kubernetes/pkg/volume"
 )
 
 // TODO: delete this global variable when we enable the validation of common
@@ -64,11 +63,6 @@ var volumeModeErrorMsg string = "must be a number between 0 and 0777 (octal), bo
 
 // BannedOwners is a black list of object that are not allowed to be owners.
 var BannedOwners = genericvalidation.BannedOwners
-var volumePlugins []volume.VolumePlugin
-
-func init() {
-	volumePlugins = probeVolumePlugins()
-}
 
 // ValidateHasLabel requires that metav1.ObjectMeta has a Label with key and expectedValue
 func ValidateHasLabel(meta metav1.ObjectMeta, fldPath *field.Path, key, expectedValue string) field.ErrorList {
@@ -1064,20 +1058,6 @@ func ValidatePersistentVolume(pv *api.PersistentVolume) field.ErrorList {
 		}
 	}
 
-	volumePlugin := findPluginBySpec(volumePlugins, pv)
-	mountOptions := volume.MountOptionFromApiPV(pv)
-
-	metaField := field.NewPath("metadata")
-	if volumePlugin == nil && len(mountOptions) > 0 {
-		allErrs = append(allErrs, field.Forbidden(metaField.Child("annotations", volume.MountOptionAnnotation), "may not specify mount options for this volume type"))
-	}
-
-	if volumePlugin != nil {
-		if !volumePlugin.SupportsMountOption() && len(mountOptions) > 0 {
-			allErrs = append(allErrs, field.Forbidden(metaField.Child("annotations", volume.MountOptionAnnotation), "may not specify mount options for this volume type"))
-		}
-	}
-
 	numVolumes := 0
 	if pv.Spec.HostPath != nil {
 		if numVolumes > 0 {

pkg/api/validation/validation_test.go

@@ -31,7 +31,6 @@ import (
 	"k8s.io/kubernetes/pkg/api/v1"
 	"k8s.io/kubernetes/pkg/capabilities"
 	"k8s.io/kubernetes/pkg/security/apparmor"
-	"k8s.io/kubernetes/pkg/volume"
 )
 
 const (
@@ -207,30 +206,6 @@ func TestValidatePersistentVolumes(t *testing.T) {
 				PersistentVolumeReclaimPolicy: api.PersistentVolumeReclaimRecycle,
 			}),
 		},
-		"volume with valid mount option for nfs": {
-			isExpectedFailure: false,
-			volume: testVolumeWithMountOption("good-nfs-mount-volume", "", "ro,nfsvers=3", api.PersistentVolumeSpec{
-				Capacity: api.ResourceList{
-					api.ResourceName(api.ResourceStorage): resource.MustParse("10G"),
-				},
-				AccessModes: []api.PersistentVolumeAccessMode{api.ReadWriteOnce},
-				PersistentVolumeSource: api.PersistentVolumeSource{
-					NFS: &api.NFSVolumeSource{Server: "localhost", Path: "/srv", ReadOnly: false},
-				},
-			}),
-		},
-		"volume with mount option for host path": {
-			isExpectedFailure: true,
-			volume: testVolumeWithMountOption("bad-hostpath-mount-volume", "", "ro,nfsvers=3", api.PersistentVolumeSpec{
-				Capacity: api.ResourceList{
-					api.ResourceName(api.ResourceStorage): resource.MustParse("10G"),
-				},
-				AccessModes: []api.PersistentVolumeAccessMode{api.ReadWriteOnce},
-				PersistentVolumeSource: api.PersistentVolumeSource{
-					HostPath: &api.HostPathVolumeSource{Path: "/a/.."},
-				},
-			}),
-		},
 		"invalid-storage-class-name": {
 			isExpectedFailure: true,
 			volume: testVolume("invalid-storage-class-name", "", api.PersistentVolumeSpec{
@@ -280,25 +255,6 @@ func testVolumeClaimStorageClass(name string, namespace string, annval string, s
 	}
 }
 
-func testVolumeWithMountOption(name string, namespace string, mountOptions string, spec api.PersistentVolumeSpec) *api.PersistentVolume {
-	annotations := map[string]string{
-		volume.MountOptionAnnotation: mountOptions,
-	}
-	objMeta := metav1.ObjectMeta{
-		Name:        name,
-		Annotations: annotations,
-	}
-
-	if namespace != "" {
-		objMeta.Namespace = namespace
-	}
-
-	return &api.PersistentVolume{
-		ObjectMeta: objMeta,
-		Spec:       spec,
-	}
-}
-
 func testVolumeClaimAnnotation(name string, namespace string, ann string, annval string, spec api.PersistentVolumeClaimSpec) *api.PersistentVolumeClaim {
 	annotations := map[string]string{
 		ann: annval,

pkg/api/validation/volume_plugins.go

@@ -1,105 +0,0 @@
-/*
-Copyright 2017 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 validation
-
-import (
-	"github.com/golang/glog"
-	"k8s.io/kubernetes/pkg/api"
-	"k8s.io/kubernetes/pkg/api/v1"
-	"k8s.io/kubernetes/pkg/volume"
-	"k8s.io/kubernetes/pkg/volume/aws_ebs"
-	"k8s.io/kubernetes/pkg/volume/azure_dd"
-	"k8s.io/kubernetes/pkg/volume/azure_file"
-	"k8s.io/kubernetes/pkg/volume/cephfs"
-	"k8s.io/kubernetes/pkg/volume/cinder"
-	"k8s.io/kubernetes/pkg/volume/configmap"
-	"k8s.io/kubernetes/pkg/volume/downwardapi"
-	"k8s.io/kubernetes/pkg/volume/empty_dir"
-	"k8s.io/kubernetes/pkg/volume/fc"
-	"k8s.io/kubernetes/pkg/volume/flexvolume"
-	"k8s.io/kubernetes/pkg/volume/flocker"
-	"k8s.io/kubernetes/pkg/volume/gce_pd"
-	"k8s.io/kubernetes/pkg/volume/git_repo"
-	"k8s.io/kubernetes/pkg/volume/glusterfs"
-	"k8s.io/kubernetes/pkg/volume/host_path"
-	"k8s.io/kubernetes/pkg/volume/iscsi"
-	"k8s.io/kubernetes/pkg/volume/nfs"
-	"k8s.io/kubernetes/pkg/volume/photon_pd"
-	"k8s.io/kubernetes/pkg/volume/projected"
-	"k8s.io/kubernetes/pkg/volume/quobyte"
-	"k8s.io/kubernetes/pkg/volume/rbd"
-	"k8s.io/kubernetes/pkg/volume/secret"
-	"k8s.io/kubernetes/pkg/volume/vsphere_volume"
-)
-
-func probeVolumePlugins() []volume.VolumePlugin {
-	allPlugins := []volume.VolumePlugin{}
-
-	// list of volume plugins to probe for
-	allPlugins = append(allPlugins, aws_ebs.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, empty_dir.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, gce_pd.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, git_repo.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, host_path.ProbeVolumePlugins(volume.VolumeConfig{})...)
-	allPlugins = append(allPlugins, nfs.ProbeVolumePlugins(volume.VolumeConfig{})...)
-	allPlugins = append(allPlugins, secret.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, iscsi.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, glusterfs.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, rbd.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, cinder.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, quobyte.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, cephfs.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, downwardapi.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, fc.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, flocker.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, flexvolume.ProbeVolumePlugins("")...)
-	allPlugins = append(allPlugins, azure_file.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, configmap.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, vsphere_volume.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, azure_dd.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, photon_pd.ProbeVolumePlugins()...)
-	allPlugins = append(allPlugins, projected.ProbeVolumePlugins()...)
-	return allPlugins
-}
-
-func findPluginBySpec(volumePlugins []volume.VolumePlugin, pv *api.PersistentVolume) volume.VolumePlugin {
-	matches := []volume.VolumePlugin{}
-	v1Pv := &v1.PersistentVolume{}
-	err := v1.Convert_api_PersistentVolume_To_v1_PersistentVolume(pv, v1Pv, nil)
-	if err != nil {
-		glog.Errorf("Error converting to v1.PersistentVolume: %v", err)
-		return nil
-	}
-	volumeSpec := &volume.Spec{PersistentVolume: v1Pv}
-	for _, plugin := range volumePlugins {
-		if plugin.CanSupport(volumeSpec) {
-			matches = append(matches, plugin)
-		}
-	}
-
-	if len(matches) == 0 {
-		glog.V(5).Infof("No matching plugin found for : %s", pv.Name)
-		return nil
-	}
-
-	if len(matches) > 1 {
-		glog.V(3).Infof("multiple volume plugins matched for : %s ", pv.Name)
-		return nil
-	}
-
-	return matches[0]
-}

pkg/registry/core/persistentvolume/BUILD

@@ -18,6 +18,7 @@ go_library(
     deps = [
         "//pkg/api:go_default_library",
         "//pkg/api/validation:go_default_library",
+        "//pkg/volume/validation:go_default_library",
         "//vendor:k8s.io/apimachinery/pkg/fields",
         "//vendor:k8s.io/apimachinery/pkg/labels",
         "//vendor:k8s.io/apimachinery/pkg/runtime",

pkg/registry/core/persistentvolume/strategy.go

@@ -29,6 +29,7 @@ import (
 	"k8s.io/apiserver/pkg/storage/names"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/validation"
+	volumevalidation "k8s.io/kubernetes/pkg/volume/validation"
 )
 
 // persistentvolumeStrategy implements behavior for PersistentVolume objects
@@ -53,7 +54,8 @@ func (persistentvolumeStrategy) PrepareForCreate(ctx genericapirequest.Context,
 
 func (persistentvolumeStrategy) Validate(ctx genericapirequest.Context, obj runtime.Object) field.ErrorList {
 	persistentvolume := obj.(*api.PersistentVolume)
-	return validation.ValidatePersistentVolume(persistentvolume)
+	errorList := validation.ValidatePersistentVolume(persistentvolume)
+	return append(errorList, volumevalidation.ValidatePersistentVolume(persistentvolume)...)
 }
 
 // Canonicalize normalizes the object after validation.
@@ -72,8 +74,10 @@ func (persistentvolumeStrategy) PrepareForUpdate(ctx genericapirequest.Context,
 }
 
 func (persistentvolumeStrategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.Object) field.ErrorList {
-	errorList := validation.ValidatePersistentVolume(obj.(*api.PersistentVolume))
+	newPv := obj.(*api.PersistentVolume)
-	return append(errorList, validation.ValidatePersistentVolumeUpdate(obj.(*api.PersistentVolume), old.(*api.PersistentVolume))...)
+	errorList := validation.ValidatePersistentVolume(newPv)
+	errorList = append(errorList, volumevalidation.ValidatePersistentVolume(newPv)...)
+	return append(errorList, validation.ValidatePersistentVolumeUpdate(newPv, old.(*api.PersistentVolume))...)
 }
 
 func (persistentvolumeStrategy) AllowUnconditionalUpdate() bool {

pkg/volume/BUILD

@@ -24,7 +24,6 @@ go_library(
     ],
     tags = ["automanaged"],
     deps = [
-        "//pkg/api:go_default_library",
         "//pkg/api/v1:go_default_library",
         "//pkg/client/clientset_generated/clientset:go_default_library",
         "//pkg/cloudprovider:go_default_library",
@@ -119,6 +118,7 @@ filegroup(
         "//pkg/volume/secret:all-srcs",
         "//pkg/volume/testing:all-srcs",
         "//pkg/volume/util:all-srcs",
+        "//pkg/volume/validation:all-srcs",
         "//pkg/volume/vsphere_volume:all-srcs",
     ],
     tags = ["automanaged"],

pkg/volume/util.go

@@ -23,7 +23,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/watch"
-	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/v1"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
 
@@ -388,16 +387,6 @@ func MountOptionFromSpec(spec *Spec, options ...string) []string {
 	return options
 }
 
-// MountOptionFromApiPV extracts mount options from api.PersistentVolume
-func MountOptionFromApiPV(pv *api.PersistentVolume) []string {
-	mountOptions := []string{}
-	if mo, ok := pv.Annotations[MountOptionAnnotation]; ok {
-		moList := strings.Split(mo, ",")
-		return JoinMountOptions(moList, mountOptions)
-	}
-	return mountOptions
-}
-
 // JoinMountOptions joins mount options eliminating duplicates
 func JoinMountOptions(userOptions []string, systemOptions []string) []string {
 	allMountOptions := sets.NewString()

pkg/volume/validation/BUILD

@@ -0,0 +1,44 @@
+package(default_visibility = ["//visibility:public"])
+
+licenses(["notice"])
+
+load(
+    "@io_bazel_rules_go//go:def.bzl",
+    "go_library",
+    "go_test",
+)
+
+go_test(
+    name = "go_default_test",
+    srcs = ["pv_validation_test.go"],
+    library = ":go_default_library",
+    tags = ["automanaged"],
+    deps = [
+        "//pkg/api:go_default_library",
+        "//vendor:k8s.io/apimachinery/pkg/api/resource",
+        "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",
+    ],
+)
+
+go_library(
+    name = "go_default_library",
+    srcs = ["pv_validation.go"],
+    tags = ["automanaged"],
+    deps = [
+        "//pkg/api:go_default_library",
+        "//vendor:k8s.io/apimachinery/pkg/util/validation/field",
+    ],
+)
+
+filegroup(
+    name = "package-srcs",
+    srcs = glob(["**"]),
+    tags = ["automanaged"],
+    visibility = ["//visibility:private"],
+)
+
+filegroup(
+    name = "all-srcs",
+    srcs = [":package-srcs"],
+    tags = ["automanaged"],
+)

pkg/volume/validation/pv_validation.go

@@ -0,0 +1,58 @@
+/*
+Copyright 2017 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 validation
+
+import (
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/kubernetes/pkg/api"
+)
+
+// MountOptionAnnotation defines mount option annotation used in PVs
+const MountOptionAnnotation = "volume.beta.kubernetes.io/mount-options"
+
+// ValidatePersistentVolume validates PV object for plugin specific validation
+// We can put here validations which are specific to volume types.
+func ValidatePersistentVolume(pv *api.PersistentVolume) field.ErrorList {
+	return checkMountOption(pv)
+}
+
+func checkMountOption(pv *api.PersistentVolume) field.ErrorList {
+	allErrs := field.ErrorList{}
+	// if PV is of these types we don't return errors
+	// since mount options is supported
+	if pv.Spec.GCEPersistentDisk != nil ||
+		pv.Spec.AWSElasticBlockStore != nil ||
+		pv.Spec.Glusterfs != nil ||
+		pv.Spec.NFS != nil ||
+		pv.Spec.RBD != nil ||
+		pv.Spec.Quobyte != nil ||
+		pv.Spec.ISCSI != nil ||
+		pv.Spec.Cinder != nil ||
+		pv.Spec.CephFS != nil ||
+		pv.Spec.AzureFile != nil ||
+		pv.Spec.VsphereVolume != nil ||
+		pv.Spec.AzureDisk != nil ||
+		pv.Spec.PhotonPersistentDisk != nil {
+		return allErrs
+	}
+	// any other type if mount option is present lets return error
+	if _, ok := pv.Annotations[MountOptionAnnotation]; ok {
+		metaField := field.NewPath("metadata")
+		allErrs = append(allErrs, field.Forbidden(metaField.Child("annotations", MountOptionAnnotation), "may not specify mount options for this volume type"))
+	}
+	return allErrs
+}

pkg/volume/validation/pv_validation_test.go

@@ -0,0 +1,86 @@
+/*
+Copyright 2017 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 validation
+
+import (
+	"testing"
+
+	"k8s.io/apimachinery/pkg/api/resource"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/kubernetes/pkg/api"
+)
+
+func TestValidatePersistentVolumes(t *testing.T) {
+	scenarios := map[string]struct {
+		isExpectedFailure bool
+		volume            *api.PersistentVolume
+	}{
+		"volume with valid mount option for nfs": {
+			isExpectedFailure: false,
+			volume: testVolumeWithMountOption("good-nfs-mount-volume", "", "ro,nfsvers=3", api.PersistentVolumeSpec{
+				Capacity: api.ResourceList{
+					api.ResourceName(api.ResourceStorage): resource.MustParse("10G"),
+				},
+				AccessModes: []api.PersistentVolumeAccessMode{api.ReadWriteOnce},
+				PersistentVolumeSource: api.PersistentVolumeSource{
+					NFS: &api.NFSVolumeSource{Server: "localhost", Path: "/srv", ReadOnly: false},
+				},
+			}),
+		},
+		"volume with mount option for host path": {
+			isExpectedFailure: true,
+			volume: testVolumeWithMountOption("bad-hostpath-mount-volume", "", "ro,nfsvers=3", api.PersistentVolumeSpec{
+				Capacity: api.ResourceList{
+					api.ResourceName(api.ResourceStorage): resource.MustParse("10G"),
+				},
+				AccessModes: []api.PersistentVolumeAccessMode{api.ReadWriteOnce},
+				PersistentVolumeSource: api.PersistentVolumeSource{
+					HostPath: &api.HostPathVolumeSource{Path: "/a/.."},
+				},
+			}),
+		},
+	}
+
+	for name, scenario := range scenarios {
+		errs := ValidatePersistentVolume(scenario.volume)
+		if len(errs) == 0 && scenario.isExpectedFailure {
+			t.Errorf("Unexpected success for scenario: %s", name)
+		}
+		if len(errs) > 0 && !scenario.isExpectedFailure {
+			t.Errorf("Unexpected failure for scenario: %s - %+v", name, errs)
+		}
+	}
+}
+
+func testVolumeWithMountOption(name string, namespace string, mountOptions string, spec api.PersistentVolumeSpec) *api.PersistentVolume {
+	annotations := map[string]string{
+		MountOptionAnnotation: mountOptions,
+	}
+	objMeta := metav1.ObjectMeta{
+		Name:        name,
+		Annotations: annotations,
+	}
+
+	if namespace != "" {
+		objMeta.Namespace = namespace
+	}
+
+	return &api.PersistentVolume{
+		ObjectMeta: objMeta,
+		Spec:       spec,
+	}
+}