From 2ee1f5021a4f8b06f2df258bda2bdf1eaeb1c579 Mon Sep 17 00:00:00 2001 From: Rizwana777 Date: Mon, 1 Apr 2024 18:03:19 +0530 Subject: [PATCH] Update unit and e2e test to check expected values Signed-off-by: Rizwana777 --- .github/workflows/e2e_tests.yml | 1 - controllers/argorollouts_controller.go | 4 + controllers/resources.go | 63 +++++++- controllers/resources_test.go | 147 ++++++++++++++++--- hack/run-rollouts-manager-e2e-tests.sh | 11 +- hack/run-upstream-argo-rollouts-e2e-tests.sh | 8 + tests/e2e/rollouts_test.go | 33 ++++- 7 files changed, 233 insertions(+), 34 deletions(-) diff --git a/.github/workflows/e2e_tests.yml b/.github/workflows/e2e_tests.yml index ac1a18a..ca25572 100644 --- a/.github/workflows/e2e_tests.yml +++ b/.github/workflows/e2e_tests.yml @@ -49,5 +49,4 @@ jobs: - name: Run tests run: | set -o pipefail - kubectl apply -f https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/release-0.52/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml make test-e2e 2>&1 | tee /tmp/e2e-test.log diff --git a/controllers/argorollouts_controller.go b/controllers/argorollouts_controller.go index 4c907c8..f9d2d36 100644 --- a/controllers/argorollouts_controller.go +++ b/controllers/argorollouts_controller.go @@ -20,6 +20,7 @@ import ( "context" rolloutsmanagerv1alpha1 "github.com/argoproj-labs/argo-rollouts-manager/api/v1alpha1" + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -127,5 +128,8 @@ func (r *RolloutManagerReconciler) SetupWithManager(mgr ctrl.Manager) error { // Watch for changes to RoleBinding sub-resources owned by RolloutManager. bld.Owns(&rbacv1.RoleBinding{}) + // Watch for changes to ServiceMonitor sub-resources owned by RolloutManager. + bld.Owns(&monitoringv1.ServiceMonitor{}) + return bld.Complete(r) } diff --git a/controllers/resources.go b/controllers/resources.go index 468c829..a4325d0 100644 --- a/controllers/resources.go +++ b/controllers/resources.go @@ -293,12 +293,18 @@ func (r *RolloutManagerReconciler) reconcileRolloutsMetricsService(ctx context.C }, } - if err := fetchObject(ctx, r.Client, smCRD.Namespace, smCRD.Name, smCRD); err == nil { - // Create ServiceMonitor for Rollouts metrics - err := r.createServiceMonitorIfAbsent(cr.Namespace, cr, actualSvc.Name, actualSvc.Name) - if err != nil { - return err + if err := fetchObject(ctx, r.Client, smCRD.Namespace, smCRD.Name, smCRD); err != nil { + if !apierrors.IsNotFound(err) { + return fmt.Errorf("failed to get the ServiceMonitor %s : %s", smCRD.Name, err) } + + return fmt.Errorf("failed to get ServiceMonitor for Rollouts metrics associated with %s : %s", smCRD.Name, err) + } + + // Create ServiceMonitor for Rollouts metrics + err := r.createServiceMonitorIfAbsent(cr.Namespace, cr, actualSvc.Name, actualSvc.Name) + if err != nil { + return err } return nil @@ -841,6 +847,20 @@ func (r *RolloutManagerReconciler) createServiceMonitorIfAbsent(namespace string if err == nil { log.Info("A ServiceMonitor instance already exists", "Namespace", existingServiceMonitor.Namespace, "Name", existingServiceMonitor.Name) + + // Check if existing ServiceMonitor matches expected content + if !serviceMonitorMatches(existingServiceMonitor, serviceMonitorLabel) { + log.Info("Updating existing ServiceMonitor instance", + "Namespace", existingServiceMonitor.Namespace, "Name", existingServiceMonitor.Name) + + // Update ServiceMonitor with expected content + updateServiceMonitor(existingServiceMonitor, serviceMonitorLabel) + if err := r.Client.Update(context.TODO(), existingServiceMonitor); err != nil { + log.Error(err, "Error updating existing ServiceMonitor instance", + "Namespace", existingServiceMonitor.Namespace, "Name", existingServiceMonitor.Name) + return err + } + } return nil } if apierrors.IsNotFound(err) { @@ -890,3 +910,36 @@ func newServiceMonitor(namespace, name, matchLabel string) *monitoringv1.Service Spec: spec, } } + +func serviceMonitorMatches(sm *monitoringv1.ServiceMonitor, matchLabel string) bool { + // Check if labels match + labels := sm.Spec.Selector.MatchLabels + if val, ok := labels["app.kubernetes.io/name"]; ok { + if val != matchLabel { + return false + } + } else { + return false + } + + // Check if endpoints match + if sm.Spec.Endpoints[0].Port != "metrics" { + return false + } + + return true +} + +func updateServiceMonitor(sm *monitoringv1.ServiceMonitor, matchLabel string) { + // Update labels + sm.Spec.Selector.MatchLabels = map[string]string{ + "app.kubernetes.io/name": matchLabel, + } + + // Update endpoints + sm.Spec.Endpoints = []monitoringv1.Endpoint{ + { + Port: "metrics", + }, + } +} diff --git a/controllers/resources_test.go b/controllers/resources_test.go index 744769f..5ab7e39 100644 --- a/controllers/resources_test.go +++ b/controllers/resources_test.go @@ -155,6 +155,15 @@ var _ = Describe("ReconcileRolloutManager tests", func() { }, }, }, + { + fmt.Sprintf("ServiceMonitor %s", DefaultArgoRolloutsResourceName), + &monitoringv1.ServiceMonitor{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultArgoRolloutsResourceName, + Namespace: a.Namespace, + }, + }, + }, } for _, test := range tt { @@ -187,50 +196,140 @@ var _ = Describe("ReconcileRolloutManager tests", func() { }) It("Verify whether RolloutManager creating ServiceMonitor", func() { - smCRD := &crdv1.CustomResourceDefinition{ - ObjectMeta: metav1.ObjectMeta{ - Name: "servicemonitors.monitoring.coreos.com", - Namespace: req.Namespace, - }, - } + smCRD, existingSvc := serviceAndServiceMonitorCRD(req.Namespace) + Expect(r.Client.Create(ctx, smCRD)).To(Succeed()) + Expect(r.Client.Create(ctx, existingSvc)).To(Succeed()) + + res, err := r.Reconcile(ctx, req) + Expect(err).ToNot(HaveOccurred()) + Expect(res.Requeue).Should(BeFalse(), "reconcile should not requeue request") + expectedServiceMonitor := serviceMonitor() + + sm := &monitoringv1.ServiceMonitor{} + Expect(r.Client.Get(ctx, types.NamespacedName{ + Name: DefaultArgoRolloutsMetricsServiceName, + Namespace: testNamespace, + }, sm)).To(Succeed()) + + Expect(sm.Name).To(Equal(expectedServiceMonitor.Name)) + Expect(sm.Namespace).To(Equal(expectedServiceMonitor.Namespace)) + Expect(sm.Spec).To(Equal(expectedServiceMonitor.Spec)) + }) + + It("Verify if ServiceMonitor exists, but has different content than we expect then it should update ServiceMonitor", func() { + smCRD, existingSvc := serviceAndServiceMonitorCRD(req.Namespace) Expect(r.Client.Create(ctx, smCRD)).To(Succeed()) + Expect(r.Client.Create(ctx, existingSvc)).To(Succeed()) - existingSvc := &corev1.Service{ + existingServiceMonitor := &monitoringv1.ServiceMonitor{ ObjectMeta: metav1.ObjectMeta{ Name: DefaultArgoRolloutsMetricsServiceName, - Namespace: req.Namespace, - Labels: map[string]string{ - "app.kubernetes.io/name": DefaultArgoRolloutsMetricsServiceName, - "app.kubernetes.io/component": "server", - }, + Namespace: testNamespace, }, - Spec: corev1.ServiceSpec{ - Ports: []corev1.ServicePort{ - { - Name: "metrics", - Port: 8090, - Protocol: corev1.ProtocolTCP, - TargetPort: intstr.FromInt(8090), + Spec: monitoringv1.ServiceMonitorSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app.kubernetes.io/name": "test-label", }, }, - Selector: map[string]string{ - DefaultRolloutsSelectorKey: DefaultArgoRolloutsResourceName, + Endpoints: []monitoringv1.Endpoint{ + { + Port: "metrics-test", + }, }, }, } - Expect(r.Client.Create(ctx, existingSvc)).To(Succeed()) + Expect(r.Client.Create(ctx, existingServiceMonitor)).To(Succeed()) res, err := r.Reconcile(ctx, req) Expect(err).ToNot(HaveOccurred()) Expect(res.Requeue).Should(BeFalse(), "reconcile should not requeue request") - sm := &monitoringv1.ServiceMonitor{} + expectedSM := serviceMonitor() + Expect(r.Client.Get(ctx, types.NamespacedName{ Name: DefaultArgoRolloutsMetricsServiceName, Namespace: testNamespace, - }, sm)).To(Succeed()) + }, existingServiceMonitor)).To(Succeed()) + + Expect(existingServiceMonitor.Name).To(Equal(expectedSM.Name)) + Expect(existingServiceMonitor.Namespace).To(Equal(expectedSM.Namespace)) + Expect(existingServiceMonitor.Spec).To(Equal(expectedSM.Spec)) + }) + It("Verify ServiceMonitor is not created if the CRD does not exist.", func() { + _, existingSvc := serviceAndServiceMonitorCRD(req.Namespace) + Expect(r.Client.Create(ctx, existingSvc)).To(Succeed()) + + res, err := r.Reconcile(ctx, req) + Expect(err).To(HaveOccurred()) + Expect(res.Requeue).Should(BeFalse(), "reconcile should not requeue request") + + sm := &monitoringv1.ServiceMonitor{} + Expect(r.Client.Get(ctx, types.NamespacedName{ + Name: DefaultArgoRolloutsMetricsServiceName, + Namespace: testNamespace, + }, sm)).ToNot(Succeed()) + }) }) + +func serviceMonitor() *monitoringv1.ServiceMonitor { + sm := &monitoringv1.ServiceMonitor{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultArgoRolloutsMetricsServiceName, + Namespace: testNamespace, + }, + Spec: monitoringv1.ServiceMonitorSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app.kubernetes.io/name": DefaultArgoRolloutsMetricsServiceName, + }, + }, + Endpoints: []monitoringv1.Endpoint{ + { + Port: "metrics", + }, + }, + }, + } + return sm +} + +func serviceAndServiceMonitorCRD(namespace string) (*crdv1.CustomResourceDefinition, *corev1.Service) { + smCRD := &crdv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "servicemonitors.monitoring.coreos.com", + Namespace: namespace, + }, + } + + existingSvc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultArgoRolloutsMetricsServiceName, + Namespace: namespace, + Labels: map[string]string{ + "app.kubernetes.io/name": DefaultArgoRolloutsResourceName, + "app.kubernetes.io/component": "server", + }, + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + { + Name: "metrics", + Port: 8090, + Protocol: corev1.ProtocolTCP, + TargetPort: intstr.FromInt(8090), + }, + }, + Selector: map[string]string{ + DefaultRolloutsSelectorKey: DefaultArgoRolloutsResourceName, + }, + }, + } + + return smCRD, existingSvc + +} diff --git a/hack/run-rollouts-manager-e2e-tests.sh b/hack/run-rollouts-manager-e2e-tests.sh index 52d430b..b979fa3 100755 --- a/hack/run-rollouts-manager-e2e-tests.sh +++ b/hack/run-rollouts-manager-e2e-tests.sh @@ -8,10 +8,17 @@ SCRIPTPATH="$( cd "$SCRIPTPATH/.." set -o pipefail -set -ex -go test -v -p=1 -timeout=10m -race -count=1 -coverprofile=coverage.out ./tests/e2e +# Check if the CRD exists +kubectl get crd/servicemonitors.monitoring.coreos.com &> /dev/null +retVal=$? +if [ $retVal -ne 0 ]; then + # If the CRD is not found, apply the CRD YAML + kubectl apply -f https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/release-0.52/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml +fi +set -ex +go test -v -p=1 -timeout=10m -race -count=1 -coverprofile=coverage.out ./tests/e2e set +e # If the output from the E2E operator is available, then check it for errors diff --git a/hack/run-upstream-argo-rollouts-e2e-tests.sh b/hack/run-upstream-argo-rollouts-e2e-tests.sh index 1d60955..71ace12 100755 --- a/hack/run-upstream-argo-rollouts-e2e-tests.sh +++ b/hack/run-upstream-argo-rollouts-e2e-tests.sh @@ -46,6 +46,14 @@ if [ -z "$SKIP_RUN_STEP" ]; then set +e + # Check if the ServiceMonitor CRD exists + kubectl get crd/servicemonitors.monitoring.coreos.com &> /dev/null + retVal=$? + if [ $retVal -ne 0 ]; then + # If the CRD is not found, apply the CRD YAML + kubectl apply -f https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/release-0.52/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml + fi + rm -f /tmp/e2e-operator-run.log || true go run ./main.go 2>&1 | tee /tmp/e2e-operator-run.log & diff --git a/tests/e2e/rollouts_test.go b/tests/e2e/rollouts_test.go index ad9a325..81f64a4 100644 --- a/tests/e2e/rollouts_test.go +++ b/tests/e2e/rollouts_test.go @@ -10,13 +10,14 @@ import ( "github.com/argoproj-labs/argo-rollouts-manager/tests/e2e/fixture/k8s" rolloutManagerFixture "github.com/argoproj-labs/argo-rollouts-manager/tests/e2e/fixture/rolloutmanager" + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" "sigs.k8s.io/controller-runtime/pkg/client" rolloutsmanagerv1alpha1 "github.com/argoproj-labs/argo-rollouts-manager/api/v1alpha1" controllers "github.com/argoproj-labs/argo-rollouts-manager/controllers" - monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -181,6 +182,11 @@ var _ = Describe("RolloutManager tests", func() { ObjectMeta: metav1.ObjectMeta{Name: controllers.DefaultRolloutsNotificationSecretName, Namespace: rolloutManager.Namespace}, }, "30s", "1s").ShouldNot(k8s.ExistByName(k8sClient)) + By("deleting the serviceMonitor") + Eventually(&monitoringv1.ServiceMonitor{ + ObjectMeta: metav1.ObjectMeta{Name: controllers.DefaultArgoRolloutsMetricsServiceName, Namespace: rolloutManager.Namespace}, + }, "30s", "1s").ShouldNot(k8s.ExistByName(k8sClient)) + // Make sure the cluster roles have not been deleted By("NOT deleting the three cluster roles") clusterRoleSuffixes := []string{"aggregate-to-admin", "aggregate-to-edit", "aggregate-to-view"} @@ -299,7 +305,26 @@ var _ = Describe("RolloutManager tests", func() { return deployment.Spec.Template.Spec.Containers[0].Image }, "10s", "1s").Should(Equal(expectedVersion)) - By("verify whether ServiceMonitor is created or not for Rolloutsmanager") + expectedServiceMonitor := &monitoringv1.ServiceMonitor{ + ObjectMeta: metav1.ObjectMeta{ + Name: "argo-rollouts-metrics", + Namespace: fixture.TestE2ENamespace, + }, + Spec: monitoringv1.ServiceMonitorSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app.kubernetes.io/name": "argo-rollouts-metrics", + }, + }, + Endpoints: []monitoringv1.Endpoint{ + { + Port: "metrics", + }, + }, + }, + } + + By("verify whether ServiceMonitor is created or not for RolloutManager") sm := &monitoringv1.ServiceMonitor{ ObjectMeta: metav1.ObjectMeta{ Name: "argo-rollouts-metrics", @@ -308,6 +333,10 @@ var _ = Describe("RolloutManager tests", func() { } Eventually(sm, "10s", "1s").Should(k8s.ExistByName(k8sClient)) + Expect(sm.Name).To(Equal(expectedServiceMonitor.Name)) + Expect(sm.Namespace).To(Equal(expectedServiceMonitor.Namespace)) + Expect(sm.Spec).To(Equal(expectedServiceMonitor.Spec)) + }) }) })