From a7a9cf956072e016924fd9742077a659b36c3002 Mon Sep 17 00:00:00 2001 From: Gidi233 Date: Wed, 11 Sep 2024 10:59:26 +0800 Subject: [PATCH] fix Signed-off-by: Gidi233 --- .../application/rollout_helper.go | 98 ++++++++++++------- .../application/rollout_helper_test.go | 15 ++- 2 files changed, 70 insertions(+), 43 deletions(-) diff --git a/pkg/fleet-manager/application/rollout_helper.go b/pkg/fleet-manager/application/rollout_helper.go index a322d2da..ec197dfb 100644 --- a/pkg/fleet-manager/application/rollout_helper.go +++ b/pkg/fleet-manager/application/rollout_helper.go @@ -148,8 +148,7 @@ func (a *ApplicationManager) syncRolloutPolicyForCluster(ctx context.Context, } result, err := controllerutil.CreateOrUpdate(ctx, fleetClusterClient, ingress, func() error { ingress.SetAnnotations(annotation) - renderIngress(ingress, rolloutPolicy) - return nil + return renderNginxIngress(ingress, rolloutPolicy) }) if err != nil { return ctrl.Result{}, errors.Wrapf(err, "failed to operate ingress") @@ -357,11 +356,16 @@ func enableSidecarInjection(ctx context.Context, kubeClient client.Client, names var newNs client.Object switch provider { case fleetapi.Kuma: - newNs = addAnnotations(ns, map[string]string{ + newNs, err = addAnnotations(ns, map[string]string{ kumaInject: "enabled", }) + if err != nil { + return err + } case fleetapi.Istio: - newNs = addLabels(ns, istioInject, "enabled") + if newNs, err = addLabels(ns, map[string]string{istioInject: "enabled"}); err != nil { + return err + } } if updateErr := kubeClient.Update(ctx, newNs); updateErr != nil { return errors.Wrapf(updateErr, "failed to update namespace %s", namespacedName.Namespace) @@ -462,21 +466,13 @@ func deleteIngressCreatedByKurator(ctx context.Context, kubeClient client.Client labels := ingress.GetLabels() if set := sets.New(strings.Split(labels[ingressLabelKey], ",")...); set.Contains(rollout.ServiceName) { if set.Len() == 1 { - if deleteErr := kubeClient.Delete(ctx, ingress); deleteErr != nil && !apierrors.IsNotFound(deleteErr) { + if deleteErr := kubeClient.Delete(ctx, ingress); deleteErr != nil { return errors.Wrapf(deleteErr, "failed to Delete ingress %s in %s", namespaceName.Name, namespaceName.Namespace) } } else { - newRules := make([]ingressv1.IngressRule, 0) - for _, rule := range ingress.Spec.Rules { - if rule.Host != rollout.RolloutPolicy.TrafficRouting.Host { - newRules = append(newRules, rule) - } - } - ingress.Spec.Rules = newRules - labels[ingressLabelKey] = strings.Join(set.Delete(rollout.ServiceName).UnsortedList(), ",") - ingress.SetLabels(labels) - if err := kubeClient.Update(ctx, ingress); err != nil { - return errors.Wrapf(err, "failed to Update ingress %s in %s", namespaceName.Name, namespaceName.Namespace) + // There are still other canaries using this ingress + if err := updateIngressRulesAndLabels(ctx, kubeClient, ingress, rollout, namespaceName, set); err != nil { + return err } } } @@ -485,17 +481,38 @@ func deleteIngressCreatedByKurator(ctx context.Context, kubeClient client.Client return nil } +func updateIngressRulesAndLabels(ctx context.Context, kubeClient client.Client, ingress *ingressv1.Ingress, rollout *applicationapi.RolloutConfig, namespaceName types.NamespacedName, set sets.Set[string]) error { + newRules := make([]ingressv1.IngressRule, 0) + for _, rule := range ingress.Spec.Rules { + if rule.Host != rollout.RolloutPolicy.TrafficRouting.Host { + newRules = append(newRules, rule) + } + } + ingress.Spec.Rules = newRules + + labels := ingress.GetLabels() + labels[ingressLabelKey] = strings.Join(set.Delete(rollout.ServiceName).UnsortedList(), ",") + ingress.SetLabels(labels) + + if err := kubeClient.Update(ctx, ingress); err != nil { + return errors.Wrapf(err, "failed to Update ingress %s in %s", namespaceName.Name, namespaceName.Namespace) + } + return nil +} + // create/update ingress configuration -func renderIngress(ingress *ingressv1.Ingress, rollout *applicationapi.RolloutConfig) { +func renderNginxIngress(ingress *ingressv1.Ingress, rollout *applicationapi.RolloutConfig) error { if labels := ingress.GetLabels(); labels == nil || labels[ingressLabelKey] == "" { ingress.SetLabels(map[string]string{ingressLabelKey: rollout.ServiceName}) } else { labels[ingressLabelKey] = strings.Join(sets.New(strings.Split(labels[ingressLabelKey], ",")...).Insert(rollout.ServiceName).UnsortedList(), ",") ingress.SetLabels(labels) } - addAnnotations(ingress, map[string]string{ + if _, err := addAnnotations(ingress, map[string]string{ ingressAnnotationKey: ingressAnnotationValue, - }) + }); err != nil { + return err + } Prefix := ingressv1.PathTypePrefix rule := ingressv1.IngressRule{ Host: rollout.RolloutPolicy.TrafficRouting.Host, @@ -517,6 +534,7 @@ func renderIngress(ingress *ingressv1.Ingress, rollout *applicationapi.RolloutCo }, } ingress.Spec.Rules = append(ingress.Spec.Rules, rule) + return nil } // create/update canary configuration @@ -691,31 +709,41 @@ func generateWebhookUrl(name, namespace string) string { return url } -func addLabels(obj client.Object, key, value string) client.Object { - labels := obj.GetLabels() - // prevent nil pointer panic - if labels == nil { - obj.SetLabels(map[string]string{ - key: value, - }) - return obj - } - labels[key] = value - obj.SetLabels(labels) - return obj +func addLabels(obj client.Object, labels map[string]string) (client.Object, error) { + existingLabels := obj.GetLabels() + if existingLabels == nil { + obj.SetLabels(labels) + } else { + for k, v := range labels { + if value, exists := existingLabels[k]; exists { + if value != v { + return nil, errors.New("label key conflict") + } + } else { + existingLabels[k] = v + } + } + obj.SetLabels(existingLabels) + } + return obj, nil } - -func addAnnotations(obj client.Object, ann map[string]string) client.Object { +func addAnnotations(obj client.Object, ann map[string]string) (client.Object, error) { annotations := obj.GetAnnotations() if annotations == nil { obj.SetAnnotations(ann) } else { for k, v := range ann { - annotations[k] = v + if value, exists := annotations[k]; exists { + if value != v { + return nil, errors.New("annotation key conflict") + } + } else { + annotations[k] = v + } } obj.SetAnnotations(annotations) } - return obj + return obj, nil } func mergeMap(map1, map2 map[string]*applicationapi.RolloutStatus) map[string]*applicationapi.RolloutStatus { diff --git a/pkg/fleet-manager/application/rollout_helper_test.go b/pkg/fleet-manager/application/rollout_helper_test.go index e24a078f..640a78a1 100644 --- a/pkg/fleet-manager/application/rollout_helper_test.go +++ b/pkg/fleet-manager/application/rollout_helper_test.go @@ -595,8 +595,7 @@ func Test_renderCanaryAnalysis(t *testing.T) { func Test_addLables(t *testing.T) { type args struct { obj client.Object - key string - value string + label map[string]string } tests := []struct { name string @@ -618,8 +617,9 @@ func Test_addLables(t *testing.T) { }, }, }, - key: "istio-injection", - value: "ebabled", + label: map[string]string{ + "istio-injection": "enabled", + }, }, want: &corev1.Namespace{ TypeMeta: metav1.TypeMeta{ @@ -630,7 +630,7 @@ func Test_addLables(t *testing.T) { Name: "webapp", Labels: map[string]string{ "xxx": "abc", - "istio-injection": "ebabled", + "istio-injection": "enabled", }, }, }, @@ -647,8 +647,7 @@ func Test_addLables(t *testing.T) { Name: "webapp", }, }, - key: "XXX", - value: "abc", + label: map[string]string{"XXX": "abc"}, }, want: &corev1.Namespace{ TypeMeta: metav1.TypeMeta{ @@ -666,7 +665,7 @@ func Test_addLables(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := addLabels(tt.args.obj, tt.args.key, tt.args.value); !reflect.DeepEqual(got, tt.want) { + if got, _ := addLabels(tt.args.obj, tt.args.label); !reflect.DeepEqual(got, tt.want) { t.Errorf("addLablesOrAnnotaions() = %v, want %v", got, tt.want) } })