From 330050b4da6719964a39d0dd15e524c58e61756d Mon Sep 17 00:00:00 2001 From: Matt Rogers Date: Mon, 18 Jul 2022 11:16:17 -0400 Subject: [PATCH 1/2] Use namespace when creating ServiceMonitor Take the given operator namespace into account when setting the ServiceMonitor ServerName field. Previously it was always assumed to be openshift-compliance. Also, add cmd/manager/operator_test.go with a unit test for the ServiceMonitor, with some refactoring. ref: https://bugzilla.redhat.com/show_bug.cgi?id=2060726 --- cmd/manager/operator.go | 60 ++++++++++++++++++++++-------------- cmd/manager/operator_test.go | 26 ++++++++++++++++ 2 files changed, 63 insertions(+), 23 deletions(-) create mode 100644 cmd/manager/operator_test.go diff --git a/cmd/manager/operator.go b/cmd/manager/operator.go index b2466591b..8ac8e19ca 100644 --- a/cmd/manager/operator.go +++ b/cmd/manager/operator.go @@ -95,7 +95,6 @@ var ( } serviceMonitorBearerTokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/token" serviceMonitorTLSCAFile = "/etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt" - serviceMonitorTLSServerName = "metrics.openshift-compliance.svc" alertName = "compliance" ) @@ -321,7 +320,7 @@ func addMetrics(ctx context.Context, cfg *rest.Config, kClient *kubernetes.Clien os.Exit(1) } - if err := createServiceMonitor(ctx, cfg, mClient, operatorNs, metricsService); err != nil { + if err := handleServiceMonitor(ctx, cfg, mClient, operatorNs, metricsService); err != nil { log.Error(err, "Error creating ServiceMonitor") os.Exit(1) } @@ -332,10 +331,8 @@ func addMetrics(ctx context.Context, cfg *rest.Config, kClient *kubernetes.Clien } } -func ensureMetricsServiceAndSecret(ctx context.Context, kClient *kubernetes.Clientset, ns string) (*v1.Service, error) { - var mService *v1.Service - var err error - mService, err = kClient.CoreV1().Services(ns).Create(ctx, &v1.Service{ +func operatorMetricService(ns string) *v1.Service { + return &v1.Service{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ "name": "compliance-operator", @@ -372,7 +369,13 @@ func ensureMetricsServiceAndSecret(ctx context.Context, kClient *kubernetes.Clie }, Type: v1.ServiceTypeClusterIP, }, - }, metav1.CreateOptions{}) + } +} + +func ensureMetricsServiceAndSecret(ctx context.Context, kClient *kubernetes.Clientset, ns string) (*v1.Service, error) { + var mService *v1.Service + var err error + mService, err = kClient.CoreV1().Services(ns).Create(ctx, operatorMetricService(ns), metav1.CreateOptions{}) if err != nil && !kerr.IsAlreadyExists(err) { return nil, err } @@ -540,20 +543,7 @@ func getDefaultRoles(platform PlatformType) []string { return defaultRolesPerPlatform[PlatformGeneric] } -// createServiceMonitor attempts to create a ServiceMonitor out of service, and updates it to include the controller -// metrics paths. -func createServiceMonitor(ctx context.Context, cfg *rest.Config, mClient *monclientv1.MonitoringV1Client, - namespace string, service *v1.Service) error { - ok, err := k8sutil.ResourceExists(discovery.NewDiscoveryClientForConfigOrDie(cfg), - "monitoring.coreos.com/v1", "ServiceMonitor") - if err != nil { - return err - } - if !ok { - log.Info("Install prometheus-operator in your cluster to create ServiceMonitor objects") - return nil - } - +func generateOperatorServiceMonitor(service *v1.Service, namespace string) *monitoring.ServiceMonitor { serviceMonitor := metrics.GenerateServiceMonitor(service) for i := range serviceMonitor.Spec.Endpoints { if serviceMonitor.Spec.Endpoints[i].Port == ctrlMetrics.ControllerMetricsServiceName { @@ -562,12 +552,17 @@ func createServiceMonitor(ctx context.Context, cfg *rest.Config, mClient *moncli serviceMonitor.Spec.Endpoints[i].BearerTokenFile = serviceMonitorBearerTokenFile serviceMonitor.Spec.Endpoints[i].TLSConfig = &monitoring.TLSConfig{ CAFile: serviceMonitorTLSCAFile, - ServerName: serviceMonitorTLSServerName, + ServerName: "metrics." + namespace + ".svc", } } } + return serviceMonitor +} - _, err = mClient.ServiceMonitors(namespace).Create(ctx, serviceMonitor, metav1.CreateOptions{}) +// createOrUpdateServiceMonitor creates or updates the ServiceMonitor if it already exists. +func createOrUpdateServiceMonitor(ctx context.Context, mClient *monclientv1.MonitoringV1Client, + namespace string, serviceMonitor *monitoring.ServiceMonitor) error { + _, err := mClient.ServiceMonitors(namespace).Create(ctx, serviceMonitor, metav1.CreateOptions{}) if err != nil && !kerr.IsAlreadyExists(err) { return err } @@ -587,6 +582,25 @@ func createServiceMonitor(ctx context.Context, cfg *rest.Config, mClient *moncli return nil } +// handleServiceMonitor attempts to create a ServiceMonitor out of service, and updates it to include the controller +// metrics paths. +func handleServiceMonitor(ctx context.Context, cfg *rest.Config, mClient *monclientv1.MonitoringV1Client, + namespace string, service *v1.Service) error { + ok, err := k8sutil.ResourceExists(discovery.NewDiscoveryClientForConfigOrDie(cfg), + "monitoring.coreos.com/v1", "ServiceMonitor") + if err != nil { + return err + } + if !ok { + log.Info("Install prometheus-operator in your cluster to create ServiceMonitor objects") + return nil + } + + serviceMonitor := generateOperatorServiceMonitor(service, namespace) + + return createOrUpdateServiceMonitor(ctx, mClient, namespace, serviceMonitor) +} + // serveCRMetrics gets the Operator/CustomResource GVKs and generates metrics based on those types. // It serves those metrics on "http://metricsHost:operatorMetricsPort". func serveCRMetrics(cfg *rest.Config, operatorNs string) error { diff --git a/cmd/manager/operator_test.go b/cmd/manager/operator_test.go new file mode 100644 index 000000000..cd5be36f0 --- /dev/null +++ b/cmd/manager/operator_test.go @@ -0,0 +1,26 @@ +package main + +import ( + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + "github.com/openshift/compliance-operator/pkg/controller/metrics" +) + +var _ = Describe("Operator Startup Function tests", func() { + Context("Service Monitor Creation", func() { + When("Installing to non-controlled namespace", func() { + It("ServiceMonitor is generated with the proper TLSConfig ServerName", func() { + metricService := operatorMetricService("foobar") + sm := generateOperatorServiceMonitor(metricService, "foobar") + controllerMetricServiceFound := false + for _, ep := range sm.Spec.Endpoints { + if ep.Port == metrics.ControllerMetricsServiceName && ep.TLSConfig != nil { + Expect(ep.TLSConfig.ServerName).To(BeEquivalentTo("metrics.foobar.svc")) + controllerMetricServiceFound = true + } + } + Expect(controllerMetricServiceFound).To(BeTrue()) + }) + }) + }) +}) From 5af5ab61c4475fb6a603e0a44e6117bc25eaa17e Mon Sep 17 00:00:00 2001 From: Matt Rogers Date: Mon, 18 Jul 2022 14:05:06 -0400 Subject: [PATCH 2/2] e2e: Allow AGGREGATING metric to register up to 3 Previously we allowed the AGGREGATING metric test to adjust for 1 or 2, but 3 was recently seen in CI. (This metric is expected to vary slightly). --- tests/e2e/e2e_test.go | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index a826ca820..249829748 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -586,16 +586,23 @@ func TestE2E(t *testing.T) { fmt.Sprintf("compliance_operator_compliance_scan_status_total{name=\"%s\",phase=\"PENDING\",result=\"\"}", scanName): 1, fmt.Sprintf("compliance_operator_compliance_scan_status_total{name=\"%s\",phase=\"RUNNING\",result=\"NOT-AVAILABLE\"}", scanName): 1, } - metricsSet[aggrString] = 1 - err = assertEachMetric(t, namespace, metricsSet) - if err != nil { - // Aggregating may be 1 or 2... try again - metricsSet[aggrString] = 2 - secondTryErr := assertEachMetric(t, namespace, metricsSet) - if secondTryErr != nil { - return secondTryErr + + var metErr error + // Aggregating may be variable, could be registered 1 to 3 times. + for i := 1; i < 4; i++ { + metricsSet[aggrString] = i + err = assertEachMetric(t, namespace, metricsSet) + if err == nil { + metErr = nil + break } + metErr = err + } + + if metErr != nil { + return metErr } + return scanHasValidPVCReference(f, namespace, scanName) }, },