From fad875ed38e26d89819c1e02c0b866735ba2f0ae Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 1 Jun 2022 18:03:20 -0400 Subject: [PATCH 01/31] Try untagged prom client --- go.mod | 3 ++- go.sum | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 6cb5a10b..ccb5aeb5 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,8 @@ require ( github.com/cespare/xxhash/v2 v2.1.2 github.com/go-logr/logr v1.2.3 github.com/pkg/errors v0.9.1 - github.com/prometheus/client_golang v1.12.2 + github.com/prometheus/client_golang v1.12.2-0.20220421062905-4dcf02ec7b3c + // github.com/prometheus/client_golang v1.12.1 k8s.io/api v0.24.0 k8s.io/apimachinery v0.24.0 k8s.io/client-go v0.24.0 diff --git a/go.sum b/go.sum index 3d24dd32..a42e5e68 100644 --- a/go.sum +++ b/go.sum @@ -443,8 +443,8 @@ github.com/prometheus/client_golang v1.0.0/go.mod h1:db9x61etRT2tGnBNRi70OPL5Fsn github.com/prometheus/client_golang v1.7.1/go.mod h1:PY5Wy2awLA44sXw4AOSfFBetzPP4j5+D6mVACh+pe2M= github.com/prometheus/client_golang v1.11.0/go.mod h1:Z6t4BnS23TR94PD6BsDNk8yVqroYurpAkEiz0P2BEV0= github.com/prometheus/client_golang v1.12.1/go.mod h1:3Z9XVyYiZYEO+YQWt3RD2R3jrbd179Rt297l4aS6nDY= -github.com/prometheus/client_golang v1.12.2 h1:51L9cDoUHVrXx4zWYlcLQIZ+d+VXHgqnYKkIuq4g/34= -github.com/prometheus/client_golang v1.12.2/go.mod h1:3Z9XVyYiZYEO+YQWt3RD2R3jrbd179Rt297l4aS6nDY= +github.com/prometheus/client_golang v1.12.2-0.20220421062905-4dcf02ec7b3c h1:UPm1o0MgQzLM9Vv2RB3xAFgAOi3hIHvpb4fUHSGVxJo= +github.com/prometheus/client_golang v1.12.2-0.20220421062905-4dcf02ec7b3c/go.mod h1:hnQ3yqQt3g4fD/UXIaxxYrafyiYfxYUS9zsKetoOmXQ= github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo= github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= @@ -456,6 +456,7 @@ github.com/prometheus/common v0.4.1/go.mod h1:TNfzLD0ON7rHzMJeJkieUDPYmFC7Snx/y8 github.com/prometheus/common v0.10.0/go.mod h1:Tlit/dnDKsSWFlCLTWaA1cyBgKHSMdTB80sz/V91rCo= github.com/prometheus/common v0.26.0/go.mod h1:M7rCNAaPfAosfx8veZJCuw84e35h3Cfd9VFqTh1DIvc= github.com/prometheus/common v0.32.1/go.mod h1:vu+V0TpY+O6vW9J44gczi3Ap/oXXR10b+M/gUGO4Hls= +github.com/prometheus/common v0.33.0/go.mod h1:gB3sOl7P0TvJabZpLY5uQMpUqRCPPCyRLCZYc7JZTNE= github.com/prometheus/common v0.34.0 h1:RBmGO9d/FVjqHT0yUGQwBJhkwKV+wPCn7KGpvfab0uE= github.com/prometheus/common v0.34.0/go.mod h1:gB3sOl7P0TvJabZpLY5uQMpUqRCPPCyRLCZYc7JZTNE= github.com/prometheus/procfs v0.0.0-20181005140218-185b4288413d/go.mod h1:c3At6R/oaqEKCNdg8wHV1ftS6bRYblBhIjjI8uT2IGk= From 6dd86855d6e02a9df636028874747a96c21acdb8 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 1 Jun 2022 18:10:54 -0400 Subject: [PATCH 02/31] Use partial match deletion --- .../vulnerabilityreport_controller.go | 112 ++++++++++-------- 1 file changed, 64 insertions(+), 48 deletions(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 1b70b58e..d2a90183 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -29,7 +29,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/metrics" @@ -64,39 +63,25 @@ func (r *VulnerabilityReportReconciler) Reconcile(ctx context.Context, req ctrl. registerMetricsOnce.Do(r.registerMetrics) - // Once deleting metrics based on partial matches is supported, the logic here will change so we clear the metric for all names not belonging to our shard. - // Then we can only fetch reports for our shard. For now, we'll try to get the report once even if it isn't our shard so that we can later clear the metric. - - report := &aqua.VulnerabilityReport{} - if err := r.Client.Get(ctx, req.NamespacedName, report); err != nil { - if apierrors.IsNotFound(err) { - // Most likely the report was deleted. - return ctrl.Result{}, nil - } - - // Error reading the object. - r.Log.Error(err, "Unable to read report") - return ctrl.Result{}, err - } - - // We have fetched the report and have four possibilities: - // - Not deleting + belongs to our shard --> give it our label (to trigger reconciliation by any previous owner) and expose the metric. - // - Not deleting + does not belong to our shard --> remove it from our metrics. Keep the finalizer. - // - Deleting + belongs to our shard --> remove it from our metrics. Remove the finalizer. - // - Deleting + does not belong to our shard --> remove it from our metrics. Keep the finalizer. + // The report has changed, meaning our metrics are out of date for this report. Clear them. + VulnerabilitySummary.DeletePartialMatch(prometheus.Labels{"report_name": req.NamespacedName.String()}) + VulnerabilityInfo.DeletePartialMatch(prometheus.Labels{"report_name": req.NamespacedName.String()}) shouldOwn := r.ShardHelper.ShouldOwn(req.NamespacedName.String()) + if shouldOwn { - if report.DeletionTimestamp.IsZero() && shouldOwn { - - // Give the report our finalizer if it doesn't have one. - if !utils.SliceContains(report.GetFinalizers(), VulnerabilityReportFinalizer) { - ctrlutil.AddFinalizer(report, VulnerabilityReportFinalizer) - if err := r.Update(ctx, report); err != nil { - return ctrl.Result{}, err + // Try to get the report. It might not exist anymore, in which case we don't need to do anything. + report := &aqua.VulnerabilityReport{} + if err := r.Client.Get(ctx, req.NamespacedName, report); err != nil { + if apierrors.IsNotFound(err) { + // Most likely the report was deleted. + return ctrl.Result{}, nil } - } + // Error reading the object. + r.Log.Error(err, "Unable to read report") + return ctrl.Result{}, err + } r.Log.Info(fmt.Sprintf("Reconciled %s || Found (C/H/M/L/N/U): %d/%d/%d/%d/%d/%d", req.NamespacedName, report.Report.Summary.CriticalCount, @@ -106,7 +91,6 @@ func (r *VulnerabilityReportReconciler) Reconcile(ctx context.Context, req ctrl. report.Report.Summary.NoneCount, report.Report.Summary.UnknownCount, )) - // Publish summary and CVE metrics for this report. r.publishImageMetrics(report) @@ -116,26 +100,58 @@ func (r *VulnerabilityReportReconciler) Reconcile(ctx context.Context, req ctrl. if err != nil { r.Log.Error(err, "unable to add shard owner label") } - - } else { - // Unfortunately, we can't yet clear the series based on one label value, - // we have to reconstruct all of the label values to delete the series. - // That's the only reason the finalizer is needed at all. - // So we first clear our metrics for the report, and then remove the finalizer - // if we're the shard which owns this report. - - // Drop the report from our metrics. - r.clearImageMetrics(report) - - if shouldOwn && utils.SliceContains(report.GetFinalizers(), VulnerabilityReportFinalizer) { - // Remove the finalizer if we're the shard owner. - ctrlutil.RemoveFinalizer(report, VulnerabilityReportFinalizer) - if err := r.Update(ctx, report); err != nil { - return ctrl.Result{}, err - } - } } + // If we are the shard owner for this report, try to fetch it and expose metrics for it. + + // Once deleting metrics based on partial matches is supported, the logic here will change so we clear the metric for all names not belonging to our shard. + // Then we can only fetch reports for our shard. For now, we'll try to get the report once even if it isn't our shard so that we can later clear the metric. + + // We have fetched the report and have four possibilities: + // - Not deleting + belongs to our shard --> give it our label (to trigger reconciliation by any previous owner) and expose the metric. + // - Not deleting + does not belong to our shard --> remove it from our metrics. Keep the finalizer. + // - Deleting + belongs to our shard --> remove it from our metrics. Remove the finalizer. + // - Deleting + does not belong to our shard --> remove it from our metrics. Keep the finalizer. + + // if report.DeletionTimestamp.IsZero() && shouldOwn { + + // // Give the report our finalizer if it doesn't have one. + // if !utils.SliceContains(report.GetFinalizers(), VulnerabilityReportFinalizer) { + // ctrlutil.AddFinalizer(report, VulnerabilityReportFinalizer) + // if err := r.Update(ctx, report); err != nil { + // return ctrl.Result{}, err + // } + // } + + // r.Log.Info(fmt.Sprintf("Reconciled %s || Found (C/H/M/L/N/U): %d/%d/%d/%d/%d/%d", + // req.NamespacedName, + // report.Report.Summary.CriticalCount, + // report.Report.Summary.HighCount, + // report.Report.Summary.MediumCount, + // report.Report.Summary.LowCount, + // report.Report.Summary.NoneCount, + // report.Report.Summary.UnknownCount, + // )) + + // } else { + // // Unfortunately, we can't yet clear the series based on one label value, + // // we have to reconstruct all of the label values to delete the series. + // // That's the only reason the finalizer is needed at all. + // // So we first clear our metrics for the report, and then remove the finalizer + // // if we're the shard which owns this report. + + // // Drop the report from our metrics. + // r.clearImageMetrics(report) + + // if shouldOwn && utils.SliceContains(report.GetFinalizers(), VulnerabilityReportFinalizer) { + // // Remove the finalizer if we're the shard owner. + // ctrlutil.RemoveFinalizer(report, VulnerabilityReportFinalizer) + // if err := r.Update(ctx, report); err != nil { + // return ctrl.Result{}, err + // } + // } + // } + return utils.JitterRequeue(controllers.DefaultRequeueDuration, r.MaxJitterPercent, r.Log), nil } From 635405bac14bc8a5d6264273a6b82564db267a21 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 1 Jun 2022 18:13:46 -0400 Subject: [PATCH 03/31] Remove unused clear functions --- .../vulnerabilityreport_controller.go | 66 +++++++++---------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index d2a90183..6c8acdf2 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -182,15 +182,15 @@ func (r *VulnerabilityReportReconciler) SetupWithManager(mgr ctrl.Manager) error return nil } -func (r *VulnerabilityReportReconciler) clearImageMetrics(report *aqua.VulnerabilityReport) { +// func (r *VulnerabilityReportReconciler) clearImageMetrics(report *aqua.VulnerabilityReport) { - clearSummaryMetrics(report) +// clearSummaryMetrics(report) - // If we have custom metrics to delete, do it. - if len(r.TargetLabels) > 0 { - clearCustomMetrics(report, r.TargetLabels) - } -} +// // If we have custom metrics to delete, do it. +// if len(r.TargetLabels) > 0 { +// clearCustomMetrics(report, r.TargetLabels) +// } +// } func (r *VulnerabilityReportReconciler) publishImageMetrics(report *aqua.VulnerabilityReport) { @@ -245,20 +245,20 @@ func getCountPerSeverity(report *aqua.VulnerabilityReport) map[string]float64 { } } -func clearSummaryMetrics(report *aqua.VulnerabilityReport) { - summaryValues := valuesForReport(report, LabelsForGroup(labelGroupSummary)) +// func clearSummaryMetrics(report *aqua.VulnerabilityReport) { +// summaryValues := valuesForReport(report, LabelsForGroup(labelGroupSummary)) - // Delete the series for each severity. - for severity := range getCountPerSeverity(report) { - v := summaryValues - v["severity"] = severity +// // Delete the series for each severity. +// for severity := range getCountPerSeverity(report) { +// v := summaryValues +// v["severity"] = severity - // Expose the metric. - VulnerabilitySummary.Delete( - v, - ) - } -} +// // Expose the metric. +// VulnerabilitySummary.Delete( +// v, +// ) +// } +// } func publishSummaryMetrics(report *aqua.VulnerabilityReport) { summaryValues := valuesForReport(report, LabelsForGroup(labelGroupSummary)) @@ -275,23 +275,23 @@ func publishSummaryMetrics(report *aqua.VulnerabilityReport) { } } -func clearCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { - reportValues := valuesForReport(report, targetLabels) +// func clearCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { +// reportValues := valuesForReport(report, targetLabels) - for _, v := range report.Report.Vulnerabilities { - vulnValues := valuesForVulnerability(v, targetLabels) +// for _, v := range report.Report.Vulnerabilities { +// vulnValues := valuesForVulnerability(v, targetLabels) - // Include the Report-level values. - for label, value := range reportValues { - vulnValues[label] = value - } +// // Include the Report-level values. +// for label, value := range reportValues { +// vulnValues[label] = value +// } - // Delete the metric - VulnerabilityInfo.Delete( - vulnValues, - ) - } -} +// // Delete the metric +// VulnerabilityInfo.Delete( +// vulnValues, +// ) +// } +// } func publishCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { reportValues := valuesForReport(report, targetLabels) From c3eb856f7f732dea6e56d59fdbfd9d76fde43313 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 1 Jun 2022 18:51:16 -0400 Subject: [PATCH 04/31] Debug why report name isn't added --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 6c8acdf2..55972f9e 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -295,7 +295,7 @@ func publishSummaryMetrics(report *aqua.VulnerabilityReport) { func publishCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { reportValues := valuesForReport(report, targetLabels) - + fmt.Println(fmt.Sprintf("setting values for report %s", reportValues["report_name"])) for _, v := range report.Report.Vulnerabilities { vulnValues := valuesForVulnerability(v, targetLabels) From 5f6a1d43b6a641bf465b398ba68bcbebed747e12 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 1 Jun 2022 18:54:19 -0400 Subject: [PATCH 05/31] Make linter happy --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 55972f9e..b8743600 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -295,7 +295,7 @@ func publishSummaryMetrics(report *aqua.VulnerabilityReport) { func publishCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { reportValues := valuesForReport(report, targetLabels) - fmt.Println(fmt.Sprintf("setting values for report %s", reportValues["report_name"])) + fmt.Printf("setting values for report %s\n", reportValues["report_name"]) for _, v := range report.Report.Vulnerabilities { vulnValues := valuesForVulnerability(v, targetLabels) From d9a777343a0e25e0a1d8793f7c5e4d97f67ad4aa Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 1 Jun 2022 19:04:23 -0400 Subject: [PATCH 06/31] Print full report --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 1 + 1 file changed, 1 insertion(+) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index b8743600..0daa2eb7 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -296,6 +296,7 @@ func publishSummaryMetrics(report *aqua.VulnerabilityReport) { func publishCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { reportValues := valuesForReport(report, targetLabels) fmt.Printf("setting values for report %s\n", reportValues["report_name"]) + fmt.Printf("report:\n%v\n", report) for _, v := range report.Report.Vulnerabilities { vulnValues := valuesForVulnerability(v, targetLabels) From f6217b81be68bd9e513e41109a38f316549ae312 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 1 Jun 2022 19:08:08 -0400 Subject: [PATCH 07/31] Remove finalizers --- .../vulnerabilityreport_controller.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 0daa2eb7..30081943 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -82,6 +82,7 @@ func (r *VulnerabilityReportReconciler) Reconcile(ctx context.Context, req ctrl. r.Log.Error(err, "Unable to read report") return ctrl.Result{}, err } + r.Log.Info(fmt.Sprintf("Reconciled %s || Found (C/H/M/L/N/U): %d/%d/%d/%d/%d/%d", req.NamespacedName, report.Report.Summary.CriticalCount, @@ -94,6 +95,15 @@ func (r *VulnerabilityReportReconciler) Reconcile(ctx context.Context, req ctrl. // Publish summary and CVE metrics for this report. r.publishImageMetrics(report) + + if utils.SliceContains(report.GetFinalizers(), VulnerabilityReportFinalizer) { + // Remove the finalizer if we're the shard owner. + ctrlutil.RemoveFinalizer(report, VulnerabilityReportFinalizer) + if err := r.Update(ctx, report); err != nil { + return ctrl.Result{}, err + } + } + // Add a label to this report so any previous owners will reconcile and drop the metric. report.Labels[controllers.ShardOwnerLabel] = r.ShardHelper.PodIP err := r.Client.Update(ctx, report, &client.UpdateOptions{}) @@ -295,6 +305,7 @@ func publishSummaryMetrics(report *aqua.VulnerabilityReport) { func publishCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { reportValues := valuesForReport(report, targetLabels) + fmt.Printf("setting values for report %s/%s\n", report.Name, report.Report.) fmt.Printf("setting values for report %s\n", reportValues["report_name"]) fmt.Printf("report:\n%v\n", report) for _, v := range report.Report.Vulnerabilities { From e3b23f1d00967412a4442a251c445eaf0304f484 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 1 Jun 2022 19:11:44 -0400 Subject: [PATCH 08/31] Fix some syntax --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 30081943..76d5caf9 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/metrics" @@ -95,7 +96,6 @@ func (r *VulnerabilityReportReconciler) Reconcile(ctx context.Context, req ctrl. // Publish summary and CVE metrics for this report. r.publishImageMetrics(report) - if utils.SliceContains(report.GetFinalizers(), VulnerabilityReportFinalizer) { // Remove the finalizer if we're the shard owner. ctrlutil.RemoveFinalizer(report, VulnerabilityReportFinalizer) @@ -305,7 +305,7 @@ func publishSummaryMetrics(report *aqua.VulnerabilityReport) { func publishCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { reportValues := valuesForReport(report, targetLabels) - fmt.Printf("setting values for report %s/%s\n", report.Name, report.Report.) + fmt.Printf("setting values for report %s/%s\n", report.Name, report.ObjectMeta.Name) fmt.Printf("setting values for report %s\n", reportValues["report_name"]) fmt.Printf("report:\n%v\n", report) for _, v := range report.Report.Vulnerabilities { From b831501cba066bc8734f12cd9f7a179365667d2b Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 1 Jun 2022 19:58:42 -0400 Subject: [PATCH 09/31] More logging --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 76d5caf9..c0b6ca11 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -305,9 +305,10 @@ func publishSummaryMetrics(report *aqua.VulnerabilityReport) { func publishCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { reportValues := valuesForReport(report, targetLabels) - fmt.Printf("setting values for report %s/%s\n", report.Name, report.ObjectMeta.Name) - fmt.Printf("setting values for report %s\n", reportValues["report_name"]) - fmt.Printf("report:\n%v\n", report) + fmt.Printf("setting values for report:\nreport.Name: %s\nreport.ObjectMeta.Name: %s\nreportValues[report_name]: %s\n", + report.Name, report.ObjectMeta.Name, reportValues["report_name"]) + fmt.Printf("report_namespace:\n report.Namespace: %s\n reportValues[image_namespace]: %s\n", report.Namespace, reportValues["image_namespace"]) + // fmt.Printf("report:\n%v\n", report) for _, v := range report.Report.Vulnerabilities { vulnValues := valuesForVulnerability(v, targetLabels) @@ -352,6 +353,7 @@ func valuesForVulnerability(vuln aqua.Vulnerability, labels []VulnerabilityLabel func reportValueFor(field string, report *aqua.VulnerabilityReport) string { switch field { case "report_name": + fmt.Printf("found report_name: %s\n", report.Name) return report.Name case "image_namespace": return report.Namespace From 22a060a379e6a1d2f1c9bd9bec08f994a4567761 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 1 Jun 2022 20:12:43 -0400 Subject: [PATCH 10/31] More debugging --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index c0b6ca11..25bef5bf 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -335,8 +335,10 @@ func valuesForReport(report *aqua.VulnerabilityReport, labels []VulnerabilityLab for _, label := range labels { if label.Scope == FieldScopeReport { result[label.Name] = reportValueFor(label.Name, report) + fmt.Printf("result[%s]: %s", label.Name, result[label.Name]) } } + fmt.Printf("result: %v\n", result) return result } From 86acf8cf6f77b2d22639ab60fa6d948392de32c8 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Thu, 2 Jun 2022 08:50:45 -0400 Subject: [PATCH 11/31] Some newlines --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 25bef5bf..4c724c07 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -335,7 +335,7 @@ func valuesForReport(report *aqua.VulnerabilityReport, labels []VulnerabilityLab for _, label := range labels { if label.Scope == FieldScopeReport { result[label.Name] = reportValueFor(label.Name, report) - fmt.Printf("result[%s]: %s", label.Name, result[label.Name]) + fmt.Printf("result[%s]: %s\n", label.Name, result[label.Name]) } } fmt.Printf("result: %v\n", result) From f8913f344358c03599d49b12de171af029a6d5ad Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Fri, 3 Jun 2022 14:19:50 -0400 Subject: [PATCH 12/31] Moar logs --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 4c724c07..77c1bd7f 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -271,6 +271,7 @@ func getCountPerSeverity(report *aqua.VulnerabilityReport) map[string]float64 { // } func publishSummaryMetrics(report *aqua.VulnerabilityReport) { + fmt.Println("publishSummaryMetrics()") summaryValues := valuesForReport(report, LabelsForGroup(labelGroupSummary)) // Add the severity label after the standard labels and expose each severity metric. @@ -304,6 +305,7 @@ func publishSummaryMetrics(report *aqua.VulnerabilityReport) { // } func publishCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { + fmt.Println("publishCustomMetrics()") reportValues := valuesForReport(report, targetLabels) fmt.Printf("setting values for report:\nreport.Name: %s\nreport.ObjectMeta.Name: %s\nreportValues[report_name]: %s\n", report.Name, report.ObjectMeta.Name, reportValues["report_name"]) From 1cb430910e0f7f3391d01aed68c2ea094976c457 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Fri, 3 Jun 2022 14:24:56 -0400 Subject: [PATCH 13/31] Check report after first publish --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 77c1bd7f..928232fe 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -203,9 +203,9 @@ func (r *VulnerabilityReportReconciler) SetupWithManager(mgr ctrl.Manager) error // } func (r *VulnerabilityReportReconciler) publishImageMetrics(report *aqua.VulnerabilityReport) { - + fmt.Printf("Report before publish: %s\n", report.Name) publishSummaryMetrics(report) - + fmt.Printf("Report after publish: %s\n", report.Name) // If we have custom metrics to expose, do it. if len(r.TargetLabels) > 0 { publishCustomMetrics(report, r.TargetLabels) From 8060074e225773335d2d152e144a65f929fe78b8 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Fri, 3 Jun 2022 14:43:11 -0400 Subject: [PATCH 14/31] Always enable report_name when exposing detail metrics --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 3 ++- main.go | 4 ++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 928232fe..821c29ef 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -273,7 +273,8 @@ func getCountPerSeverity(report *aqua.VulnerabilityReport) map[string]float64 { func publishSummaryMetrics(report *aqua.VulnerabilityReport) { fmt.Println("publishSummaryMetrics()") summaryValues := valuesForReport(report, LabelsForGroup(labelGroupSummary)) - + fmt.Printf("setting values for report:\nreport.Name: %s\nreport.ObjectMeta.Name: %s\nsummaryValues[report_name]: %s\n", + report.Name, report.ObjectMeta.Name, summaryValues["report_name"]) // Add the severity label after the standard labels and expose each severity metric. for severity, count := range getCountPerSeverity(report) { v := summaryValues diff --git a/main.go b/main.go index 54cf6ae7..f10b8a7f 100644 --- a/main.go +++ b/main.go @@ -116,6 +116,10 @@ func main() { targetLabels = appendIfNotExists(targetLabels, []vulnerabilityreport.VulnerabilityLabel{label}) } + // If exposing detail metrics, we must always include the report name in order to delete them by name later. + reportNameLabel, _ := vulnerabilityreport.LabelWithName("report_name") + targetLabels = appendIfNotExists(targetLabels, []vulnerabilityreport.VulnerabilityLabel{reportNameLabel}) + return nil }) From bb98aceaaaa5ad4fef4f3902c758dcbb4e315f3f Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Mon, 6 Jun 2022 11:53:02 -0400 Subject: [PATCH 15/31] Add log when clearing metrics --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 821c29ef..c5c19e50 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -65,8 +65,12 @@ func (r *VulnerabilityReportReconciler) Reconcile(ctx context.Context, req ctrl. registerMetricsOnce.Do(r.registerMetrics) // The report has changed, meaning our metrics are out of date for this report. Clear them. - VulnerabilitySummary.DeletePartialMatch(prometheus.Labels{"report_name": req.NamespacedName.String()}) - VulnerabilityInfo.DeletePartialMatch(prometheus.Labels{"report_name": req.NamespacedName.String()}) + deletedSummaries := VulnerabilitySummary.DeletePartialMatch(prometheus.Labels{"report_name": req.NamespacedName.String()}) + deletedDetails := VulnerabilityInfo.DeletePartialMatch(prometheus.Labels{"report_name": req.NamespacedName.String()}) + r.Log.Info(fmt.Sprintf("cleared %d summary and %d detail metrics", deletedSummaries, deletedDetails)) + + r.Log.Info(fmt.Sprintf("Peer %s should handle report %s", r.ShardHelper.GetShardOwner(req.NamespacedName.String()), req.NamespacedName.String())) + r.Log.Info(fmt.Sprintf("This instance should own report: %v", r.ShardHelper.ShouldOwn(req.NamespacedName.String()))) shouldOwn := r.ShardHelper.ShouldOwn(req.NamespacedName.String()) if shouldOwn { From 15d7d550df7e35bb037f820114131fa9b8a65508 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Mon, 6 Jun 2022 11:54:47 -0400 Subject: [PATCH 16/31] Remove debug logs --- .../vulnerabilityreport_controller.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index c5c19e50..64b690f4 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -207,9 +207,7 @@ func (r *VulnerabilityReportReconciler) SetupWithManager(mgr ctrl.Manager) error // } func (r *VulnerabilityReportReconciler) publishImageMetrics(report *aqua.VulnerabilityReport) { - fmt.Printf("Report before publish: %s\n", report.Name) publishSummaryMetrics(report) - fmt.Printf("Report after publish: %s\n", report.Name) // If we have custom metrics to expose, do it. if len(r.TargetLabels) > 0 { publishCustomMetrics(report, r.TargetLabels) @@ -275,10 +273,7 @@ func getCountPerSeverity(report *aqua.VulnerabilityReport) map[string]float64 { // } func publishSummaryMetrics(report *aqua.VulnerabilityReport) { - fmt.Println("publishSummaryMetrics()") summaryValues := valuesForReport(report, LabelsForGroup(labelGroupSummary)) - fmt.Printf("setting values for report:\nreport.Name: %s\nreport.ObjectMeta.Name: %s\nsummaryValues[report_name]: %s\n", - report.Name, report.ObjectMeta.Name, summaryValues["report_name"]) // Add the severity label after the standard labels and expose each severity metric. for severity, count := range getCountPerSeverity(report) { v := summaryValues @@ -310,11 +305,7 @@ func publishSummaryMetrics(report *aqua.VulnerabilityReport) { // } func publishCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { - fmt.Println("publishCustomMetrics()") reportValues := valuesForReport(report, targetLabels) - fmt.Printf("setting values for report:\nreport.Name: %s\nreport.ObjectMeta.Name: %s\nreportValues[report_name]: %s\n", - report.Name, report.ObjectMeta.Name, reportValues["report_name"]) - fmt.Printf("report_namespace:\n report.Namespace: %s\n reportValues[image_namespace]: %s\n", report.Namespace, reportValues["image_namespace"]) // fmt.Printf("report:\n%v\n", report) for _, v := range report.Report.Vulnerabilities { vulnValues := valuesForVulnerability(v, targetLabels) @@ -342,10 +333,8 @@ func valuesForReport(report *aqua.VulnerabilityReport, labels []VulnerabilityLab for _, label := range labels { if label.Scope == FieldScopeReport { result[label.Name] = reportValueFor(label.Name, report) - fmt.Printf("result[%s]: %s\n", label.Name, result[label.Name]) } } - fmt.Printf("result: %v\n", result) return result } @@ -362,7 +351,6 @@ func valuesForVulnerability(vuln aqua.Vulnerability, labels []VulnerabilityLabel func reportValueFor(field string, report *aqua.VulnerabilityReport) string { switch field { case "report_name": - fmt.Printf("found report_name: %s\n", report.Name) return report.Name case "image_namespace": return report.Namespace From ff9f8de9196f0f6fbf70eedf6e32feff0d66e3eb Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Mon, 6 Jun 2022 12:18:26 -0400 Subject: [PATCH 17/31] Add log when updating endpoints --- utils/sharding.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/utils/sharding.go b/utils/sharding.go index a41adc32..a3b723a4 100644 --- a/utils/sharding.go +++ b/utils/sharding.go @@ -1,6 +1,7 @@ package utils import ( + "fmt" "os" "sync" @@ -114,21 +115,13 @@ func BuildPeerInformer(stopper chan struct{}, peerRing *ShardHelper, ringConfig // Set handlers for new/updated endpoints. handlers := cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - added, kept, _, ok := getEndpointChanges(obj, nil, log) - if !ok { - return - } - peerRing.SetMembersFromLists(added, kept) + updateEndpoints(obj, nil, peerRing, log) }, UpdateFunc: func(oldObj, newObj interface{}) { // In the future, we might need to re-queue objects which belong to deleted peers. // When scaling down, it is possible that metrics will be double reported for up to the reconciliation period. // For now, we'll just set the desired peers. - added, kept, _, ok := getEndpointChanges(newObj, oldObj, log) - if !ok { - return - } - peerRing.SetMembersFromLists(added, kept) + updateEndpoints(oldObj, newObj, peerRing, log) }, // We can add a delete handler here. Not sure yet what it should do. } @@ -137,6 +130,15 @@ func BuildPeerInformer(stopper chan struct{}, peerRing *ShardHelper, ringConfig return informer } +func updateEndpoints(currentObj interface{}, previousObj interface{}, ring *ShardHelper, log logr.Logger) { + added, kept, _, ok := getEndpointChanges(currentObj, previousObj, log) + if !ok { + return + } + ring.SetMembersFromLists(added, kept) + log.Info(fmt.Sprintf("found %d peers from service endpoints", len(added)+len(kept))) +} + // getEndpointChanges takes a current and optional previous object and returns the added, kept, and removed items, plus a success boolean. func getEndpointChanges(currentObj interface{}, previousObj interface{}, log logr.Logger) ([]string, []string, []string, bool) { current, err := toEndpoint(currentObj, log) From bf8b22d4bf3cafaf9953a854b48e970e3941ce69 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Mon, 6 Jun 2022 15:27:19 -0400 Subject: [PATCH 18/31] Fix shard endpoint call --- utils/sharding.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/utils/sharding.go b/utils/sharding.go index a3b723a4..8ac84b18 100644 --- a/utils/sharding.go +++ b/utils/sharding.go @@ -121,7 +121,7 @@ func BuildPeerInformer(stopper chan struct{}, peerRing *ShardHelper, ringConfig // In the future, we might need to re-queue objects which belong to deleted peers. // When scaling down, it is possible that metrics will be double reported for up to the reconciliation period. // For now, we'll just set the desired peers. - updateEndpoints(oldObj, newObj, peerRing, log) + updateEndpoints(newObj, oldObj, peerRing, log) }, // We can add a delete handler here. Not sure yet what it should do. } @@ -136,7 +136,7 @@ func updateEndpoints(currentObj interface{}, previousObj interface{}, ring *Shar return } ring.SetMembersFromLists(added, kept) - log.Info(fmt.Sprintf("found %d peers from service endpoints", len(added)+len(kept))) + log.Info(fmt.Sprintf("updated peer list with %d endpoints", len(added)+len(kept))) } // getEndpointChanges takes a current and optional previous object and returns the added, kept, and removed items, plus a success boolean. From a061997b4c62bdc4cb1d2db20052dd3c12eb212b Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Mon, 6 Jun 2022 15:52:06 -0400 Subject: [PATCH 19/31] Use namespacedname as report name --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 64b690f4..ef5c2453 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -27,6 +27,7 @@ import ( "github.com/prometheus/client_golang/prometheus" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + apitypes "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -351,7 +352,8 @@ func valuesForVulnerability(vuln aqua.Vulnerability, labels []VulnerabilityLabel func reportValueFor(field string, report *aqua.VulnerabilityReport) string { switch field { case "report_name": - return report.Name + return apitypes.NamespacedName{Name: report.Name, Namespace: report.Namespace} + // return report.Name case "image_namespace": return report.Namespace case "image_registry": From 33deecef819cf5c1687e300947f396d1024e7f97 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Mon, 6 Jun 2022 15:59:13 -0400 Subject: [PATCH 20/31] Stringify namespacedname --- .../vulnerabilityreport/vulnerabilityreport_controller.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index ef5c2453..d8dbc677 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -352,8 +352,7 @@ func valuesForVulnerability(vuln aqua.Vulnerability, labels []VulnerabilityLabel func reportValueFor(field string, report *aqua.VulnerabilityReport) string { switch field { case "report_name": - return apitypes.NamespacedName{Name: report.Name, Namespace: report.Namespace} - // return report.Name + return apitypes.NamespacedName{Name: report.Name, Namespace: report.Namespace}.String() case "image_namespace": return report.Namespace case "image_registry": From 16cf24cee2f8d8812e1c03d7b66a479d29cd64d0 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Mon, 6 Jun 2022 16:21:24 -0400 Subject: [PATCH 21/31] Cleanup --- .../vulnerabilityreport_controller.go | 103 +----------------- 1 file changed, 5 insertions(+), 98 deletions(-) diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index d8dbc677..366bc400 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -68,10 +68,6 @@ func (r *VulnerabilityReportReconciler) Reconcile(ctx context.Context, req ctrl. // The report has changed, meaning our metrics are out of date for this report. Clear them. deletedSummaries := VulnerabilitySummary.DeletePartialMatch(prometheus.Labels{"report_name": req.NamespacedName.String()}) deletedDetails := VulnerabilityInfo.DeletePartialMatch(prometheus.Labels{"report_name": req.NamespacedName.String()}) - r.Log.Info(fmt.Sprintf("cleared %d summary and %d detail metrics", deletedSummaries, deletedDetails)) - - r.Log.Info(fmt.Sprintf("Peer %s should handle report %s", r.ShardHelper.GetShardOwner(req.NamespacedName.String()), req.NamespacedName.String())) - r.Log.Info(fmt.Sprintf("This instance should own report: %v", r.ShardHelper.ShouldOwn(req.NamespacedName.String()))) shouldOwn := r.ShardHelper.ShouldOwn(req.NamespacedName.String()) if shouldOwn { @@ -115,58 +111,12 @@ func (r *VulnerabilityReportReconciler) Reconcile(ctx context.Context, req ctrl. if err != nil { r.Log.Error(err, "unable to add shard owner label") } + } else { + if deletedSummaries > 0 || deletedDetails > 0 { + r.Log.Info(fmt.Sprintf("cleared %d summary and %d detail metrics", deletedSummaries, deletedDetails)) + } } - // If we are the shard owner for this report, try to fetch it and expose metrics for it. - - // Once deleting metrics based on partial matches is supported, the logic here will change so we clear the metric for all names not belonging to our shard. - // Then we can only fetch reports for our shard. For now, we'll try to get the report once even if it isn't our shard so that we can later clear the metric. - - // We have fetched the report and have four possibilities: - // - Not deleting + belongs to our shard --> give it our label (to trigger reconciliation by any previous owner) and expose the metric. - // - Not deleting + does not belong to our shard --> remove it from our metrics. Keep the finalizer. - // - Deleting + belongs to our shard --> remove it from our metrics. Remove the finalizer. - // - Deleting + does not belong to our shard --> remove it from our metrics. Keep the finalizer. - - // if report.DeletionTimestamp.IsZero() && shouldOwn { - - // // Give the report our finalizer if it doesn't have one. - // if !utils.SliceContains(report.GetFinalizers(), VulnerabilityReportFinalizer) { - // ctrlutil.AddFinalizer(report, VulnerabilityReportFinalizer) - // if err := r.Update(ctx, report); err != nil { - // return ctrl.Result{}, err - // } - // } - - // r.Log.Info(fmt.Sprintf("Reconciled %s || Found (C/H/M/L/N/U): %d/%d/%d/%d/%d/%d", - // req.NamespacedName, - // report.Report.Summary.CriticalCount, - // report.Report.Summary.HighCount, - // report.Report.Summary.MediumCount, - // report.Report.Summary.LowCount, - // report.Report.Summary.NoneCount, - // report.Report.Summary.UnknownCount, - // )) - - // } else { - // // Unfortunately, we can't yet clear the series based on one label value, - // // we have to reconstruct all of the label values to delete the series. - // // That's the only reason the finalizer is needed at all. - // // So we first clear our metrics for the report, and then remove the finalizer - // // if we're the shard which owns this report. - - // // Drop the report from our metrics. - // r.clearImageMetrics(report) - - // if shouldOwn && utils.SliceContains(report.GetFinalizers(), VulnerabilityReportFinalizer) { - // // Remove the finalizer if we're the shard owner. - // ctrlutil.RemoveFinalizer(report, VulnerabilityReportFinalizer) - // if err := r.Update(ctx, report); err != nil { - // return ctrl.Result{}, err - // } - // } - // } - return utils.JitterRequeue(controllers.DefaultRequeueDuration, r.MaxJitterPercent, r.Log), nil } @@ -197,16 +147,6 @@ func (r *VulnerabilityReportReconciler) SetupWithManager(mgr ctrl.Manager) error return nil } -// func (r *VulnerabilityReportReconciler) clearImageMetrics(report *aqua.VulnerabilityReport) { - -// clearSummaryMetrics(report) - -// // If we have custom metrics to delete, do it. -// if len(r.TargetLabels) > 0 { -// clearCustomMetrics(report, r.TargetLabels) -// } -// } - func (r *VulnerabilityReportReconciler) publishImageMetrics(report *aqua.VulnerabilityReport) { publishSummaryMetrics(report) // If we have custom metrics to expose, do it. @@ -258,21 +198,6 @@ func getCountPerSeverity(report *aqua.VulnerabilityReport) map[string]float64 { } } -// func clearSummaryMetrics(report *aqua.VulnerabilityReport) { -// summaryValues := valuesForReport(report, LabelsForGroup(labelGroupSummary)) - -// // Delete the series for each severity. -// for severity := range getCountPerSeverity(report) { -// v := summaryValues -// v["severity"] = severity - -// // Expose the metric. -// VulnerabilitySummary.Delete( -// v, -// ) -// } -// } - func publishSummaryMetrics(report *aqua.VulnerabilityReport) { summaryValues := valuesForReport(report, LabelsForGroup(labelGroupSummary)) // Add the severity label after the standard labels and expose each severity metric. @@ -287,27 +212,8 @@ func publishSummaryMetrics(report *aqua.VulnerabilityReport) { } } -// func clearCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { -// reportValues := valuesForReport(report, targetLabels) - -// for _, v := range report.Report.Vulnerabilities { -// vulnValues := valuesForVulnerability(v, targetLabels) - -// // Include the Report-level values. -// for label, value := range reportValues { -// vulnValues[label] = value -// } - -// // Delete the metric -// VulnerabilityInfo.Delete( -// vulnValues, -// ) -// } -// } - func publishCustomMetrics(report *aqua.VulnerabilityReport, targetLabels []VulnerabilityLabel) { reportValues := valuesForReport(report, targetLabels) - // fmt.Printf("report:\n%v\n", report) for _, v := range report.Report.Vulnerabilities { vulnValues := valuesForVulnerability(v, targetLabels) @@ -352,6 +258,7 @@ func valuesForVulnerability(vuln aqua.Vulnerability, labels []VulnerabilityLabel func reportValueFor(field string, report *aqua.VulnerabilityReport) string { switch field { case "report_name": + // Construct the namespacedname which we'll later be given at reconciliation. return apitypes.NamespacedName{Name: report.Name, Namespace: report.Namespace}.String() case "image_namespace": return report.Namespace From 711ea18cb27af1293dcde1957ca9d6f1436eb4d0 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Tue, 7 Jun 2022 17:36:58 -0400 Subject: [PATCH 22/31] WIP basic shard tests --- utils/sharding_test.go | 122 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 utils/sharding_test.go diff --git a/utils/sharding_test.go b/utils/sharding_test.go new file mode 100644 index 00000000..0de28879 --- /dev/null +++ b/utils/sharding_test.go @@ -0,0 +1,122 @@ +package utils + +import ( + "fmt" + "strconv" + "testing" + + "github.com/go-logr/logr/testr" + corev1 "k8s.io/api/core/v1" +) + +func Test_getEndpointChanges(t *testing.T) { + testCases := []struct { + field string + anotherfield string + }{ + { + field: "todo", + }, + { + field: "todo", + anotherfield: "todo", + }, + } + + // u := &unstructured.Unstructured{ + // Object: map[string]interface{}{ + // "apiVersion": "cr.bar.com/v1", + // "kind": "Foo", + // "spec": map[string]interface{}{"field": 1}, + // "metadata": map[string]interface{}{ + // "name": "test-1", + // "annotations": map[string]interface{}{ + // "foo": "bar", + // }, + // }, + // }, + // } + + // a1 := map[string]interface{}{ + // "ip": "10.0.131.187", + // "nodeName": "worker-000026", + // "targetRef": map[string]string{ + // "kind": "Pod", + // "name": "starboard-exporter-765756bd69-vzx49", + // "namespace": "giantswarm", + // }, + // } + + // addresses := make([]map[string]interface{}) + // addresses = append(addresses) + + // ep := &unstructured.Unstructured{ + // Object: map[string]interface{}{ + // "apiVersion": "v1", + // "kind": "Endpoints", + // "metadata": map[string]interface{}{ + // "name": "starboard-exporter", + // "namespace": "giantswarm", + // }, + // // "subsets": map[string][]map[string]interface{}{ + // "subsets": map[string]interface{}{ + // "addresses": []map[string]interface{}{ + // { + // "ip": "10.0.131.187", + // "nodeName": "worker-000026", + // "targetRef": map[string]string{ + // "kind": "Pod", + // "name": "starboard-exporter-765756bd69-vzx49", + // "namespace": "giantswarm", + // }, + // }, + // }, + // "ports": []map[string]interface{}{ + // { + // "name": "metrics", + // "port": "8080", + // "protocol": "TCP", + // }, + // }, + // }, + // }, + // } + + epp := &corev1.Endpoints{} + subsets := corev1.EndpointSubset{ + Addresses: []corev1.EndpointAddress{ + { + IP: "1.2.3.4", + // NodeName: "worker-000026", + }, + }, + } + + epp.Subsets = []corev1.EndpointSubset{subsets} + + log := testr.New(t) + + for i, testCase := range testCases { + t.Run(strconv.Itoa(i), func(t *testing.T) { + log.Info("test") + log.Info(fmt.Sprintf("%v", epp)) + added, kept, removed, ok := getEndpointChanges(epp, nil, log) + t.Logf("case %s: added: %s, kept: %s, removed: %s\n", testCase, added, kept, removed) + + if !ok { + t.Fatalf("case %s: added: %s, kept: %s, removed: %s\n", testCase, added, kept, removed) + } + }) + } +} + +// { +// - ip: 10.0.133.125 +// nodeName: worker-000025 +// targetRef: +// kind: Pod +// name: starboard-exporter-765756bd69-r795x +// namespace: giantswarm +// resourceVersion: "694068745" +// uid: 02758daa-e56a-499f-af8a-fb795890700f +// } From 5c97b887be55d8f1cd39c3391e1f54ffe473aab2 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Tue, 7 Jun 2022 18:10:56 -0400 Subject: [PATCH 23/31] Refactor endpoint updates and add first basic test --- go.mod | 2 + go.sum | 2 + utils/sharding.go | 45 ++++++++++---- utils/sharding_test.go | 135 ++++++++++++----------------------------- 4 files changed, 76 insertions(+), 108 deletions(-) diff --git a/go.mod b/go.mod index ccb5aeb5..10d643e5 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,8 @@ require ( sigs.k8s.io/controller-runtime v0.12.1 ) +require gotest.tools v2.2.0+incompatible + require ( cloud.google.com/go/compute v1.6.1 // indirect github.com/Azure/go-autorest v14.2.0+incompatible // indirect diff --git a/go.sum b/go.sum index a42e5e68..4da905b9 100644 --- a/go.sum +++ b/go.sum @@ -1066,6 +1066,8 @@ gopkg.in/yaml.v3 v3.0.0-20200615113413-eeeca48fe776/go.mod h1:K4uyk7z7BCEPqu6E+C gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.0 h1:hjy8E9ON/egN1tAYqKb61G10WtihqetD4sz2H+8nIeA= gopkg.in/yaml.v3 v3.0.0/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gotest.tools v2.2.0+incompatible h1:VsBPFP1AI068pPrMxtb/S8Zkgf9xEmTLJjfM+P5UIEo= +gotest.tools v2.2.0+incompatible/go.mod h1:DsYFclhRJ6vuDpmuTbkuFWG+y2sxOXAzmJt81HFBacw= gotest.tools/v3 v3.0.2/go.mod h1:3SzNCllyD9/Y+b5r9JIKQ474KzkZyqLqEfYqMsX94Bk= gotest.tools/v3 v3.0.3/go.mod h1:Z7Lb0S5l+klDB31fvDQX8ss/FlKDxtlFlw3Oa8Ymbl8= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= diff --git a/utils/sharding.go b/utils/sharding.go index 8ac84b18..f12a6e98 100644 --- a/utils/sharding.go +++ b/utils/sharding.go @@ -131,7 +131,26 @@ func BuildPeerInformer(stopper chan struct{}, peerRing *ShardHelper, ringConfig } func updateEndpoints(currentObj interface{}, previousObj interface{}, ring *ShardHelper, log logr.Logger) { - added, kept, _, ok := getEndpointChanges(currentObj, previousObj, log) + current, err := toEndpoint(currentObj, log) + if err != nil { + log.Error(err, "could not convert obj to Endpoints") + return + } + + var previous *corev1.Endpoints + { + previous = nil + + if previousObj != nil { + previous, err = toEndpoint(currentObj, log) + if err != nil { + log.Error(err, "could not convert obj to Endpoints") + return + } + } + } + + added, kept, _, ok := getEndpointChanges(current, previous, log) if !ok { return } @@ -140,12 +159,12 @@ func updateEndpoints(currentObj interface{}, previousObj interface{}, ring *Shar } // getEndpointChanges takes a current and optional previous object and returns the added, kept, and removed items, plus a success boolean. -func getEndpointChanges(currentObj interface{}, previousObj interface{}, log logr.Logger) ([]string, []string, []string, bool) { - current, err := toEndpoint(currentObj, log) - if err != nil { - log.Error(err, "could not convert obj to Endpoints") - return nil, nil, nil, false - } +func getEndpointChanges(current *corev1.Endpoints, previous *corev1.Endpoints, log logr.Logger) ([]string, []string, []string, bool) { + // current, err := toEndpoint(currentObj, log) + // if err != nil { + // log.Error(err, "could not convert obj to Endpoints") + // return nil, nil, nil, false + // } currentEndpoints := []string{} // Stores current endpoints to return directly if we don't have a previous state. currentEndpointsMap := make(map[string]struct{}) // Stores the endpoints as a map for quicker comparisons to previous state. @@ -160,17 +179,17 @@ func getEndpointChanges(currentObj interface{}, previousObj interface{}, log log } } - if previousObj == nil { + if previous == nil { // If there is no previous object, we're only adding new (initial) endpoints. // Just return the current endpoint list. return currentEndpoints, nil, nil, true } - previous, err := toEndpoint(previousObj, log) - if err != nil { - log.Error(err, "could not convert obj to Endpoints") - return nil, nil, nil, false - } + // previous, err := toEndpoint(previousObj, log) + // if err != nil { + // log.Error(err, "could not convert obj to Endpoints") + // return nil, nil, nil, false + // } added := []string{} kept := []string{} diff --git a/utils/sharding_test.go b/utils/sharding_test.go index 0de28879..6a1cfcd6 100644 --- a/utils/sharding_test.go +++ b/utils/sharding_test.go @@ -1,122 +1,67 @@ package utils import ( - "fmt" "strconv" "testing" "github.com/go-logr/logr/testr" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "gotest.tools/assert" corev1 "k8s.io/api/core/v1" ) func Test_getEndpointChanges(t *testing.T) { testCases := []struct { - field string - anotherfield string + name string + current *corev1.Endpoints + previous *corev1.Endpoints + expectedAdded []string + expectedKept []string + expectedRemoved []string }{ { - field: "todo", - }, - { - field: "todo", - anotherfield: "todo", - }, - } - - // u := &unstructured.Unstructured{ - // Object: map[string]interface{}{ - // "apiVersion": "cr.bar.com/v1", - // "kind": "Foo", - // "spec": map[string]interface{}{"field": 1}, - // "metadata": map[string]interface{}{ - // "name": "test-1", - // "annotations": map[string]interface{}{ - // "foo": "bar", - // }, - // }, - // }, - // } - - // a1 := map[string]interface{}{ - // "ip": "10.0.131.187", - // "nodeName": "worker-000026", - // "targetRef": map[string]string{ - // "kind": "Pod", - // "name": "starboard-exporter-765756bd69-vzx49", - // "namespace": "giantswarm", - // }, - // } - - // addresses := make([]map[string]interface{}) - // addresses = append(addresses) - - // ep := &unstructured.Unstructured{ - // Object: map[string]interface{}{ - // "apiVersion": "v1", - // "kind": "Endpoints", - // "metadata": map[string]interface{}{ - // "name": "starboard-exporter", - // "namespace": "giantswarm", - // }, - // // "subsets": map[string][]map[string]interface{}{ - // "subsets": map[string]interface{}{ - // "addresses": []map[string]interface{}{ - // { - // "ip": "10.0.131.187", - // "nodeName": "worker-000026", - // "targetRef": map[string]string{ - // "kind": "Pod", - // "name": "starboard-exporter-765756bd69-vzx49", - // "namespace": "giantswarm", - // }, - // }, - // }, - // "ports": []map[string]interface{}{ - // { - // "name": "metrics", - // "port": "8080", - // "protocol": "TCP", - // }, - // }, - // }, - // }, - // } - - epp := &corev1.Endpoints{} - subsets := corev1.EndpointSubset{ - Addresses: []corev1.EndpointAddress{ - { - IP: "1.2.3.4", - // NodeName: "worker-000026", + name: "add one new endpoint with no previous state", + current: &corev1.Endpoints{ + Subsets: []corev1.EndpointSubset{ + { + Addresses: []corev1.EndpointAddress{ + { + IP: "1.2.3.4", + }, + }, + }, + }, }, + expectedAdded: []string{"1.2.3.4"}, + expectedKept: []string{}, + expectedRemoved: []string{}, }, } - epp.Subsets = []corev1.EndpointSubset{subsets} - log := testr.New(t) - for i, testCase := range testCases { + for i, tc := range testCases { t.Run(strconv.Itoa(i), func(t *testing.T) { - log.Info("test") - log.Info(fmt.Sprintf("%v", epp)) - added, kept, removed, ok := getEndpointChanges(epp, nil, log) - t.Logf("case %s: added: %s, kept: %s, removed: %s\n", testCase, added, kept, removed) + var previous *corev1.Endpoints + { + previous = nil + if tc.previous != nil { + previous = tc.previous + } + } + + added, kept, removed, ok := getEndpointChanges(tc.current, previous, log) + + t.Logf("case %v: added: %v, kept: %v, removed: %v\n", tc, added, kept, removed) if !ok { - t.Fatalf("case %s: added: %s, kept: %s, removed: %s\n", testCase, added, kept, removed) + t.Fatalf("unable to parse endpoint changes for case %v: added: %s, kept: %s, removed: %s\n", tc, added, kept, removed) } + + assert.Assert(t, cmp.Equal(tc.expectedAdded, added, cmpopts.EquateEmpty()), "test case %v failed.", tc.name) + assert.Assert(t, cmp.Equal(tc.expectedKept, kept, cmpopts.EquateEmpty()), "test case %v failed.", tc.name) + assert.Assert(t, cmp.Equal(tc.expectedRemoved, removed, cmpopts.EquateEmpty()), "test case %v failed.", tc.name) }) } } - -// { -// - ip: 10.0.133.125 -// nodeName: worker-000025 -// targetRef: -// kind: Pod -// name: starboard-exporter-765756bd69-r795x -// namespace: giantswarm -// resourceVersion: "694068745" -// uid: 02758daa-e56a-499f-af8a-fb795890700f -// } From bdeb8eb851e4817b28b95e9e6991b5107e6e3c71 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Tue, 7 Jun 2022 18:37:24 -0400 Subject: [PATCH 24/31] More tests --- utils/sharding_test.go | 164 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 159 insertions(+), 5 deletions(-) diff --git a/utils/sharding_test.go b/utils/sharding_test.go index 6a1cfcd6..13345b70 100644 --- a/utils/sharding_test.go +++ b/utils/sharding_test.go @@ -1,6 +1,7 @@ package utils import ( + "sort" "strconv" "testing" @@ -37,8 +38,157 @@ func Test_getEndpointChanges(t *testing.T) { expectedKept: []string{}, expectedRemoved: []string{}, }, + { + name: "add one new endpoint to one previous endpoint", + current: &corev1.Endpoints{ + Subsets: []corev1.EndpointSubset{ + { + Addresses: []corev1.EndpointAddress{ + { + IP: "1.2.3.4", + }, + { + IP: "5.6.7.8", + }, + }, + }, + }, + }, + previous: &corev1.Endpoints{ + Subsets: []corev1.EndpointSubset{ + { + Addresses: []corev1.EndpointAddress{ + { + IP: "1.2.3.4", + }, + }, + }, + }, + }, + expectedAdded: []string{"5.6.7.8"}, + expectedKept: []string{"1.2.3.4"}, + expectedRemoved: []string{}, + }, + { + name: "add multiple new endpoints to two previous endpoints", + current: &corev1.Endpoints{ + Subsets: []corev1.EndpointSubset{ + { + Addresses: []corev1.EndpointAddress{ + { + IP: "8.8.8.8", + }, + { + IP: "8.8.4.4", + }, + { + IP: "1.2.3.4", + }, + { + IP: "5.6.7.8", + }, + }, + }, + }, + }, + previous: &corev1.Endpoints{ + Subsets: []corev1.EndpointSubset{ + { + Addresses: []corev1.EndpointAddress{ + { + IP: "1.2.3.4", + }, + { + IP: "5.6.7.8", + }, + }, + }, + }, + }, + expectedAdded: []string{"8.8.4.4", "8.8.8.8"}, + expectedKept: []string{"1.2.3.4", "5.6.7.8"}, + expectedRemoved: []string{}, + }, + { + name: "remove multiple endpoints", + current: &corev1.Endpoints{ + Subsets: []corev1.EndpointSubset{ + { + Addresses: []corev1.EndpointAddress{ + { + IP: "8.8.8.8", + }, + { + IP: "1.2.3.4", + }, + }, + }, + }, + }, + previous: &corev1.Endpoints{ + Subsets: []corev1.EndpointSubset{ + { + Addresses: []corev1.EndpointAddress{ + { + IP: "8.8.8.8", + }, + { + IP: "8.8.4.4", + }, + { + IP: "1.2.3.4", + }, + { + IP: "5.6.7.8", + }, + }, + }, + }, + }, + expectedAdded: []string{}, + expectedKept: []string{"1.2.3.4", "8.8.8.8"}, + expectedRemoved: []string{"5.6.7.8", "8.8.4.4"}, + }, + { + name: "add and remove endpoints in one udpate", + current: &corev1.Endpoints{ + Subsets: []corev1.EndpointSubset{ + { + Addresses: []corev1.EndpointAddress{ + { + IP: "8.8.4.4", + }, + { + IP: "1.2.3.4", + }, + { + IP: "5.6.7.8", + }, + }, + }, + }, + }, + previous: &corev1.Endpoints{ + Subsets: []corev1.EndpointSubset{ + { + Addresses: []corev1.EndpointAddress{ + { + IP: "8.8.8.8", + }, + { + IP: "1.2.3.4", + }, + }, + }, + }, + }, + expectedAdded: []string{"5.6.7.8", "8.8.4.4"}, + expectedKept: []string{"1.2.3.4"}, + expectedRemoved: []string{"8.8.8.8"}, + }, } + // Logger to pass to helper functions. Wraps testing.T. log := testr.New(t) for i, tc := range testCases { @@ -51,17 +201,21 @@ func Test_getEndpointChanges(t *testing.T) { } } + // Calculate endpoint updates. added, kept, removed, ok := getEndpointChanges(tc.current, previous, log) - t.Logf("case %v: added: %v, kept: %v, removed: %v\n", tc, added, kept, removed) + t.Logf("case %v: added: %v, kept: %v, removed: %v\n", tc, sort.StringSlice(added), kept, removed) if !ok { - t.Fatalf("unable to parse endpoint changes for case %v: added: %s, kept: %s, removed: %s\n", tc, added, kept, removed) + t.Fatalf("unable to parse endpoint changes for case %v: added: %s, kept: %s, removed: %s\n", tc, sort.StringSlice(added), kept, removed) } - assert.Assert(t, cmp.Equal(tc.expectedAdded, added, cmpopts.EquateEmpty()), "test case %v failed.", tc.name) - assert.Assert(t, cmp.Equal(tc.expectedKept, kept, cmpopts.EquateEmpty()), "test case %v failed.", tc.name) - assert.Assert(t, cmp.Equal(tc.expectedRemoved, removed, cmpopts.EquateEmpty()), "test case %v failed.", tc.name) + compareStringFunc := func(a, b string) bool { return a < b } + + // Check added, kept, and removed contain the expected items, ignoring order. + assert.Assert(t, cmp.Equal(tc.expectedAdded, added, cmpopts.EquateEmpty(), cmpopts.SortSlices(compareStringFunc)), "test case %v failed.", tc.name) + assert.Assert(t, cmp.Equal(tc.expectedKept, kept, cmpopts.EquateEmpty(), cmpopts.SortSlices(compareStringFunc)), "test case %v failed.", tc.name) + assert.Assert(t, cmp.Equal(tc.expectedRemoved, removed, cmpopts.EquateEmpty(), cmpopts.SortSlices(compareStringFunc)), "test case %v failed.", tc.name) }) } } From d7981d0776057a00cd968bca313a327c17220d46 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Tue, 7 Jun 2022 18:37:56 -0400 Subject: [PATCH 25/31] Tidy go mod --- go.mod | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 10d643e5..e462e21c 100644 --- a/go.mod +++ b/go.mod @@ -16,7 +16,10 @@ require ( sigs.k8s.io/controller-runtime v0.12.1 ) -require gotest.tools v2.2.0+incompatible +require ( + github.com/google/go-cmp v0.5.8 + gotest.tools v2.2.0+incompatible +) require ( cloud.google.com/go/compute v1.6.1 // indirect @@ -40,7 +43,6 @@ require ( github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/golang/protobuf v1.5.2 // indirect github.com/google/gnostic v0.6.9 // indirect - github.com/google/go-cmp v0.5.8 // indirect github.com/google/gofuzz v1.2.0 // indirect github.com/google/uuid v1.3.0 // indirect github.com/imdario/mergo v0.3.12 // indirect From 87ba18b8a99be778f844b5231ed992be7de5a548 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Tue, 7 Jun 2022 18:39:53 -0400 Subject: [PATCH 26/31] Cleanup --- utils/sharding_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/utils/sharding_test.go b/utils/sharding_test.go index 13345b70..5ee18cbb 100644 --- a/utils/sharding_test.go +++ b/utils/sharding_test.go @@ -1,7 +1,6 @@ package utils import ( - "sort" "strconv" "testing" @@ -204,10 +203,10 @@ func Test_getEndpointChanges(t *testing.T) { // Calculate endpoint updates. added, kept, removed, ok := getEndpointChanges(tc.current, previous, log) - t.Logf("case %v: added: %v, kept: %v, removed: %v\n", tc, sort.StringSlice(added), kept, removed) + t.Logf("case %v: added: %v, kept: %v, removed: %v\n", tc, added, kept, removed) if !ok { - t.Fatalf("unable to parse endpoint changes for case %v: added: %s, kept: %s, removed: %s\n", tc, sort.StringSlice(added), kept, removed) + t.Fatalf("unable to parse endpoint changes for case %v: added: %s, kept: %s, removed: %s\n", tc, added, kept, removed) } compareStringFunc := func(a, b string) bool { return a < b } From 0a9c321cea819679e77b7f79ce69e81860c29f8e Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Tue, 7 Jun 2022 18:40:54 -0400 Subject: [PATCH 27/31] More cleanup --- utils/sharding.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/utils/sharding.go b/utils/sharding.go index f12a6e98..a41bf1e8 100644 --- a/utils/sharding.go +++ b/utils/sharding.go @@ -160,11 +160,6 @@ func updateEndpoints(currentObj interface{}, previousObj interface{}, ring *Shar // getEndpointChanges takes a current and optional previous object and returns the added, kept, and removed items, plus a success boolean. func getEndpointChanges(current *corev1.Endpoints, previous *corev1.Endpoints, log logr.Logger) ([]string, []string, []string, bool) { - // current, err := toEndpoint(currentObj, log) - // if err != nil { - // log.Error(err, "could not convert obj to Endpoints") - // return nil, nil, nil, false - // } currentEndpoints := []string{} // Stores current endpoints to return directly if we don't have a previous state. currentEndpointsMap := make(map[string]struct{}) // Stores the endpoints as a map for quicker comparisons to previous state. @@ -185,12 +180,6 @@ func getEndpointChanges(current *corev1.Endpoints, previous *corev1.Endpoints, l return currentEndpoints, nil, nil, true } - // previous, err := toEndpoint(previousObj, log) - // if err != nil { - // log.Error(err, "could not convert obj to Endpoints") - // return nil, nil, nil, false - // } - added := []string{} kept := []string{} removed := []string{} From 585a114af45c03281cc5a596548f929b848342fd Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 8 Jun 2022 09:25:54 -0400 Subject: [PATCH 28/31] Use new client in configauditreport controller --- .../configauditreport_controller.go | 132 ++++++++++-------- .../vulnerabilityreport_controller.go | 1 + 2 files changed, 78 insertions(+), 55 deletions(-) diff --git a/controllers/configauditreport/configauditreport_controller.go b/controllers/configauditreport/configauditreport_controller.go index d2ddd23e..db64a6e7 100644 --- a/controllers/configauditreport/configauditreport_controller.go +++ b/controllers/configauditreport/configauditreport_controller.go @@ -23,6 +23,7 @@ import ( aqua "github.com/aquasecurity/starboard/pkg/apis/aquasecurity/v1alpha1" "github.com/go-logr/logr" "github.com/pkg/errors" + "github.com/prometheus/client_golang/prometheus" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" @@ -55,33 +56,22 @@ func (r *ConfigAuditReportReconciler) Reconcile(ctx context.Context, req ctrl.Re _ = log.FromContext(ctx) _ = r.Log.WithValues("configauditreport", req.NamespacedName) - report := &aqua.ConfigAuditReport{} - if err := r.Client.Get(ctx, req.NamespacedName, report); err != nil { - if apierrors.IsNotFound(err) { - // Most likely the report was deleted. - return ctrl.Result{}, nil - } - - // Error reading the object. - r.Log.Error(err, "Unable to read configauditreport") - return ctrl.Result{}, err - } - - // We have fetched the report and have four possibilities: - // - Not deleting + belongs to our shard --> give it our label (to trigger reconciliation by any previous owner) and expose the metric. - // - Not deleting + does not belong to our shard --> remove it from our metrics. Keep the finalizer. - // - Deleting + belongs to our shard --> remove it from our metrics. Remove the finalizer. - // - Deleting + does not belong to our shard --> remove it from our metrics. Keep the finalizer. + deletedSummaries := ConfigAuditSummary.DeletePartialMatch(prometheus.Labels{"report_name": req.NamespacedName.String()}) shouldOwn := r.ShardHelper.ShouldOwn(req.NamespacedName.String()) + if shouldOwn { - if report.DeletionTimestamp.IsZero() && shouldOwn { - // Give the report our finalizer if it doesn't have one. - if !utils.SliceContains(report.GetFinalizers(), ConfigAuditReportFinalizer) { - ctrlutil.AddFinalizer(report, ConfigAuditReportFinalizer) - if err := r.Update(ctx, report); err != nil { - return ctrl.Result{}, err + // Try to get the report. It might not exist anymore, in which case we don't need to do anything. + report := &aqua.ConfigAuditReport{} + if err := r.Client.Get(ctx, req.NamespacedName, report); err != nil { + if apierrors.IsNotFound(err) { + // Most likely the report was deleted. + return ctrl.Result{}, nil } + + // Error reading the object. + r.Log.Error(err, "Unable to read report") + return ctrl.Result{}, err } r.Log.Info(fmt.Sprintf("Reconciled %s || Found (C/H/M/L): %d/%d/%d/%d", @@ -92,35 +82,67 @@ func (r *ConfigAuditReportReconciler) Reconcile(ctx context.Context, req ctrl.Re report.Report.Summary.LowCount, )) - // Publish summary metrics for this report. + // Publish summary and CVE metrics for this report. publishSummaryMetrics(report) + if utils.SliceContains(report.GetFinalizers(), ConfigAuditReportFinalizer) { + // Remove the finalizer if we're the shard owner. + ctrlutil.RemoveFinalizer(report, ConfigAuditReportFinalizer) + if err := r.Update(ctx, report); err != nil { + return ctrl.Result{}, err + } + } + // Add a label to this report so any previous owners will reconcile and drop the metric. report.Labels[controllers.ShardOwnerLabel] = r.ShardHelper.PodIP err := r.Client.Update(ctx, report, &client.UpdateOptions{}) if err != nil { r.Log.Error(err, "unable to add shard owner label") } - } else { - // Unfortunately, we can't yet clear the series based on one label value, - // we have to reconstruct all of the label values to delete the series. - // That's the only reason the finalizer is needed at all. - // So we first clear our metrics for the report, and then remove the finalizer - // if we're the shard which owns this report. - - // Drop the report from our metrics. - r.clearImageMetrics(report) - - if shouldOwn && utils.SliceContains(report.GetFinalizers(), ConfigAuditReportFinalizer) { - // Remove the finalizer if we're the shard owner. - ctrlutil.RemoveFinalizer(report, ConfigAuditReportFinalizer) - if err := r.Update(ctx, report); err != nil { - return ctrl.Result{}, err - } + if deletedSummaries > 0 { + r.Log.Info(fmt.Sprintf("cleared %d summary metrics", deletedSummaries)) } } + // if report.DeletionTimestamp.IsZero() && shouldOwn { + // // Give the report our finalizer if it doesn't have one. + // if !utils.SliceContains(report.GetFinalizers(), ConfigAuditReportFinalizer) { + // ctrlutil.AddFinalizer(report, ConfigAuditReportFinalizer) + // if err := r.Update(ctx, report); err != nil { + // return ctrl.Result{}, err + // } + // } + + // // Publish summary metrics for this report. + // publishSummaryMetrics(report) + + // // Add a label to this report so any previous owners will reconcile and drop the metric. + // report.Labels[controllers.ShardOwnerLabel] = r.ShardHelper.PodIP + // err := r.Client.Update(ctx, report, &client.UpdateOptions{}) + // if err != nil { + // r.Log.Error(err, "unable to add shard owner label") + // } + + // } else { + // // Unfortunately, we can't yet clear the series based on one label value, + // // we have to reconstruct all of the label values to delete the series. + // // That's the only reason the finalizer is needed at all. + // // So we first clear our metrics for the report, and then remove the finalizer + // // if we're the shard which owns this report. + + // // Drop the report from our metrics. + // r.clearImageMetrics(report) + + // if shouldOwn && utils.SliceContains(report.GetFinalizers(), ConfigAuditReportFinalizer) { + // // Remove the finalizer if we're the shard owner. + // ctrlutil.RemoveFinalizer(report, ConfigAuditReportFinalizer) + // if err := r.Update(ctx, report); err != nil { + // return ctrl.Result{}, err + // } + // } + // } + return utils.JitterRequeue(controllers.DefaultRequeueDuration, r.MaxJitterPercent, r.Log), nil } @@ -136,21 +158,21 @@ func (r *ConfigAuditReportReconciler) SetupWithManager(mgr ctrl.Manager) error { return nil } -func (r *ConfigAuditReportReconciler) clearImageMetrics(report *aqua.ConfigAuditReport) { - // clear summary metrics - summaryValues := valuesForReport(report, metricLabels) - - // Delete the series for each severity. - for severity := range getCountPerSeverity(report) { - v := summaryValues - v["severity"] = severity - - // Delete the metric. - ConfigAuditSummary.Delete( - v, - ) - } -} +// func (r *ConfigAuditReportReconciler) clearImageMetrics(report *aqua.ConfigAuditReport) { +// // clear summary metrics +// summaryValues := valuesForReport(report, metricLabels) + +// // Delete the series for each severity. +// for severity := range getCountPerSeverity(report) { +// v := summaryValues +// v["severity"] = severity + +// // Delete the metric. +// ConfigAuditSummary.Delete( +// v, +// ) +// } +// } func RequeueReportsForPod(c client.Client, log logr.Logger, podIP string) { reportList := &aqua.ConfigAuditReportList{} diff --git a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go index 366bc400..8bfba8ee 100644 --- a/controllers/vulnerabilityreport/vulnerabilityreport_controller.go +++ b/controllers/vulnerabilityreport/vulnerabilityreport_controller.go @@ -94,6 +94,7 @@ func (r *VulnerabilityReportReconciler) Reconcile(ctx context.Context, req ctrl. report.Report.Summary.NoneCount, report.Report.Summary.UnknownCount, )) + // Publish summary and CVE metrics for this report. r.publishImageMetrics(report) From 784ab738022b7d568fa9a5c23f3e69c1600517e3 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 8 Jun 2022 09:26:59 -0400 Subject: [PATCH 29/31] Cleanup --- .../configauditreport_controller.go | 54 ------------------- 1 file changed, 54 deletions(-) diff --git a/controllers/configauditreport/configauditreport_controller.go b/controllers/configauditreport/configauditreport_controller.go index db64a6e7..c708b197 100644 --- a/controllers/configauditreport/configauditreport_controller.go +++ b/controllers/configauditreport/configauditreport_controller.go @@ -105,44 +105,6 @@ func (r *ConfigAuditReportReconciler) Reconcile(ctx context.Context, req ctrl.Re } } - // if report.DeletionTimestamp.IsZero() && shouldOwn { - // // Give the report our finalizer if it doesn't have one. - // if !utils.SliceContains(report.GetFinalizers(), ConfigAuditReportFinalizer) { - // ctrlutil.AddFinalizer(report, ConfigAuditReportFinalizer) - // if err := r.Update(ctx, report); err != nil { - // return ctrl.Result{}, err - // } - // } - - // // Publish summary metrics for this report. - // publishSummaryMetrics(report) - - // // Add a label to this report so any previous owners will reconcile and drop the metric. - // report.Labels[controllers.ShardOwnerLabel] = r.ShardHelper.PodIP - // err := r.Client.Update(ctx, report, &client.UpdateOptions{}) - // if err != nil { - // r.Log.Error(err, "unable to add shard owner label") - // } - - // } else { - // // Unfortunately, we can't yet clear the series based on one label value, - // // we have to reconstruct all of the label values to delete the series. - // // That's the only reason the finalizer is needed at all. - // // So we first clear our metrics for the report, and then remove the finalizer - // // if we're the shard which owns this report. - - // // Drop the report from our metrics. - // r.clearImageMetrics(report) - - // if shouldOwn && utils.SliceContains(report.GetFinalizers(), ConfigAuditReportFinalizer) { - // // Remove the finalizer if we're the shard owner. - // ctrlutil.RemoveFinalizer(report, ConfigAuditReportFinalizer) - // if err := r.Update(ctx, report); err != nil { - // return ctrl.Result{}, err - // } - // } - // } - return utils.JitterRequeue(controllers.DefaultRequeueDuration, r.MaxJitterPercent, r.Log), nil } @@ -158,22 +120,6 @@ func (r *ConfigAuditReportReconciler) SetupWithManager(mgr ctrl.Manager) error { return nil } -// func (r *ConfigAuditReportReconciler) clearImageMetrics(report *aqua.ConfigAuditReport) { -// // clear summary metrics -// summaryValues := valuesForReport(report, metricLabels) - -// // Delete the series for each severity. -// for severity := range getCountPerSeverity(report) { -// v := summaryValues -// v["severity"] = severity - -// // Delete the metric. -// ConfigAuditSummary.Delete( -// v, -// ) -// } -// } - func RequeueReportsForPod(c client.Client, log logr.Logger, podIP string) { reportList := &aqua.ConfigAuditReportList{} opts := []client.ListOption{ From 65e90742022fe7ff163e321ee2cf96e6fddc6a53 Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 8 Jun 2022 09:45:27 -0400 Subject: [PATCH 30/31] Set report_name for configauditreports --- controllers/configauditreport/configauditreport_controller.go | 3 +++ controllers/configauditreport/configauditreport_metrics.go | 1 + 2 files changed, 4 insertions(+) diff --git a/controllers/configauditreport/configauditreport_controller.go b/controllers/configauditreport/configauditreport_controller.go index c708b197..56cd8ad7 100644 --- a/controllers/configauditreport/configauditreport_controller.go +++ b/controllers/configauditreport/configauditreport_controller.go @@ -26,6 +26,7 @@ import ( "github.com/prometheus/client_golang/prometheus" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + apitypes "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -187,6 +188,8 @@ func valuesForReport(report *aqua.ConfigAuditReport, labels []string) map[string func reportValueFor(field string, report *aqua.ConfigAuditReport) string { switch field { + case "report_name": + return apitypes.NamespacedName{Name: report.Name, Namespace: report.Namespace}.String() case "resource_name": return report.Name case "resource_namespace": diff --git a/controllers/configauditreport/configauditreport_metrics.go b/controllers/configauditreport/configauditreport_metrics.go index 01d40d39..e7625895 100644 --- a/controllers/configauditreport/configauditreport_metrics.go +++ b/controllers/configauditreport/configauditreport_metrics.go @@ -11,6 +11,7 @@ const ( ) var metricLabels = []string{ + "report_name", "resource_name", "resource_namespace", "severity", From 1783026092f735883e0159445e067aa7c2f5584e Mon Sep 17 00:00:00 2001 From: Zach Stone Date: Wed, 8 Jun 2022 13:33:49 -0400 Subject: [PATCH 31/31] Typo --- controllers/configauditreport/configauditreport_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/configauditreport/configauditreport_controller.go b/controllers/configauditreport/configauditreport_controller.go index 56cd8ad7..f0b9d4e4 100644 --- a/controllers/configauditreport/configauditreport_controller.go +++ b/controllers/configauditreport/configauditreport_controller.go @@ -195,7 +195,7 @@ func reportValueFor(field string, report *aqua.ConfigAuditReport) string { case "resource_namespace": return report.Namespace case "severity": - return "" // this value will be overwritten on publishSummaryMetrics + return "" // this value will be overwritten in publishSummaryMetrics default: // Error? return ""