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

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.

Related issue: https://github.com/kubernetes/kubernetes/issues/60764

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
 }