From 1468fa0306e611e16d2514cc70565b25bed67b09 Mon Sep 17 00:00:00 2001 From: AiRanthem Date: Thu, 12 Jun 2025 20:19:28 +0800 Subject: [PATCH] Fix: Webhook will take over Deployment even Rollout CR doesn't exist Signed-off-by: AiRanthem --- pkg/controller/rollout/rollout_canary.go | 2 +- pkg/controller/rollout/rollout_controller.go | 9 ------ .../mutating/workload_update_handler.go | 14 +++++---- .../mutating/workload_update_handler_test.go | 31 +++++++++++++++++++ 4 files changed, 40 insertions(+), 16 deletions(-) diff --git a/pkg/controller/rollout/rollout_canary.go b/pkg/controller/rollout/rollout_canary.go index dfef594..525222a 100644 --- a/pkg/controller/rollout/rollout_canary.go +++ b/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: diff --git a/pkg/controller/rollout/rollout_controller.go b/pkg/controller/rollout/rollout_controller.go index cccc82a..8232ba0 100755 --- a/pkg/controller/rollout/rollout_controller.go +++ b/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{} diff --git a/pkg/webhook/workload/mutating/workload_update_handler.go b/pkg/webhook/workload/mutating/workload_update_handler.go index e337616..90c9d15 100644 --- a/pkg/webhook/workload/mutating/workload_update_handler.go +++ b/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) diff --git a/pkg/webhook/workload/mutating/workload_update_handler_test.go b/pkg/webhook/workload/mutating/workload_update_handler_test.go index 13282de..17aaafb 100644 --- a/pkg/webhook/workload/mutating/workload_update_handler_test.go +++ b/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) {