From b7c63aa25c5c93acfd8f2cfda68666aca14597a6 Mon Sep 17 00:00:00 2001 From: Vladimir Videlov Date: Thu, 30 Jan 2025 11:31:14 +0200 Subject: [PATCH] Refactor proxy reset (#1253) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Initial * State * State * configuration test suite * State * State * State * State * State * State * State * One more test * More tests * remove stdoutlogger * State * Docs + RN * error on Kyma pod restart warnings * Do not return error on warnings also do not log it * Update internal/restarter/predicates/image_resources_test.go Co-authored-by: Marek Kołodziejczak <69915024+kolodziejczak@users.noreply.github.com> * Review adjustments * Fix mustMatch and optional predicate case * Improve logs on controller reconcile terminating for different cases * Improve logs and requeue in a minuate for the customer sidecar restart error case (warning) * Update docs/contributor/04-10-technical-design.md Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com> * Update docs/contributor/04-10-technical-design.md Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com> * Update docs/contributor/04-10-technical-design.md Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com> * Update docs/release-notes/1.14.0.md Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com> --------- Co-authored-by: Marek Kołodziejczak <69915024+kolodziejczak@users.noreply.github.com> Co-authored-by: Natalia Sitko <80401180+nataliasitko@users.noreply.github.com> --- controllers/istio_controller.go | 29 +- controllers/istio_controller_test.go | 7 +- docs/contributor/04-10-technical-design.md | 9 +- docs/release-notes/1.14.0.md | 3 + .../{ => configuration}/configuration.go | 5 +- .../istio/configuration/configuration_test.go | 88 +++ .../istio/configuration_test.go | 38 -- internal/reconciliations/istio/install.go | 6 +- internal/reconciliations/istio/uninstall.go | 4 +- internal/restarter/ingress_gateway.go | 9 +- internal/restarter/ingress_gateway_test.go | 10 +- .../predicates/compatibility.go} | 18 +- .../predicates/compatibility_test.go} | 32 +- .../restarter/predicates/customer_workload.go | 20 + .../predicates/customer_workload_test.go | 43 ++ .../restarter/predicates/image_resources.go | 40 +- .../predicates/image_resources_test.go | 103 ++++ .../restarter/predicates}/ingressgateway.go | 12 +- .../predicates}/ingressgateway_test.go | 32 +- .../restarter/predicates/kyma_workload.go | 20 + .../predicates/kyma_workload_test.go | 43 ++ .../predicates}/predicate.go | 5 +- .../predicates}/suite_test.go | 9 +- internal/restarter/sidecars.go | 63 +- internal/restarter/sidecars_test.go | 169 +++--- pkg/lib/sidecars/pods/filter_test.go | 66 -- pkg/lib/sidecars/pods/get.go | 153 ++--- pkg/lib/sidecars/pods/get_test.go | 81 ++- pkg/lib/sidecars/proxy.go | 131 +++- pkg/lib/sidecars/proxy_test.go | 571 ++++++++++++++++++ pkg/lib/sidecars/remove/remove.go | 9 +- pkg/lib/sidecars/remove/remove_test.go | 6 +- .../sidecars/restart/replica_set_action.go | 9 +- pkg/lib/sidecars/restart/restart.go | 35 +- pkg/lib/sidecars/restart/restart_factory.go | 1 + pkg/lib/sidecars/restart/restart_test.go | 171 ++++-- pkg/lib/sidecars/restart/rollout_action.go | 6 +- pkg/lib/sidecars/test/cases.go | 42 +- pkg/lib/sidecars/test/helpers/helpers.go | 31 + pkg/lib/sidecars/test/pod_states.go | 6 +- 40 files changed, 1623 insertions(+), 512 deletions(-) create mode 100644 docs/release-notes/1.14.0.md rename internal/reconciliations/istio/{ => configuration}/configuration.go (97%) create mode 100644 internal/reconciliations/istio/configuration/configuration_test.go delete mode 100644 internal/reconciliations/istio/configuration_test.go rename internal/{compatibility/proxy_restart.go => restarter/predicates/compatibility.go} (62%) rename internal/{compatibility/proxy_restart_test.go => restarter/predicates/compatibility_test.go} (77%) create mode 100644 internal/restarter/predicates/customer_workload.go create mode 100644 internal/restarter/predicates/customer_workload_test.go rename pkg/lib/sidecars/pods/filter.go => internal/restarter/predicates/image_resources.go (71%) create mode 100644 internal/restarter/predicates/image_resources_test.go rename {pkg/lib/ingressgateway => internal/restarter/predicates}/ingressgateway.go (77%) rename {pkg/lib/ingressgateway => internal/restarter/predicates}/ingressgateway_test.go (73%) create mode 100644 internal/restarter/predicates/kyma_workload.go create mode 100644 internal/restarter/predicates/kyma_workload_test.go rename internal/{filter => restarter/predicates}/predicate.go (88%) rename internal/{compatibility => restarter/predicates}/suite_test.go (69%) delete mode 100644 pkg/lib/sidecars/pods/filter_test.go create mode 100644 pkg/lib/sidecars/proxy_test.go diff --git a/controllers/istio_controller.go b/controllers/istio_controller.go index 9036913a4..eb2230d79 100644 --- a/controllers/istio_controller.go +++ b/controllers/istio_controller.go @@ -22,12 +22,15 @@ import ( "time" "github.com/kyma-project/istio/operator/internal/restarter" + "github.com/kyma-project/istio/operator/internal/restarter/predicates" "github.com/kyma-project/istio/operator/internal/validation" - "github.com/kyma-project/istio/operator/internal/filter" "github.com/kyma-project/istio/operator/pkg/lib/sidecars" + "github.com/kyma-project/istio/operator/pkg/lib/sidecars/pods" + "github.com/kyma-project/istio/operator/pkg/lib/sidecars/restart" "github.com/kyma-project/istio/operator/internal/described_errors" + "github.com/kyma-project/istio/operator/internal/reconciliations/istio/configuration" "github.com/kyma-project/istio/operator/internal/reconciliations/istio_resources" "github.com/kyma-project/istio/operator/internal/status" "k8s.io/client-go/util/retry" @@ -56,9 +59,12 @@ func NewController(mgr manager.Manager, reconciliationInterval time.Duration) *I merger := istiooperator.NewDefaultIstioMerger() statusHandler := status.NewStatusHandler(mgr.GetClient()) + logger := mgr.GetLogger() + podsLister := pods.NewPods(mgr.GetClient(), &logger) + actionRestarter := restart.NewActionRestarter(mgr.GetClient(), &logger) restarters := []restarter.Restarter{ - restarter.NewIngressGatewayRestarter(mgr.GetClient(), []filter.IngressGatewayPredicate{}, statusHandler), - restarter.NewSidecarsRestarter(mgr.GetLogger(), mgr.GetClient(), &merger, sidecars.NewProxyResetter(), statusHandler), + restarter.NewIngressGatewayRestarter(mgr.GetClient(), []predicates.IngressGatewayPredicate{}, statusHandler), + restarter.NewSidecarsRestarter(mgr.GetLogger(), mgr.GetClient(), &merger, sidecars.NewProxyRestarter(mgr.GetClient(), podsLister, actionRestarter, &logger), statusHandler), } return &IstioReconciler{ @@ -153,10 +159,15 @@ func (r *IstioReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl if statusUpdateErr != nil { r.log.Error(statusUpdateErr, "Error during updating status to error") } + if err.Level() == described_errors.Warning { + r.log.Info("Reconcile requeued") + return ctrl.Result{RequeueAfter: time.Minute * 1}, nil + } + r.log.Info("Reconcile failed") return ctrl.Result{}, err } else if requeue { r.statusHandler.SetCondition(&istioCR, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonReconcileRequeued)) - return r.requeueReconciliationWithoutError(ctx, &istioCR) + return r.requeueReconciliationRestartNotFinished(ctx, &istioCR) } return r.finishReconcile(ctx, &istioCR, istioImageVersion.Tag()) @@ -171,17 +182,17 @@ func (r *IstioReconciler) requeueReconciliation(ctx context.Context, istioCR *op if statusUpdateErr != nil { r.log.Error(statusUpdateErr, "Error during updating status to error") } - r.log.Error(err, "Reconcile failed") + r.log.Info("Reconcile failed") return ctrl.Result{}, err } -func (r *IstioReconciler) requeueReconciliationWithoutError(ctx context.Context, istioCR *operatorv1alpha2.Istio) (ctrl.Result, error) { +func (r *IstioReconciler) requeueReconciliationRestartNotFinished(ctx context.Context, istioCR *operatorv1alpha2.Istio) (ctrl.Result, error) { statusUpdateErr := r.statusHandler.UpdateToProcessing(ctx, istioCR) if statusUpdateErr != nil { - r.log.Error(statusUpdateErr, "Error during updating status to error") + r.log.Error(statusUpdateErr, "Error during updating status to processing") } r.log.Info("Reconcile requeued") - return ctrl.Result{Requeue: true, RequeueAfter: time.Minute * 1}, nil + return ctrl.Result{RequeueAfter: time.Minute * 1}, nil } // terminateReconciliation stops the reconciliation and does not requeue the request. @@ -276,7 +287,7 @@ func (r *IstioReconciler) updateLastAppliedConfiguration(ctx context.Context, ob if err := r.Client.Get(ctx, objectKey, &lacIstioCR); err != nil { return err } - lastAppliedErr := istio.UpdateLastAppliedConfiguration(&lacIstioCR, istioTag) + lastAppliedErr := configuration.UpdateLastAppliedConfiguration(&lacIstioCR, istioTag) if lastAppliedErr != nil { return lastAppliedErr } diff --git a/controllers/istio_controller_test.go b/controllers/istio_controller_test.go index cc6e17be9..2919e8b54 100644 --- a/controllers/istio_controller_test.go +++ b/controllers/istio_controller_test.go @@ -941,8 +941,9 @@ var _ = Describe("Istio Controller", func() { result, err := sut.Reconcile(context.Background(), reconcile.Request{NamespacedName: types.NamespacedName{Namespace: testNamespace, Name: istioCrName}}) // then - Expect(err).To(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) Expect(result.Requeue).To(BeFalse()) + Expect(result.RequeueAfter).To(Equal(time.Minute * 1)) updatedIstioCR := operatorv1alpha2.Istio{} err = fakeClient.Get(context.Background(), client.ObjectKeyFromObject(istioCR), &updatedIstioCR) @@ -985,7 +986,7 @@ var _ = Describe("Istio Controller", func() { //then Expect(err).ToNot(HaveOccurred()) - Expect(reconcileResult.Requeue).To(BeTrue()) + Expect(reconcileResult.Requeue).To(BeFalse()) Expect(reconcileResult.RequeueAfter).To(Equal(time.Minute * 1)) Expect(ingressGatewayRestarter.RestartCalled()).To(BeTrue()) @@ -1043,7 +1044,7 @@ type shouldFailClient struct { func (p *shouldFailClient) List(ctx context.Context, list client.ObjectList, _ ...client.ListOption) error { if p.FailOnList { - return errors.New("intentionally failing client on list") + return errors.New("intentionally failing client on client.List") } return p.Client.List(ctx, list) } diff --git a/docs/contributor/04-10-technical-design.md b/docs/contributor/04-10-technical-design.md index a9b8f023b..b41896e67 100644 --- a/docs/contributor/04-10-technical-design.md +++ b/docs/contributor/04-10-technical-design.md @@ -114,10 +114,16 @@ The predicate initiates a restart of the sidecar and Ingress Gateway if the targ The SidecarsRestarter is responsible for keeping the proxy sidecars in the desired state. It restarts Pods that are in the `Running` state, are part of the service mesh, and have the annotation `sidecar.istio.io/status`. The Istio CR and the [Istio version](#istio-version) represent the desired state. Pods are restarted in chunks with limits on the number that can be restarted in one reconciliation and the number that can be listed when requesting from the Kubernetes API Server. If the number of Pods that must be restarted exceeds the limits, it happens in the next reconciliation. In such a case, the reconciliation request is requeued with a 1-minute delay to allow time for the Kubernetes scheduler to restart the Deployments. -During the proxy sidecars restarting phase, the Istio CR remains in the `Processing` state having the following status conditions: +Restarting sidecars is divided into two phases: +- In the first phase, only Kyma workloads are restarted. A workload is considered a Kyma workload if it runs in the `kyma-system` namespace or has the `kyma-project.io/module` annotation. All Kyma workloads are restarted without pagination. If there is a problem with the restart, Istio CR is set to the `Error`, and the reconciliation is requeued. +- In the second phase, only customer workloads are restarted. A workload is considered a customer workload if it does not run in the `kyma-system` namespace and does not have the `kyma-project.io/module` annotation. All customer workloads are restarted with pagination. If there is a problem with the restart, Istio CR is set to the `Warning` state, and the reconciliation is requeued with a 1-minute delay. + +During the customer sidecars restarting phase, the Istio CR remains in the `Processing` state, having the following status conditions: - The `Ready` condition is set to `false` with the reason `ReconcileRequeued`. - The `ProxySidecarRestartSucceeded` condition is set to `false` with the reason `ProxySidecarPartiallySucceeded`. +After completing the customer sidecars restart, Istio CR's `Ready` condition is set to `true`, and `ProxySidecarRestartSucceeded` is set to `true` with the reason `ProxySidecarRestartSucceeded`. + This component covers the following restart triggers: - Restart Pods with proxy sidecar when CNI config changes. @@ -128,4 +134,3 @@ This component covers the following restart triggers: ### IngressGatewayRestarter IngressGateway Restarter is responsible for restarting Istio Ingress Gateway. The component consumes a list of [Restart Predicates](#restart-predicates) that determine when the restart should occur. Restarter triggers the restart if there's a change in the `numTrustedProxies` configuration. - diff --git a/docs/release-notes/1.14.0.md b/docs/release-notes/1.14.0.md new file mode 100644 index 000000000..20d80ee78 --- /dev/null +++ b/docs/release-notes/1.14.0.md @@ -0,0 +1,3 @@ +## New Features + +- Restarting sidecars is now divided into two phases. In the first phase, only Kyma workloads are restarted. If this phase fails, the Istio Custom Resource (CR) is set to the `Error` state. In the second phase, customer workloads are restarted in chunks. If any iteration of this phase fails, the Istio CR is set to the `Warning` state. diff --git a/internal/reconciliations/istio/configuration.go b/internal/reconciliations/istio/configuration/configuration.go similarity index 97% rename from internal/reconciliations/istio/configuration.go rename to internal/reconciliations/istio/configuration/configuration.go index 22ac6d1f8..7bfa1a2ab 100644 --- a/internal/reconciliations/istio/configuration.go +++ b/internal/reconciliations/istio/configuration/configuration.go @@ -1,8 +1,9 @@ -package istio +package configuration import ( "encoding/json" "fmt" + "github.com/kyma-project/istio/operator/api/v1alpha2" "github.com/kyma-project/istio/operator/pkg/labels" @@ -51,7 +52,7 @@ func GetLastAppliedConfiguration(istioCR *v1alpha2.Istio) (AppliedConfig, error) return lastAppliedConfig, nil } -func checkIstioVersionUpdate(currentIstioVersionString, targetIstioVersionString string) error { +func CheckIstioVersionUpdate(currentIstioVersionString, targetIstioVersionString string) error { currentIstioVersion, err := semver.NewVersion(currentIstioVersionString) if err != nil { return err diff --git a/internal/reconciliations/istio/configuration/configuration_test.go b/internal/reconciliations/istio/configuration/configuration_test.go new file mode 100644 index 000000000..b065413b9 --- /dev/null +++ b/internal/reconciliations/istio/configuration/configuration_test.go @@ -0,0 +1,88 @@ +package configuration_test + +import ( + "fmt" + "testing" + + operatorv1alpha2 "github.com/kyma-project/istio/operator/api/v1alpha2" + "github.com/kyma-project/istio/operator/internal/reconciliations/istio/configuration" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/kyma-project/istio/operator/internal/tests" + "github.com/onsi/ginkgo/v2/types" +) + +const ( + mockIstioTag string = "1.16.1-distroless" + lastAppliedConfiguration string = "operator.kyma-project.io/lastAppliedConfiguration" +) + +func TestRestarter(t *testing.T) { + RegisterFailHandler(Fail) + + RunSpecs(t, "Istio Configuration Suite") +} + +var _ = ReportAfterSuite("custom reporter", func(report types.Report) { + tests.GenerateGinkgoJunitReport("istio-configuration-suite", report) +}) + +var _ = Describe("Istio Configuration", func() { + Context("LastAppliedConfiguration", func() { + It("should update lastAppliedConfiguration and is able to unmarshal it back from annotation", func() { + // given + numTrustedProxies := 1 + istioCR := operatorv1alpha2.Istio{Spec: operatorv1alpha2.IstioSpec{Config: operatorv1alpha2.Config{NumTrustedProxies: &numTrustedProxies}}} + + // when + err := configuration.UpdateLastAppliedConfiguration(&istioCR, mockIstioTag) + + // then + Expect(err).ShouldNot(HaveOccurred()) + Expect(istioCR.Annotations).To(Not(BeEmpty())) + Expect(istioCR.Annotations[lastAppliedConfiguration]).To(Equal(fmt.Sprintf(`{"config":{"numTrustedProxies":1},"IstioTag":"%s"}`, mockIstioTag))) + + appliedConfig, err := configuration.GetLastAppliedConfiguration(&istioCR) + + Expect(err).ShouldNot(HaveOccurred()) + Expect(*appliedConfig.Config.NumTrustedProxies).To(Equal(1)) + }) + }) + + Context("CheckIstioVersionUpdate", func() { + It("should return nil when target version is the same as current version", func() { + err := configuration.CheckIstioVersionUpdate("1.10.0", "1.10.0") + Expect(err).ShouldNot(HaveOccurred()) + }) + + It("should return nil when target version is one minor version higher than current version", func() { + err := configuration.CheckIstioVersionUpdate("1.10.0", "1.11.0") + Expect(err).ShouldNot(HaveOccurred()) + }) + + It("should return error when target version is lower than current version", func() { + err := configuration.CheckIstioVersionUpdate("1.11.0", "1.10.0") + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("downgrade not supported")) + }) + + It("should return error when target version is more than one minor version higher than current version", func() { + err := configuration.CheckIstioVersionUpdate("1.10.0", "1.12.0") + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("the difference between versions exceed one minor version")) + }) + + It("should return error when target version has a different major version", func() { + err := configuration.CheckIstioVersionUpdate("1.10.0", "2.10.0") + Expect(err).Should(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("major version upgrade is not supported")) + }) + + It("should return nil when target version is a pre-release of the same version", func() { + err := configuration.CheckIstioVersionUpdate("1.10.0", "1.10.0-beta.1") + Expect(err).ShouldNot(HaveOccurred()) + }) + }) +}) diff --git a/internal/reconciliations/istio/configuration_test.go b/internal/reconciliations/istio/configuration_test.go deleted file mode 100644 index 6b26e3286..000000000 --- a/internal/reconciliations/istio/configuration_test.go +++ /dev/null @@ -1,38 +0,0 @@ -package istio_test - -import ( - "fmt" - operatorv1alpha2 "github.com/kyma-project/istio/operator/api/v1alpha2" - "github.com/kyma-project/istio/operator/internal/reconciliations/istio" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -const ( - mockIstioTag string = "1.16.1-distroless" - lastAppliedConfiguration string = "operator.kyma-project.io/lastAppliedConfiguration" -) - -var _ = Describe("Istio Configuration", func() { - Context("LastAppliedConfiguration", func() { - It("should update lastAppliedConfiguration and is able to unmarshal it back from annotation", func() { - // given - numTrustedProxies := 1 - istioCR := operatorv1alpha2.Istio{Spec: operatorv1alpha2.IstioSpec{Config: operatorv1alpha2.Config{NumTrustedProxies: &numTrustedProxies}}} - - // when - err := istio.UpdateLastAppliedConfiguration(&istioCR, mockIstioTag) - - // then - Expect(err).ShouldNot(HaveOccurred()) - Expect(istioCR.Annotations).To(Not(BeEmpty())) - Expect(istioCR.Annotations[lastAppliedConfiguration]).To(Equal(fmt.Sprintf(`{"config":{"numTrustedProxies":1},"IstioTag":"%s"}`, mockIstioTag))) - - appliedConfig, err := istio.GetLastAppliedConfiguration(&istioCR) - - Expect(err).ShouldNot(HaveOccurred()) - Expect(*appliedConfig.Config.NumTrustedProxies).To(Equal(1)) - }) - }) -}) diff --git a/internal/reconciliations/istio/install.go b/internal/reconciliations/istio/install.go index f3fbb33a4..426625222 100644 --- a/internal/reconciliations/istio/install.go +++ b/internal/reconciliations/istio/install.go @@ -2,10 +2,12 @@ package istio import ( "context" + operatorv1alpha2 "github.com/kyma-project/istio/operator/api/v1alpha2" "github.com/kyma-project/istio/operator/internal/clusterconfig" "github.com/kyma-project/istio/operator/internal/described_errors" "github.com/kyma-project/istio/operator/internal/istiooperator" + "github.com/kyma-project/istio/operator/internal/reconciliations/istio/configuration" "github.com/kyma-project/istio/operator/internal/status" "github.com/kyma-project/istio/operator/internal/webhooks" "github.com/kyma-project/istio/operator/pkg/labels" @@ -35,13 +37,13 @@ func installIstio(ctx context.Context, args installArgs) (istiooperator.IstioIma ctrl.Log.Info("Starting Istio install", "istio version", istioImageVersion.Version()) if _, ok := istioCR.Annotations[labels.LastAppliedConfiguration]; ok { - lastAppliedConfig, err := GetLastAppliedConfiguration(istioCR) + lastAppliedConfig, err := configuration.GetLastAppliedConfiguration(istioCR) if err != nil { ctrl.Log.Error(err, "Error evaluating Istio CR changes") return istioImageVersion, described_errors.NewDescribedError(err, "Istio install check failed") } - if err := checkIstioVersionUpdate(lastAppliedConfig.IstioTag, istioImageVersion.Tag()); err != nil { + if err := configuration.CheckIstioVersionUpdate(lastAppliedConfig.IstioTag, istioImageVersion.Tag()); err != nil { statusHandler.SetCondition(istioCR, operatorv1alpha2.NewReasonWithMessage(operatorv1alpha2.ConditionReasonIstioVersionUpdateNotAllowed)) // We are already updating the condition, that's why we need to avoid another condition update by applying SetCondition(false) return istioImageVersion, described_errors.NewDescribedError(err, "Istio version update is not allowed").SetWarning().SetCondition(false) diff --git a/internal/reconciliations/istio/uninstall.go b/internal/reconciliations/istio/uninstall.go index d63c29176..e60ab1a5a 100644 --- a/internal/reconciliations/istio/uninstall.go +++ b/internal/reconciliations/istio/uninstall.go @@ -9,7 +9,7 @@ import ( "github.com/kyma-project/istio/operator/internal/istiooperator" "github.com/kyma-project/istio/operator/internal/resources" "github.com/kyma-project/istio/operator/internal/status" - sidecarRemover "github.com/kyma-project/istio/operator/pkg/lib/sidecars/remove" + "github.com/kyma-project/istio/operator/pkg/lib/sidecars/remove" "github.com/thoas/go-funk" ctrl "sigs.k8s.io/controller-runtime" @@ -58,7 +58,7 @@ func uninstallIstio(ctx context.Context, args uninstallArgs) (istiooperator.Isti return istioImageVersion, described_errors.NewDescribedError(err, "Could not uninstall istio") } - warnings, err := sidecarRemover.RemoveSidecars(ctx, k8sClient, &ctrl.Log) + warnings, err := remove.RemoveSidecars(ctx, k8sClient, &ctrl.Log) if err != nil { return istioImageVersion, described_errors.NewDescribedError(err, "Could not remove istio sidecars") } diff --git a/internal/restarter/ingress_gateway.go b/internal/restarter/ingress_gateway.go index b188c5570..4cd239dab 100644 --- a/internal/restarter/ingress_gateway.go +++ b/internal/restarter/ingress_gateway.go @@ -5,10 +5,9 @@ import ( "github.com/kyma-project/istio/operator/api/v1alpha2" "github.com/kyma-project/istio/operator/internal/described_errors" - "github.com/kyma-project/istio/operator/internal/filter" + "github.com/kyma-project/istio/operator/internal/restarter/predicates" "github.com/kyma-project/istio/operator/internal/status" "github.com/kyma-project/istio/operator/pkg/lib/annotations" - "github.com/kyma-project/istio/operator/pkg/lib/ingressgateway" "github.com/kyma-project/istio/operator/pkg/lib/sidecars/retry" appsv1 "k8s.io/api/apps/v1" "k8s.io/apimachinery/pkg/types" @@ -25,11 +24,11 @@ const ( type IngressGatewayRestarter struct { client client.Client - predicates []filter.IngressGatewayPredicate + predicates []predicates.IngressGatewayPredicate statusHandler status.Status } -func NewIngressGatewayRestarter(client client.Client, predicates []filter.IngressGatewayPredicate, statusHandler status.Status) *IngressGatewayRestarter { +func NewIngressGatewayRestarter(client client.Client, predicates []predicates.IngressGatewayPredicate, statusHandler status.Status) *IngressGatewayRestarter { return &IngressGatewayRestarter{ client: client, predicates: predicates, @@ -40,7 +39,7 @@ func NewIngressGatewayRestarter(client client.Client, predicates []filter.Ingres func (r *IngressGatewayRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio) (described_errors.DescribedError, bool) { ctrl.Log.Info("Restarting Istio Ingress Gateway") - r.predicates = append(r.predicates, ingressgateway.NewRestartPredicate(istioCR)) + r.predicates = append(r.predicates, predicates.NewIngressGatewayRestartPredicate(istioCR)) for _, predicate := range r.predicates { evaluator, err := predicate.NewIngressGatewayEvaluator(ctx) if err != nil { diff --git a/internal/restarter/ingress_gateway_test.go b/internal/restarter/ingress_gateway_test.go index 19f46e02f..88c173ebf 100644 --- a/internal/restarter/ingress_gateway_test.go +++ b/internal/restarter/ingress_gateway_test.go @@ -5,8 +5,8 @@ import ( "time" operatorv1alpha2 "github.com/kyma-project/istio/operator/api/v1alpha2" - "github.com/kyma-project/istio/operator/internal/filter" "github.com/kyma-project/istio/operator/internal/restarter" + "github.com/kyma-project/istio/operator/internal/restarter/predicates" "github.com/kyma-project/istio/operator/internal/status" "github.com/kyma-project/istio/operator/pkg/lib/annotations" "github.com/kyma-project/istio/operator/pkg/lib/gatherer" @@ -36,7 +36,7 @@ var _ = Describe("Istio Ingress Gateway restart", func() { igPod := createIgPodWithCreationTimestamp("istio-ingressgateway", gatherer.IstioNamespace, "discovery", "1.16.1", time.Now().Add(-time.Hour)) fakeClient := createFakeClient(istioCR, istiod, igPod, igDep) statusHandler := status.NewStatusHandler(fakeClient) - igRestarter := restarter.NewIngressGatewayRestarter(fakeClient, []filter.IngressGatewayPredicate{mockIgPredicate{shouldRestart: true}}, statusHandler) + igRestarter := restarter.NewIngressGatewayRestarter(fakeClient, []predicates.IngressGatewayPredicate{mockIgPredicate{shouldRestart: true}}, statusHandler) //when err, requeue := igRestarter.Restart(context.Background(), istioCR) @@ -70,7 +70,7 @@ var _ = Describe("Istio Ingress Gateway restart", func() { igPod := createIgPodWithCreationTimestamp("istio-ingressgateway", gatherer.IstioNamespace, "discovery", "1.16.1", time.Now()) fakeClient := createFakeClient(istioCR, istiod, igDep, igPod) statusHandler := status.NewStatusHandler(fakeClient) - igRestarter := restarter.NewIngressGatewayRestarter(fakeClient, []filter.IngressGatewayPredicate{mockIgPredicate{shouldRestart: false}}, statusHandler) + igRestarter := restarter.NewIngressGatewayRestarter(fakeClient, []predicates.IngressGatewayPredicate{mockIgPredicate{shouldRestart: false}}, statusHandler) //when err, requeue := igRestarter.Restart(context.Background(), istioCR) @@ -102,7 +102,7 @@ var _ = Describe("Istio Ingress Gateway restart", func() { istiod := createPod("istiod", gatherer.IstioNamespace, "discovery", "1.16.1") fakeClient := createFakeClient(istioCR, istiod) statusHandler := status.NewStatusHandler(fakeClient) - igRestarter := restarter.NewIngressGatewayRestarter(fakeClient, []filter.IngressGatewayPredicate{mockIgPredicate{shouldRestart: true}}, statusHandler) + igRestarter := restarter.NewIngressGatewayRestarter(fakeClient, []predicates.IngressGatewayPredicate{mockIgPredicate{shouldRestart: true}}, statusHandler) //when err, requeue := igRestarter.Restart(context.Background(), istioCR) @@ -153,7 +153,7 @@ func (m mockIgPredicate) RequiresIngressGatewayRestart() bool { return m.shouldRestart } -func (m mockIgPredicate) NewIngressGatewayEvaluator(_ context.Context) (filter.IngressGatewayRestartEvaluator, error) { +func (m mockIgPredicate) NewIngressGatewayEvaluator(_ context.Context) (predicates.IngressGatewayRestartEvaluator, error) { return m, nil } diff --git a/internal/compatibility/proxy_restart.go b/internal/restarter/predicates/compatibility.go similarity index 62% rename from internal/compatibility/proxy_restart.go rename to internal/restarter/predicates/compatibility.go index 9f500e79d..877f999d6 100644 --- a/internal/compatibility/proxy_restart.go +++ b/internal/restarter/predicates/compatibility.go @@ -1,24 +1,24 @@ -package compatibility +package predicates import ( "github.com/kyma-project/istio/operator/api/v1alpha2" - "github.com/kyma-project/istio/operator/internal/reconciliations/istio" + "github.com/kyma-project/istio/operator/internal/reconciliations/istio/configuration" v1 "k8s.io/api/core/v1" ) -type ProxyRestartPredicate struct { +type CompatibilityRestartPredicate struct { oldCompatibilityMode bool newCompatibilityMode bool config config } -func NewRestartPredicate(istioCR *v1alpha2.Istio) (*ProxyRestartPredicate, error) { - lastAppliedConfig, err := istio.GetLastAppliedConfiguration(istioCR) +func NewCompatibilityRestartPredicate(istioCR *v1alpha2.Istio) (*CompatibilityRestartPredicate, error) { + lastAppliedConfig, err := configuration.GetLastAppliedConfiguration(istioCR) if err != nil { return nil, err } - return &ProxyRestartPredicate{ + return &CompatibilityRestartPredicate{ oldCompatibilityMode: lastAppliedConfig.IstioSpec.CompatibilityMode, newCompatibilityMode: istioCR.Spec.CompatibilityMode, config: config{proxyMetadata: v1alpha2.ProxyMetaDataCompatibility}, @@ -33,10 +33,14 @@ func (c config) hasProxyMetadata() bool { return len(c.proxyMetadata) > 0 } -func (p ProxyRestartPredicate) RequiresProxyRestart(_ v1.Pod) bool { +func (p CompatibilityRestartPredicate) Matches(_ v1.Pod) bool { if p.config.hasProxyMetadata() && p.oldCompatibilityMode != p.newCompatibilityMode { return true } return false } + +func (p CompatibilityRestartPredicate) MustMatch() bool { + return false +} diff --git a/internal/compatibility/proxy_restart_test.go b/internal/restarter/predicates/compatibility_test.go similarity index 77% rename from internal/compatibility/proxy_restart_test.go rename to internal/restarter/predicates/compatibility_test.go index 4cc4de9fd..d52e88dda 100644 --- a/internal/compatibility/proxy_restart_test.go +++ b/internal/restarter/predicates/compatibility_test.go @@ -1,4 +1,4 @@ -package compatibility +package predicates import ( "github.com/kyma-project/istio/operator/pkg/labels" @@ -10,50 +10,50 @@ import ( . "github.com/onsi/gomega" ) -var _ = Describe("Proxy Restarter", func() { - Context("RequiresProxyRestart", func() { +var _ = Describe("Compatibility Predicate", func() { + Context("Matches", func() { It("should evaluate to true when proxy metadata values exist and new and old compatibility mode is different", func() { - predicate := ProxyRestartPredicate{ + predicate := CompatibilityRestartPredicate{ oldCompatibilityMode: true, newCompatibilityMode: false, config: config{ proxyMetadata: map[string]string{"key": "value"}, }, } - Expect(predicate.RequiresProxyRestart(v1.Pod{})).To(BeTrue()) + Expect(predicate.Matches(v1.Pod{})).To(BeTrue()) }) It("should evaluate to false when proxy metadata values exist and new and old compatibility mode is equal", func() { - predicate := ProxyRestartPredicate{ + predicate := CompatibilityRestartPredicate{ oldCompatibilityMode: true, newCompatibilityMode: true, config: config{ proxyMetadata: map[string]string{"key": "value"}, }, } - Expect(predicate.RequiresProxyRestart(v1.Pod{})).To(BeFalse()) + Expect(predicate.Matches(v1.Pod{})).To(BeFalse()) }) It("should evaluate to false when no proxy metadata values exist and new and old compatibility mode is different", func() { - predicate := ProxyRestartPredicate{ + predicate := CompatibilityRestartPredicate{ oldCompatibilityMode: true, newCompatibilityMode: false, } - Expect(predicate.RequiresProxyRestart(v1.Pod{})).To(BeFalse()) + Expect(predicate.Matches(v1.Pod{})).To(BeFalse()) }) It("should evaluate to false when no proxy metadata values exist and new and old compatibility mode is equal", func() { - predicate := ProxyRestartPredicate{ + predicate := CompatibilityRestartPredicate{ oldCompatibilityMode: true, newCompatibilityMode: true, } - Expect(predicate.RequiresProxyRestart(v1.Pod{})).To(BeFalse()) + Expect(predicate.Matches(v1.Pod{})).To(BeFalse()) }) }) - Context("NewRestartPredicate", func() { + Context("NewCompatibilityRestartPredicate", func() { It("should return an error if getLastAppliedConfiguration fails", func() { - _, err := NewRestartPredicate(&operatorv1alpha2.Istio{ + _, err := NewCompatibilityRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ labels.LastAppliedConfiguration: `{"compatibilityMode":abc}`, @@ -64,7 +64,7 @@ var _ = Describe("Proxy Restarter", func() { }) It("should return false for old compatibility mode if lastAppliedConfiguration is empty", func() { - predicate, err := NewRestartPredicate(&operatorv1alpha2.Istio{ + predicate, err := NewCompatibilityRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{}, }, @@ -75,7 +75,7 @@ var _ = Describe("Proxy Restarter", func() { }) It("should return value for old compatibility mode from lastAppliedConfiguration", func() { - predicate, err := NewRestartPredicate(&operatorv1alpha2.Istio{ + predicate, err := NewCompatibilityRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ labels.LastAppliedConfiguration: `{"compatibilityMode":true}`, @@ -88,7 +88,7 @@ var _ = Describe("Proxy Restarter", func() { }) It("should return value for new compatibility mode from istio CR", func() { - predicate, err := NewRestartPredicate(&operatorv1alpha2.Istio{ + predicate, err := NewCompatibilityRestartPredicate(&operatorv1alpha2.Istio{ Spec: operatorv1alpha2.IstioSpec{ CompatibilityMode: true, }, diff --git a/internal/restarter/predicates/customer_workload.go b/internal/restarter/predicates/customer_workload.go new file mode 100644 index 000000000..828db400f --- /dev/null +++ b/internal/restarter/predicates/customer_workload.go @@ -0,0 +1,20 @@ +package predicates + +import ( + v1 "k8s.io/api/core/v1" +) + +type CustomerWorkloadRestartPredicate struct { +} + +func (p CustomerWorkloadRestartPredicate) Matches(pod v1.Pod) bool { + return pod.Namespace != "kyma-system" && pod.Labels["kyma-project.io/module"] == "" +} + +func (p CustomerWorkloadRestartPredicate) MustMatch() bool { + return true +} + +func NewCustomerWorkloadRestartPredicate() *CustomerWorkloadRestartPredicate { + return &CustomerWorkloadRestartPredicate{} +} diff --git a/internal/restarter/predicates/customer_workload_test.go b/internal/restarter/predicates/customer_workload_test.go new file mode 100644 index 000000000..7aad000f3 --- /dev/null +++ b/internal/restarter/predicates/customer_workload_test.go @@ -0,0 +1,43 @@ +package predicates + +import ( + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Customer Workload Predicate", func() { + Context("Matches", func() { + It("should return true if pod in default namespace", func() { + predicate := CustomerWorkloadRestartPredicate{} + Expect(predicate.Matches(v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + })).To(BeTrue()) + }) + + It("should return false if pod in kyma-system", func() { + predicate := CustomerWorkloadRestartPredicate{} + Expect(predicate.Matches(v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "kyma-system", + }, + })).To(BeFalse()) + }) + + It("should return false if pod has kyma label", func() { + predicate := CustomerWorkloadRestartPredicate{} + Expect(predicate.Matches(v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Labels: map[string]string{ + "kyma-project.io/module": "test", + }, + }, + })).To(BeFalse()) + }) + }) +}) diff --git a/pkg/lib/sidecars/pods/filter.go b/internal/restarter/predicates/image_resources.go similarity index 71% rename from pkg/lib/sidecars/pods/filter.go rename to internal/restarter/predicates/image_resources.go index 5f56e9a1d..32ff24222 100644 --- a/pkg/lib/sidecars/pods/filter.go +++ b/internal/restarter/predicates/image_resources.go @@ -1,6 +1,8 @@ -package pods +package predicates import ( + "fmt" + v1 "k8s.io/api/core/v1" ) @@ -9,26 +11,50 @@ const ( istioSidecarCustomImageAnnotation string = "sidecar.istio.io/proxyImage" ) -type RestartProxyPredicate struct { +type SidecarImage struct { + Repository string + Tag string +} + +func NewSidecarImage(hub, tag string) SidecarImage { + return SidecarImage{ + Repository: fmt.Sprintf("%s/proxyv2", hub), + Tag: tag, + } +} + +func (r SidecarImage) String() string { + return fmt.Sprintf("%s:%s", r.Repository, r.Tag) +} + +func (r SidecarImage) matchesImageIn(container v1.Container) bool { + return container.Image == r.String() +} + +type ImageResourcesPredicate struct { expectedImage SidecarImage expectedResources v1.ResourceRequirements } -// NewRestartProxyPredicate creates a new RestartProxyPredicate that checks if a pod needs a restart based on the expected image and resources. -func NewRestartProxyPredicate(expectedImage SidecarImage, expectedResources v1.ResourceRequirements) *RestartProxyPredicate { - return &RestartProxyPredicate{expectedImage: expectedImage, expectedResources: expectedResources} +// NewImageResourcesPredicate creates a new ImageResourcesPredicate that checks if a pod needs a restart based on the expected image and resources. +func NewImageResourcesPredicate(expectedImage SidecarImage, expectedResources v1.ResourceRequirements) *ImageResourcesPredicate { + return &ImageResourcesPredicate{expectedImage: expectedImage, expectedResources: expectedResources} } -func (p RestartProxyPredicate) RequiresProxyRestart(pod v1.Pod) bool { +func (p ImageResourcesPredicate) Matches(pod v1.Pod) bool { return needsRestart(pod, p.expectedImage, *p.expectedResources.DeepCopy()) } +func (p ImageResourcesPredicate) MustMatch() bool { + return false +} + func needsRestart(pod v1.Pod, expectedImage SidecarImage, expectedResources v1.ResourceRequirements) bool { return !hasCustomImageAnnotation(pod) && (hasSidecarContainerWithWithDifferentImage(pod, expectedImage) || hasDifferentSidecarResources(pod, expectedResources)) } -func isReadyWithIstioAnnotation(pod v1.Pod) bool { +func IsReadyWithIstioAnnotation(pod v1.Pod) bool { return IsPodReady(pod) && HasIstioSidecarStatusAnnotation(pod) } diff --git a/internal/restarter/predicates/image_resources_test.go b/internal/restarter/predicates/image_resources_test.go new file mode 100644 index 000000000..a98ada823 --- /dev/null +++ b/internal/restarter/predicates/image_resources_test.go @@ -0,0 +1,103 @@ +package predicates_test + +import ( + "github.com/kyma-project/istio/operator/internal/restarter/predicates" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +var _ = Describe("Matches", func() { + It("should should return false when pod has custom image annotation", func() { + // given + pod := createPodWithProxySidecar("test-pod", "test-namespace", "1.21.0", map[string]string{"sidecar.istio.io/proxyImage": "istio/proxyv2:1.21.0"}) + predicate := predicates.NewImageResourcesPredicate(predicates.NewSidecarImage("istio", "1.22.0"), v1.ResourceRequirements{}) + + // when + shouldRestart := predicate.Matches(pod) + + // then + Expect(shouldRestart).To(BeFalse()) + }) + + It("should return true when pod does not have custom image annotation", func() { + // given + pod := createPodWithProxySidecar("test-pod", "test-namespace", "1.21.0", map[string]string{}) + predicate := predicates.NewImageResourcesPredicate(predicates.NewSidecarImage("istio", "1.22.0"), v1.ResourceRequirements{}) + + // when + shouldRestart := predicate.Matches(pod) + + // then + Expect(shouldRestart).To(BeTrue()) + }) +}) + +func createPodWithProxySidecar(name, namespace, proxyIstioVersion string, annotations map[string]string) v1.Pod { + if annotations == nil { + annotations = map[string]string{} + } + annotations["sidecar.istio.io/status"] = "true" + return v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Annotations: annotations, + }, + Status: v1.PodStatus{ + Phase: v1.PodRunning, + Conditions: []v1.PodCondition{ + { + Type: v1.PodReady, + Status: v1.ConditionTrue, + }, + }, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "istio-proxy", + Image: "istio/proxyv2:" + proxyIstioVersion, + }, + }, + }, + } +} + +var _ = Describe("IsReadyWithIstioAnnotation", func() { + It("should return true when pod is ready and has istio sidecar status annotation", func() { + // given + pod := createPodWithProxySidecar("test-pod", "test-namespace", "1.21.0", map[string]string{"sidecar.istio.io/status": "true"}) + + // when + isReady := predicates.IsReadyWithIstioAnnotation(pod) + + // then + Expect(isReady).To(BeTrue()) + }) + + It("should return false when pod is not ready", func() { + // given + pod := createPodWithProxySidecar("test-pod", "test-namespace", "1.21.0", map[string]string{"sidecar.istio.io/status": "true"}) + pod.Status.Conditions[0].Status = v1.ConditionFalse + + // when + isReady := predicates.IsReadyWithIstioAnnotation(pod) + + // then + Expect(isReady).To(BeFalse()) + }) + + It("should return false when pod does not have istio sidecar status annotation", func() { + // given + pod := createPodWithProxySidecar("test-pod", "test-namespace", "1.21.0", nil) + delete(pod.Annotations, "sidecar.istio.io/status") + + // when + isReady := predicates.IsReadyWithIstioAnnotation(pod) + + // then + Expect(isReady).To(BeFalse()) + }) +}) diff --git a/pkg/lib/ingressgateway/ingressgateway.go b/internal/restarter/predicates/ingressgateway.go similarity index 77% rename from pkg/lib/ingressgateway/ingressgateway.go rename to internal/restarter/predicates/ingressgateway.go index 4f28cfec9..199af5bc2 100644 --- a/pkg/lib/ingressgateway/ingressgateway.go +++ b/internal/restarter/predicates/ingressgateway.go @@ -1,22 +1,22 @@ -package ingressgateway +package predicates import ( "context" + operatorv1alpha2 "github.com/kyma-project/istio/operator/api/v1alpha2" - "github.com/kyma-project/istio/operator/internal/filter" - "github.com/kyma-project/istio/operator/internal/reconciliations/istio" + "github.com/kyma-project/istio/operator/internal/reconciliations/istio/configuration" ) type RestartPredicate struct { istioCR *operatorv1alpha2.Istio } -func NewRestartPredicate(istioCR *operatorv1alpha2.Istio) *RestartPredicate { +func NewIngressGatewayRestartPredicate(istioCR *operatorv1alpha2.Istio) *RestartPredicate { return &RestartPredicate{istioCR: istioCR} } -func (i RestartPredicate) NewIngressGatewayEvaluator(_ context.Context) (filter.IngressGatewayRestartEvaluator, error) { - lastAppliedConfig, err := istio.GetLastAppliedConfiguration(i.istioCR) +func (i RestartPredicate) NewIngressGatewayEvaluator(_ context.Context) (IngressGatewayRestartEvaluator, error) { + lastAppliedConfig, err := configuration.GetLastAppliedConfiguration(i.istioCR) if err != nil { return nil, err } diff --git a/pkg/lib/ingressgateway/ingressgateway_test.go b/internal/restarter/predicates/ingressgateway_test.go similarity index 73% rename from pkg/lib/ingressgateway/ingressgateway_test.go rename to internal/restarter/predicates/ingressgateway_test.go index e40a3d65b..5a95e15b7 100644 --- a/pkg/lib/ingressgateway/ingressgateway_test.go +++ b/internal/restarter/predicates/ingressgateway_test.go @@ -1,4 +1,4 @@ -package ingressgateway_test +package predicates_test import ( "context" @@ -8,16 +8,16 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" operatorv1alpha2 "github.com/kyma-project/istio/operator/api/v1alpha2" - "github.com/kyma-project/istio/operator/pkg/lib/ingressgateway" + predicates "github.com/kyma-project/istio/operator/internal/restarter/predicates" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "k8s.io/utils/ptr" ) -var _ = Describe("Ingress Gateway Restarter", func() { +var _ = Describe("Ingress Gateway Predicate", func() { Context("RequiresIngressGatewayRestart", func() { It("should evaluate to true if new is nil and old is not nil", func() { - evaluator := ingressgateway.NumTrustedProxiesRestartEvaluator{ + evaluator := predicates.NumTrustedProxiesRestartEvaluator{ NewNumTrustedProxies: nil, OldNumTrustedProxies: new(int), } @@ -26,7 +26,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { }) It("should evaluate to true if new is not nil and old is nil", func() { - evaluator := ingressgateway.NumTrustedProxiesRestartEvaluator{ + evaluator := predicates.NumTrustedProxiesRestartEvaluator{ NewNumTrustedProxies: new(int), OldNumTrustedProxies: nil, } @@ -38,7 +38,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { newNumTrustedProxies := 1 oldNumTrustedProxies := 2 - evaluator := ingressgateway.NumTrustedProxiesRestartEvaluator{ + evaluator := predicates.NumTrustedProxiesRestartEvaluator{ NewNumTrustedProxies: &newNumTrustedProxies, OldNumTrustedProxies: &oldNumTrustedProxies, } @@ -50,7 +50,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { numTrustedProxies := 1 oldNumTrustedProxies := 1 - evaluator := ingressgateway.NumTrustedProxiesRestartEvaluator{ + evaluator := predicates.NumTrustedProxiesRestartEvaluator{ NewNumTrustedProxies: &oldNumTrustedProxies, OldNumTrustedProxies: &numTrustedProxies, } @@ -67,7 +67,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { ) It("should return an error if getLastAppliedConfiguration fails", func() { - predicate := ingressgateway.NewRestartPredicate(&operatorv1alpha2.Istio{ + predicate := predicates.NewIngressGatewayRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ lastAppliedConfiguration: `{"config":{"numTrustedProxies":abc},"IstioTag":w}`, @@ -80,7 +80,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { }) It("should return nil for old numTrustedProxies if lastAppliedConfiguration is empty", func() { - predicate := ingressgateway.NewRestartPredicate(&operatorv1alpha2.Istio{ + predicate := predicates.NewIngressGatewayRestartPredicate(&operatorv1alpha2.Istio{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{}, }, @@ -89,7 +89,7 @@ var _ = Describe("Ingress Gateway Restarter", func() { Expect(err).NotTo(HaveOccurred()) Expect(evaluator).NotTo(BeNil()) - Expect(evaluator.(ingressgateway.NumTrustedProxiesRestartEvaluator).OldNumTrustedProxies).To(BeNil()) + Expect(evaluator.(predicates.NumTrustedProxiesRestartEvaluator).OldNumTrustedProxies).To(BeNil()) }) It("should return correct not nil value for new and old numTrustedProxies", func() { @@ -104,13 +104,13 @@ var _ = Describe("Ingress Gateway Restarter", func() { }, } - predicate := ingressgateway.NewRestartPredicate(istio) + predicate := predicates.NewIngressGatewayRestartPredicate(istio) evaluator, err := predicate.NewIngressGatewayEvaluator(context.Background()) Expect(err).NotTo(HaveOccurred()) Expect(evaluator).NotTo(BeNil()) - Expect(*(evaluator.(ingressgateway.NumTrustedProxiesRestartEvaluator).NewNumTrustedProxies)).To(Equal(1)) - Expect(*(evaluator.(ingressgateway.NumTrustedProxiesRestartEvaluator).OldNumTrustedProxies)).To(Equal(2)) + Expect(*(evaluator.(predicates.NumTrustedProxiesRestartEvaluator).NewNumTrustedProxies)).To(Equal(1)) + Expect(*(evaluator.(predicates.NumTrustedProxiesRestartEvaluator).OldNumTrustedProxies)).To(Equal(2)) }) It("should return correct nil value for new and old numTrustedProxies", func() { @@ -125,13 +125,13 @@ var _ = Describe("Ingress Gateway Restarter", func() { }, } - predicate := ingressgateway.NewRestartPredicate(istio) + predicate := predicates.NewIngressGatewayRestartPredicate(istio) evaluator, err := predicate.NewIngressGatewayEvaluator(context.Background()) Expect(err).NotTo(HaveOccurred()) Expect(evaluator).NotTo(BeNil()) - Expect(evaluator.(ingressgateway.NumTrustedProxiesRestartEvaluator).NewNumTrustedProxies).To(BeNil()) - Expect(evaluator.(ingressgateway.NumTrustedProxiesRestartEvaluator).OldNumTrustedProxies).To(BeNil()) + Expect(evaluator.(predicates.NumTrustedProxiesRestartEvaluator).NewNumTrustedProxies).To(BeNil()) + Expect(evaluator.(predicates.NumTrustedProxiesRestartEvaluator).OldNumTrustedProxies).To(BeNil()) }) }) diff --git a/internal/restarter/predicates/kyma_workload.go b/internal/restarter/predicates/kyma_workload.go new file mode 100644 index 000000000..7b199a051 --- /dev/null +++ b/internal/restarter/predicates/kyma_workload.go @@ -0,0 +1,20 @@ +package predicates + +import ( + v1 "k8s.io/api/core/v1" +) + +type KymaWorkloadRestartPredicate struct { +} + +func (p KymaWorkloadRestartPredicate) Matches(pod v1.Pod) bool { + return pod.Namespace == "kyma-system" || pod.Labels["kyma-project.io/module"] != "" +} + +func (p KymaWorkloadRestartPredicate) MustMatch() bool { + return true +} + +func NewKymaWorkloadRestartPredicate() *KymaWorkloadRestartPredicate { + return &KymaWorkloadRestartPredicate{} +} diff --git a/internal/restarter/predicates/kyma_workload_test.go b/internal/restarter/predicates/kyma_workload_test.go new file mode 100644 index 000000000..48b9c90ec --- /dev/null +++ b/internal/restarter/predicates/kyma_workload_test.go @@ -0,0 +1,43 @@ +package predicates + +import ( + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Kyma Workload Predicate", func() { + Context("Matches", func() { + It("should return true if pod in kyma-system", func() { + predicate := KymaWorkloadRestartPredicate{} + Expect(predicate.Matches(v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "kyma-system", + }, + })).To(BeTrue()) + }) + + It("should return true if pod in has kyma label", func() { + predicate := KymaWorkloadRestartPredicate{} + Expect(predicate.Matches(v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Labels: map[string]string{ + "kyma-project.io/module": "test", + }, + }, + })).To(BeTrue()) + }) + + It("should return false if pod in default namespace and do not have kyma label", func() { + predicate := KymaWorkloadRestartPredicate{} + Expect(predicate.Matches(v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + }, + })).To(BeFalse()) + }) + }) +}) diff --git a/internal/filter/predicate.go b/internal/restarter/predicates/predicate.go similarity index 88% rename from internal/filter/predicate.go rename to internal/restarter/predicates/predicate.go index 82dbf9ea7..50f7d6da5 100644 --- a/internal/filter/predicate.go +++ b/internal/restarter/predicates/predicate.go @@ -1,4 +1,4 @@ -package filter +package predicates import ( "context" @@ -7,7 +7,8 @@ import ( ) type SidecarProxyPredicate interface { - RequiresProxyRestart(v1.Pod) bool + Matches(v1.Pod) bool + MustMatch() bool } type IngressGatewayPredicate interface { diff --git a/internal/compatibility/suite_test.go b/internal/restarter/predicates/suite_test.go similarity index 69% rename from internal/compatibility/suite_test.go rename to internal/restarter/predicates/suite_test.go index 2a7cf85bf..c38eb4949 100644 --- a/internal/compatibility/suite_test.go +++ b/internal/restarter/predicates/suite_test.go @@ -1,19 +1,20 @@ -package compatibility_test +package predicates_test import ( + "testing" + "github.com/kyma-project/istio/operator/internal/tests" . "github.com/onsi/ginkgo/v2" "github.com/onsi/ginkgo/v2/types" . "github.com/onsi/gomega" - "testing" ) func TestRestarter(t *testing.T) { RegisterFailHandler(Fail) - RunSpecs(t, "Compatibility Mode Suite") + RunSpecs(t, "Restarter Predicates Suite") } var _ = ReportAfterSuite("custom reporter", func(report types.Report) { - tests.GenerateGinkgoJunitReport("compatibility-mode", report) + tests.GenerateGinkgoJunitReport("restarter-predicates-suite", report) }) diff --git a/internal/restarter/sidecars.go b/internal/restarter/sidecars.go index 43680100a..6dd268010 100644 --- a/internal/restarter/sidecars.go +++ b/internal/restarter/sidecars.go @@ -2,49 +2,44 @@ package restarter import ( "context" - "fmt" - "strings" - - "github.com/kyma-project/istio/operator/internal/compatibility" "github.com/kyma-project/istio/operator/api/v1alpha2" "github.com/kyma-project/istio/operator/internal/described_errors" + "github.com/kyma-project/istio/operator/internal/restarter/predicates" "github.com/pkg/errors" "github.com/go-logr/logr" "github.com/kyma-project/istio/operator/internal/clusterconfig" - "github.com/kyma-project/istio/operator/internal/filter" "github.com/kyma-project/istio/operator/internal/istiooperator" "github.com/kyma-project/istio/operator/internal/status" "github.com/kyma-project/istio/operator/pkg/lib/gatherer" "github.com/kyma-project/istio/operator/pkg/lib/sidecars" - "github.com/kyma-project/istio/operator/pkg/lib/sidecars/pods" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" ) const errorDescription = "Error occurred during reconciliation of Istio Sidecars" -type SidecarsRestarter struct { - Log logr.Logger - Client client.Client - Merger istiooperator.Merger - ProxyResetter sidecars.ProxyResetter - StatusHandler status.Status +type SidecarRestarter struct { + Log logr.Logger + Client client.Client + Merger istiooperator.Merger + ProxyRestarter sidecars.ProxyRestarter + StatusHandler status.Status } -func NewSidecarsRestarter(logger logr.Logger, client client.Client, merger istiooperator.Merger, resetter sidecars.ProxyResetter, statusHandler status.Status) *SidecarsRestarter { - return &SidecarsRestarter{ - Log: logger, - Client: client, - Merger: merger, - ProxyResetter: resetter, - StatusHandler: statusHandler, +func NewSidecarsRestarter(logger logr.Logger, client client.Client, merger istiooperator.Merger, proxyRestarter sidecars.ProxyRestarter, statusHandler status.Status) *SidecarRestarter { + return &SidecarRestarter{ + Log: logger, + Client: client, + Merger: merger, + ProxyRestarter: proxyRestarter, + StatusHandler: statusHandler, } } // Restart runs Proxy Reset action, which checks if any of sidecars need a restart and proceed with rollout. -func (s *SidecarsRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio) (described_errors.DescribedError, bool) { +func (s *SidecarRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio) (described_errors.DescribedError, bool) { clusterSize, err := clusterconfig.EvaluateClusterSize(ctx, s.Client) if err != nil { s.Log.Error(err, "Error occurred during evaluation of cluster size") @@ -74,7 +69,7 @@ func (s *SidecarsRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio return described_errors.NewDescribedError(err, "Could not get Istio tag from istio operator file"), false } - expectedImage := pods.NewSidecarImage(iop.Spec.Hub, tag) + expectedImage := predicates.NewSidecarImage(iop.Spec.Hub, tag) s.Log.Info("Running proxy sidecar reset", "expected image", expectedImage) err = gatherer.VerifyIstioPodsVersion(ctx, s.Client, istioImageVersion.Version()) @@ -90,35 +85,15 @@ func (s *SidecarsRestarter) Restart(ctx context.Context, istioCR *v1alpha2.Istio return described_errors.NewDescribedError(err, errorDescription), false } - compatibiltyPredicate, err := compatibility.NewRestartPredicate(istioCR) - if err != nil { - s.Log.Error(err, "Failed to create restart compatibility predicate") - s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartFailed)) - return described_errors.NewDescribedError(err, errorDescription), false - } - - warnings, hasMorePods, err := s.ProxyResetter.ProxyReset(ctx, s.Client, expectedImage, expectedResources, []filter.SidecarProxyPredicate{compatibiltyPredicate}, &s.Log) + warnings, hasMorePods, err := s.ProxyRestarter.RestartProxies(ctx, expectedImage, expectedResources, istioCR) if err != nil { s.Log.Error(err, "Failed to reset proxy") s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarRestartFailed)) return described_errors.NewDescribedError(err, errorDescription), false } - warningsCount := len(warnings) - if warningsCount > 0 { - podsLimit := 5 - pods := []string{} - for _, w := range warnings { - if podsLimit--; podsLimit >= 0 { - pods = append(pods, fmt.Sprintf("%s/%s", w.Namespace, w.Name)) - } - s.Log.Info("Proxy reset warning:", "name", w.Name, "namespace", w.Namespace, "kind", w.Kind, "message", w.Message) - } - warningMessage := fmt.Sprintf("The sidecars of the following workloads could not be restarted: %s", - strings.Join(pods, ", ")) - if warningsCount-len(pods) > 0 { - warningMessage += fmt.Sprintf(" and %d additional workload(s)", warningsCount-len(pods)) - } + warningMessage := sidecars.BuildWarningMessage(warnings, &s.Log) + if warningMessage != "" { warningErr := described_errors.NewDescribedError(errors.New("Istio Controller could not restart one or more Istio-injected Pods."), "Some Pods with Istio sidecar injection failed to restart. To learn more about the warning, see kyma-system/istio-controller-manager logs").SetWarning() s.StatusHandler.SetCondition(istioCR, v1alpha2.NewReasonWithMessage(v1alpha2.ConditionReasonProxySidecarManualRestartRequired, warningMessage)) s.Log.Info(warningMessage) diff --git a/internal/restarter/sidecars_test.go b/internal/restarter/sidecars_test.go index c9f364a7b..4c64aece3 100644 --- a/internal/restarter/sidecars_test.go +++ b/internal/restarter/sidecars_test.go @@ -5,12 +5,13 @@ import ( "os" "github.com/go-logr/logr" + "github.com/kyma-project/istio/operator/api/v1alpha2" operatorv1alpha2 "github.com/kyma-project/istio/operator/api/v1alpha2" "github.com/kyma-project/istio/operator/internal/clusterconfig" "github.com/kyma-project/istio/operator/internal/described_errors" - "github.com/kyma-project/istio/operator/internal/filter" "github.com/kyma-project/istio/operator/internal/istiooperator" "github.com/kyma-project/istio/operator/internal/restarter" + "github.com/kyma-project/istio/operator/internal/restarter/predicates" "github.com/kyma-project/istio/operator/internal/status" "github.com/kyma-project/istio/operator/pkg/lib/gatherer" "github.com/kyma-project/istio/operator/pkg/lib/sidecars" @@ -18,6 +19,7 @@ import ( "github.com/kyma-project/istio/operator/pkg/lib/sidecars/restart" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/pkg/errors" networkingv1 "istio.io/client-go/pkg/apis/networking/v1" iopv1alpha1 "istio.io/istio/operator/pkg/apis" corev1 "k8s.io/api/core/v1" @@ -30,27 +32,21 @@ import ( ) var _ = Describe("SidecarsRestarter reconciliation", func() { + logger := logr.Discard() It("should fail proxy reset if Istio pods do not match target version", func() { // given - numTrustedProxies := 1 - istioCr := operatorv1alpha2.Istio{ObjectMeta: metav1.ObjectMeta{ - Name: "default", - ResourceVersion: "1", - Annotations: map[string]string{}, - }, - Spec: operatorv1alpha2.IstioSpec{ - Config: operatorv1alpha2.Config{ - NumTrustedProxies: &numTrustedProxies, - }, - }, - } + istioCr := createIstioCR() istiod := createPod("istiod", gatherer.IstioNamespace, "discovery", "1.16.0") - fakeClient := createFakeClient(&istioCr, istiod) + fakeClient := createFakeClient(istioCr, istiod) statusHandler := status.NewStatusHandler(fakeClient) - sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(&istioCr, istiod), - &MergerMock{"1.16.1-distroless"}, sidecars.NewProxyResetter(), statusHandler) + podsLister := pods.NewPods(fakeClient, &logger) + actionRestarter := restart.NewActionRestarter(fakeClient, &logger) + proxyRestarter := sidecars.NewProxyRestarter(fakeClient, podsLister, actionRestarter, &logger) + sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(istioCr, istiod), + &MergerMock{"1.16.1-distroless"}, proxyRestarter, statusHandler) + // when - err, requeue := sidecarsRestarter.Restart(context.Background(), &istioCr) + err, requeue := sidecarsRestarter.Restart(context.Background(), istioCr) // then Expect(err).Should(HaveOccurred()) @@ -65,20 +61,9 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { It("should succeed proxy reset even if more than 5 proxies could not be reset and will return a warning", func() { // given - numTrustedProxies := 1 - istioCr := operatorv1alpha2.Istio{ObjectMeta: metav1.ObjectMeta{ - Name: "default", - ResourceVersion: "1", - Annotations: map[string]string{}, - }, - Spec: operatorv1alpha2.IstioSpec{ - Config: operatorv1alpha2.Config{ - NumTrustedProxies: &numTrustedProxies, - }, - }, - } + istioCr := createIstioCR() istiod := createPod("istiod", gatherer.IstioNamespace, "discovery", "1.16.1") - proxyResetter := &proxyResetterMock{ + proxyRestarter := &proxyRestarterMock{ restartWarnings: []restart.RestartWarning{ { Name: "name1", @@ -107,13 +92,13 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { }, hasMorePods: true, } - fakeClient := createFakeClient(&istioCr, istiod) + fakeClient := createFakeClient(istioCr, istiod) statusHandler := status.NewStatusHandler(fakeClient) - sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(&istioCr, istiod), - &MergerMock{"1.16.1-distroless"}, proxyResetter, statusHandler) + sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(istioCr, istiod), + &MergerMock{"1.16.1-distroless"}, proxyRestarter, statusHandler) // when - err, requeue := sidecarsRestarter.Restart(context.Background(), &istioCr) + err, requeue := sidecarsRestarter.Restart(context.Background(), istioCr) // then Expect(err).Should(HaveOccurred()) @@ -127,20 +112,9 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { It("should succeed proxy reset even if less than 5 proxies could not be reset and will return a warning", func() { // given - numTrustedProxies := 1 - istioCr := operatorv1alpha2.Istio{ObjectMeta: metav1.ObjectMeta{ - Name: "default", - ResourceVersion: "1", - Annotations: map[string]string{}, - }, - Spec: operatorv1alpha2.IstioSpec{ - Config: operatorv1alpha2.Config{ - NumTrustedProxies: &numTrustedProxies, - }, - }, - } + istioCr := createIstioCR() istiod := createPod("istiod", gatherer.IstioNamespace, "discovery", "1.16.1") - proxyResetter := &proxyResetterMock{ + proxyRestarter := &proxyRestarterMock{ restartWarnings: []restart.RestartWarning{ { Name: "name1", @@ -153,13 +127,13 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { }, hasMorePods: true, } - fakeClient := createFakeClient(&istioCr, istiod) + fakeClient := createFakeClient(istioCr, istiod) statusHandler := status.NewStatusHandler(fakeClient) - sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(&istioCr, istiod), - &MergerMock{"1.16.1-distroless"}, proxyResetter, statusHandler) + sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(istioCr, istiod), + &MergerMock{"1.16.1-distroless"}, proxyRestarter, statusHandler) // when - err, requeue := sidecarsRestarter.Restart(context.Background(), &istioCr) + err, requeue := sidecarsRestarter.Restart(context.Background(), istioCr) // then Expect(err).Should(HaveOccurred()) @@ -173,27 +147,16 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { It("should succeed proxy reset when there is no warning or errors", func() { // given - numTrustedProxies := 1 - istioCr := operatorv1alpha2.Istio{ObjectMeta: metav1.ObjectMeta{ - Name: "default", - ResourceVersion: "1", - Annotations: map[string]string{}, - }, - Spec: operatorv1alpha2.IstioSpec{ - Config: operatorv1alpha2.Config{ - NumTrustedProxies: &numTrustedProxies, - }, - }, - } + istioCr := createIstioCR() istiod := createPod("istiod", gatherer.IstioNamespace, "discovery", "1.16.1") - proxyResetter := &proxyResetterMock{} - fakeClient := createFakeClient(&istioCr, istiod) + proxyRestarter := &proxyRestarterMock{} + fakeClient := createFakeClient(istioCr, istiod) statusHandler := status.NewStatusHandler(fakeClient) - sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(&istioCr, istiod), - &MergerMock{"1.16.1-distroless"}, proxyResetter, statusHandler) + sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(istioCr, istiod), + &MergerMock{"1.16.1-distroless"}, proxyRestarter, statusHandler) // when - err, requeue := sidecarsRestarter.Restart(context.Background(), &istioCr) + err, requeue := sidecarsRestarter.Restart(context.Background(), istioCr) // then Expect(err).Should(Not(HaveOccurred())) @@ -204,31 +167,44 @@ var _ = Describe("SidecarsRestarter reconciliation", func() { Expect((*istioCr.Status.Conditions)[0].Status).To(Equal(metav1.ConditionTrue)) }) + It("should return error when proxy reset fails", func() { + // given + istioCr := createIstioCR() + istiod := createPod("istiod", gatherer.IstioNamespace, "discovery", "1.16.1") + proxyRestarter := &proxyRestarterMock{err: errors.New("intentional error")} + fakeClient := createFakeClient(istioCr, istiod) + statusHandler := status.NewStatusHandler(fakeClient) + sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(istioCr, istiod), + &MergerMock{"1.16.1-distroless"}, proxyRestarter, statusHandler) + + // when + err, requeue := sidecarsRestarter.Restart(context.Background(), istioCr) + + // then + Expect(err).Should(HaveOccurred()) + Expect(err.Level()).To(Equal(described_errors.Error)) + Expect(err.Description()).To(Equal("Error occurred during reconciliation of Istio Sidecars: intentional error")) + Expect(requeue).To(BeFalse()) + Expect((*istioCr.Status.Conditions)[0].Type).To(Equal(string(operatorv1alpha2.ConditionTypeProxySidecarRestartSucceeded))) + Expect((*istioCr.Status.Conditions)[0].Reason).To(Equal(string(operatorv1alpha2.ConditionReasonProxySidecarRestartFailed))) + Expect((*istioCr.Status.Conditions)[0].Message).To(Equal(operatorv1alpha2.ConditionReasonProxySidecarRestartFailedMessage)) + Expect((*istioCr.Status.Conditions)[0].Status).To(Equal(metav1.ConditionFalse)) + }) + It("should succeed proxy reset even if not all proxies are reset and requeue is required", func() { // given - numTrustedProxies := 1 - istioCr := operatorv1alpha2.Istio{ObjectMeta: metav1.ObjectMeta{ - Name: "default", - ResourceVersion: "1", - Annotations: map[string]string{}, - }, - Spec: operatorv1alpha2.IstioSpec{ - Config: operatorv1alpha2.Config{ - NumTrustedProxies: &numTrustedProxies, - }, - }, - } + istioCr := createIstioCR() istiod := createPod("istiod", gatherer.IstioNamespace, "discovery", "1.16.1") - proxyResetter := &proxyResetterMock{ + proxyRestarter := &proxyRestarterMock{ hasMorePods: true, } - fakeClient := createFakeClient(&istioCr, istiod) + fakeClient := createFakeClient(istioCr, istiod) statusHandler := status.NewStatusHandler(fakeClient) - sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(&istioCr, istiod), - &MergerMock{"1.16.1-distroless"}, proxyResetter, statusHandler) + sidecarsRestarter := restarter.NewSidecarsRestarter(logr.Discard(), createFakeClient(istioCr, istiod), + &MergerMock{"1.16.1-distroless"}, proxyRestarter, statusHandler) // when - err, requeue := sidecarsRestarter.Restart(context.Background(), &istioCr) + err, requeue := sidecarsRestarter.Restart(context.Background(), istioCr) // then Expect(err).ToNot(HaveOccurred()) @@ -275,6 +251,21 @@ func createPod(name, namespace, containerName, imageVersion string) *corev1.Pod } } +func createIstioCR() *operatorv1alpha2.Istio { + numTrustedProxies := 1 + return &operatorv1alpha2.Istio{ObjectMeta: metav1.ObjectMeta{ + Name: "default", + ResourceVersion: "1", + Annotations: map[string]string{}, + }, + Spec: operatorv1alpha2.IstioSpec{ + Config: operatorv1alpha2.Config{ + NumTrustedProxies: &numTrustedProxies, + }, + }, + } +} + type MergerMock struct { tag string } @@ -299,12 +290,16 @@ func (m MergerMock) GetIstioImageVersion() (istiooperator.IstioImageVersion, err func (m MergerMock) SetIstioInstallFlavor(_ clusterconfig.ClusterSize) {} -type proxyResetterMock struct { +type proxyRestarterMock struct { restartWarnings []restart.RestartWarning hasMorePods bool err error } -func (p *proxyResetterMock) ProxyReset(_ context.Context, _ client.Client, _ pods.SidecarImage, _ v1.ResourceRequirements, _ []filter.SidecarProxyPredicate, _ *logr.Logger) ([]restart.RestartWarning, bool, error) { +func (p *proxyRestarterMock) RestartProxies(_ context.Context, _ predicates.SidecarImage, _ v1.ResourceRequirements, _ *v1alpha2.Istio) ([]restart.RestartWarning, bool, error) { + return p.restartWarnings, p.hasMorePods, p.err +} + +func (p *proxyRestarterMock) RestartWithPredicates(_ context.Context, preds []predicates.SidecarProxyPredicate, _ *pods.PodsRestartLimits, _ bool) ([]restart.RestartWarning, bool, error) { return p.restartWarnings, p.hasMorePods, p.err } diff --git a/pkg/lib/sidecars/pods/filter_test.go b/pkg/lib/sidecars/pods/filter_test.go deleted file mode 100644 index f7fb33d30..000000000 --- a/pkg/lib/sidecars/pods/filter_test.go +++ /dev/null @@ -1,66 +0,0 @@ -package pods_test - -import ( - "github.com/kyma-project/istio/operator/pkg/lib/sidecars/pods" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - v1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -var _ = Describe("RequiresProxyRestart", func() { - It("should should return false when pod has custom image annotation", func() { - // given - pod := createPodWithProxySidecar("test-pod", "test-namespace", "1.21.0", map[string]string{"sidecar.istio.io/proxyImage": "istio/proxyv2:1.21.0"}) - predicate := pods.NewRestartProxyPredicate(pods.NewSidecarImage("istio", "1.22.0"), v1.ResourceRequirements{}) - - // when - shouldRestart := predicate.RequiresProxyRestart(pod) - - // then - Expect(shouldRestart).To(BeFalse()) - }) - - It("should should return true when pod does not have custom image annotation", func() { - // given - pod := createPodWithProxySidecar("test-pod", "test-namespace", "1.21.0", map[string]string{}) - predicate := pods.NewRestartProxyPredicate(pods.NewSidecarImage("istio", "1.22.0"), v1.ResourceRequirements{}) - - // when - shouldRestart := predicate.RequiresProxyRestart(pod) - - // then - Expect(shouldRestart).To(BeTrue()) - }) -}) - -func createPodWithProxySidecar(name, namespace, proxyIstioVersion string, annotations map[string]string) v1.Pod { - if annotations == nil { - annotations = map[string]string{} - } - annotations["sidecar.istio.io/status"] = "true" - return v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - Annotations: annotations, - }, - Status: v1.PodStatus{ - Phase: v1.PodRunning, - Conditions: []v1.PodCondition{ - { - Type: v1.PodReady, - Status: v1.ConditionTrue, - }, - }, - }, - Spec: v1.PodSpec{ - Containers: []v1.Container{ - { - Name: "istio-proxy", - Image: "istio/proxyv2:" + proxyIstioVersion, - }, - }, - }, - } -} diff --git a/pkg/lib/sidecars/pods/get.go b/pkg/lib/sidecars/pods/get.go index f7a8e3a83..e6a331b4e 100644 --- a/pkg/lib/sidecars/pods/get.go +++ b/pkg/lib/sidecars/pods/get.go @@ -2,10 +2,9 @@ package pods import ( "context" - "fmt" "github.com/go-logr/logr" - "github.com/kyma-project/istio/operator/internal/filter" + "github.com/kyma-project/istio/operator/internal/restarter/predicates" "github.com/kyma-project/istio/operator/pkg/lib/sidecars/retry" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/fields" @@ -16,36 +15,95 @@ const ( istioSidecarContainerName string = "istio-proxy" ) -type SidecarImage struct { - Repository string - Tag string +type PodsRestartLimits struct { + PodsToRestartLimit int + PodsToListLimit int } -func NewSidecarImage(hub, tag string) SidecarImage { - return SidecarImage{ - Repository: fmt.Sprintf("%s/proxyv2", hub), - Tag: tag, +func NewPodsRestartLimits(restartLimit, listLimit int) *PodsRestartLimits { + return &PodsRestartLimits{ + PodsToRestartLimit: restartLimit, + PodsToListLimit: listLimit, } } -func (r SidecarImage) String() string { - return fmt.Sprintf("%s:%s", r.Repository, r.Tag) +type PodsGetter interface { + GetPodsToRestart(ctx context.Context, preds []predicates.SidecarProxyPredicate, limits *PodsRestartLimits) (*v1.PodList, error) + GetAllInjectedPods(context context.Context) (*v1.PodList, error) } -func (r SidecarImage) matchesImageIn(container v1.Container) bool { - return container.Image == r.String() +type Pods struct { + k8sClient client.Client + logger *logr.Logger } -type PodsRestartLimits struct { - podsToRestartLimit int - podsToListLimit int +func NewPods(k8sClient client.Client, logger *logr.Logger) *Pods { + return &Pods{ + k8sClient: k8sClient, + logger: logger, + } } -func NewPodsRestartLimits(restartLimit, listLimit int) *PodsRestartLimits { - return &PodsRestartLimits{ - podsToRestartLimit: restartLimit, - podsToListLimit: listLimit, +func (p *Pods) GetPodsToRestart(ctx context.Context, preds []predicates.SidecarProxyPredicate, limits *PodsRestartLimits) (*v1.PodList, error) { + podsToRestart := &v1.PodList{} + for while := true; while; { + podsWithSidecar, err := getSidecarPods(ctx, p.k8sClient, p.logger, limits.PodsToListLimit, podsToRestart.Continue) + if err != nil { + return nil, err + } + for _, pod := range podsWithSidecar.Items { + optionalMatched := false + requiredMatched := true + for _, predicate := range preds { + matched := predicate.Matches(pod) + if predicate.MustMatch() { // if predicate must match, all must match + if !matched { + requiredMatched = false + break + } + } else if !optionalMatched && matched { // if predicate is optional, at least one must match + optionalMatched = true + } + } + if requiredMatched && optionalMatched { + podsToRestart.Items = append(podsToRestart.Items, pod) + } + if len(podsToRestart.Items) >= limits.PodsToRestartLimit { + break + } + } + podsToRestart.Continue = podsWithSidecar.Continue + while = len(podsToRestart.Items) < limits.PodsToRestartLimit && podsToRestart.Continue != "" + } + + if len(podsToRestart.Items) > 0 { + p.logger.Info("Pods to restart", "number of pods", len(podsToRestart.Items), "has more pods", podsToRestart.Continue != "") + } else { + p.logger.Info("No pods to restart with matching predicates") + } + + return podsToRestart, nil +} + +func (p *Pods) GetAllInjectedPods(ctx context.Context) (outputPodList *v1.PodList, err error) { + podList := &v1.PodList{} + outputPodList = &v1.PodList{} + outputPodList.Items = make([]v1.Pod, len(podList.Items)) + + err = retry.RetryOnError(retry.DefaultRetry, func() error { + return p.k8sClient.List(ctx, podList, &client.ListOptions{}) + }) + if err != nil { + return podList, err + } + + for _, pod := range podList.Items { + if containsSidecar(pod) { + outputPodList.Items = append(outputPodList.Items, pod) + } } + + return outputPodList, nil } func listRunningPods(ctx context.Context, c client.Client, listLimit int, continueToken string) (*v1.PodList, error) { @@ -77,7 +135,7 @@ func getSidecarPods(ctx context.Context, c client.Client, logger *logr.Logger, l podsWithSidecar.Continue = podList.Continue for _, pod := range podList.Items { - if isReadyWithIstioAnnotation(pod) { + if predicates.IsReadyWithIstioAnnotation(pod) { podsWithSidecar.Items = append(podsWithSidecar.Items, pod) } } @@ -86,38 +144,6 @@ func getSidecarPods(ctx context.Context, c client.Client, logger *logr.Logger, l return podsWithSidecar, nil } -func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, limits *PodsRestartLimits, logger *logr.Logger) (*v1.PodList, error) { - //Add predicate for image version and resources configuration - predicates = append(predicates, NewRestartProxyPredicate(expectedImage, expectedResources)) - - podsToRestart := &v1.PodList{} - for while := true; while; { - podsWithSidecar, err := getSidecarPods(ctx, c, logger, limits.podsToListLimit, podsToRestart.Continue) - if err != nil { - return nil, err - } - for _, pod := range podsWithSidecar.Items { - for _, predicate := range predicates { - if predicate.RequiresProxyRestart(pod) { - podsToRestart.Items = append(podsToRestart.Items, pod) - break - } - } - if len(podsToRestart.Items) >= limits.podsToRestartLimit { - break - } - } - podsToRestart.Continue = podsWithSidecar.Continue - while = len(podsToRestart.Items) < limits.podsToRestartLimit && podsToRestart.Continue != "" - } - - if len(podsToRestart.Items) > 0 { - logger.Info("Pods to restart", "number of pods", len(podsToRestart.Items), "has more pods", podsToRestart.Continue != "") - } - - return podsToRestart, nil -} - func containsSidecar(pod v1.Pod) bool { // If the pod has one container it is not injected // This skips IngressGateway and EgressGateway pods, as those only have istio-proxy @@ -131,24 +157,3 @@ func containsSidecar(pod v1.Pod) bool { } return false } - -func GetAllInjectedPods(ctx context.Context, k8sclient client.Client) (outputPodList *v1.PodList, err error) { - podList := &v1.PodList{} - outputPodList = &v1.PodList{} - outputPodList.Items = make([]v1.Pod, len(podList.Items)) - - err = retry.RetryOnError(retry.DefaultRetry, func() error { - return k8sclient.List(ctx, podList, &client.ListOptions{}) - }) - if err != nil { - return podList, err - } - - for _, pod := range podList.Items { - if containsSidecar(pod) { - outputPodList.Items = append(outputPodList.Items, pod) - } - } - - return outputPodList, nil -} diff --git a/pkg/lib/sidecars/pods/get_test.go b/pkg/lib/sidecars/pods/get_test.go index 7db6a8aef..0f83e71f8 100644 --- a/pkg/lib/sidecars/pods/get_test.go +++ b/pkg/lib/sidecars/pods/get_test.go @@ -6,7 +6,7 @@ import ( "testing" "time" - "github.com/kyma-project/istio/operator/internal/filter" + "github.com/kyma-project/istio/operator/internal/restarter/predicates" "github.com/kyma-project/istio/operator/internal/tests" . "github.com/onsi/ginkgo/v2" @@ -15,12 +15,12 @@ import ( "github.com/go-logr/logr" "github.com/kyma-project/istio/operator/api/v1alpha2" - "github.com/kyma-project/istio/operator/pkg/lib/sidecars/pods" v1 "k8s.io/api/core/v1" "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "github.com/kyma-project/istio/operator/pkg/lib/sidecars/pods" "github.com/kyma-project/istio/operator/pkg/lib/sidecars/test/helpers" ) @@ -37,12 +37,12 @@ var _ = Describe("GetPodsToRestart", func() { ctx := context.Background() logger := logr.Discard() - When("Istio image changed", func() { - expectedImage := pods.NewSidecarImage("istio", "1.10.0") + When("Image changed", func() { + expectedImage := predicates.NewSidecarImage("istio", "1.10.0") tests := []struct { name string c client.Client - predicates []filter.SidecarProxyPredicate + predicates []predicates.SidecarProxyPredicate limits *pods.PodsRestartLimits assertFunc func(podList *v1.PodList) }{ @@ -147,19 +147,69 @@ var _ = Describe("GetPodsToRestart", func() { }, }, { - name: "Should contain only one pod when there are multiple predicates that would restart the pod", + name: "Should contain only one pod when there are multiple predicates that match the pod", c: createClientSet( helpers.NewSidecarPodBuilder(). SetName("changedSidecarPod"). SetSidecarImageRepository("istio/different-proxy"). Build(), ), - limits: pods.NewPodsRestartLimits(5, 5), - predicates: []filter.SidecarProxyPredicate{pods.NewRestartProxyPredicate(expectedImage, helpers.DefaultSidecarResources)}, + limits: pods.NewPodsRestartLimits(5, 5), + predicates: []predicates.SidecarProxyPredicate{ + predicates.NewImageResourcesPredicate(expectedImage, helpers.DifferentSidecarResources), + }, assertFunc: func(podList *v1.PodList) { Expect(podList.Items).To(HaveLen(1)) }, }, + { + name: "Should contain only one pod when there are must match predicates that do match the pod", + c: createClientSet( + helpers.NewSidecarPodBuilder(). + SetName("changedSidecarPod"). + SetSidecarImageRepository("istio/different-proxy"). + Build(), + ), + limits: pods.NewPodsRestartLimits(5, 5), + predicates: []predicates.SidecarProxyPredicate{ + predicates.NewImageResourcesPredicate(expectedImage, helpers.DifferentSidecarResources), + predicates.CustomerWorkloadRestartPredicate{}, + }, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(HaveLen(1)) + }, + }, + { + name: "Should ignore the pod when there are must match predicates that do not match the pod", + c: createClientSet( + helpers.NewSidecarPodBuilder(). + SetName("changedSidecarPod"). + SetSidecarImageRepository("istio/different-proxy"). + Build(), + ), + limits: pods.NewPodsRestartLimits(5, 5), + predicates: []predicates.SidecarProxyPredicate{ + predicates.NewImageResourcesPredicate(expectedImage, helpers.DifferentSidecarResources), + predicates.KymaWorkloadRestartPredicate{}, + }, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, + }, + { + name: "Should ignore the pod when there are must match predicate that matches pod but other predicate do not", + c: createClientSet( + helpers.NewSidecarPodBuilder().Build(), + ), + limits: pods.NewPodsRestartLimits(5, 5), + predicates: []predicates.SidecarProxyPredicate{ + predicates.NewImageResourcesPredicate(expectedImage, helpers.DefaultSidecarResources), + predicates.CustomerWorkloadRestartPredicate{}, + }, + assertFunc: func(podList *v1.PodList) { + Expect(podList.Items).To(BeEmpty()) + }, + }, { name: "Should respect limit set when getting pods to restart if all pods listed", c: NewFakeClientWithLimit( @@ -224,14 +274,16 @@ var _ = Describe("GetPodsToRestart", func() { } for _, tt := range tests { It(tt.name, func() { - podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, tt.predicates, tt.limits, &logger) + tt.predicates = append(tt.predicates, predicates.NewImageResourcesPredicate(expectedImage, helpers.DefaultSidecarResources)) + podsLister := pods.NewPods(tt.c, &logger) + podList, err := podsLister.GetPodsToRestart(ctx, tt.predicates, tt.limits) Expect(err).NotTo(HaveOccurred()) tt.assertFunc(podList) }) } }) - When("Sidecar Resources changed", func() { + When("Resources changed", func() { tests := []struct { name string c client.Client @@ -301,8 +353,9 @@ var _ = Describe("GetPodsToRestart", func() { } for _, tt := range tests { It(tt.name, func() { - expectedImage := pods.NewSidecarImage("istio", "1.10.0") - podList, err := pods.GetPodsToRestart(ctx, tt.c, expectedImage, helpers.DefaultSidecarResources, []filter.SidecarProxyPredicate{}, pods.NewPodsRestartLimits(5, 5), &logger) + expectedImage := predicates.NewSidecarImage("istio", "1.10.0") + podsLister := pods.NewPods(tt.c, &logger) + podList, err := podsLister.GetPodsToRestart(ctx, []predicates.SidecarProxyPredicate{predicates.NewImageResourcesPredicate(expectedImage, helpers.DefaultSidecarResources)}, pods.NewPodsRestartLimits(5, 5)) Expect(err).NotTo(HaveOccurred()) tt.assertFunc(podList) }) @@ -312,6 +365,7 @@ var _ = Describe("GetPodsToRestart", func() { var _ = Describe("GetAllInjectedPods", func() { ctx := context.Background() + logger := logr.Discard() tests := []struct { name string @@ -340,7 +394,8 @@ var _ = Describe("GetAllInjectedPods", func() { } for _, tt := range tests { It(tt.name, func() { - podList, err := pods.GetAllInjectedPods(ctx, tt.c) + podsLister := pods.NewPods(tt.c, &logger) + podList, err := podsLister.GetAllInjectedPods(ctx) Expect(err).NotTo(HaveOccurred()) tt.assertFunc(podList) }) diff --git a/pkg/lib/sidecars/proxy.go b/pkg/lib/sidecars/proxy.go index 8e6315aae..931e9dea2 100644 --- a/pkg/lib/sidecars/proxy.go +++ b/pkg/lib/sidecars/proxy.go @@ -2,8 +2,13 @@ package sidecars import ( "context" + "errors" + "fmt" + "math" + "strings" - "github.com/kyma-project/istio/operator/internal/filter" + "github.com/kyma-project/istio/operator/api/v1alpha2" + "github.com/kyma-project/istio/operator/internal/restarter/predicates" "github.com/go-logr/logr" "github.com/kyma-project/istio/operator/pkg/lib/sidecars/pods" @@ -17,36 +22,132 @@ const ( podsToListLimit = 100 ) -type ProxyResetter interface { - ProxyReset(ctx context.Context, c client.Client, expectedImage pods.SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) ([]restart.RestartWarning, bool, error) +type ProxyRestarter interface { + RestartProxies(ctx context.Context, expectedImage predicates.SidecarImage, expectedResources v1.ResourceRequirements, istioCR *v1alpha2.Istio) ([]restart.RestartWarning, bool, error) + RestartWithPredicates(ctx context.Context, preds []predicates.SidecarProxyPredicate, limits *pods.PodsRestartLimits, failOnError bool) ([]restart.RestartWarning, bool, error) } -type ProxyReset struct { +type ProxyRestart struct { + k8sClient client.Client + podsLister pods.PodsGetter + actionRestarter restart.ActionRestarter + logger *logr.Logger } -func NewProxyResetter() *ProxyReset { - return &ProxyReset{} +func NewProxyRestarter(c client.Client, podsLister pods.PodsGetter, actionRestarter restart.ActionRestarter, logger *logr.Logger) *ProxyRestart { + return &ProxyRestart{ + k8sClient: c, + podsLister: podsLister, + actionRestarter: actionRestarter, + logger: logger, + } } -func (p *ProxyReset) ProxyReset(ctx context.Context, c client.Client, expectedImage pods.SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) ([]restart.RestartWarning, bool, error) { - limits := pods.NewPodsRestartLimits(podsToRestartLimit, podsToListLimit) - podsToRestart, err := pods.GetPodsToRestart(ctx, c, expectedImage, expectedResources, predicates, limits, logger) +func (p *ProxyRestart) RestartProxies(ctx context.Context, expectedImage predicates.SidecarImage, expectedResources v1.ResourceRequirements, istioCR *v1alpha2.Istio) ([]restart.RestartWarning, bool, error) { + compatibiltyPredicate, err := predicates.NewCompatibilityRestartPredicate(istioCR) + if err != nil { + p.logger.Error(err, "Failed to create restart compatibility predicate") + return []restart.RestartWarning{}, false, err + } + + predicates := []predicates.SidecarProxyPredicate{compatibiltyPredicate, + predicates.NewImageResourcesPredicate(expectedImage, expectedResources), + } + + err = p.restartKymaProxies(ctx, predicates) + if err != nil { + p.logger.Error(err, "Failed to restart Kyma proxies") + return []restart.RestartWarning{}, false, err + } + + warnings, hasMorePodsToRestart, err := p.restartCustomerProxies(ctx, predicates) + if err != nil { + p.logger.Error(err, "failed to restart Customer proxies") + warnings = []restart.RestartWarning{ // errors on Customer proxies are considered as a warning + { + Name: "n/a", + Namespace: "n/a", + Kind: "n/a", + Message: "failed to restart Customer proxies", + }, + } + } + + return warnings, hasMorePodsToRestart, nil +} + +func (p *ProxyRestart) RestartWithPredicates(ctx context.Context, preds []predicates.SidecarProxyPredicate, limits *pods.PodsRestartLimits, failOnError bool) ([]restart.RestartWarning, bool, error) { + podsToRestart, err := p.podsLister.GetPodsToRestart(ctx, preds, limits) if err != nil { - return nil, false, err + p.logger.Error(err, "Getting pods to restart failed") + return []restart.RestartWarning{}, false, err + } + + warnings, err := p.actionRestarter.Restart(ctx, podsToRestart, failOnError) + if err != nil { + p.logger.Error(err, "Restarting pods failed") + return warnings, false, err } // if there are more pods to restart there should be a continue token in the pod list - hasMorePodsToRestart := podsToRestart.Continue != "" + return warnings, podsToRestart.Continue != "", nil +} + +func (p *ProxyRestart) restartKymaProxies(ctx context.Context, preds []predicates.SidecarProxyPredicate) error { + preds = append(preds, predicates.NewKymaWorkloadRestartPredicate()) + limits := pods.NewPodsRestartLimits(math.MaxInt, math.MaxInt) + + warnings, _, err := p.RestartWithPredicates(ctx, preds, limits, true) + if err != nil { + p.logger.Error(err, "Failed to restart Kyma proxies") + return err + } + warningMessage := BuildWarningMessage(warnings, p.logger) + if warningMessage != "" { + err := errors.New(warningMessage) + p.logger.Error(err, "Failed to restart Kyma proxies") + return err + } + + p.logger.Info("Kyma proxy restart completed") + return nil +} + +func BuildWarningMessage(warnings []restart.RestartWarning, logger *logr.Logger) string { + warningMessage := "" + warningsCount := len(warnings) + if warningsCount > 0 { + podsLimit := 5 + pods := []string{} + for _, w := range warnings { + if podsLimit--; podsLimit >= 0 { + pods = append(pods, fmt.Sprintf("%s/%s", w.Namespace, w.Name)) + } + logger.Info("Proxy reset failed:", "name", w.Name, "namespace", w.Namespace, "kind", w.Kind, "message", w.Message) + } + warningMessage = fmt.Sprintf("The sidecars of the following workloads could not be restarted: %s", + strings.Join(pods, ", ")) + if warningsCount-len(pods) > 0 { + warningMessage += fmt.Sprintf(" and %d additional workload(s)", warningsCount-len(pods)) + } + } + return warningMessage +} + +func (p *ProxyRestart) restartCustomerProxies(ctx context.Context, preds []predicates.SidecarProxyPredicate) ([]restart.RestartWarning, bool, error) { + preds = append(preds, predicates.NewCustomerWorkloadRestartPredicate()) + limits := pods.NewPodsRestartLimits(podsToRestartLimit, podsToListLimit) - warnings, err := restart.Restart(ctx, c, podsToRestart, logger) + warnings, hasMorePodsToRestart, err := p.RestartWithPredicates(ctx, preds, limits, false) if err != nil { - return nil, false, err + p.logger.Error(err, "Failed to restart Customer proxies") + return warnings, false, err } if !hasMorePodsToRestart { - logger.Info("Proxy reset completed") + p.logger.Info("Customer proxy restart completed") } else { - logger.Info("Proxy reset only partially completed") + p.logger.Info("Customer proxy restart only partially completed") } return warnings, hasMorePodsToRestart, nil diff --git a/pkg/lib/sidecars/proxy_test.go b/pkg/lib/sidecars/proxy_test.go new file mode 100644 index 000000000..446670afc --- /dev/null +++ b/pkg/lib/sidecars/proxy_test.go @@ -0,0 +1,571 @@ +package sidecars_test + +import ( + "context" + "errors" + "math" + "testing" + + "github.com/go-logr/logr" + "github.com/kyma-project/istio/operator/internal/restarter/predicates" + "github.com/kyma-project/istio/operator/internal/tests" + "github.com/kyma-project/istio/operator/pkg/labels" + "github.com/kyma-project/istio/operator/pkg/lib/sidecars" + "github.com/kyma-project/istio/operator/pkg/lib/sidecars/pods" + "github.com/kyma-project/istio/operator/pkg/lib/sidecars/restart" + "github.com/kyma-project/istio/operator/pkg/lib/sidecars/test/helpers" + . "github.com/onsi/ginkgo/v2" + ginkgotypes "github.com/onsi/ginkgo/v2/types" + . "github.com/onsi/gomega" + + appsv1 "k8s.io/api/apps/v1" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestRestartProxies(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Proxy Restart Suite") +} + +var _ = ReportAfterSuite("custom reporter", func(report ginkgotypes.Report) { + tests.GenerateGinkgoJunitReport("proxy-restart-suite", report) +}) + +var _ = Describe("RestartProxies", func() { + ctx := context.Background() + logger := logr.Discard() + + It("should succeed without errors or warnings", func() { + // given + pod := getPod("test-pods", "test-namespace", "podOwner", "ReplicaSet") + rsOwner := getReplicaSet("podOwner", "test-namespace", "rsOwner", "ReplicaSet") + rsOwnerRS := getReplicaSet("rsOwner", "test-namespace", "base", "ReplicaSet") + + c := fakeClient(pod, rsOwner, rsOwnerRS) + + // when + podsLister := pods.NewPods(c, &logger) + expectedImage := predicates.NewSidecarImage("istio", "1.1.0") + istioCR := helpers.GetIstioCR(expectedImage.Tag) + actionRestarter := restart.NewActionRestarter(c, &logger) + proxyRestarter := sidecars.NewProxyRestarter(c, podsLister, actionRestarter, &logger) + warnings, hasMorePods, err := proxyRestarter.RestartProxies(ctx, expectedImage, helpers.DefaultSidecarResources, &istioCR) + + // then + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + Expect(hasMorePods).To(BeFalse()) + + err = c.Get(ctx, client.ObjectKey{Name: rsOwnerRS.Name, Namespace: rsOwnerRS.Namespace}, rsOwnerRS) + Expect(err).NotTo(HaveOccurred()) + Expect(rsOwnerRS.Spec.Template.Annotations).To(HaveKey("istio-operator.kyma-project.io/restartedAt")) + }) + + It("should call restart proxies with respective predicates", func() { + // given + c := fakeClient() + + // when + failClient := &shouldFailClient{c, false, true} + + podsListerMock := NewPodsMock() + expectedImage := predicates.NewSidecarImage("istio", "1.1.0") + istioCR := helpers.GetIstioCR(expectedImage.Tag) + actionRestarter := restart.NewActionRestarter(failClient, &logger) + proxyRestarter := sidecars.NewProxyRestarter(failClient, podsListerMock, actionRestarter, &logger) + warnings, hasMorePods, err := proxyRestarter.RestartProxies(ctx, expectedImage, helpers.DefaultSidecarResources, &istioCR) + + // then + Expect(err).ToNot(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + Expect(hasMorePods).To(BeFalse()) + + Expect(podsListerMock.Called).To(Equal(2)) + + Expect(podsListerMock.Predicates).To(HaveLen(2)) + Expect(podsListerMock.Predicates[0]).To(HaveLen(3)) + Expect(podsListerMock.Predicates[0][0]).To(BeAssignableToTypeOf(&predicates.CompatibilityRestartPredicate{})) + Expect(podsListerMock.Predicates[0][1]).To(BeAssignableToTypeOf(&predicates.ImageResourcesPredicate{})) + Expect(podsListerMock.Predicates[0][2]).To(BeAssignableToTypeOf(&predicates.KymaWorkloadRestartPredicate{})) + Expect(podsListerMock.Predicates[1]).To(HaveLen(3)) + Expect(podsListerMock.Predicates[0][0]).To(BeAssignableToTypeOf(&predicates.CompatibilityRestartPredicate{})) + Expect(podsListerMock.Predicates[1][1]).To(BeAssignableToTypeOf(&predicates.ImageResourcesPredicate{})) + Expect(podsListerMock.Predicates[1][2]).To(BeAssignableToTypeOf(&predicates.CustomerWorkloadRestartPredicate{})) + + Expect(podsListerMock.Limits).To(HaveLen(2)) + Expect(podsListerMock.Limits[0].PodsToRestartLimit).To(Equal(math.MaxInt)) + Expect(podsListerMock.Limits[0].PodsToListLimit).To(Equal(math.MaxInt)) + Expect(podsListerMock.Limits[1].PodsToRestartLimit).To(Equal(30)) + Expect(podsListerMock.Limits[1].PodsToListLimit).To(Equal(100)) + }) + + It("should return error if compatibility predicate creation fails", func() { + // given + c := fakeClient() + podsListerMock := NewPodsMock() + expectedImage := predicates.NewSidecarImage("istio", "1.1.0") + istioCR := helpers.GetIstioCR(expectedImage.Tag) + istioCR.Annotations[labels.LastAppliedConfiguration] = "invalid-last-applied-configuration" // This should cause the compatibility predicate to fail + actionRestarter := restart.NewActionRestarter(c, &logger) + proxyRestarter := sidecars.NewProxyRestarter(c, podsListerMock, actionRestarter, &logger) + + // when + warnings, hasMorePods, err := proxyRestarter.RestartProxies(ctx, expectedImage, helpers.DefaultSidecarResources, &istioCR) + + // then + Expect(err).To(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + Expect(hasMorePods).To(BeFalse()) + Expect(err.Error()).To(ContainSubstring("invalid character")) + }) + + It("should return error if restarting Kyma proxies fails", func() { + // given + c := fakeClient() + podsListerMock := NewPodsMock() + podsListerMock.FailOnKymaWorkload = true + expectedImage := predicates.NewSidecarImage("istio", "1.1.0") + istioCR := helpers.GetIstioCR(expectedImage.Tag) + actionRestarter := restart.NewActionRestarter(c, &logger) + proxyRestarter := sidecars.NewProxyRestarter(c, podsListerMock, actionRestarter, &logger) + + // when + warnings, hasMorePods, err := proxyRestarter.RestartProxies(ctx, expectedImage, helpers.DefaultSidecarResources, &istioCR) + + // then + Expect(err).To(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + Expect(hasMorePods).To(BeFalse()) + Expect(err.Error()).To(ContainSubstring("intentionally failed on Kyma workload predicate")) + }) + + It("should not return error if restarting Customer proxies fails", func() { + // given + c := fakeClient() + + podsListerMock := NewPodsMock() + podsListerMock.FailOnCustomerWorkload = true + expectedImage := predicates.NewSidecarImage("istio", "1.1.0") + istioCR := helpers.GetIstioCR(expectedImage.Tag) + actionRestarter := restart.NewActionRestarter(c, &logger) + proxyRestarter := sidecars.NewProxyRestarter(c, podsListerMock, actionRestarter, &logger) + + // when + warnings, hasMorePods, err := proxyRestarter.RestartProxies(ctx, expectedImage, helpers.DefaultSidecarResources, &istioCR) + + // then + Expect(err).ToNot(HaveOccurred()) + Expect(warnings).To(ContainElement(restart.RestartWarning{ + Name: "n/a", + Namespace: "n/a", + Kind: "n/a", + Message: "failed to restart Customer proxies", + })) + Expect(hasMorePods).To(BeFalse()) + }) + + It("should return error if restarting Kyma pods have warnings", func() { + // given + pod := getPod("test-pod", "kyma-system", "podOwner", "ReplicaSet") + rsOwner := getReplicaSet("podOwner", "kyma-system", "rsOwner", "ReplicaSet") + rsOwnerRS := getReplicaSet("rsOwner", "kyma-system", "base", "ReplicaSet") + c := fakeClient(pod, rsOwner, rsOwnerRS) + + // when + podsLister := pods.NewPods(c, &logger) + expectedImage := predicates.NewSidecarImage("istio", "1.1.0") + istioCR := helpers.GetIstioCR(expectedImage.Tag) + actionRestarter := NewActionRestartMock([]restart.RestartWarning{{Name: "test-pod", Namespace: "kyma-system", Kind: "Pod", Message: "failed to restart"}}, nil) + proxyRestarter := sidecars.NewProxyRestarter(c, podsLister, actionRestarter, &logger) + warnings, hasMorePods, err := proxyRestarter.RestartProxies(ctx, expectedImage, helpers.DefaultSidecarResources, &istioCR) + + // then + Expect(err).To(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + Expect(hasMorePods).To(BeFalse()) + + Expect(err.Error()).To(Equal("The sidecars of the following workloads could not be restarted: kyma-system/test-pod")) + }) + + It("should not return error but a warning when it fails on restart customer proxies", func() { + // given + pod := getPod("test-pods", "test-namespace", "podOwner", "ReplicaSet") + rsOwner := getReplicaSet("podOwner", "test-namespace", "rsOwner", "ReplicaSet") + rsOwnerRS := getReplicaSet("rsOwner", "test-namespace", "base", "ReplicaSet") + + c := fakeClient(pod, rsOwner, rsOwnerRS) + + // when + failClient := &shouldFailClient{c, false, true} + + podsLister := pods.NewPods(c, &logger) + expectedImage := predicates.NewSidecarImage("istio", "1.1.0") + istioCR := helpers.GetIstioCR(expectedImage.Tag) + actionRestarter := restart.NewActionRestarter(failClient, &logger) + proxyRestarter := sidecars.NewProxyRestarter(failClient, podsLister, actionRestarter, &logger) + warnings, hasMorePods, err := proxyRestarter.RestartProxies(ctx, expectedImage, helpers.DefaultSidecarResources, &istioCR) + + // then + Expect(err).ToNot(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + Expect(hasMorePods).To(BeFalse()) + }) +}) + +var _ = Describe("RestartWithPredicates", func() { + ctx := context.Background() + logger := logr.Discard() + + It("should succeed without errors or warnings", func() { + // given + pod := getPod("test-pod", "test-namespace", "podOwner", "ReplicaSet") + rsOwner := getReplicaSet("podOwner", "test-namespace", "rsOwner", "ReplicaSet") + rsOwnerRS := getReplicaSet("rsOwner", "test-namespace", "base", "ReplicaSet") + + c := fakeClient(pod, rsOwner, rsOwnerRS) + preds := []predicates.SidecarProxyPredicate{ + predicates.NewImageResourcesPredicate(predicates.SidecarImage{Repository: "istio", Tag: "1.1.0"}, helpers.DefaultSidecarResources), + } + limits := pods.NewPodsRestartLimits(10, 10) + + // when + podsLister := pods.NewPods(c, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + proxyRestarter := sidecars.NewProxyRestarter(c, podsLister, actionRestarter, &logger) + warnings, hasMorePods, err := proxyRestarter.RestartWithPredicates(ctx, preds, limits, true) + + // then + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + Expect(hasMorePods).To(BeFalse()) + + err = c.Get(ctx, client.ObjectKey{Name: rsOwnerRS.Name, Namespace: rsOwnerRS.Namespace}, rsOwnerRS) + Expect(err).NotTo(HaveOccurred()) + Expect(rsOwnerRS.Spec.Template.Annotations).To(HaveKey("istio-operator.kyma-project.io/restartedAt")) + }) + + It("should return warning that pod not have OwnerReferences", func() { + // given + pod := getPod("test-pod", "test-namespace", "podOwner", "ReplicaSet") + c := fakeClient(pod) + + preds := []predicates.SidecarProxyPredicate{ + predicates.NewImageResourcesPredicate(predicates.SidecarImage{Repository: "istio", Tag: "1.1.0"}, helpers.DefaultSidecarResources), + } + limits := pods.NewPodsRestartLimits(2, 2) + + // when + podsLister := pods.NewPods(c, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + proxyRestarter := sidecars.NewProxyRestarter(c, podsLister, actionRestarter, &logger) + warnings, hasMorePods, err := proxyRestarter.RestartWithPredicates(ctx, preds, limits, true) + + // then + Expect(err).NotTo(HaveOccurred()) + Expect(warnings).ToNot(BeEmpty()) + Expect(hasMorePods).To(BeFalse()) + + Expect(warnings).To(HaveLen(1)) + Expect(warnings[0].Message).To(Equal("pod sidecar could not be updated because OwnerReferences was not found.")) + }) + + It("should return error if getting pods to restart fails", func() { + // given + preds := []predicates.SidecarProxyPredicate{ + predicates.NewImageResourcesPredicate(predicates.SidecarImage{Repository: "istio", Tag: "1.1.0"}, helpers.DefaultSidecarResources), + } + limits := pods.NewPodsRestartLimits(2, 2) + + // when + c := fakeClient() + failClient := &shouldFailClient{c, true, false} + + podsLister := pods.NewPods(failClient, &logger) + actionRestarter := restart.NewActionRestarter(failClient, &logger) + proxyRestarter := sidecars.NewProxyRestarter(failClient, podsLister, actionRestarter, &logger) + warnings, hasMorePods, err := proxyRestarter.RestartWithPredicates(ctx, preds, limits, true) + + // then + Expect(err).To(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + Expect(hasMorePods).To(BeFalse()) + + Expect(err.Error()).To(Equal("intentionally failing client on client.List")) + }) + + It("should return error if restarting pods fails", func() { + // given + pod := getPod("test-pod", "test-namespace", "podOwner", "ReplicaSet") + rsOwner := getReplicaSet("podOwner", "test-namespace", "rsOwner", "ReplicaSet") + rsOwnerRS := getReplicaSet("rsOwner", "test-namespace", "base", "ReplicaSet") + c := fakeClient(pod, rsOwner, rsOwnerRS) + + preds := []predicates.SidecarProxyPredicate{ + predicates.NewImageResourcesPredicate(predicates.SidecarImage{Repository: "istio", Tag: "1.1.0"}, helpers.DefaultSidecarResources), + } + limits := pods.NewPodsRestartLimits(2, 2) + + // when + failClient := &shouldFailClient{c, false, true} + + podsLister := pods.NewPods(failClient, &logger) + actionRestarter := restart.NewActionRestarter(failClient, &logger) + proxyRestarter := sidecars.NewProxyRestarter(failClient, podsLister, actionRestarter, &logger) + warnings, hasMorePods, err := proxyRestarter.RestartWithPredicates(ctx, preds, limits, true) + + // then + Expect(err).To(HaveOccurred()) + Expect(warnings).To(BeEmpty()) + Expect(hasMorePods).To(BeFalse()) + + Expect(err.Error()).To(Equal("running pod restart action failed: intentionally failing client on client.Patch")) + }) + + It("should not return error and warnings if restarting pods fails with failOnError is false", func() { + // given + pod := getPod("test-pod", "test-namespace", "podOwner", "ReplicaSet") + rsOwner := getReplicaSet("podOwner", "test-namespace", "rsOwner", "ReplicaSet") + rsOwnerRS := getReplicaSet("rsOwner", "test-namespace", "base", "ReplicaSet") + c := fakeClient(pod, rsOwner, rsOwnerRS) + + preds := []predicates.SidecarProxyPredicate{ + predicates.NewImageResourcesPredicate(predicates.SidecarImage{Repository: "istio", Tag: "1.1.0"}, helpers.DefaultSidecarResources), + } + limits := pods.NewPodsRestartLimits(2, 2) + + // when + failClient := &shouldFailClient{c, false, true} + + podsLister := pods.NewPods(failClient, &logger) + actionRestarter := restart.NewActionRestarter(failClient, &logger) + proxyRestarter := sidecars.NewProxyRestarter(failClient, podsLister, actionRestarter, &logger) + warnings, hasMorePods, err := proxyRestarter.RestartWithPredicates(ctx, preds, limits, false) + + // then + Expect(err).ToNot(HaveOccurred()) + Expect(warnings).To(Equal([]restart.RestartWarning{})) + Expect(hasMorePods).To(BeFalse()) + }) +}) + +var _ = Describe("BuildWarningMessage", func() { + logger := logr.Discard() + + It("should return an empty string when there are no warnings", func() { + // given + warnings := []restart.RestartWarning{} + + // when + warningMessage := sidecars.BuildWarningMessage(warnings, &logger) + + // then + Expect(warningMessage).To(BeEmpty()) + }) + + It("should return a warning message with pod details when there are warnings", func() { + // given + warnings := []restart.RestartWarning{ + { + Name: "pod1", + Namespace: "namespace1", + Kind: "Pod", + Message: "failed to restart", + }, + { + Name: "pod2", + Namespace: "namespace2", + Kind: "Pod", + Message: "failed to restart", + }, + } + + // when + warningMessage := sidecars.BuildWarningMessage(warnings, &logger) + + // then + Expect(warningMessage).To(ContainSubstring("The sidecars of the following workloads could not be restarted: namespace1/pod1, namespace2/pod2")) + }) + + It("should limit the number of pods in the warning message to 5", func() { + // given + warnings := []restart.RestartWarning{ + {Name: "pod1", Namespace: "namespace1", Kind: "Pod", Message: "failed to restart"}, + {Name: "pod2", Namespace: "namespace2", Kind: "Pod", Message: "failed to restart"}, + {Name: "pod3", Namespace: "namespace3", Kind: "Pod", Message: "failed to restart"}, + {Name: "pod4", Namespace: "namespace4", Kind: "Pod", Message: "failed to restart"}, + {Name: "pod5", Namespace: "namespace5", Kind: "Pod", Message: "failed to restart"}, + {Name: "pod6", Namespace: "namespace6", Kind: "Pod", Message: "failed to restart"}, + } + + // when + warningMessage := sidecars.BuildWarningMessage(warnings, &logger) + + // then + Expect(warningMessage).To(ContainSubstring("The sidecars of the following workloads could not be restarted: namespace1/pod1, namespace2/pod2, namespace3/pod3, namespace4/pod4, namespace5/pod5 and 1 additional workload(s)")) + }) + + It("should log each warning message", func() { + // given + warnings := []restart.RestartWarning{ + {Name: "pod1", Namespace: "namespace1", Kind: "Pod", Message: "failed to restart"}, + } + + // when + warningMessage := sidecars.BuildWarningMessage(warnings, &logger) + + // then + Expect(warningMessage).To(ContainSubstring("The sidecars of the following workloads could not be restarted: namespace1/pod1")) + }) +}) + +func fakeClient(objects ...client.Object) client.Client { + err := v1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) + err = appsv1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithObjects(objects...). + WithIndex(&v1.Pod{}, "status.phase", helpers.FakePodStatusPhaseIndexer). + Build() + + return fakeClient +} + +func getPod(name, namespace, ownerName, ownerKind string) *v1.Pod { + return &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + OwnerReferences: []metav1.OwnerReference{ + { + Name: ownerName, + Kind: ownerKind, + }, + }, + Annotations: map[string]string{ + "sidecar.istio.io/status": "abc", + }, + }, + TypeMeta: metav1.TypeMeta{ + Kind: "Pod", + APIVersion: "v1", + }, + Status: v1.PodStatus{ + Phase: "Running", + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "istio-proxy", + Image: "istio/istio-proxy:1.0.0", + Resources: helpers.DefaultSidecarResources, + }, + }, + }, + } +} + +func getReplicaSet(name, namespace, ownerName, ownerKind string) *appsv1.ReplicaSet { + return &appsv1.ReplicaSet{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + Name: ownerName, + Kind: ownerKind, + }, + }, + Name: name, + Namespace: namespace, + }, + Spec: appsv1.ReplicaSetSpec{ + Template: v1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"dummy": "annotation"}, + }, + }, + }, + } +} + +type shouldFailClient struct { + client.Client + FailOnList bool + FailOnPatch bool +} + +func (p *shouldFailClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + if p.FailOnList { + return errors.New("intentionally failing client on client.List") + } + return p.Client.List(ctx, list, opts...) +} + +func (p *shouldFailClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if p.FailOnPatch { + return errors.New("intentionally failing client on client.Patch") + } + return p.Client.Patch(ctx, obj, patch, opts...) +} + +type PodsMock struct { + Called int + Predicates map[int][]predicates.SidecarProxyPredicate + Limits map[int]*pods.PodsRestartLimits + FailOnKymaWorkload bool + FailOnCustomerWorkload bool +} + +func NewPodsMock() *PodsMock { + return &PodsMock{ + Called: 0, + Predicates: map[int][]predicates.SidecarProxyPredicate{}, + Limits: map[int]*pods.PodsRestartLimits{}, + FailOnKymaWorkload: false, + FailOnCustomerWorkload: false, + } +} + +func (p *PodsMock) GetPodsToRestart(_ context.Context, preds []predicates.SidecarProxyPredicate, limits *pods.PodsRestartLimits) (*v1.PodList, error) { + if p.FailOnKymaWorkload { + _, ok := preds[len(preds)-1].(*predicates.KymaWorkloadRestartPredicate) + if ok { + return &v1.PodList{}, errors.New("intentionally failed on Kyma workload predicate") + } + } + if p.FailOnCustomerWorkload { + _, ok := preds[len(preds)-1].(*predicates.CustomerWorkloadRestartPredicate) + if ok { + return &v1.PodList{}, errors.New("intentionally failed on Customer workload predicate") + } + } + p.Predicates[p.Called] = preds + p.Limits[p.Called] = limits + p.Called++ + return &v1.PodList{}, nil +} + +func (p *PodsMock) GetAllInjectedPods(_ context.Context) (*v1.PodList, error) { + return &v1.PodList{}, nil +} + +type ActionRestartMock struct { + warnings []restart.RestartWarning + err error +} + +func NewActionRestartMock(warnings []restart.RestartWarning, err error) *ActionRestartMock { + return &ActionRestartMock{ + warnings: warnings, + err: err, + } +} + +func (p *ActionRestartMock) Restart(ctx context.Context, podList *v1.PodList, failOnError bool) ([]restart.RestartWarning, error) { + return p.warnings, p.err +} diff --git a/pkg/lib/sidecars/remove/remove.go b/pkg/lib/sidecars/remove/remove.go index 16aede487..9ac6c8e30 100644 --- a/pkg/lib/sidecars/remove/remove.go +++ b/pkg/lib/sidecars/remove/remove.go @@ -4,16 +4,17 @@ import ( "context" "github.com/go-logr/logr" - podInfo "github.com/kyma-project/istio/operator/pkg/lib/sidecars/pods" + "github.com/kyma-project/istio/operator/pkg/lib/sidecars/pods" "github.com/kyma-project/istio/operator/pkg/lib/sidecars/restart" "sigs.k8s.io/controller-runtime/pkg/client" ) func RemoveSidecars(ctx context.Context, k8sclient client.Client, logger *logr.Logger) ([]restart.RestartWarning, error) { - toRestart, err := podInfo.GetAllInjectedPods(ctx, k8sclient) + podsLister := pods.NewPods(k8sclient, logger) + toRestart, err := podsLister.GetAllInjectedPods(ctx) if err != nil { return nil, err } - - return restart.Restart(ctx, k8sclient, toRestart, logger) + actionRestarter := restart.NewActionRestarter(k8sclient, logger) + return actionRestarter.Restart(ctx, toRestart, false) } diff --git a/pkg/lib/sidecars/remove/remove_test.go b/pkg/lib/sidecars/remove/remove_test.go index b64256cf4..ccc022d2c 100644 --- a/pkg/lib/sidecars/remove/remove_test.go +++ b/pkg/lib/sidecars/remove/remove_test.go @@ -33,7 +33,7 @@ var _ = ReportAfterSuite("custom reporter", func(report ginkgotypes.Report) { }) var _ = Describe("Remove Sidecar", func() { - ctx := context.TODO() + ctx := context.Background() logger := logr.Discard() It("should rollout restart Deployment if the pod has sidecar", func() { @@ -54,7 +54,7 @@ var _ = Describe("Remove Sidecar", func() { Expect(warnings).To(BeEmpty()) obj := appsv1.Deployment{} - err = c.Get(context.TODO(), types.NamespacedName{Namespace: "test-ns", Name: "owner"}, &obj) + err = c.Get(context.Background(), types.NamespacedName{Namespace: "test-ns", Name: "owner"}, &obj) Expect(err).NotTo(HaveOccurred()) Expect(obj.Spec.Template.Annotations[restartAnnotationName]).NotTo(BeEmpty()) @@ -78,7 +78,7 @@ var _ = Describe("Remove Sidecar", func() { Expect(warnings).To(BeEmpty()) obj := appsv1.Deployment{} - err = c.Get(context.TODO(), types.NamespacedName{Namespace: "test-ns", Name: "owner"}, &obj) + err = c.Get(context.Background(), types.NamespacedName{Namespace: "test-ns", Name: "owner"}, &obj) Expect(err).NotTo(HaveOccurred()) Expect(obj.Spec.Template.Annotations[restartAnnotationName]).To(BeEmpty()) diff --git a/pkg/lib/sidecars/restart/replica_set_action.go b/pkg/lib/sidecars/restart/replica_set_action.go index 8b6c644a1..638deef44 100644 --- a/pkg/lib/sidecars/restart/replica_set_action.go +++ b/pkg/lib/sidecars/restart/replica_set_action.go @@ -13,10 +13,9 @@ import ( ) func getReplicaSetAction(ctx context.Context, c client.Client, pod v1.Pod, replicaSetRef *metav1.OwnerReference) (restartAction, error) { - replicaSetKey := client.ObjectKey{ - Namespace: pod.Namespace, Name: replicaSetRef.Name, + Namespace: pod.Namespace, } var replicaSet = &appsv1.ReplicaSet{} @@ -27,7 +26,11 @@ func getReplicaSetAction(ctx context.Context, c client.Client, pod v1.Pod, repli if k8serrors.IsNotFound(err) { return newOwnerNotFoundAction(pod), nil } - return restartAction{}, err + return restartAction{object: actionObject{ + Name: replicaSetRef.Name, + Namespace: pod.Namespace, + Kind: "ReplicaSet", + }}, err } if rsOwnedBy, exists := getReplicaSetOwner(replicaSet); !exists { diff --git a/pkg/lib/sidecars/restart/restart.go b/pkg/lib/sidecars/restart/restart.go index fbf2c7191..8c4dca6a7 100644 --- a/pkg/lib/sidecars/restart/restart.go +++ b/pkg/lib/sidecars/restart/restart.go @@ -2,6 +2,8 @@ package restart import ( "context" + "fmt" + "github.com/go-logr/logr" v1 "k8s.io/api/core/v1" @@ -13,6 +15,22 @@ const ( ownedByJobMessage = "pod sidecar could not be updated because it is owned by a Job." ) +type ActionRestarter interface { + Restart(ctx context.Context, podList *v1.PodList, failOnError bool) ([]RestartWarning, error) +} + +type actionRestarter struct { + k8sClient client.Client + logger *logr.Logger +} + +func NewActionRestarter(c client.Client, logger *logr.Logger) ActionRestarter { + return &actionRestarter{ + k8sClient: c, + logger: logger, + } +} + type RestartWarning struct { Name, Namespace, Kind, Message string } @@ -26,22 +44,29 @@ func newRestartWarning(o actionObject, message string) RestartWarning { } } -func Restart(ctx context.Context, c client.Client, podList *v1.PodList, logger *logr.Logger) ([]RestartWarning, error) { +// Restarts pods in the given list through their respective owners by adding an annotation. If failOnError is set to true, the function will return an error if any of the restart actions fail. +func (s *actionRestarter) Restart(ctx context.Context, podList *v1.PodList, failOnError bool) ([]RestartWarning, error) { warnings := make([]RestartWarning, 0) processedActionObjects := make(map[string]bool) for _, pod := range podList.Items { - action, err := restartActionFactory(ctx, c, pod) + action, err := restartActionFactory(ctx, s.k8sClient, pod) if err != nil { - logger.Error(err, "creating an action for a pod failed") + s.logger.Error(err, "pod", action.object.getKey(), "Creating pod restart action failed") + if failOnError { + return warnings, fmt.Errorf("creating pod restart action failed: %w", err) + } continue } // We want to avoid performing the same action multiple times for a parent if it contains multiple pods that need to be restarted. if _, exists := processedActionObjects[action.object.getKey()]; !exists { - currentWarnings, err := action.run(ctx, c, action.object, logger) + currentWarnings, err := action.run(ctx, s.k8sClient, action.object, s.logger) if err != nil { - logger.Error(err, "running an action for a pod failed") + s.logger.Error(err, "pod", action.object.getKey(), "Running pod restart action failed") + if failOnError { + return warnings, fmt.Errorf("running pod restart action failed: %w", err) + } } warnings = append(warnings, currentWarnings...) processedActionObjects[action.object.getKey()] = true diff --git a/pkg/lib/sidecars/restart/restart_factory.go b/pkg/lib/sidecars/restart/restart_factory.go index 9267068a4..e2dcff7a2 100644 --- a/pkg/lib/sidecars/restart/restart_factory.go +++ b/pkg/lib/sidecars/restart/restart_factory.go @@ -2,6 +2,7 @@ package restart import ( "context" + "github.com/go-logr/logr" v1 "k8s.io/api/core/v1" diff --git a/pkg/lib/sidecars/restart/restart_test.go b/pkg/lib/sidecars/restart/restart_test.go index 602d17a25..c609fc0c8 100644 --- a/pkg/lib/sidecars/restart/restart_test.go +++ b/pkg/lib/sidecars/restart/restart_test.go @@ -2,6 +2,9 @@ package restart_test import ( "context" + "errors" + "testing" + "github.com/go-logr/logr" "github.com/kyma-project/istio/operator/internal/tests" . "github.com/onsi/ginkgo/v2" @@ -10,7 +13,6 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "testing" "github.com/kyma-project/istio/operator/pkg/lib/sidecars/restart" appsv1 "k8s.io/api/apps/v1" @@ -32,7 +34,7 @@ var _ = ReportAfterSuite("custom reporter", func(report ginkgotypes.Report) { }) var _ = Describe("Restart Pods", func() { - ctx := context.TODO() + ctx := context.Background() logger := logr.Discard() It("should return warning when pod has no owner", func() { @@ -46,7 +48,8 @@ var _ = Describe("Restart Pods", func() { } // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, false) // then Expect(err).NotTo(HaveOccurred()) @@ -67,28 +70,8 @@ var _ = Describe("Restart Pods", func() { } // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) - - // then - Expect(err).NotTo(HaveOccurred()) - Expect(warnings).NotTo(BeEmpty()) - - Expect(warnings[0].Name).To(Equal("p1")) - Expect(warnings[0].Message).To(ContainSubstring("owned by a Job")) - }) - - It("should return warning when pod is owned by a Job", func() { - // given - c := fakeClient() - - podList := v1.PodList{ - Items: []v1.Pod{ - podFixture("p1", "test-ns", "Job", "owningJob"), - }, - } - - // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, false) // then Expect(err).NotTo(HaveOccurred()) @@ -109,14 +92,15 @@ var _ = Describe("Restart Pods", func() { } // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, false) // then Expect(err).NotTo(HaveOccurred()) Expect(warnings).To(BeEmpty()) obj := appsv1.Deployment{} - err = c.Get(context.TODO(), types.NamespacedName{Namespace: "test-ns", Name: "owner"}, &obj) + err = c.Get(context.Background(), types.NamespacedName{Namespace: "test-ns", Name: "owner"}, &obj) Expect(err).NotTo(HaveOccurred()) Expect(obj.Spec.Template.Annotations[restartAnnotationName]).NotTo(BeEmpty()) @@ -134,14 +118,15 @@ var _ = Describe("Restart Pods", func() { } // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, false) // then Expect(err).NotTo(HaveOccurred()) Expect(warnings).To(BeEmpty()) obj := appsv1.Deployment{} - err = c.Get(context.TODO(), types.NamespacedName{Namespace: "test-ns", Name: "owner"}, &obj) + err = c.Get(context.Background(), types.NamespacedName{Namespace: "test-ns", Name: "owner"}, &obj) Expect(err).NotTo(HaveOccurred()) Expect(obj.Spec.Template.Annotations[restartAnnotationName]).NotTo(BeEmpty()) @@ -158,14 +143,15 @@ var _ = Describe("Restart Pods", func() { } // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, false) // then Expect(err).NotTo(HaveOccurred()) Expect(warnings).To(BeEmpty()) obj := appsv1.DaemonSet{} - err = c.Get(context.TODO(), types.NamespacedName{Namespace: "test-ns", Name: "owner"}, &obj) + err = c.Get(context.Background(), types.NamespacedName{Namespace: "test-ns", Name: "owner"}, &obj) Expect(err).NotTo(HaveOccurred()) Expect(obj.Spec.Template.Annotations[restartAnnotationName]).NotTo(BeEmpty()) @@ -181,14 +167,15 @@ var _ = Describe("Restart Pods", func() { } // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, false) // then Expect(err).NotTo(HaveOccurred()) Expect(warnings).To(BeEmpty()) obj := v1.Pod{} - err = c.Get(context.TODO(), types.NamespacedName{Namespace: "test-ns", Name: "p1"}, &obj) + err = c.Get(context.Background(), types.NamespacedName{Namespace: "test-ns", Name: "p1"}, &obj) Expect(err).To(HaveOccurred()) Expect(k8serrors.IsNotFound(err)).To(BeTrue()) @@ -204,14 +191,15 @@ var _ = Describe("Restart Pods", func() { } // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, false) // then Expect(err).NotTo(HaveOccurred()) Expect(warnings).To(BeEmpty()) obj := v1.Pod{} - err = c.Get(context.TODO(), types.NamespacedName{Namespace: "test-ns", Name: "p1"}, &obj) + err = c.Get(context.Background(), types.NamespacedName{Namespace: "test-ns", Name: "p1"}, &obj) Expect(err).To(HaveOccurred()) Expect(k8serrors.IsNotFound(err)).To(BeTrue()) @@ -230,14 +218,15 @@ var _ = Describe("Restart Pods", func() { } // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, false) // then Expect(err).NotTo(HaveOccurred()) Expect(warnings).To(BeEmpty()) obj := appsv1.StatefulSet{} - err = c.Get(context.TODO(), types.NamespacedName{Namespace: "test-ns", Name: "owner"}, &obj) + err = c.Get(context.Background(), types.NamespacedName{Namespace: "test-ns", Name: "owner"}, &obj) Expect(err).NotTo(HaveOccurred()) Expect(obj.Spec.Template.Annotations[restartAnnotationName]).NotTo(BeEmpty()) @@ -256,14 +245,15 @@ var _ = Describe("Restart Pods", func() { c := fakeClient(&pod) // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, false) // then Expect(err).NotTo(HaveOccurred()) Expect(warnings).NotTo(BeEmpty()) pods := v1.PodList{} - err = c.List(context.TODO(), &pods) + err = c.List(context.Background(), &pods) Expect(err).NotTo(HaveOccurred()) Expect(pods.Items).NotTo(BeEmpty()) @@ -291,14 +281,15 @@ var _ = Describe("Restart Pods", func() { }}) // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, false) // then Expect(err).NotTo(HaveOccurred()) Expect(warnings).To(BeEmpty()) pods := v1.PodList{} - err = c.List(context.TODO(), &pods) + err = c.List(context.Background(), &pods) Expect(err).NotTo(HaveOccurred()) Expect(pods.Items).NotTo(BeEmpty()) @@ -316,14 +307,15 @@ var _ = Describe("Restart Pods", func() { } // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, false) // then Expect(err).NotTo(HaveOccurred()) Expect(warnings).To(BeEmpty()) dep := appsv1.StatefulSet{} - err = c.Get(context.TODO(), types.NamespacedName{Namespace: "test-ns", Name: "podOwner"}, &dep) + err = c.Get(context.Background(), types.NamespacedName{Namespace: "test-ns", Name: "podOwner"}, &dep) Expect(err).NotTo(HaveOccurred()) // "StatefulSet should patch only once" Expect(dep.ResourceVersion).To(Equal("1000")) @@ -351,18 +343,87 @@ var _ = Describe("Restart Pods", func() { }}) // when - warnings, err := restart.Restart(ctx, c, &podList, &logger) + actionRestarter := restart.NewActionRestarter(c, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, false) // then Expect(err).NotTo(HaveOccurred()) Expect(warnings).To(BeEmpty()) replicaSet := appsv1.ReplicaSet{} - err = c.Get(context.TODO(), types.NamespacedName{Name: "rsOwner", Namespace: "test-ns"}, &replicaSet) + err = c.Get(context.Background(), types.NamespacedName{Name: "rsOwner", Namespace: "test-ns"}, &replicaSet) Expect(err).NotTo(HaveOccurred()) Expect(replicaSet.Spec.Template.Annotations[restartAnnotationName]).NotTo(BeEmpty()) }) + + It("should return an error when specified for a Pod owned by a ReplicaSet that is not found", func() { + // given + pod := podFixture("p1", "test-ns", "ReplicaSet", "podOwner") + + podList := v1.PodList{ + Items: []v1.Pod{ + pod, + }, + } + + c := fakeClient(&pod) + failClient := &shouldFailClient{c, true, false} + + // when + actionRestarter := restart.NewActionRestarter(failClient, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, true) + + // then + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("creating pod restart action failed: intentionally failing client on client.Get")) + Expect(warnings).To(BeEmpty()) + + pods := v1.PodList{} + err = c.List(context.Background(), &pods) + + Expect(err).NotTo(HaveOccurred()) + Expect(pods.Items).NotTo(BeEmpty()) + }) + + It("should return an error when specified for a Pod owned by a ReplicaSet that did not succeed to be patched", func() { + // given + pod := podFixture("p1", "test-ns", "ReplicaSet", "podOwner") + + podList := v1.PodList{ + Items: []v1.Pod{ + pod, + }, + } + + c := fakeClient(&pod, &appsv1.ReplicaSet{ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + {Name: "rsOwner", Kind: "ReplicaSet"}, + }, + Name: "podOwner", + Namespace: "test-ns", + }}, &appsv1.ReplicaSet{ObjectMeta: metav1.ObjectMeta{ + Name: "rsOwner", + Namespace: "test-ns", + }}) + + failClient := &shouldFailClient{c, false, true} + + // when + actionRestarter := restart.NewActionRestarter(failClient, &logger) + warnings, err := actionRestarter.Restart(ctx, &podList, true) + + // then + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("running pod restart action failed: intentionally failing client on client.Patch")) + Expect(warnings).To(BeEmpty()) + + pods := v1.PodList{} + err = c.List(context.Background(), &pods) + + Expect(err).NotTo(HaveOccurred()) + Expect(pods.Items).NotTo(BeEmpty()) + }) }) func fakeClient(objects ...client.Object) client.Client { @@ -375,3 +436,23 @@ func fakeClient(objects ...client.Object) client.Client { return fakeClient } + +type shouldFailClient struct { + client.Client + FailOnGet bool + FailOnPatch bool +} + +func (p *shouldFailClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if p.FailOnGet { + return errors.New("intentionally failing client on client.Get") + } + return p.Client.Get(ctx, key, obj, opts...) +} + +func (p *shouldFailClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if p.FailOnPatch { + return errors.New("intentionally failing client on client.Patch") + } + return p.Client.Patch(ctx, obj, patch, opts...) +} diff --git a/pkg/lib/sidecars/restart/rollout_action.go b/pkg/lib/sidecars/restart/rollout_action.go index 754f18a32..7e0c2ed14 100644 --- a/pkg/lib/sidecars/restart/rollout_action.go +++ b/pkg/lib/sidecars/restart/rollout_action.go @@ -21,7 +21,7 @@ func newRolloutAction(object actionObject) restartAction { } func rolloutRun(ctx context.Context, k8sclient client.Client, object actionObject, logger *logr.Logger) ([]RestartWarning, error) { - logger.Info("Roll out pod due to proxy restart", "name", object.Name, "namespace", object.Namespace) + logger.Info("Rollout pod due to proxy restart", "name", object.Name, "namespace", object.Namespace, "kind", object.Kind) var obj client.Object var err error @@ -37,7 +37,6 @@ func rolloutRun(ctx context.Context, k8sclient client.Client, object actionObjec ds := obj.(*appsv1.DaemonSet) patch := client.StrategicMergeFrom(ds.DeepCopy()) ds.Spec.Template.Annotations = annotations.AddRestartAnnotation(ds.Spec.Template.Annotations) - return k8sclient.Patch(ctx, ds, patch) }) case "Deployment": @@ -50,7 +49,6 @@ func rolloutRun(ctx context.Context, k8sclient client.Client, object actionObjec dep := obj.(*appsv1.Deployment) patch := client.StrategicMergeFrom(dep.DeepCopy()) dep.Spec.Template.Annotations = annotations.AddRestartAnnotation(dep.Spec.Template.Annotations) - return k8sclient.Patch(ctx, dep, patch) }) case "ReplicaSet": @@ -63,7 +61,6 @@ func rolloutRun(ctx context.Context, k8sclient client.Client, object actionObjec rs := obj.(*appsv1.ReplicaSet) patch := client.StrategicMergeFrom(rs.DeepCopy()) rs.Spec.Template.Annotations = annotations.AddRestartAnnotation(rs.Spec.Template.Annotations) - return k8sclient.Patch(ctx, rs, patch) }) case "StatefulSet": @@ -76,7 +73,6 @@ func rolloutRun(ctx context.Context, k8sclient client.Client, object actionObjec ss := obj.(*appsv1.StatefulSet) patch := client.StrategicMergeFrom(ss.DeepCopy()) ss.Spec.Template.Annotations = annotations.AddRestartAnnotation(ss.Spec.Template.Annotations) - return k8sclient.Patch(ctx, ss, patch) }) default: diff --git a/pkg/lib/sidecars/test/cases.go b/pkg/lib/sidecars/test/cases.go index a6502502d..5f62d7cf2 100644 --- a/pkg/lib/sidecars/test/cases.go +++ b/pkg/lib/sidecars/test/cases.go @@ -4,14 +4,15 @@ import ( "context" "fmt" - "github.com/kyma-project/istio/operator/internal/filter" + "github.com/kyma-project/istio/operator/internal/restarter/predicates" + "github.com/kyma-project/istio/operator/pkg/lib/sidecars/pods" + "github.com/kyma-project/istio/operator/pkg/lib/sidecars/restart" "github.com/kyma-project/istio/operator/pkg/lib/sidecars/test/helpers" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "github.com/cucumber/godog" "github.com/kyma-project/istio/operator/pkg/lib/sidecars" - "github.com/kyma-project/istio/operator/pkg/lib/sidecars/pods" appsv1 "k8s.io/api/apps/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" @@ -20,13 +21,15 @@ import ( const restartAnnotationName = "istio-operator.kyma-project.io/restartedAt" func (s *scenario) aRestartHappens(sidecarImage string) error { - pr := sidecars.NewProxyResetter() - warnings, hasMorePods, err := pr.ProxyReset(context.TODO(), - s.Client, - pods.SidecarImage{Repository: "istio/proxyv2", Tag: sidecarImage}, + podsLister := pods.NewPods(s.Client, &s.logger) + actionRestarter := restart.NewActionRestarter(s.Client, &s.logger) + pr := sidecars.NewProxyRestarter(s.Client, podsLister, actionRestarter, &s.logger) + istioCR := helpers.GetIstioCR(sidecarImage) + warnings, hasMorePods, err := pr.RestartProxies( + context.Background(), + predicates.SidecarImage{Repository: "istio/proxyv2", Tag: sidecarImage}, helpers.DefaultSidecarResources, - []filter.SidecarProxyPredicate{}, - &s.logger) + &istioCR) s.restartWarnings = warnings s.hasMorePodsToRestart = hasMorePods return err @@ -34,7 +37,6 @@ func (s *scenario) aRestartHappens(sidecarImage string) error { func (s *scenario) aRestartHappensWithUpdatedResources(sidecarImage string, resourceType string, cpu string, memory string) error { resources := helpers.DefaultSidecarResources - switch resourceType { case "requests": resources.Requests[v1.ResourceCPU] = resource.MustParse(cpu) @@ -45,13 +47,15 @@ func (s *scenario) aRestartHappensWithUpdatedResources(sidecarImage string, reso default: return fmt.Errorf("unknown resource type %s", resourceType) } - pr := sidecars.NewProxyResetter() - warnings, hasMorePods, err := pr.ProxyReset(context.TODO(), - s.Client, - pods.SidecarImage{Repository: "istio/proxyv2", Tag: sidecarImage}, + istioCR := helpers.GetIstioCR(sidecarImage) + podsLister := pods.NewPods(s.Client, &s.logger) + actionRestarter := restart.NewActionRestarter(s.Client, &s.logger) + pr := sidecars.NewProxyRestarter(s.Client, podsLister, actionRestarter, &s.logger) + warnings, hasMorePods, err := pr.RestartProxies( + context.Background(), + predicates.SidecarImage{Repository: "istio/proxyv2", Tag: sidecarImage}, resources, - []filter.SidecarProxyPredicate{}, - &s.logger) + &istioCR) s.restartWarnings = warnings s.hasMorePodsToRestart = hasMorePods return err @@ -60,7 +64,7 @@ func (s *scenario) aRestartHappensWithUpdatedResources(sidecarImage string, reso func (s *scenario) allRequiredResourcesAreDeleted() error { for _, v := range s.ToBeDeletedObjects { obj := v - err := s.Client.Get(context.TODO(), types.NamespacedName{Name: v.GetName(), Namespace: v.GetNamespace()}, obj) + err := s.Client.Get(context.Background(), types.NamespacedName{Name: v.GetName(), Namespace: v.GetNamespace()}, obj) if err == nil { return fmt.Errorf("the pod %s/%s should have been deleted, but was not deleted", v.GetNamespace(), v.GetName()) } @@ -75,7 +79,7 @@ func (s *scenario) allRequiredResourcesAreDeleted() error { func (s *scenario) allRequiredResourcesAreRestarted() error { for _, v := range s.ToBeRestartedObjects { obj := v - err := s.Client.Get(context.TODO(), types.NamespacedName{Name: v.GetName(), Namespace: v.GetNamespace()}, obj) + err := s.Client.Get(context.Background(), types.NamespacedName{Name: v.GetName(), Namespace: v.GetNamespace()}, obj) if err != nil { return err } @@ -115,7 +119,7 @@ func (s *scenario) allRequiredResourcesAreRestarted() error { func (s *scenario) onlyRequiredResourcesAreDeleted() error { for _, v := range s.NotToBeDeletedObjects { obj := v - err := s.Client.Get(context.TODO(), types.NamespacedName{Name: v.GetName(), Namespace: v.GetNamespace()}, obj) + err := s.Client.Get(context.Background(), types.NamespacedName{Name: v.GetName(), Namespace: v.GetNamespace()}, obj) if err != nil { return err } @@ -126,7 +130,7 @@ func (s *scenario) onlyRequiredResourcesAreDeleted() error { func (s *scenario) onlyRequiredresourcesAreRestarted() error { for _, v := range s.NotToBeRestartedObjects { obj := v - err := s.Client.Get(context.TODO(), types.NamespacedName{Name: v.GetName(), Namespace: v.GetNamespace()}, obj) + err := s.Client.Get(context.Background(), types.NamespacedName{Name: v.GetName(), Namespace: v.GetNamespace()}, obj) if err != nil { return err } diff --git a/pkg/lib/sidecars/test/helpers/helpers.go b/pkg/lib/sidecars/test/helpers/helpers.go index cc7b4a2ed..56626c1b3 100644 --- a/pkg/lib/sidecars/test/helpers/helpers.go +++ b/pkg/lib/sidecars/test/helpers/helpers.go @@ -5,6 +5,8 @@ import ( "reflect" "time" + operatorv1alpha2 "github.com/kyma-project/istio/operator/api/v1alpha2" + "github.com/kyma-project/istio/operator/pkg/labels" "k8s.io/apimachinery/pkg/api/resource" v1 "k8s.io/api/core/v1" @@ -37,6 +39,17 @@ var DefaultSidecarResources = v1.ResourceRequirements{ }, } +var DifferentSidecarResources = v1.ResourceRequirements{ + Limits: v1.ResourceList{ + v1.ResourceCPU: resource.MustParse("150m"), + v1.ResourceMemory: resource.MustParse("250Mi"), + }, + Requests: v1.ResourceList{ + v1.ResourceCPU: resource.MustParse("250m"), + v1.ResourceMemory: resource.MustParse("450Mi"), + }, +} + func NewSidecarPodBuilder() *SidecarPodFixtureBuilder { resources := DefaultSidecarResources.DeepCopy() return &SidecarPodFixtureBuilder{ @@ -277,3 +290,21 @@ func Clone(oldObj interface{}) interface{} { return newObj.Interface() } + +func GetIstioCR(sidecarImage string) operatorv1alpha2.Istio { + numTrustedProxies := 1 + return operatorv1alpha2.Istio{ObjectMeta: metav1.ObjectMeta{ + Name: "default", + ResourceVersion: "1", + Annotations: map[string]string{ + labels.LastAppliedConfiguration: fmt.Sprintf(`{"config":{"numTrustedProxies":%d},"compatibilityMode":false,"IstioTag":"%s"}`, numTrustedProxies, sidecarImage), + }, + }, + Spec: operatorv1alpha2.IstioSpec{ + Config: operatorv1alpha2.Config{ + NumTrustedProxies: &numTrustedProxies, + }, + CompatibilityMode: false, + }, + } +} diff --git a/pkg/lib/sidecars/test/pod_states.go b/pkg/lib/sidecars/test/pod_states.go index 6f9745008..55eeb0fff 100644 --- a/pkg/lib/sidecars/test/pod_states.go +++ b/pkg/lib/sidecars/test/pod_states.go @@ -14,7 +14,7 @@ import ( func (s *scenario) createObjectInAllNamespaces(toCreate client.Object, deleteIn NamespaceSelector, restartIn NamespaceSelector) error { toCreateDefault := helpers.Clone(toCreate).(client.Object) toCreateDefault.SetNamespace(noAnnotationNamespace) - err := s.Client.Create(context.TODO(), toCreateDefault) + err := s.Client.Create(context.Background(), toCreateDefault) if err != nil { return err } @@ -33,7 +33,7 @@ func (s *scenario) createObjectInAllNamespaces(toCreate client.Object, deleteIn toCreateDisabled := helpers.Clone(toCreate).(client.Object) toCreateDisabled.SetNamespace(sidecarDisabledNamespace) - err = s.Client.Create(context.TODO(), toCreateDisabled) + err = s.Client.Create(context.Background(), toCreateDisabled) if err != nil { return err } @@ -52,7 +52,7 @@ func (s *scenario) createObjectInAllNamespaces(toCreate client.Object, deleteIn toCreateEnabled := helpers.Clone(toCreate).(client.Object) toCreateEnabled.SetNamespace(sidecarEnabledNamespace) - err = s.Client.Create(context.TODO(), toCreateEnabled) + err = s.Client.Create(context.Background(), toCreateEnabled) if err != nil { return err }