Fix: Webhook will take over Deployment even Rollout CR doesn't exist (#278)

Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>

pkg/controller/rollout/rollout_canary.go

@@ -367,7 +367,7 @@ func (m *canaryReleaseManager) doCanaryFinalising(c *RolloutContext) (bool, erro
 	klog.Infof("rollout(%s/%s) Finalising Step is %s", c.Rollout.Namespace, c.Rollout.Name, canaryStatus.FinalisingStep)
 
 	var retry bool
-	// the order of steps is maitained by calculating the nextStep
+	// the order of steps is maintained by calculating the nextStep
 	switch canaryStatus.FinalisingStep {
 	// set workload.pause=false; set workload.partition=0
 	case v1beta1.FinalisingStepResumeWorkload:

pkg/controller/rollout/rollout_controller.go

@@ -86,15 +86,6 @@ type RolloutReconciler struct {
 // +kubebuilder:rbac:groups=apps.kruise.io,resources=daemonsets/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=networking.istio.io,resources=virtualservices;destinationrules,verbs=get;list;watch;update;patch
 
-// Reconcile is part of the main kubernetes reconciliation loop which aims to
-// move the current state of the cluster closer to the desired state.
-// TODO(user): Modify the Reconcile function to compare the state specified by
-// the Rollout object against the actual cluster state, and then
-// perform operations to make the cluster state reflect the state specified by
-// the user.
-//
-// For more details, check Reconcile and its Result here:
-// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.8.3/pkg/reconcile
 func (r *RolloutReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	// Fetch the Rollout instance
 	rollout := &v1beta1.Rollout{}

pkg/webhook/workload/mutating/workload_update_handler.go

@@ -193,6 +193,14 @@ func (h *WorkloadHandler) Handle(ctx context.Context, req admission.Request) adm
 }
 
 func (h *WorkloadHandler) handleDeployment(newObj, oldObj *apps.Deployment) (bool, error) {
+	// make sure matched Rollout CR always exists
+	rollout, err := h.fetchMatchedRollout(newObj)
+	if err != nil {
+		return false, err
+	} else if rollout == nil || rollout.Spec.Strategy.IsEmptyRelease() {
+		return false, nil
+	}
+
 	// in rollout progressing
 	if newObj.Annotations[util.InRolloutProgressingAnnotation] != "" {
 		modified := false
@@ -257,12 +265,6 @@ func (h *WorkloadHandler) handleDeployment(newObj, oldObj *apps.Deployment) (boo
 		return false, nil
 	}
 
-	rollout, err := h.fetchMatchedRollout(newObj)
-	if err != nil {
-		return false, err
-	} else if rollout == nil || rollout.Spec.Strategy.IsEmptyRelease() {
-		return false, nil
-	}
 	rss, err := h.Finder.GetReplicaSetsForDeployment(newObj)
 	if err != nil || len(rss) == 0 {
 		klog.Warningf("Cannot find any activate replicaset for deployment %s/%s, no need to rolling", newObj.Namespace, newObj.Name)

pkg/webhook/workload/mutating/workload_update_handler_test.go

@@ -395,6 +395,37 @@ func TestHandlerDeployment(t *testing.T) {
 				return obj
 			},
 		},
+		{
+			name: "deployment image v1->v2, no matched rollout, but has in-progress annotation",
+			getObjs: func() (*apps.Deployment, *apps.Deployment) {
+				oldObj := deploymentDemo.DeepCopy()
+				newObj := deploymentDemo.DeepCopy()
+				newObj.Spec.Template.Spec.Containers[0].Image = "echoserver:v2"
+				newObj.Annotations[util.InRolloutProgressingAnnotation] = "{\"rolloutName\":\"rollouts-demo\"}"
+				newObj.Annotations[appsv1alpha1.DeploymentStrategyAnnotation] = "{\"rollingStyle\":\"Partition\",\"rollingUpdate\":{\"maxUnavailable\":\"25%\",\"maxSurge\":\"25%\"},\"paused\":true,\"partition\":1}"
+				return oldObj, newObj
+			},
+			expectObj: func() *apps.Deployment {
+				obj := deploymentDemo.DeepCopy()
+				obj.Spec.Template.Spec.Containers[0].Image = "echoserver:v2"
+				obj.Annotations[util.InRolloutProgressingAnnotation] = "{\"rolloutName\":\"rollouts-demo\"}"
+				obj.Annotations[appsv1alpha1.DeploymentStrategyAnnotation] = "{\"rollingStyle\":\"Partition\",\"rollingUpdate\":{\"maxUnavailable\":\"25%\",\"maxSurge\":\"25%\"},\"paused\":true,\"partition\":1}"
+				return obj
+			},
+			getRs: func() []*apps.ReplicaSet {
+				rs := rsDemo.DeepCopy()
+				return []*apps.ReplicaSet{rs}
+			},
+			getRollout: func() *appsv1beta1.Rollout {
+				obj := rolloutDemo.DeepCopy()
+				obj.Spec.WorkloadRef = appsv1beta1.ObjectRef{
+					APIVersion: "apps/v1",
+					Kind:       "Deployment",
+					Name:       "other",
+				}
+				return obj
+			},
+		},
 		{
 			name: "deployment image v2->v3, matched rollout, but multiple rss",
 			getObjs: func() (*apps.Deployment, *apps.Deployment) {