Merge pull request #61324 from pospispa/60764-K8s-1.10-StorageObjectInUseProtection-downgrade-issue

Automatic merge from submit-queue (batch tested with PRs 61324, 62880, 62765). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Always Start pvc-protection-controller and pv-protection-controller

**What this PR does / why we need it**:
After K8s 1.10 is upgraded to K8s 1.11 finalizer `[kubernetes.io/pvc-protection]` is added to PVCs
because `StorageObjectInUseProtection` feature will be GA in K8s 1.11.
However, when K8s 1.11 is downgraded to K8s 1.10 and the `StorageObjectInUseProtection` feature is disabled the finalizers remain in the PVCs and as `pvc-protection-controller` is not started in K8s 1.10 finalizers are not removed automatically from deleted PVCs and that's why deleted PVC are not removed from the system but remain in `Terminating` phase.
The same applies to `pv-protection-controller` and `[kubernetes.io/pvc-protection]` finalizer in PVs.

That's why `pvc-protection-controller` is always started because the `pvc-protection-controller` removes finalizers from PVCs automatically when a PVC is not in active use by a pod.
Also the `pv-protection-controller` is always started to remove finalizers from PVs automatically when a PV is not `Bound` to a PVC.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes N/A
This issue https://github.com/kubernetes/kubernetes/issues/60764 is for downgrade from K8s 1.10 to K8s 1.9.
This PR fixes the same problem but for downgrade from K8s 1.11 to K8s 1.10.

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```

cmd/kube-controller-manager/app/core.go

@@ -395,24 +395,20 @@ func startGarbageCollectorController(ctx ControllerContext) (bool, error) {
 }
 
 func startPVCProtectionController(ctx ControllerContext) (bool, error) {
-	if utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection) {
-		go pvcprotection.NewPVCProtectionController(
-			ctx.InformerFactory.Core().V1().PersistentVolumeClaims(),
-			ctx.InformerFactory.Core().V1().Pods(),
-			ctx.ClientBuilder.ClientOrDie("pvc-protection-controller"),
-		).Run(1, ctx.Stop)
-		return true, nil
-	}
-	return false, nil
+	go pvcprotection.NewPVCProtectionController(
+		ctx.InformerFactory.Core().V1().PersistentVolumeClaims(),
+		ctx.InformerFactory.Core().V1().Pods(),
+		ctx.ClientBuilder.ClientOrDie("pvc-protection-controller"),
+		utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection),
+	).Run(1, ctx.Stop)
+	return true, nil
 }
 
 func startPVProtectionController(ctx ControllerContext) (bool, error) {
-	if utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection) {
-		go pvprotection.NewPVProtectionController(
-			ctx.InformerFactory.Core().V1().PersistentVolumes(),
-			ctx.ClientBuilder.ClientOrDie("pv-protection-controller"),
-		).Run(1, ctx.Stop)
-		return true, nil
-	}
-	return false, nil
+	go pvprotection.NewPVProtectionController(
+		ctx.InformerFactory.Core().V1().PersistentVolumes(),
+		ctx.ClientBuilder.ClientOrDie("pv-protection-controller"),
+		utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection),
+	).Run(1, ctx.Stop)
+	return true, nil
 }

pkg/controller/volume/pvcprotection/pvc_protection_controller.go

@@ -49,13 +49,17 @@ type Controller struct {
 	podListerSynced cache.InformerSynced
 
 	queue workqueue.RateLimitingInterface
+
+	// allows overriding of StorageObjectInUseProtection feature Enabled/Disabled for testing
+	storageObjectInUseProtectionEnabled bool
 }
 
 // NewPVCProtectionController returns a new *{VCProtectionController.
-func NewPVCProtectionController(pvcInformer coreinformers.PersistentVolumeClaimInformer, podInformer coreinformers.PodInformer, cl clientset.Interface) *Controller {
+func NewPVCProtectionController(pvcInformer coreinformers.PersistentVolumeClaimInformer, podInformer coreinformers.PodInformer, cl clientset.Interface, storageObjectInUseProtectionFeatureEnabled bool) *Controller {
 	e := &Controller{
 		client: cl,
 		queue:  workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "pvcprotection"),
+		storageObjectInUseProtectionEnabled: storageObjectInUseProtectionFeatureEnabled,
 	}
 	if cl != nil && cl.CoreV1().RESTClient().GetRateLimiter() != nil {
 		metrics.RegisterMetricAndTrackRateLimiterUsage("persistentvolumeclaim_protection_controller", cl.CoreV1().RESTClient().GetRateLimiter())
@@ -176,6 +180,10 @@ func (c *Controller) processPVC(pvcNamespace, pvcName string) error {
 }
 
 func (c *Controller) addFinalizer(pvc *v1.PersistentVolumeClaim) error {
+	// Skip adding Finalizer in case the StorageObjectInUseProtection feature is not enabled
+	if !c.storageObjectInUseProtectionEnabled {
+		return nil
+	}
 	claimClone := pvc.DeepCopy()
 	claimClone.ObjectMeta.Finalizers = append(claimClone.ObjectMeta.Finalizers, volumeutil.PVCProtectionFinalizer)
 	_, err := c.client.CoreV1().PersistentVolumeClaims(claimClone.Namespace).Update(claimClone)

pkg/controller/volume/pvcprotection/pvc_protection_controller_test.go

@@ -162,22 +162,31 @@ func TestPVCProtectionController(t *testing.T) {
 		deletedPod *v1.Pod
 		// List of expected kubeclient actions that should happen during the
 		// test.
-		expectedActions []clienttesting.Action
+		expectedActions                     []clienttesting.Action
+		storageObjectInUseProtectionEnabled bool
 	}{
 		//
 		// PVC events
 		//
 		{
-			name:       "PVC without finalizer -> finalizer is added",
+			name:       "StorageObjectInUseProtection Enabled, PVC without finalizer -> finalizer is added",
 			updatedPVC: pvc(),
 			expectedActions: []clienttesting.Action{
 				clienttesting.NewUpdateAction(pvcVer, defaultNS, withProtectionFinalizer(pvc())),
 			},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		{
-			name:            "PVC with finalizer -> no action",
-			updatedPVC:      withProtectionFinalizer(pvc()),
-			expectedActions: []clienttesting.Action{},
+			name:                                "StorageObjectInUseProtection Disabled, PVC without finalizer -> finalizer is added",
+			updatedPVC:                          pvc(),
+			expectedActions:                     []clienttesting.Action{},
+			storageObjectInUseProtectionEnabled: false,
+		},
+		{
+			name:                                "PVC with finalizer -> no action",
+			updatedPVC:                          withProtectionFinalizer(pvc()),
+			expectedActions:                     []clienttesting.Action{},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		{
 			name:       "saving PVC finalizer fails -> controller retries",
@@ -197,13 +206,23 @@ func TestPVCProtectionController(t *testing.T) {
 				// This succeeds
 				clienttesting.NewUpdateAction(pvcVer, defaultNS, withProtectionFinalizer(pvc())),
 			},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		{
-			name:       "deleted PVC with finalizer -> finalizer is removed",
+			name:       "StorageObjectInUseProtection Enabled, deleted PVC with finalizer -> finalizer is removed",
 			updatedPVC: deleted(withProtectionFinalizer(pvc())),
 			expectedActions: []clienttesting.Action{
 				clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
 			},
+			storageObjectInUseProtectionEnabled: true,
+		},
+		{
+			name:       "StorageObjectInUseProtection Disabled, deleted PVC with finalizer -> finalizer is removed",
+			updatedPVC: deleted(withProtectionFinalizer(pvc())),
+			expectedActions: []clienttesting.Action{
+				clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
+			},
+			storageObjectInUseProtectionEnabled: false,
 		},
 		{
 			name:       "finalizer removal fails -> controller retries",
@@ -223,6 +242,7 @@ func TestPVCProtectionController(t *testing.T) {
 				// Succeeds
 				clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
 			},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		{
 			name: "deleted PVC with finalizer + pods with the PVC exists -> finalizer is not removed",
@@ -241,9 +261,10 @@ func TestPVCProtectionController(t *testing.T) {
 			expectedActions: []clienttesting.Action{
 				clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
 			},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		{
-			name: "deleted PVC with finalizer + pods with the PVC andis finished -> finalizer is removed",
+			name: "deleted PVC with finalizer + pods with the PVC and is finished -> finalizer is removed",
 			initialObjects: []runtime.Object{
 				withStatus(v1.PodFailed, withPVC(defaultPVCName, pod())),
 			},
@@ -251,6 +272,7 @@ func TestPVCProtectionController(t *testing.T) {
 			expectedActions: []clienttesting.Action{
 				clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
 			},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		//
 		// Pod events
@@ -260,8 +282,9 @@ func TestPVCProtectionController(t *testing.T) {
 			initialObjects: []runtime.Object{
 				deleted(withProtectionFinalizer(pvc())),
 			},
-			updatedPod:      withStatus(v1.PodRunning, withPVC(defaultPVCName, pod())),
-			expectedActions: []clienttesting.Action{},
+			updatedPod:                          withStatus(v1.PodRunning, withPVC(defaultPVCName, pod())),
+			expectedActions:                     []clienttesting.Action{},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		{
 			name: "updated finished Pod -> finalizer is removed",
@@ -272,6 +295,7 @@ func TestPVCProtectionController(t *testing.T) {
 			expectedActions: []clienttesting.Action{
 				clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
 			},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		{
 			name: "updated unscheduled Pod -> finalizer is removed",
@@ -282,6 +306,7 @@ func TestPVCProtectionController(t *testing.T) {
 			expectedActions: []clienttesting.Action{
 				clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
 			},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		{
 			name: "deleted running Pod -> finalizer is removed",
@@ -292,6 +317,7 @@ func TestPVCProtectionController(t *testing.T) {
 			expectedActions: []clienttesting.Action{
 				clienttesting.NewUpdateAction(pvcVer, defaultNS, deleted(pvc())),
 			},
+			storageObjectInUseProtectionEnabled: true,
 		},
 	}
 
@@ -330,7 +356,7 @@ func TestPVCProtectionController(t *testing.T) {
 		}
 
 		// Create the controller
-		ctrl := NewPVCProtectionController(pvcInformer, podInformer, client)
+		ctrl := NewPVCProtectionController(pvcInformer, podInformer, client, test.storageObjectInUseProtectionEnabled)
 
 		// Start the test by simulating an event
 		if test.updatedPVC != nil {

pkg/controller/volume/pvprotection/pv_protection_controller.go

@@ -45,13 +45,17 @@ type Controller struct {
 	pvListerSynced cache.InformerSynced
 
 	queue workqueue.RateLimitingInterface
+
+	// allows overriding of StorageObjectInUseProtection feature Enabled/Disabled for testing
+	storageObjectInUseProtectionEnabled bool
 }
 
 // NewPVProtectionController returns a new *Controller.
-func NewPVProtectionController(pvInformer coreinformers.PersistentVolumeInformer, cl clientset.Interface) *Controller {
+func NewPVProtectionController(pvInformer coreinformers.PersistentVolumeInformer, cl clientset.Interface, storageObjectInUseProtectionFeatureEnabled bool) *Controller {
 	e := &Controller{
 		client: cl,
 		queue:  workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "pvprotection"),
+		storageObjectInUseProtectionEnabled: storageObjectInUseProtectionFeatureEnabled,
 	}
 	if cl != nil && cl.CoreV1().RESTClient().GetRateLimiter() != nil {
 		metrics.RegisterMetricAndTrackRateLimiterUsage("persistentvolume_protection_controller", cl.CoreV1().RESTClient().GetRateLimiter())
@@ -151,6 +155,10 @@ func (c *Controller) processPV(pvName string) error {
 }
 
 func (c *Controller) addFinalizer(pv *v1.PersistentVolume) error {
+	// Skip adding Finalizer in case the StorageObjectInUseProtection feature is not enabled
+	if !c.storageObjectInUseProtectionEnabled {
+		return nil
+	}
 	pvClone := pv.DeepCopy()
 	pvClone.ObjectMeta.Finalizers = append(pvClone.ObjectMeta.Finalizers, volumeutil.PVProtectionFinalizer)
 	_, err := c.client.CoreV1().PersistentVolumes().Update(pvClone)

pkg/controller/volume/pvprotection/pv_protection_controller_test.go

@@ -111,21 +111,30 @@ func TestPVProtectionController(t *testing.T) {
 		updatedPV *v1.PersistentVolume
 		// List of expected kubeclient actions that should happen during the
 		// test.
-		expectedActions []clienttesting.Action
+		expectedActions                     []clienttesting.Action
+		storageObjectInUseProtectionEnabled bool
 	}{
 		// PV events
 		//
 		{
-			name:      "PV without finalizer -> finalizer is added",
+			name:      "StorageObjectInUseProtection Enabled, PV without finalizer -> finalizer is added",
 			updatedPV: pv(),
 			expectedActions: []clienttesting.Action{
 				clienttesting.NewUpdateAction(pvVer, "", withProtectionFinalizer(pv())),
 			},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		{
-			name:            "PVC with finalizer -> no action",
-			updatedPV:       withProtectionFinalizer(pv()),
-			expectedActions: []clienttesting.Action{},
+			name:                                "StorageObjectInUseProtection Disabled, PV without finalizer -> finalizer is added",
+			updatedPV:                           pv(),
+			expectedActions:                     []clienttesting.Action{},
+			storageObjectInUseProtectionEnabled: false,
+		},
+		{
+			name:                                "PVC with finalizer -> no action",
+			updatedPV:                           withProtectionFinalizer(pv()),
+			expectedActions:                     []clienttesting.Action{},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		{
 			name:      "saving PVC finalizer fails -> controller retries",
@@ -145,13 +154,23 @@ func TestPVProtectionController(t *testing.T) {
 				// This succeeds
 				clienttesting.NewUpdateAction(pvVer, "", withProtectionFinalizer(pv())),
 			},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		{
-			name:      "deleted PV with finalizer -> finalizer is removed",
+			name:      "StorageObjectInUseProtection Enabled, deleted PV with finalizer -> finalizer is removed",
 			updatedPV: deleted(withProtectionFinalizer(pv())),
 			expectedActions: []clienttesting.Action{
 				clienttesting.NewUpdateAction(pvVer, "", deleted(pv())),
 			},
+			storageObjectInUseProtectionEnabled: true,
+		},
+		{
+			name:      "StorageObjectInUseProtection Disabled, deleted PV with finalizer -> finalizer is removed",
+			updatedPV: deleted(withProtectionFinalizer(pv())),
+			expectedActions: []clienttesting.Action{
+				clienttesting.NewUpdateAction(pvVer, "", deleted(pv())),
+			},
+			storageObjectInUseProtectionEnabled: false,
 		},
 		{
 			name:      "finalizer removal fails -> controller retries",
@@ -171,11 +190,13 @@ func TestPVProtectionController(t *testing.T) {
 				// Succeeds
 				clienttesting.NewUpdateAction(pvVer, "", deleted(pv())),
 			},
+			storageObjectInUseProtectionEnabled: true,
 		},
 		{
-			name:            "deleted PVC with finalizer + PV is bound -> finalizer is not removed",
-			updatedPV:       deleted(withProtectionFinalizer(boundPV())),
-			expectedActions: []clienttesting.Action{},
+			name:                                "deleted PVC with finalizer + PV is bound -> finalizer is not removed",
+			updatedPV:                           deleted(withProtectionFinalizer(boundPV())),
+			expectedActions:                     []clienttesting.Action{},
+			storageObjectInUseProtectionEnabled: true,
 		},
 	}
 
@@ -209,7 +230,7 @@ func TestPVProtectionController(t *testing.T) {
 		}
 
 		// Create the controller
-		ctrl := NewPVProtectionController(pvInformer, client)
+		ctrl := NewPVProtectionController(pvInformer, client, test.storageObjectInUseProtectionEnabled)
 
 		// Start the test by simulating an event
 		if test.updatedPV != nil {

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go

@@ -324,25 +324,21 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
 			eventsRule(),
 		},
 	})
-	if utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection) {
-		addControllerRole(&controllerRoles, &controllerRoleBindings, rbac.ClusterRole{
-			ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "pvc-protection-controller"},
-			Rules: []rbac.PolicyRule{
-				rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumeclaims").RuleOrDie(),
-				rbac.NewRule("list", "watch", "get").Groups(legacyGroup).Resources("pods").RuleOrDie(),
-				eventsRule(),
-			},
-		})
-	}
-	if utilfeature.DefaultFeatureGate.Enabled(features.StorageObjectInUseProtection) {
-		addControllerRole(&controllerRoles, &controllerRoleBindings, rbac.ClusterRole{
-			ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "pv-protection-controller"},
-			Rules: []rbac.PolicyRule{
-				rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumes").RuleOrDie(),
-				eventsRule(),
-			},
-		})
-	}
+	addControllerRole(&controllerRoles, &controllerRoleBindings, rbac.ClusterRole{
+		ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "pvc-protection-controller"},
+		Rules: []rbac.PolicyRule{
+			rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumeclaims").RuleOrDie(),
+			rbac.NewRule("list", "watch", "get").Groups(legacyGroup).Resources("pods").RuleOrDie(),
+			eventsRule(),
+		},
+	})
+	addControllerRole(&controllerRoles, &controllerRoleBindings, rbac.ClusterRole{
+		ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + "pv-protection-controller"},
+		Rules: []rbac.PolicyRule{
+			rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumes").RuleOrDie(),
+			eventsRule(),
+		},
+	})
 
 	return controllerRoles, controllerRoleBindings
 }