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

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

(cherry picked from commit deaa5f38b5)
Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>

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

(cherry picked from commit deaa5f38b5)
Signed-off-by: AiRanthem <zhongtianyun.zty@alibaba-inc.com>

---------

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

.github/workflows/ci.yaml

@@ -16,7 +16,7 @@ env:
 jobs:
 
   golangci-lint:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - name: Checkout
         uses: actions/checkout@v2
@@ -27,7 +27,7 @@ jobs:
         with:
           go-version: ${{ env.GO_VERSION }}
       - name: Cache Go Dependencies
-        uses: actions/cache@v2
+        uses: actions/cache@v4
         with:
           path: ~/go/pkg/mod
           key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
@@ -42,7 +42,7 @@ jobs:
           args: --verbose
 
   unit-tests:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:
@@ -54,7 +54,7 @@ jobs:
         with:
           go-version: ${{ env.GO_VERSION }}
       - name: Cache Go Dependencies
-        uses: actions/cache@v2
+        uses: actions/cache@v4
         with:
           path: ~/go/pkg/mod
           key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}

.github/workflows/docker-image.yaml

@@ -0,0 +1,28 @@
+name: Docker Image CI
+
+on:
+  workflow_dispatch:
+
+# Declare default permissions as read only.
+permissions: read-all
+
+jobs:
+
+  build:
+
+    runs-on: ubuntu-latest
+
+    steps:
+      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+      - name: Set up Docker Buildx
+        uses: docker/setup-buildx-action@v3
+      - name: Login to Docker Hub
+        uses: docker/login-action@v3
+        with:
+          username: ${{ vars.DOCKERHUB_USERNAME }}
+          password: ${{ secrets.HUB_KRUISE }}
+      - name: Build the Docker image
+        run: |
+          docker buildx create --use --platform=linux/amd64,linux/arm64,linux/ppc64le --name multi-platform-builder
+          docker buildx ls
+          IMG=openkruise/kruise-rollout:${{ github.ref_name }} make docker-multiarch

.github/workflows/e2e-advanced-deployment-1.19.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

.github/workflows/e2e-advanced-deployment-1.23.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

.github/workflows/e2e-cloneset-1.19.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

.github/workflows/e2e-cloneset-1.23.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

.github/workflows/e2e-daemonset-1.19.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

.github/workflows/e2e-daemonset-1.23.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

.github/workflows/e2e-deployment-1.19.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

.github/workflows/e2e-deployment-1.23.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

.github/workflows/e2e-gateway.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

.github/workflows/e2e-others-1.19.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

.github/workflows/e2e-others-1.23.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

.github/workflows/e2e-statefulset-1.19.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

.github/workflows/e2e-statefulset-1.23.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
 
   rollout:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
         with:

Dockerfile_multiarch

@@ -1,7 +1,7 @@
 # Build the manager binary
 ARG BASE_IMAGE=alpine
 ARG BASE_IMAGE_VERION=3.17
-FROM --platform=$BUILDPLATFORM golang:1.18-alpine3.17 as builder
+FROM --platform=$BUILDPLATFORM golang:1.19-alpine3.17 as builder
 
 WORKDIR /workspace
 
@@ -23,12 +23,25 @@ ARG BASE_IMAGE
 ARG BASE_IMAGE_VERION
 FROM ${BASE_IMAGE}:${BASE_IMAGE_VERION}
 
-RUN apk add --no-cache ca-certificates=~20220614-r4 bash=~5.2.15-r0 expat=~2.5.0-r0 \
-    && rm -rf /var/cache/apk/*
+RUN set -eux; \
+    apk --no-cache --update upgrade && \
+    apk --no-cache add ca-certificates && \
+    apk --no-cache add tzdata && \
+    rm -rf /var/cache/apk/* && \
+    update-ca-certificates && \
+    echo "only include root and nobody user" && \
+    echo -e "root:x:0:0:root:/root:/bin/ash\nnobody:x:65534:65534:nobody:/:/sbin/nologin" | tee /etc/passwd && \
+    echo -e "root:x:0:root\nnobody:x:65534:" | tee /etc/group && \
+    rm -rf /usr/local/sbin/* && \
+    rm -rf /usr/local/bin/* && \
+    rm -rf /usr/sbin/* && \
+    rm -rf /usr/bin/* && \
+    rm -rf /sbin/* && \
+    rm -rf /bin/*
 
 WORKDIR /
 COPY --from=builder /workspace/manager .
 COPY lua_configuration /lua_configuration
-USER 1000
+USER 65534
 
 ENTRYPOINT ["/manager"]

pkg/controller/deployment/deployment_controller.go

@@ -81,7 +81,22 @@ func (dc *DeploymentController) getReplicaSetsForDeployment(ctx context.Context,
 	}
 	// List all ReplicaSets to find those we own but that no longer match our
 	// selector. They will be orphaned by ClaimReplicaSets().
-	return dc.rsLister.ReplicaSets(d.Namespace).List(deploymentSelector)
+	allRSs, err := dc.rsLister.ReplicaSets(d.Namespace).List(deploymentSelector)
+	if err != nil {
+		return nil, fmt.Errorf("list %s/%s rs failed:%v", d.Namespace, d.Name, err)
+	}
+	// select rs owner by current deployment
+	ownedRSs := make([]*apps.ReplicaSet, 0)
+	for _, rs := range allRSs {
+		if !rs.DeletionTimestamp.IsZero() {
+			continue
+		}
+
+		if metav1.IsControlledBy(rs, d) {
+			ownedRSs = append(ownedRSs, rs)
+		}
+	}
+	return ownedRSs, nil
 }
 
 // syncDeployment will sync the deployment with the given key.

pkg/controller/deployment/sync.go

@@ -302,6 +302,30 @@ func (dc *DeploymentController) scale(ctx context.Context, deployment *apps.Depl
 		// replica sets.
 		deploymentReplicasToAdd := allowedSize - allRSsReplicas
 
+		// Scale down the unhealthy replicas in old replica sets firstly to avoid some bad cases.
+		// For example:
+		//       _______________________________________________________________________________________
+		//       | ReplicaSet    |       oldRS-1      |   oldRS-2            |          newRS           |
+		//       | --------------| -------------------|----------------------|--------------------------|
+		//       | Replicas      |  4 healthy Pods    |  2 unhealthy Pods    |    4 unhealthy Pods      |
+		//       ---------------------------------------------------------------------------------------
+		// If we want to scale down these replica sets from 10 to 6, we expect to scale down the oldRS-2
+		// from 2 to 0 firstly, then scale down oldRS-1 1 Pod and newRS 1 Pod based on proportion.
+		//
+		// We do not scale down the newRS unhealthy Pods with higher priority, because these new revision
+		// Pods may be just created, not the one with the crash or other problems.
+		var err error
+		var cleanupCount int32
+		if deploymentReplicasToAdd < 0 {
+			oldRSs, cleanupCount, err = dc.cleanupUnhealthyReplicas(ctx, oldRSs, deployment, -deploymentReplicasToAdd)
+			if err != nil {
+				return err
+			}
+			klog.V(4).Infof("Cleaned up unhealthy replicas from old RSes by %d during scaling", cleanupCount)
+			deploymentReplicasToAdd += cleanupCount
+			allRSs = deploymentutil.FilterActiveReplicaSets(append(oldRSs, newRS))
+		}
+
 		// The additional replicas should be distributed proportionally amongst the active
 		// replica sets from the larger to the smaller in size replica set. Scaling direction
 		// drives what happens in case we are trying to scale replica sets of the same size.

pkg/controller/rollout/rollout_controller.go

@@ -84,15 +84,6 @@ type RolloutReconciler struct {
 // +kubebuilder:rbac:groups=apps.kruise.io,resources=daemonsets,verbs=get;list;watch;update;patch
 // +kubebuilder:rbac:groups=apps.kruise.io,resources=daemonsets/status,verbs=get;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 := &v1alpha1.Rollout{}

pkg/trafficrouting/network/gateway/gateway.go

@@ -19,6 +19,7 @@ import (
 
 	rolloutv1alpha1 "github.com/openkruise/rollouts/api/v1alpha1"
 	"github.com/openkruise/rollouts/pkg/trafficrouting/network"
+	"github.com/openkruise/rollouts/pkg/util"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/util/retry"
@@ -83,7 +84,7 @@ func (r *gatewayController) EnsureRoutes(ctx context.Context, strategy *rolloutv
 		klog.Errorf("update %s httpRoute(%s) failed: %s", r.conf.Key, httpRoute.Name, err.Error())
 		return false, err
 	}
-	klog.Infof("%s set HTTPRoute(name:%s weight:%d) success", r.conf.Key, *r.conf.TrafficConf.HTTPRouteName, *weight)
+	klog.Infof("%s set HTTPRoute(%s) desiredRule(%s) success", r.conf.Key, *r.conf.TrafficConf.HTTPRouteName, util.DumpJSON(desiredRule))
 	return false, nil
 }
 

pkg/webhook/workload/mutating/workload_update_handler.go

@@ -99,48 +99,58 @@ func (h *WorkloadHandler) Handle(ctx context.Context, req admission.Request) adm
 			if err := h.Decoder.Decode(req, newObj); err != nil {
 				return admission.Errored(http.StatusBadRequest, err)
 			}
+			newObjClone := newObj.DeepCopy()
 			oldObj := &kruiseappsv1alpha1.CloneSet{}
 			if err := h.Decoder.Decode(
 				admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Object: req.AdmissionRequest.OldObject}},
 				oldObj); err != nil {
 				return admission.Errored(http.StatusBadRequest, err)
 			}
-			changed, err := h.handleCloneSet(newObj, oldObj)
+			changed, err := h.handleCloneSet(newObjClone, oldObj)
 			if err != nil {
 				return admission.Errored(http.StatusBadRequest, err)
 			}
 			if !changed {
 				return admission.Allowed("")
 			}
-			marshalled, err := json.Marshal(newObj)
+			marshalled, err := json.Marshal(newObjClone)
 			if err != nil {
 				return admission.Errored(http.StatusInternalServerError, err)
 			}
-			return admission.PatchResponseFromRaw(req.AdmissionRequest.Object.Raw, marshalled)
+			original, err := json.Marshal(newObj)
+			if err != nil {
+				return admission.Errored(http.StatusInternalServerError, err)
+			}
+			return admission.PatchResponseFromRaw(original, marshalled)
 		case util.ControllerKruiseKindDS.Kind:
 			// check daemonset
 			newObj := &kruiseappsv1alpha1.DaemonSet{}
 			if err := h.Decoder.Decode(req, newObj); err != nil {
 				return admission.Errored(http.StatusBadRequest, err)
 			}
+			newObjClone := newObj.DeepCopy()
 			oldObj := &kruiseappsv1alpha1.DaemonSet{}
 			if err := h.Decoder.Decode(
 				admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Object: req.AdmissionRequest.OldObject}},
 				oldObj); err != nil {
 				return admission.Errored(http.StatusBadRequest, err)
 			}
-			changed, err := h.handleDaemonSet(newObj, oldObj)
+			changed, err := h.handleDaemonSet(newObjClone, oldObj)
 			if err != nil {
 				return admission.Errored(http.StatusBadRequest, err)
 			}
 			if !changed {
 				return admission.Allowed("")
 			}
-			marshalled, err := json.Marshal(newObj)
+			marshalled, err := json.Marshal(newObjClone)
 			if err != nil {
 				return admission.Errored(http.StatusInternalServerError, err)
 			}
-			return admission.PatchResponseFromRaw(req.AdmissionRequest.Object.Raw, marshalled)
+			original, err := json.Marshal(newObj)
+			if err != nil {
+				return admission.Errored(http.StatusInternalServerError, err)
+			}
+			return admission.PatchResponseFromRaw(original, marshalled)
 		}
 
 	// native k8s deloyment
@@ -152,24 +162,29 @@ func (h *WorkloadHandler) Handle(ctx context.Context, req admission.Request) adm
 			if err := h.Decoder.Decode(req, newObj); err != nil {
 				return admission.Errored(http.StatusBadRequest, err)
 			}
+			newObjClone := newObj.DeepCopy()
 			oldObj := &apps.Deployment{}
 			if err := h.Decoder.Decode(
 				admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{Object: req.AdmissionRequest.OldObject}},
 				oldObj); err != nil {
 				return admission.Errored(http.StatusBadRequest, err)
 			}
-			changed, err := h.handleDeployment(newObj, oldObj)
+			changed, err := h.handleDeployment(newObjClone, oldObj)
 			if err != nil {
 				return admission.Errored(http.StatusBadRequest, err)
 			}
 			if !changed {
 				return admission.Allowed("")
 			}
-			marshalled, err := json.Marshal(newObj)
+			marshalled, err := json.Marshal(newObjClone)
 			if err != nil {
 				return admission.Errored(http.StatusInternalServerError, err)
 			}
-			return admission.PatchResponseFromRaw(req.AdmissionRequest.Object.Raw, marshalled)
+			original, err := json.Marshal(newObj)
+			if err != nil {
+				return admission.Errored(http.StatusInternalServerError, err)
+			}
+			return admission.PatchResponseFromRaw(original, marshalled)
 		}
 	}
 
@@ -244,6 +259,14 @@ func (h *WorkloadHandler) handleStatefulSetLikeWorkload(newObj, oldObj *unstruct
 }
 
 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.Canary == nil {
+		return false, nil
+	}
+
 	// in rollout progressing
 	if newObj.Annotations[util.InRolloutProgressingAnnotation] != "" {
 		modified := false
@@ -290,12 +313,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.Canary == nil {
-		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

@@ -352,6 +352,39 @@ 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() *appsv1alpha1.Rollout {
+				obj := rolloutDemo.DeepCopy()
+				obj.Spec.ObjectRef = appsv1alpha1.ObjectRef{
+					WorkloadRef: &appsv1alpha1.WorkloadRef{
+						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) {

test/e2e/deployment_test.go

@@ -44,7 +44,7 @@ var _ = SIGDescribe("Advanced Deployment", func() {
 		return k8sClient.Get(context.TODO(), key, object)
 	}
 
-	UpdateDeployment := func(deployment *apps.Deployment, version string) *apps.Deployment {
+	UpdateDeployment := func(deployment *apps.Deployment, version string, images ...string) *apps.Deployment {
 		By(fmt.Sprintf("update deployment %v to version: %v", client.ObjectKeyFromObject(deployment), version))
 		var clone *apps.Deployment
 		Expect(retry.RetryOnConflict(defaultRetry, func() error {
@@ -53,7 +53,11 @@ var _ = SIGDescribe("Advanced Deployment", func() {
 			if err != nil {
 				return err
 			}
-			clone.Spec.Template.Spec.Containers[0].Image = deployment.Spec.Template.Spec.Containers[0].Image
+			if len(images) == 0 {
+				clone.Spec.Template.Spec.Containers[0].Image = deployment.Spec.Template.Spec.Containers[0].Image
+			} else {
+				clone.Spec.Template.Spec.Containers[0].Image = images[0]
+			}
 			clone.Spec.Template.Spec.Containers[0].Env[0].Value = version
 			strategy := unmarshal(clone.Annotations[rolloutsv1alpha1.DeploymentStrategyAnnotation])
 			strategy.Paused = true
@@ -324,6 +328,19 @@ var _ = SIGDescribe("Advanced Deployment", func() {
 			UpdatePartitionWithCheck(deployment, intstr.FromInt(3))
 			UpdatePartitionWithCheck(deployment, intstr.FromInt(5))
 		})
+
+		It("scale down old unhealthy first", func() {
+			deployment := &apps.Deployment{}
+			deployment.Namespace = namespace
+			Expect(ReadYamlToObject("./test_data/deployment/deployment.yaml", deployment)).ToNot(HaveOccurred())
+			deployment.Annotations["rollouts.kruise.io/deployment-strategy"] = `{"rollingUpdate":{"maxUnavailable":0,"maxSurge":1}}`
+			CreateObject(deployment)
+			UpdateDeployment(deployment, "version2", "busybox:not-exists")
+			UpdatePartitionWithoutCheck(deployment, intstr.FromInt(1))
+			CheckReplicas(deployment, 6, 5, 1)
+			UpdateDeployment(deployment, "version3", "busybox:1.32")
+			CheckReplicas(deployment, 5, 5, 0)
+		})
 	})
 })
 

test/e2e/rollout_test.go

@@ -131,6 +131,7 @@ var _ = SIGDescribe("Rollout", func() {
 			}
 			// daemon.Spec.Replicas = utilpointer.Int32(*object.Spec.Replicas)
 			daemon.Spec.Template = *object.Spec.Template.DeepCopy()
+			daemon.Spec.UpdateStrategy = *object.Spec.UpdateStrategy.DeepCopy()
 			daemon.Labels = mergeMap(daemon.Labels, object.Labels)
 			daemon.Annotations = mergeMap(daemon.Annotations, object.Annotations)
 			return k8sClient.Update(context.TODO(), daemon)