Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Flip SELinuxMountReadWriteOncePod to Beta #116425

Merged
merged 3 commits into from Mar 14, 2023
Merged

Flip SELinuxMountReadWriteOncePod to Beta #116425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a84dc2d
Flip SELinuxMountReadWriteOncePod to Beta
jsafrane Mar 9, 2023
a53c6f1
Add validation of CSIDriver.SELinuxMount
jsafrane Mar 14, 2023
58c4ead
Add featureGate to CSIDriver.SELinuxMount
jsafrane Mar 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/apis/storage/fuzzer/fuzzer.go
View file
Open in desktop
Expand Up @@ -96,6 +96,10 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} {
storage.VolumeLifecyclePersistent,
}
}
if obj.Spec.SELinuxMount == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we missed this during the api review for alpha, but do we want validation on the new field similar to other boolean fields?

kubernetes/pkg/apis/storage/validation/validation.go

Line 433 in de9ce03

allErrs = append(allErrs, validateAttachRequired(spec.AttachRequired, fldPath.Child("attachedRequired"))...)

Copy link
Member Author

@jsafrane jsafrane Mar 14, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some validation + unit tests in a new commit, PTAL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kubernetes/staging/src/k8s.io/api/storage/v1/types.go

Line 416 in e660a17

SELinuxMount *bool `json:"seLinuxMount,omitempty" protobuf:"varint,8,opt,name=seLinuxMount"`
+ internal types.go should be updated with the new "+featureGate" tag:

kubernetes/pkg/apis/core/types.go

Line 3141 in a9008b5

// +featureGate=PodSchedulingReadiness

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to v1 and internal types.go

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and v1beta1

obj.Spec.SELinuxMount = new(bool)
*(obj.Spec.SELinuxMount) = false
}
},
}
}
1 change: 1 addition & 0 deletions pkg/apis/storage/types.go
View file
Open in desktop
Expand Up @@ -409,6 +409,7 @@ type CSIDriverSpec struct {
//
// Default is "false".
//
// +featureGate=SELinuxMountReadWriteOncePod
// +optional
SELinuxMount *bool
}
Expand Down
13 changes: 13 additions & 0 deletions pkg/apis/storage/validation/validation.go
View file
Open in desktop
Expand Up @@ -27,10 +27,12 @@ import (
metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation/field"
utilfeature "k8s.io/apiserver/pkg/util/feature"
api "k8s.io/kubernetes/pkg/apis/core"
"k8s.io/kubernetes/pkg/apis/core/helper"
apivalidation "k8s.io/kubernetes/pkg/apis/core/validation"
"k8s.io/kubernetes/pkg/apis/storage"
"k8s.io/kubernetes/pkg/features"
)

const (
Expand Down Expand Up @@ -436,6 +438,7 @@ func validateCSIDriverSpec(
allErrs = append(allErrs, validateFSGroupPolicy(spec.FSGroupPolicy, fldPath.Child("fsGroupPolicy"))...)
allErrs = append(allErrs, validateTokenRequests(spec.TokenRequests, fldPath.Child("tokenRequests"))...)
allErrs = append(allErrs, validateVolumeLifecycleModes(spec.VolumeLifecycleModes, fldPath.Child("volumeLifecycleModes"))...)
allErrs = append(allErrs, validateSELinuxMount(spec.SELinuxMount, fldPath.Child("seLinuxMount"))...)
return allErrs
}

Expand Down Expand Up @@ -533,6 +536,16 @@ func validateVolumeLifecycleModes(modes []storage.VolumeLifecycleMode, fldPath *
return allErrs
}

// validateSELinuxMount tests if seLinuxMount is set for CSIDriver.
func validateSELinuxMount(seLinuxMount *bool, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
if seLinuxMount == nil && utilfeature.DefaultFeatureGate.Enabled(features.SELinuxMountReadWriteOncePod) {
allErrs = append(allErrs, field.Required(fldPath, ""))
}

return allErrs
}

// ValidateStorageCapacityName checks that a name is appropriate for a
// CSIStorageCapacity object.
var ValidateStorageCapacityName = apimachineryvalidation.NameIsDNSSubdomain
Expand Down
119 changes: 119 additions & 0 deletions pkg/apis/storage/validation/validation_test.go
View file
Open in desktop
Expand Up @@ -23,8 +23,11 @@ import (

"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
utilfeature "k8s.io/apiserver/pkg/util/feature"
featuregatetesting "k8s.io/component-base/featuregate/testing"
api "k8s.io/kubernetes/pkg/apis/core"
"k8s.io/kubernetes/pkg/apis/storage"
"k8s.io/kubernetes/pkg/features"
utilpointer "k8s.io/utils/pointer"
)

Expand Down Expand Up @@ -1657,6 +1660,8 @@ func TestCSIDriverValidation(t *testing.T) {
notRequiresRepublish := false
storageCapacity := true
notStorageCapacity := false
seLinuxMount := true
notSELinuxMount := false
supportedFSGroupPolicy := storage.FileFSGroupPolicy
invalidFSGroupPolicy := storage.FSGroupPolicy("invalid-mode")
successCases := []storage.CSIDriver{
Expand All @@ -1667,6 +1672,7 @@ func TestCSIDriverValidation(t *testing.T) {
PodInfoOnMount: &podInfoOnMount,
RequiresRepublish: &notRequiresRepublish,
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1677,6 +1683,7 @@ func TestCSIDriverValidation(t *testing.T) {
PodInfoOnMount: &podInfoOnMount,
RequiresRepublish: &notRequiresRepublish,
StorageCapacity: &notStorageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1687,6 +1694,7 @@ func TestCSIDriverValidation(t *testing.T) {
PodInfoOnMount: &notPodInfoOnMount,
RequiresRepublish: &notRequiresRepublish,
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1697,6 +1705,7 @@ func TestCSIDriverValidation(t *testing.T) {
PodInfoOnMount: &podInfoOnMount,
RequiresRepublish: &notRequiresRepublish,
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1707,6 +1716,7 @@ func TestCSIDriverValidation(t *testing.T) {
PodInfoOnMount: &podInfoOnMount,
RequiresRepublish: &notRequiresRepublish,
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1716,6 +1726,7 @@ func TestCSIDriverValidation(t *testing.T) {
PodInfoOnMount: &notPodInfoOnMount,
RequiresRepublish: &notRequiresRepublish,
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1725,6 +1736,7 @@ func TestCSIDriverValidation(t *testing.T) {
PodInfoOnMount: &podInfoOnMount,
RequiresRepublish: &notRequiresRepublish,
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1734,6 +1746,7 @@ func TestCSIDriverValidation(t *testing.T) {
PodInfoOnMount: &notPodInfoOnMount,
RequiresRepublish: &notRequiresRepublish,
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1746,6 +1759,7 @@ func TestCSIDriverValidation(t *testing.T) {
VolumeLifecycleModes: []storage.VolumeLifecycleMode{
storage.VolumeLifecyclePersistent,
},
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1758,6 +1772,7 @@ func TestCSIDriverValidation(t *testing.T) {
VolumeLifecycleModes: []storage.VolumeLifecycleMode{
storage.VolumeLifecycleEphemeral,
},
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1771,6 +1786,7 @@ func TestCSIDriverValidation(t *testing.T) {
storage.VolumeLifecycleEphemeral,
storage.VolumeLifecyclePersistent,
},
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1785,6 +1801,7 @@ func TestCSIDriverValidation(t *testing.T) {
storage.VolumeLifecyclePersistent,
storage.VolumeLifecycleEphemeral,
},
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1795,6 +1812,18 @@ func TestCSIDriverValidation(t *testing.T) {
RequiresRepublish: &notRequiresRepublish,
StorageCapacity: &storageCapacity,
FSGroupPolicy: &supportedFSGroupPolicy,
SELinuxMount: &seLinuxMount,
},
},
{
// SELinuxMount: false
ObjectMeta: metav1.ObjectMeta{Name: driverName},
Spec: storage.CSIDriverSpec{
AttachRequired: &attachNotRequired,
PodInfoOnMount: &notPodInfoOnMount,
RequiresRepublish: &notRequiresRepublish,
StorageCapacity: &storageCapacity,
SELinuxMount: &notSELinuxMount,
},
},
}
Expand All @@ -1811,6 +1840,7 @@ func TestCSIDriverValidation(t *testing.T) {
AttachRequired: &attachRequired,
PodInfoOnMount: &podInfoOnMount,
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1819,6 +1849,7 @@ func TestCSIDriverValidation(t *testing.T) {
AttachRequired: &attachNotRequired,
PodInfoOnMount: &notPodInfoOnMount,
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1828,6 +1859,7 @@ func TestCSIDriverValidation(t *testing.T) {
AttachRequired: nil,
PodInfoOnMount: &podInfoOnMount,
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1837,6 +1869,7 @@ func TestCSIDriverValidation(t *testing.T) {
AttachRequired: &attachNotRequired,
PodInfoOnMount: nil,
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1846,6 +1879,7 @@ func TestCSIDriverValidation(t *testing.T) {
AttachRequired: &attachNotRequired,
PodInfoOnMount: &podInfoOnMount,
StorageCapacity: nil,
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1858,6 +1892,7 @@ func TestCSIDriverValidation(t *testing.T) {
VolumeLifecycleModes: []storage.VolumeLifecycleMode{
"no-such-mode",
},
SELinuxMount: &seLinuxMount,
},
},
{
Expand All @@ -1868,6 +1903,16 @@ func TestCSIDriverValidation(t *testing.T) {
PodInfoOnMount: &notPodInfoOnMount,
FSGroupPolicy: &invalidFSGroupPolicy,
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
},
{
// no SELinuxMount
ObjectMeta: metav1.ObjectMeta{Name: driverName},
Spec: storage.CSIDriverSpec{
AttachRequired: &attachNotRequired,
PodInfoOnMount: &notPodInfoOnMount,
StorageCapacity: &storageCapacity,
},
},
}
Expand All @@ -1892,6 +1937,8 @@ func TestCSIDriverValidationUpdate(t *testing.T) {
requiresRepublish := true
notRequiresRepublish := false
notStorageCapacity := false
seLinuxMount := true
notSELinuxMount := false
resourceVersion := "1"
old := storage.CSIDriver{
ObjectMeta: metav1.ObjectMeta{Name: driverName, ResourceVersion: resourceVersion},
Expand All @@ -1904,6 +1951,7 @@ func TestCSIDriverValidationUpdate(t *testing.T) {
storage.VolumeLifecyclePersistent,
},
StorageCapacity: &storageCapacity,
SELinuxMount: &seLinuxMount,
},
}

Expand Down Expand Up @@ -1933,6 +1981,12 @@ func TestCSIDriverValidationUpdate(t *testing.T) {
new.Spec.StorageCapacity = &notStorageCapacity
},
},
{
name: "SELinuxMount changed",
modify: func(new *storage.CSIDriver) {
new.Spec.SELinuxMount = &notSELinuxMount
},
},
}
for _, test := range successCases {
t.Run(test.name, func(t *testing.T) {
Expand Down Expand Up @@ -2041,6 +2095,12 @@ func TestCSIDriverValidationUpdate(t *testing.T) {
new.Spec.StorageCapacity = nil
},
},
{
name: "SELinuxMount not set",
modify: func(new *storage.CSIDriver) {
new.Spec.SELinuxMount = nil
},
},
}

for _, test := range errorCases {
Expand All @@ -2061,12 +2121,14 @@ func TestCSIDriverStorageCapacityEnablement(t *testing.T) {
podInfoOnMount := true
requiresRepublish := true
storageCapacity := true
seLinuxMount := false
csiDriver := storage.CSIDriver{
ObjectMeta: metav1.ObjectMeta{Name: driverName},
Spec: storage.CSIDriverSpec{
AttachRequired: &attachRequired,
PodInfoOnMount: &podInfoOnMount,
RequiresRepublish: &requiresRepublish,
SELinuxMount: &seLinuxMount,
},
}
if withField {
Expand Down Expand Up @@ -2260,8 +2322,65 @@ func TestCSIServiceAccountToken(t *testing.T) {
test.csiDriver.Spec.AttachRequired = new(bool)
test.csiDriver.Spec.PodInfoOnMount = new(bool)
test.csiDriver.Spec.StorageCapacity = new(bool)
test.csiDriver.Spec.SELinuxMount = new(bool)
if errs := ValidateCSIDriver(test.csiDriver); test.wantErr != (len(errs) != 0) {
t.Errorf("ValidateCSIDriver = %v, want err: %v", errs, test.wantErr)
}
}
}

func TestCSIDriverValidationSELinuxMountAlpha(t *testing.T) {
tests := []struct {
name string
featureEnabled bool
seLinuxMountValue *bool
expectError bool
}{
{
name: "feature enabled, nil value",
featureEnabled: true,
seLinuxMountValue: nil,
expectError: true,
},
{
name: "feature enabled, non-nil value",
featureEnabled: true,
seLinuxMountValue: utilpointer.Bool(true),
expectError: false,
},
{
name: "feature disabled, nil value",
featureEnabled: false,
seLinuxMountValue: nil,
expectError: false,
},
{
name: "feature disabled, non-nil value",
featureEnabled: false,
seLinuxMountValue: utilpointer.Bool(true),
expectError: false,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SELinuxMountReadWriteOncePod, test.featureEnabled)()
csiDriver := &storage.CSIDriver{
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
Spec: storage.CSIDriverSpec{
AttachRequired: utilpointer.Bool(true),
PodInfoOnMount: utilpointer.Bool(true),
RequiresRepublish: utilpointer.Bool(true),
StorageCapacity: utilpointer.Bool(true),
SELinuxMount: test.seLinuxMountValue,
},
}
err := ValidateCSIDriver(csiDriver)
if test.expectError && err == nil {
t.Error("Expected validation error, got nil")
}
if !test.expectError && err != nil {
t.Errorf("Validation returned error: %s", err)
}
})
}
}
3 changes: 2 additions & 1 deletion pkg/features/kube_features.go
View file
Open in desktop
Expand Up @@ -858,6 +858,7 @@ const (
// owner: @jsafrane
// kep: https://kep.k8s.io/1710
// alpha: v1.25
// beta: v1.27
// Speed up container startup by mounting volumes with the correct SELinux label
// instead of changing each file on the volumes recursively.
// Initial implementation focused on ReadWriteOncePod volumes.
Expand Down Expand Up @@ -1098,7 +1099,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS

NodeInclusionPolicyInPodTopologySpread: {Default: true, PreRelease: featuregate.Beta},

SELinuxMountReadWriteOncePod: {Default: false, PreRelease: featuregate.Alpha},
SELinuxMountReadWriteOncePod: {Default: true, PreRelease: featuregate.Beta},

InPlacePodVerticalScaling: {Default: false, PreRelease: featuregate.Alpha},

Expand Down