diff --git a/controllers/reconcile.go b/controllers/reconcile.go index 21acd45..77359dc 100644 --- a/controllers/reconcile.go +++ b/controllers/reconcile.go @@ -122,7 +122,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsManager(ctx context.Context, } log.Info("reconciling Rollouts Metrics Service") - if err := r.reconcileRolloutsMetricsService(ctx, cr); err != nil { + if err := r.reconcileRolloutsMetricsServiceAndMonitor(ctx, cr); err != nil { log.Error(err, "failed to reconcile Rollout's Metrics Service.") return wrapCondition(createCondition(err.Error())), err } diff --git a/controllers/resources.go b/controllers/resources.go index 09604b2..48fb495 100644 --- a/controllers/resources.go +++ b/controllers/resources.go @@ -520,8 +520,74 @@ func (r *RolloutManagerReconciler) reconcileRolloutsAggregateToViewClusterRole(c return nil } -// Reconcile Rollouts Metrics Service. -func (r *RolloutManagerReconciler) reconcileRolloutsMetricsService(ctx context.Context, cr rolloutsmanagerv1alpha1.RolloutManager) error { +// reconcileRolloutsMetricsServiceAndMonitor reconciles the Rollouts Metrics Service and ServiceMonitor +func (r *RolloutManagerReconciler) reconcileRolloutsMetricsServiceAndMonitor(ctx context.Context, cr rolloutsmanagerv1alpha1.RolloutManager) error { + + reconciledSvc, err := r.reconcileRolloutsMetricsService(ctx, cr) + if err != nil { + return fmt.Errorf("unable to reconcile metrics service: %w", err) + } + + // Checks if user is using the Prometheus operator by checking CustomResourceDefinition for ServiceMonitor + smCRD := &crdv1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: serviceMonitorsCRDName, + }, + } + + 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 nil + } + + // Create ServiceMonitor for Rollouts metrics + existingServiceMonitor := &monitoringv1.ServiceMonitor{} + if err := fetchObject(ctx, r.Client, cr.Namespace, DefaultArgoRolloutsResourceName, existingServiceMonitor); err != nil { + if apierrors.IsNotFound(err) { + if err := r.createServiceMonitorIfAbsent(ctx, cr.Namespace, cr, DefaultArgoRolloutsResourceName, reconciledSvc.Name); err != nil { + return err + } + return nil + + } else { + log.Error(err, "Error querying for ServiceMonitor", "Namespace", cr.Namespace, "Name", reconciledSvc.Name) + return err + } + + } else { + log.Info("A ServiceMonitor instance already exists", + "Namespace", existingServiceMonitor.Namespace, "Name", existingServiceMonitor.Name) + + // Check if existing ServiceMonitor matches expected content + if !serviceMonitorMatches(existingServiceMonitor, reconciledSvc.Name) { + log.Info("Updating existing ServiceMonitor instance", + "Namespace", existingServiceMonitor.Namespace, "Name", existingServiceMonitor.Name) + + // Update ServiceMonitor with expected content + existingServiceMonitor.Spec.Selector.MatchLabels = map[string]string{ + "app.kubernetes.io/name": reconciledSvc.Name, + } + existingServiceMonitor.Spec.Endpoints = []monitoringv1.Endpoint{ + { + Port: "metrics", + }, + } + + if err := r.Client.Update(ctx, existingServiceMonitor); err != nil { + log.Error(err, "Error updating existing ServiceMonitor instance", + "Namespace", existingServiceMonitor.Namespace, "Name", existingServiceMonitor.Name) + return err + } + } + return nil + } + +} + +// reconcileRolloutsMetricsService reconciles the Service which is used to gather metrics from Rollouts install +func (r *RolloutManagerReconciler) reconcileRolloutsMetricsService(ctx context.Context, cr rolloutsmanagerv1alpha1.RolloutManager) (*corev1.Service, error) { expectedSvc := &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ @@ -550,17 +616,17 @@ func (r *RolloutManagerReconciler) reconcileRolloutsMetricsService(ctx context.C liveService := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name: expectedSvc.Name, Namespace: expectedSvc.Namespace}} if err := fetchObject(ctx, r.Client, cr.Namespace, liveService.Name, liveService); err != nil { if !apierrors.IsNotFound(err) { - return fmt.Errorf("failed to get the Service %s: %w", expectedSvc.Name, err) + return nil, fmt.Errorf("failed to get the Service %s: %w", expectedSvc.Name, err) } if err := controllerutil.SetControllerReference(&cr, expectedSvc, r.Scheme); err != nil { - return err + return nil, err } log.Info(fmt.Sprintf("Creating Service %s", expectedSvc.Name)) if err := r.Client.Create(ctx, expectedSvc); err != nil { log.Error(err, "Error creating Service", "Name", expectedSvc.Name) - return err + return nil, err } liveService = expectedSvc @@ -588,65 +654,11 @@ func (r *RolloutManagerReconciler) reconcileRolloutsMetricsService(ctx context.C // Update if the Service already exists and needs to be modified if err := r.Client.Update(ctx, liveService); err != nil { log.Error(err, "Error updating Ports of metrics Service", "Name", liveService.Name) - return err + return liveService, err } } - // Checks if user is using the Prometheus operator by checking CustomResourceDefinition for ServiceMonitor - smCRD := &crdv1.CustomResourceDefinition{ - ObjectMeta: metav1.ObjectMeta{ - Name: serviceMonitorsCRDName, - }, - } - - 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 nil - } - - // Create ServiceMonitor for Rollouts metrics - existingServiceMonitor := &monitoringv1.ServiceMonitor{} - if err := fetchObject(ctx, r.Client, cr.Namespace, DefaultArgoRolloutsResourceName, existingServiceMonitor); err != nil { - if apierrors.IsNotFound(err) { - if err := r.createServiceMonitorIfAbsent(ctx, cr.Namespace, cr, DefaultArgoRolloutsResourceName, expectedSvc.Name); err != nil { - return err - } - return nil - - } else { - log.Error(err, "Error querying for ServiceMonitor", "Namespace", cr.Namespace, "Name", liveService.Name) - return err - } - - } else { - log.Info("A ServiceMonitor instance already exists", - "Namespace", existingServiceMonitor.Namespace, "Name", existingServiceMonitor.Name) - - // Check if existing ServiceMonitor matches expected content - if !serviceMonitorMatches(existingServiceMonitor, liveService.Name) { - log.Info("Updating existing ServiceMonitor instance", - "Namespace", existingServiceMonitor.Namespace, "Name", existingServiceMonitor.Name) - - // Update ServiceMonitor with expected content - existingServiceMonitor.Spec.Selector.MatchLabels = map[string]string{ - "app.kubernetes.io/name": liveService.Name, - } - existingServiceMonitor.Spec.Endpoints = []monitoringv1.Endpoint{ - { - Port: "metrics", - }, - } - - if err := r.Client.Update(ctx, existingServiceMonitor); err != nil { - log.Error(err, "Error updating existing ServiceMonitor instance", - "Namespace", existingServiceMonitor.Namespace, "Name", existingServiceMonitor.Name) - return err - } - } - return nil - } + return liveService, nil } diff --git a/controllers/resources_test.go b/controllers/resources_test.go index a7de0b5..deb1670 100644 --- a/controllers/resources_test.go +++ b/controllers/resources_test.go @@ -19,7 +19,222 @@ import ( var _ = Describe("Resource creation and cleanup tests", func() { - Context("Resource creation test", func() { + Context("Verify resource creation when RolloutManger does not contain a user-defined label/annotation", func() { + var ( + ctx context.Context + a v1alpha1.RolloutManager + r *RolloutManagerReconciler + ) + + BeforeEach(func() { + ctx = context.Background() + a = *makeTestRolloutManager() + r = makeTestReconciler(&a) + err := createNamespace(r, a.Namespace) + Expect(err).ToNot(HaveOccurred()) + }) + + It("Test for reconcileRolloutsServiceAccount function", func() { + _, err := r.reconcileRolloutsServiceAccount(ctx, a) + Expect(err).ToNot(HaveOccurred()) + }) + + It("Test for reconcileRolloutsRole function", func() { + role, err := r.reconcileRolloutsRole(ctx, a) + Expect(err).ToNot(HaveOccurred()) + + By("Modify Rules of Role.") + role.Rules[0].Verbs = append(role.Rules[0].Verbs, "test") + Expect(r.Client.Update(ctx, role)).To(Succeed()) + + By("Reconciler should revert modifications.") + role, err = r.reconcileRolloutsRole(ctx, a) + Expect(err).ToNot(HaveOccurred()) + Expect(role.Rules).To(Equal(GetPolicyRules())) + }) + + It("Test for reconcileRolloutsClusterRole function", func() { + clusterRole, err := r.reconcileRolloutsClusterRole(ctx, a) + Expect(err).ToNot(HaveOccurred()) + + By("Modify Rules of Role.") + clusterRole.Rules[0].Verbs = append(clusterRole.Rules[0].Verbs, "test") + Expect(r.Client.Update(ctx, clusterRole)).To(Succeed()) + + By("Reconciler should revert modifications.") + clusterRole, err = r.reconcileRolloutsClusterRole(ctx, a) + Expect(err).ToNot(HaveOccurred()) + Expect(clusterRole.Rules).To(Equal(GetPolicyRules())) + }) + + It("Test for reconcileRolloutsRoleBinding function", func() { + sa, err := r.reconcileRolloutsServiceAccount(ctx, a) + Expect(err).ToNot(HaveOccurred()) + role, err := r.reconcileRolloutsRole(ctx, a) + Expect(err).ToNot(HaveOccurred()) + + Expect(r.reconcileRolloutsRoleBinding(ctx, a, role, sa)).To(Succeed()) + + By("Modify Subject of RoleBinding.") + rb := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultArgoRolloutsResourceName, + Namespace: a.Namespace, + }, + } + Expect(fetchObject(ctx, r.Client, a.Namespace, rb.Name, rb)).To(Succeed()) + + subTemp := rb.Subjects + rb.Subjects = append(rb.Subjects, rbacv1.Subject{Kind: rbacv1.ServiceAccountKind, Name: "test", Namespace: "test"}) + Expect(r.Client.Update(ctx, rb)).To(Succeed()) + + By("Reconciler should revert modifications.") + Expect(r.reconcileRolloutsRoleBinding(ctx, a, role, sa)).To(Succeed()) + Expect(fetchObject(ctx, r.Client, a.Namespace, rb.Name, rb)).To(Succeed()) + Expect(rb.Subjects).To(Equal(subTemp)) + }) + + It("Test for reconcileRolloutsClusterRoleBinding function", func() { + sa, err := r.reconcileRolloutsServiceAccount(ctx, a) + Expect(err).ToNot(HaveOccurred()) + clusterRole, err := r.reconcileRolloutsClusterRole(ctx, a) + Expect(err).ToNot(HaveOccurred()) + + Expect(r.reconcileRolloutsClusterRoleBinding(ctx, clusterRole, sa, a)).To(Succeed()) + + By("Modify Subject of ClusterRoleBinding.") + crb := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultArgoRolloutsResourceName, + }, + } + Expect(fetchObject(ctx, r.Client, "", crb.Name, crb)).To(Succeed()) + + subTemp := crb.Subjects + crb.Subjects = append(crb.Subjects, rbacv1.Subject{Kind: rbacv1.ServiceAccountKind, Name: "test", Namespace: "test"}) + Expect(r.Client.Update(ctx, crb)).To(Succeed()) + + By("Reconciler should revert modifications.") + Expect(r.reconcileRolloutsClusterRoleBinding(ctx, clusterRole, sa, a)).To(Succeed()) + Expect(fetchObject(ctx, r.Client, "", crb.Name, crb)).To(Succeed()) + Expect(crb.Subjects).To(Equal(subTemp)) + }) + + It("Test for reconcileRolloutsAggregateToAdminClusterRole function", func() { + Expect(r.reconcileRolloutsAggregateToAdminClusterRole(ctx, a)).To(Succeed()) + + By("Modify Rules of ClusterRole.") + clusterRole := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "argo-rollouts-aggregate-to-admin", + }, + } + Expect(fetchObject(ctx, r.Client, "", clusterRole.Name, clusterRole)).To(Succeed()) + clusterRole.Rules[0].Verbs = append(clusterRole.Rules[0].Verbs, "test") + Expect(r.Client.Update(ctx, clusterRole)).To(Succeed()) + + By("Reconciler should revert modifications.") + Expect(r.reconcileRolloutsAggregateToAdminClusterRole(ctx, a)).To(Succeed()) + Expect(fetchObject(ctx, r.Client, "", clusterRole.Name, clusterRole)).To(Succeed()) + Expect(clusterRole.Rules).To(Equal(GetAggregateToAdminPolicyRules())) + }) + + It("Test for reconcileRolloutsAggregateToEditClusterRole function", func() { + Expect(r.reconcileRolloutsAggregateToEditClusterRole(ctx, a)).To(Succeed()) + + By("Modify Rules of ClusterRole.") + clusterRole := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "argo-rollouts-aggregate-to-edit", + }, + } + Expect(fetchObject(ctx, r.Client, "", clusterRole.Name, clusterRole)).To(Succeed()) + clusterRole.Rules[0].Verbs = append(clusterRole.Rules[0].Verbs, "test") + Expect(r.Client.Update(ctx, clusterRole)).To(Succeed()) + + By("Reconciler should revert modifications.") + Expect(r.reconcileRolloutsAggregateToEditClusterRole(ctx, a)).To(Succeed()) + Expect(fetchObject(ctx, r.Client, "", clusterRole.Name, clusterRole)).To(Succeed()) + Expect(clusterRole.Rules).To(Equal(GetAggregateToEditPolicyRules())) + }) + + It("Test for reconcileRolloutsAggregateToViewClusterRole function", func() { + Expect(r.reconcileRolloutsAggregateToViewClusterRole(ctx, a)).To(Succeed()) + + By("Modify Rules of ClusterRole.") + clusterRole := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "argo-rollouts-aggregate-to-view", + }, + } + Expect(fetchObject(ctx, r.Client, "", clusterRole.Name, clusterRole)).To(Succeed()) + clusterRole.Rules[0].Verbs = append(clusterRole.Rules[0].Verbs, "test") + Expect(r.Client.Update(ctx, clusterRole)).To(Succeed()) + + By("Reconciler should revert modifications.") + Expect(r.reconcileRolloutsAggregateToViewClusterRole(ctx, a)).To(Succeed()) + Expect(fetchObject(ctx, r.Client, "", clusterRole.Name, clusterRole)).To(Succeed()) + Expect(clusterRole.Rules).To(Equal(GetAggregateToViewPolicyRules())) + }) + + It("Test for reconcileRolloutsMetricsService function", func() { + Expect(r.reconcileRolloutsMetricsServiceAndMonitor(ctx, a)).To(Succeed()) + }) + + It("Test for reconcileRolloutsSecrets function", func() { + Expect(r.reconcileRolloutsSecrets(ctx, a)).To(Succeed()) + }) + + It("test for removeClusterScopedResourcesIfApplicable function", func() { + + By("creating default cluster-scoped ClusterRole/ClusterRoleBinding. These should be deleted by the call to removeClusterScopedResourcesIfApplicable") + clusterRole := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultArgoRolloutsResourceName, + }, + } + Expect(r.Client.Create(ctx, clusterRole)).To(Succeed()) + + clusterRoleBinding := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultArgoRolloutsResourceName, + }, + } + Expect(r.Client.Create(ctx, clusterRoleBinding)).To(Succeed()) + + By("creating default cluster-scoped ClusterRole/ClusterRoleBinding with a different name. These should not be deleted") + + unrelatedRole := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unrelated-resource-should-not-be-deleted", + }, + } + Expect(r.Client.Create(ctx, unrelatedRole)).To(Succeed()) + + unrelatedRoleBinding := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unrelated-resource-should-not-be-deleted", + }, + } + Expect(r.Client.Create(ctx, unrelatedRoleBinding)).To(Succeed()) + + By("calling removeClusterScopedResourcesIfApplicable, which should delete the cluster scoped resources") + Expect(r.removeClusterScopedResourcesIfApplicable(ctx)).To(Succeed()) + + Expect(r.Client.Get(ctx, client.ObjectKeyFromObject(clusterRole), clusterRole)).ToNot(Succeed(), + "ClusterRole should have been deleted") + Expect(r.Client.Get(ctx, client.ObjectKeyFromObject(clusterRoleBinding), clusterRoleBinding)).ToNot(Succeed(), "ClusterRoleBinding should have been deleted") + + Expect(r.Client.Get(ctx, client.ObjectKeyFromObject(unrelatedRole), unrelatedRole)).To(Succeed(), + "Unrelated ClusterRole should not have been deleted") + Expect(r.Client.Get(ctx, client.ObjectKeyFromObject(unrelatedRoleBinding), unrelatedRoleBinding)).To(Succeed(), "Unrelated ClusterRoleBinding should not have been deleted") + + Expect(r.removeClusterScopedResourcesIfApplicable(ctx)).To(Succeed(), "calling the function again should not return an error") + + }) + }) + + Context("Verify resource creation when RolloutManger contains a user-defined label/annotation", func() { var ( ctx context.Context a v1alpha1.RolloutManager @@ -268,7 +483,7 @@ var _ = Describe("Resource creation and cleanup tests", func() { }) It("Test for reconcileRolloutsMetricsService function", func() { - Expect(r.reconcileRolloutsMetricsService(ctx, a)).To(Succeed()) + Expect(r.reconcileRolloutsMetricsServiceAndMonitor(ctx, a)).To(Succeed()) service := &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: DefaultArgoRolloutsMetricsServiceName, @@ -527,7 +742,7 @@ var _ = Describe("Resource creation and cleanup tests", func() { } Expect(r.Client.Create(ctx, svc)).To(Succeed()) - err = r.reconcileRolloutsMetricsService(ctx, a) + err = r.reconcileRolloutsMetricsServiceAndMonitor(ctx, a) Expect(err).ToNot(HaveOccurred()) Expect(fetchObject(ctx, r.Client, a.Namespace, svc.Name, svc)).To(Succeed()) diff --git a/controllers/utils.go b/controllers/utils.go index 352fda8..55de897 100644 --- a/controllers/utils.go +++ b/controllers/utils.go @@ -402,7 +402,7 @@ func createCondition(message string, reason ...string) metav1.Condition { } } -// removeUserLabelsAndAnnotations will remove any miscellaneous labels/annotations from obj, that are not used or expected by argo-rollouts-manager. For example, if a user added a label, "my-key": "my-value", to annotations of a Role that is created by our operator, this function wouls remove that label from 'obj'. +// removeUserLabelsAndAnnotations will remove any miscellaneous labels/annotations from obj, that are not used or expected by argo-rollouts-manager. For example, if a user added a label, "my-key": "my-value", to annotations of a Role that is created by our operator, this function would remove that label from 'obj'. func removeUserLabelsAndAnnotations(obj *metav1.ObjectMeta, cr rolloutsmanagerv1alpha1.RolloutManager) { defaultLabelsAndAnnotations := metav1.ObjectMeta{} diff --git a/controllers/utils_test.go b/controllers/utils_test.go index 3b8a79b..1af2527 100644 --- a/controllers/utils_test.go +++ b/controllers/utils_test.go @@ -445,13 +445,25 @@ var _ = Describe("removeUserLabelsAndAnnotations tests", func() { "app.kubernetes.io/component": DefaultArgoRolloutsResourceName}, map[string]string{}, ), - Entry("when user-defined labels are present and annotations are empty", + Entry("when user-defined labels are empty and annotations are present", map[string]string{}, map[string]string{"user-annotation": "value"}, map[string]string{"app.kubernetes.io/name": DefaultArgoRolloutsResourceName, "app.kubernetes.io/part-of": DefaultArgoRolloutsResourceName, "app.kubernetes.io/component": DefaultArgoRolloutsResourceName}, map[string]string{}, ), + + Entry("when both user and non-user-defined labels are present, and annotations are empty", + map[string]string{"user-label": "value", + "app.kubernetes.io/name": DefaultArgoRolloutsResourceName, + "app.kubernetes.io/part-of": DefaultArgoRolloutsResourceName, + "app.kubernetes.io/component": DefaultArgoRolloutsResourceName}, + map[string]string{}, + map[string]string{"app.kubernetes.io/name": DefaultArgoRolloutsResourceName, + "app.kubernetes.io/part-of": DefaultArgoRolloutsResourceName, + "app.kubernetes.io/component": DefaultArgoRolloutsResourceName}, + map[string]string{}, + ), ) })