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

Promote VolumeSubpathEnvExpansion feature gate to GA #82578

Merged
merged 1 commit into from
Oct 10, 2019
Merged

Promote VolumeSubpathEnvExpansion feature gate to GA #82578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion api/openapi-spec/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

79 changes: 39 additions & 40 deletions pkg/api/pod/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1935,52 +1935,51 @@ func TestDropSubPathExpr(t *testing.T) {
},
}

for _, enabled := range []bool{true, false} {
for _, oldPodInfo := range podInfo {
for _, newPodInfo := range podInfo {
oldPodHasSubpaths, oldPod := oldPodInfo.hasSubpaths, oldPodInfo.pod()
newPodHasSubpaths, newPod := newPodInfo.hasSubpaths, newPodInfo.pod()
if newPod == nil {
continue
enabled := true
for _, oldPodInfo := range podInfo {
for _, newPodInfo := range podInfo {
oldPodHasSubpaths, oldPod := oldPodInfo.hasSubpaths, oldPodInfo.pod()
newPodHasSubpaths, newPod := newPodInfo.hasSubpaths, newPodInfo.pod()
if newPod == nil {
continue
}

t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeSubpathEnvExpansion, enabled)()

var oldPodSpec *api.PodSpec
if oldPod != nil {
oldPodSpec = &oldPod.Spec
}
dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)

t.Run(fmt.Sprintf("feature enabled=%v, old pod %v, new pod %v", enabled, oldPodInfo.description, newPodInfo.description), func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeSubpathEnvExpansion, enabled)()
// old pod should never be changed
if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
t.Errorf("old pod changed: %v", diff.ObjectReflectDiff(oldPod, oldPodInfo.pod()))
}

var oldPodSpec *api.PodSpec
if oldPod != nil {
oldPodSpec = &oldPod.Spec
switch {
case enabled || oldPodHasSubpaths:
// new pod should not be changed if the feature is enabled, or if the old pod had subpaths
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
}
dropDisabledFields(&newPod.Spec, nil, oldPodSpec, nil)

// old pod should never be changed
if !reflect.DeepEqual(oldPod, oldPodInfo.pod()) {
t.Errorf("old pod changed: %v", diff.ObjectReflectDiff(oldPod, oldPodInfo.pod()))
case newPodHasSubpaths:
// new pod should be changed
if reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod was not changed")
}

switch {
case enabled || oldPodHasSubpaths:
// new pod should not be changed if the feature is enabled, or if the old pod had subpaths
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
}
case newPodHasSubpaths:
// new pod should be changed
if reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod was not changed")
}
// new pod should not have subpaths
if !reflect.DeepEqual(newPod, podWithoutSubpaths()) {
t.Errorf("new pod had subpaths: %v", diff.ObjectReflectDiff(newPod, podWithoutSubpaths()))
}
default:
// new pod should not need to be changed
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
}
// new pod should not have subpaths
if !reflect.DeepEqual(newPod, podWithoutSubpaths()) {
t.Errorf("new pod had subpaths: %v", diff.ObjectReflectDiff(newPod, podWithoutSubpaths()))
}
})
}
default:
// new pod should not need to be changed
if !reflect.DeepEqual(newPod, newPodInfo.pod()) {
t.Errorf("new pod changed: %v", diff.ObjectReflectDiff(newPod, newPodInfo.pod()))
}
}
})
}
}
}
Expand Down
1 change: 0 additions & 1 deletion pkg/apis/core/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1715,7 +1715,6 @@ type VolumeMount struct {
// Behaves similarly to SubPath but environment variable references $(VAR_NAME) are expanded using the container's environment.
// Defaults to "" (volume's root).
// SubPathExpr and SubPath are mutually exclusive.
// This field is beta in 1.15.
// +optional
SubPathExpr string
}
Expand Down
39 changes: 0 additions & 39 deletions pkg/apis/core/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5160,45 +5160,6 @@ func TestValidateDisabledSubpathExpr(t *testing.T) {
}
}

// Repeat with feature gate off
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeSubpathEnvExpansion, false)()
cases = map[string]struct {
mounts []core.VolumeMount
expectError bool
}{
"subpath expr not specified": {
[]core.VolumeMount{
{
Name: "abc-123",
MountPath: "/bab",
},
},
false,
},
"subpath expr specified": {
[]core.VolumeMount{
{
Name: "abc-123",
MountPath: "/bab",
SubPathExpr: "$(POD_NAME)",
},
},
false, // validation should not fail, dropping the field is handled in PrepareForCreate/PrepareForUpdate
},
}

for name, test := range cases {
errs := ValidateVolumeMounts(test.mounts, GetVolumeDeviceMap(goodVolumeDevices), vols, &container, field.NewPath("field"))

if len(errs) != 0 && !test.expectError {
t.Errorf("test %v failed: %+v", name, errs)
}

if len(errs) == 0 && test.expectError {
t.Errorf("test %v failed, expected error", name)
}
}

// Repeat with subpath feature gate off
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeSubpath, false)()
cases = map[string]struct {
Expand Down
4 changes: 3 additions & 1 deletion pkg/features/kube_features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,9 @@ const (
BalanceAttachedNodeVolumes featuregate.Feature = "BalanceAttachedNodeVolumes"

// owner: @kevtaylor
// alpha: v1.14
// beta: v1.15
// ga: v1.17
Copy link
Member

Choose a reason for hiding this comment

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

Add GA as a new line so we can easily see feature graduation history

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added graduation steps (I put the alpha one back in for completeness)

//
// Allow subpath environment variable substitution
// Only applicable if the VolumeSubpath feature is also enabled
Expand Down Expand Up @@ -539,7 +541,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS
CSIMigrationOpenStack: {Default: false, PreRelease: featuregate.Alpha},
VolumeSubpath: {Default: true, PreRelease: featuregate.GA},
BalanceAttachedNodeVolumes: {Default: false, PreRelease: featuregate.Alpha},
VolumeSubpathEnvExpansion: {Default: true, PreRelease: featuregate.Beta},
VolumeSubpathEnvExpansion: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.19,
ResourceQuotaScopeSelectors: {Default: true, PreRelease: featuregate.Beta},
CSIBlockVolume: {Default: true, PreRelease: featuregate.Beta},
CSIInlineVolume: {Default: true, PreRelease: featuregate.Beta},
Expand Down
1 change: 0 additions & 1 deletion staging/src/k8s.io/api/core/v1/generated.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion staging/src/k8s.io/api/core/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1784,7 +1784,6 @@ type VolumeMount struct {
// Behaves similarly to SubPath but environment variable references $(VAR_NAME) are expanded using the container's environment.
// Defaults to "" (volume's root).
// SubPathExpr and SubPath are mutually exclusive.
// This field is beta in 1.15.
// +optional
SubPathExpr string `json:"subPathExpr,omitempty" protobuf:"bytes,6,opt,name=subPathExpr"`
}
Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions test/e2e/common/expansion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ var _ = framework.KubeDescribe("Variable Expansion", func() {
Description: Make sure a container's subpath can be set using an
expansion of environment variables.
*/
ginkgo.It("should allow substituting values in a volume subpath [sig-storage][NodeFeature:VolumeSubpathEnvExpansion]", func() {
ginkgo.It("should allow substituting values in a volume subpath [sig-storage]", func() {
podName := "var-expansion-" + string(uuid.NewUUID())
pod := &v1.Pod{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -218,7 +218,7 @@ var _ = framework.KubeDescribe("Variable Expansion", func() {
Description: Make sure a container's subpath can not be set using an
expansion of environment variables when backticks are supplied.
*/
ginkgo.It("should fail substituting values in a volume subpath with backticks [sig-storage][NodeFeature:VolumeSubpathEnvExpansion][Slow]", func() {
ginkgo.It("should fail substituting values in a volume subpath with backticks [sig-storage][Slow]", func() {

podName := "var-expansion-" + string(uuid.NewUUID())
pod := &v1.Pod{
Expand Down Expand Up @@ -267,7 +267,7 @@ var _ = framework.KubeDescribe("Variable Expansion", func() {
Description: Make sure a container's subpath can not be set using an
expansion of environment variables when absolute path is supplied.
*/
ginkgo.It("should fail substituting values in a volume subpath with absolute path [sig-storage][NodeFeature:VolumeSubpathEnvExpansion][Slow]", func() {
ginkgo.It("should fail substituting values in a volume subpath with absolute path [sig-storage][Slow]", func() {

podName := "var-expansion-" + string(uuid.NewUUID())
pod := &v1.Pod{
Expand Down Expand Up @@ -315,7 +315,7 @@ var _ = framework.KubeDescribe("Variable Expansion", func() {
Testname: var-expansion-subpath-ready-from-failed-state
Description: Verify that a failing subpath expansion can be modified during the lifecycle of a container.
*/
ginkgo.It("should verify that a failing subpath expansion can be modified during the lifecycle of a container [sig-storage][NodeFeature:VolumeSubpathEnvExpansion][Slow]", func() {
ginkgo.It("should verify that a failing subpath expansion can be modified during the lifecycle of a container [sig-storage][Slow]", func() {

podName := "var-expansion-" + string(uuid.NewUUID())
containerName := "dapi-container"
Expand Down Expand Up @@ -406,7 +406,7 @@ var _ = framework.KubeDescribe("Variable Expansion", func() {
3. successful expansion of the subpathexpr isn't required for volume cleanup

*/
ginkgo.It("should succeed in writing subpaths in container [sig-storage][NodeFeature:VolumeSubpathEnvExpansion][Slow]", func() {
ginkgo.It("should succeed in writing subpaths in container [sig-storage][Slow]", func() {

podName := "var-expansion-" + string(uuid.NewUUID())
containerName := "dapi-container"
Expand Down Expand Up @@ -515,7 +515,7 @@ var _ = framework.KubeDescribe("Variable Expansion", func() {

*/

ginkgo.It("should not change the subpath mount on a container restart if the environment variable changes [sig-storage][NodeFeature:VolumeSubpathEnvExpansion][Slow]", func() {
ginkgo.It("should not change the subpath mount on a container restart if the environment variable changes [sig-storage][Slow]", func() {

suffix := string(uuid.NewUUID())
podName := fmt.Sprintf("var-expansion-%s", suffix)
Expand Down