[SQUASH ME] address review feedbacks (1)

keps/sig-network/20190423-service-lb-finalizer.md

@@ -34,6 +34,7 @@ superseded-by:
   - [Motivation](#motivation)
     - [Goals](#goals)
   - [Proposal](#proposal)
+    - [Risks and Mitigations](#risks-and-mitigations)
     - [Test Plan](#test-plan)
     - [Graduation Criteria](#graduation-criteria)
   - [Implementation History](#implementation-history)
@@ -52,8 +53,8 @@ superseded-by:
 ## Summary
 
 We will be adding finalizer protection to ensure the Service resource is not
-fully deleted until the correlating load balancing resources are also deleted.
-Any service that has `type=LoadBalancer` (both existing and newly created ones)
+fully deleted until the correlating load balancing resources are deleted. Any
+service that has `type=LoadBalancer` (both existing and newly created ones)
 will be attached a service LoadBalancer finalizer, which should be removed by
 service controller upon the cleanup of related load balancing resources. Such
 finalizer protection mechanism will be released with phases to ensure downgrades
@@ -72,13 +73,13 @@ the cleanup of load balancing resources.
 ### Goals
 
 Ensure the Service resource is not fully deleted until the correlating load
-balancing resources are also deleted.
+balancing resources are deleted.
 
 ## Proposal
 
 We are going to define a finalizer for service LoadBalancers with name
-`kubernetes.io/service-load-balancer`. This finalizer will be attached to any
-service that has `type=LoadBalancer`. Upon the deletion of such servicce, the
+`service.kubernetes.io/load-balancer-cleanup`. This finalizer will be attached to
+any service that has `type=LoadBalancer`. Upon the deletion of such service, the
 actual deletion of the resource will be blocked until this finalizer is removed.
 This finalizer will not be removed until cleanup of the correlating load
 balancing resources are considered finished by service controller.
@@ -95,7 +96,7 @@ The lifecyle of a `LoadBalancer` type serivce with finalizer would look like:
   3. Provision of load balancing resources.
 - Deletion
   1. User issues a deletion for the service.
-  2. Service resoure deletion is blocked due to the finalizer.
+  2. Service resource deletion is blocked due to the finalizer.
   3. Service controller observed the deletion timestamp is added.
   4. Cleanup of load balancing resources.
   5. Service controller removes finalizer from the service.
@@ -123,12 +124,33 @@ later release we will remove the feature gate and enable both the "remove" and
 "add" logic by default.
 
 As such, we are proposing Alpha/Beta/GA phase for this enhancement as below:
-- Alpha: The finailizer cleanup logic would be on by default while the finalizer
-addition logic will be put behind a feature gate that is off by default.
-- Beta: Both the cleanup and addition logic will be default on, but allowing
-disabling via feature gate.
+- Alpha: Finalizer cleanup will always be on. Finalizer addition will be off by
+default but can be enabled via a feature gate.
+- Beta: Finalizer cleanup will always be on. Finalizer addition will be on by
+default but can be disabled via a feature gate.
 - GA: Service LoadBalancers Finalizer Protection will always be on.
 
+### Risks and Mitigations
+
+#### n+2 upgrade/downgrade is not supported
+
+If user does n+2 upgrade from v1.14 -> v1.16 and then does a downgrade back to v1.14.
+They would have added finalizers to the Service but then lose the removal logic on
+the downgrade. And hence Service with `type=LoadBalancer` can't be deleted until the
+finalizer on it is manually removed.
+
+To keep the upgrade/downgrade safe a user would always do n+1 upgrade/downgrade as
+stated on https://kubernetes.io/docs/setup/version-skew-policy/#supported-component-upgrade-order.
+
+### Other notes
+
+If the cloud provider opts-out of [LoadBalancer](https://github.com/kubernetes/cloud-provider/blob/402566916174f020983cb0bd467daeae6206ae02/cloud.go#L48-L49)
+support, service controller won't be run at all (see [here](https://github.com/kubernetes/kubernetes/blob/3e52ea8081abc13398de6283c31056cd6aecf6b4/pkg/controller/service/service_controller.go#L229-L232)).
+Hence finalizer won't be added/removed by service controller.
+
+If any other custom controller that watches Service with `type=LoadBalancer`, it
+should implement its own finalizer protection.
+
 ### Test Plan
 
 We will implement e2e test cases to ensure: