Re-review API validation for CSI Inline Migration

[msau42]

What happened:
While glancing through storage validation.go, I noticed that we were doing feature gate checking in validation for CSI inline migration. At first glance, I think this will cause validation errors when downgrading to a version with the feature disabled.

c34309a#diff-5aba802af469dbe928456a4b2c7988adR177

What you expected to happen:
We generally don't do feature gate checking in validation in order to support downgrade cases.

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:

Environment:

• Kubernetes version (use kubectl version):
• Cloud provider or hardware configuration:
• OS (e.g: cat /etc/os-release):
• Kernel (e.g. uname -a):
• Install tools:
• Network plugin and version (if this is a network-related bug):
• Others:

[msau42]

@kubernetes/sig-storage-bugs
/assign @msau42 @liggitt @jsafrane
cc @davidz627 @ddebroy

[tedyu]

In validateVolumeAttachmentSource :

	case source.InlineVolumeSpec != nil:
		if utilfeature.DefaultFeatureGate.Enabled(features.CSIMigration) {
			allErrs = append(allErrs, storagevalidation.ValidatePersistentVolumeSpec(source.InlineVolumeSpec, "", true, fldPath.Child("inlineVolumeSpec"))...)
		} else {
			allErrs = append(allErrs, field.Forbidden(fldPath, "may not specify inlineVolumeSpec when CSIMigration feature is disabled"))
		}
	}

If we don't check feature gate, it seems source.InlineVolumeSpec != nil case can be dropped.
Otherwise:

--- FAIL: TestVolumeAttachmentValidation (0.00s)
    validation_test.go:231: expected success: {{ } {foo-with-inlinespec      0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[] [] []  []} {myattacher {<nil> 0x21fcb80} mynode} {false map[] <nil> <nil>}} [spec.source: Forbidden: may not specify inlineVolumeSpec when CSIMigration feature is disabled]
    validation_test.go:231: expected success: {{ } {foo-with-inlinespec-and-status      0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[] [] []  []} {myattacher {<nil> 0x21fcb80} mynode} {true map[foo:bar] 0xc0001c2ff0 0xc0001c3020}} [spec.source: Forbidden: may not specify inlineVolumeSpec when CSIMigration feature is disabled]

[ddebroy]

These were added based on presence of other feature-gate validation like features.PodOverhead. Now it makes sense to align with their removal/being made unconditional as well.

There are a couple of instances of feature-gate checking in validation in non-storage areas as well: features.HPAScaleToZero and features.SCTPSupport. I assume those will be changed to unconditional too?

[davidz627]

/cc @DXist @janosi @egernst
for other identified features that may share this issue.

[janosi]

I wonder if this PR from @liggitt has already corrected the fallback problem.
#73774

[liggitt]

Both SCTPSupport (in #73774) and HPAScaleToZero (in the PR where it was introduced) ensure existing objects with feature-gated data are still considered valid even when the feature gate is disabled.

This is a particularly fragile/nuanced area of the project. I'm working on a comprehensive way to tag fields as alpha and exercise the following cases in integration tests to ensure we don't regress:

• feature enabled
  • creating object with alpha data is allowed and persisted
  • creating object without alpha data is allowed and persisted
  • update existing object without alpha data to one with alpha data is allowed and persisted (for objects that allow mutation)
  • update existing object with alpha data to one without alpha data is allowed and persisted (for objects that allow mutation)
• feature disabled
  • creating object with alpha data is either rejected or persisted without the alpha data
  • creating object without alpha data is allowed and persisted
  • update existing object without alpha data to one with alpha data is either rejected or persisted without alpha data (for objects that allow mutation)
  • update existing object with alpha data to one without alpha data is allowed and persisted (for objects that allow mutation)