chore: updating pubsub system #3646

JaydipGabani · 2024-10-15T05:50:11Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #1037 #3497

Special notes for your reviewer:

codecov-commenter · 2024-10-15T22:06:18Z

Codecov Report

Attention: Patch coverage is 52.83019% with 75 lines in your changes missing coverage. Please review.

Project coverage is 47.67%. Comparing base (3350319) to head (2fba5c2).
Report is 245 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/export/dapr/fake_dapr_client.go	24.13%	22 Missing ⚠️
pkg/export/dapr/dapr.go	65.95%	14 Missing and 2 partials ⚠️
pkg/export/testdriver/testdriver.go	36.84%	11 Missing and 1 partial ⚠️
pkg/controller/export/export_config_controller.go	8.33%	11 Missing ⚠️
pkg/export/system.go	79.54%	6 Missing and 3 partials ⚠️
pkg/audit/manager.go	0.00%	5 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (3350319) and HEAD (2fba5c2). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (3350319) HEAD (2fba5c2)

unittests 2 1

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3646      +/-   ##
==========================================
- Coverage   54.49%   47.67%   -6.83%     
==========================================
  Files         134      234     +100     
  Lines       12329    19855    +7526     
==========================================
+ Hits         6719     9465    +2746     
- Misses       5116     9498    +4382     
- Partials      494      892     +398

Flag	Coverage Δ
unittests	`47.67% <52.83%> (-6.83%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JaydipGabani · 2024-10-23T01:32:19Z

Still working on doc changes.

JaydipGabani · 2024-12-05T23:31:13Z

pkg/controller/export/export_config_controller.go

@@ -26,37 +26,37 @@ import (
 )

 var (
-	PubsubEnabled = flag.Bool("enable-pub-sub", false, "(alpha) Enabled pubsub to publish messages")
+	ExportEnabled = flag.Bool("enable-pub-sub", false, "(alpha) Enabled pubsub to publish messages")


Do we want to rename the flag to remove pub-sub word?

May as well while it's still alpha

should we provide a warning for the user if we change the name? we can remove it after a release?

What do you mean? How would we provide warning for changing a name before release?

JaydipGabani · 2024-12-31T19:26:10Z

Fixes #3497

maxsmythe · 2025-01-16T02:44:54Z

pkg/controller/export/export_config_controller.go

@@ -114,22 +114,22 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	}

 	if len(cfg.Data) == 0 {
-		return reconcile.Result{}, fmt.Errorf(fmt.Sprintf("data missing in configmap %s, unable to configure respective pubsub", request.NamespacedName))
+		return reconcile.Result{}, fmt.Errorf(fmt.Sprintf("data missing in configmap %s, unable to establish connection", request.NamespacedName))


is it a "connection" necessarily?

"link" may be? I just couldn't come up with better alternative at the time.

"unable to configure exporter"?

maxsmythe · 2025-01-16T02:46:34Z

pkg/controller/export/export_config_controller.go

 	}
-	if _, ok := cfg.Data["provider"]; !ok {
-		return reconcile.Result{}, fmt.Errorf(fmt.Sprintf("missing provider field in configmap %s, unable to configure respective pubsub", request.NamespacedName))
+	if _, ok := cfg.Data["driver"]; !ok {


Is changing from config map to CRD coming later?

I can raise a follow up PR, we have the design finalized as per last discussion.

sozercan

minor comments, otherwise LGTM

sozercan · 2025-01-23T19:29:26Z

pkg/controller/export/export_config_controller.go

 }

 func (a *Adder) Add(mgr manager.Manager) error {
-	if !*PubsubEnabled {
+	if !*ExportEnabled {
 		return nil
 	}
 	log.Info("Warning: Alpha flag enable-pub-sub is set to true. This flag may change in the future.")


update this too if we change the flag name

sozercan · 2025-01-23T19:29:41Z

pkg/controller/export/export_config_controller.go

@@ -114,22 +114,22 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	}

 	if len(cfg.Data) == 0 {
-		return reconcile.Result{}, fmt.Errorf(fmt.Sprintf("data missing in configmap %s, unable to configure respective pubsub", request.NamespacedName))
+		return reconcile.Result{}, fmt.Errorf(fmt.Sprintf("data missing in configmap %s, unable to establish connection", request.NamespacedName))


"unable to configure exporter"?

sozercan · 2025-01-23T19:31:59Z

website/docs/export-driver-walkthrough.md

+title: Export Interface/Driver walkthrough
+---
+
+This guide provides an overview of the driver interface, including details of its structure and functionality. Additionally, it offers instructions on adding a new driver and utilizing different backends to export violations.


Suggested change

This guide provides an overview of the driver interface, including details of its structure and functionality. Additionally, it offers instructions on adding a new driver and utilizing different backends to export violations.

This guide provides an overview of the driver interface, including details of its structure and functionality. Additionally, it offers instructions on adding a new driver and utilizing different backends to export audit violations.

sozercan · 2025-01-28T18:25:49Z

pkg/audit/manager.go

@@ -67,8 +67,8 @@ var (
 	auditEventsInvolvedNamespace = flag.Bool("audit-events-involved-namespace", false, "emit audit events for each violation in the involved objects namespace, the default (false) generates events in the namespace Gatekeeper is installed in. Audit events from cluster-scoped resources will still follow the default behavior")
 	auditMatchKindOnly           = flag.Bool("audit-match-kind-only", false, "only use kinds specified in all constraints for auditing cluster resources. if kind is not specified in any of the constraints, it will audit all resources (same as setting this flag to false)")
 	apiCacheDir                  = flag.String("api-cache-dir", defaultAPICacheDir, "The directory where audit from api server cache are stored, defaults to /tmp/audit")
-	auditConnection              = flag.String("audit-connection", defaultConnection, "(alpha) Connection name for publishing audit violation messages. Defaults to audit-connection")
-	auditChannel                 = flag.String("audit-channel", defaultChannel, "(alpha) Channel name for publishing audit violation messages. Defaults to audit-channel")
+	auditConnection              = flag.String("audit-connection", defaultConnection, "(alpha) Connection name for exporting audit violation messages. Defaults to audit-connection")


exporter-connection?

we may want to change default to something else in this case?

I am wondering if removing audit-connection and audit-channel flags altogether makes sense. We would create connections for configmaps created in gatekeeper-system when gatekeeper.sh/exporter-config: yes annotation is attached to it. This opens up door for supporting multiple connections at the same time as well by creating connections with multiple config_map rather than the one passed down with --audit-connection. And audit-channel should be passed in as part of the config. Additionally, replacing config_map with CRD should also be relatively seamless with this modification.

I am more than happy to just change flag names and helm variables in revisit this later. LMK your thoughts @sozercan @maxsmythe @ritazh

As per community discussion, keep the flag names as is and then remove once we have CRD inplace.

sozercan · 2025-01-28T18:26:10Z

pkg/audit/manager.go

-	auditConnection              = flag.String("audit-connection", defaultConnection, "(alpha) Connection name for publishing audit violation messages. Defaults to audit-connection")
-	auditChannel                 = flag.String("audit-channel", defaultChannel, "(alpha) Channel name for publishing audit violation messages. Defaults to audit-channel")
+	auditConnection              = flag.String("audit-connection", defaultConnection, "(alpha) Connection name for exporting audit violation messages. Defaults to audit-connection")
+	auditChannel                 = flag.String("audit-channel", defaultChannel, "(alpha) Channel name for exporting audit violation messages. Defaults to audit-channel")


export-channel?

Signed-off-by: Jaydip Gabani <[email protected]>

JaydipGabani changed the title ~~[WIP] chore: updating violation system~~ [WIP] chore: updating pubsub system Oct 21, 2024

JaydipGabani force-pushed the exporter-interface branch from 570505d to 7c48595 Compare October 22, 2024 23:55

JaydipGabani changed the title ~~[WIP] chore: updating pubsub system~~ chore: updating pubsub system Oct 23, 2024

JaydipGabani marked this pull request as ready for review October 23, 2024 01:31

JaydipGabani requested a review from a team as a code owner October 23, 2024 01:31

JaydipGabani commented Dec 6, 2024

View reviewed changes

ritazh added this to the v3.19.0 milestone Dec 11, 2024

JaydipGabani mentioned this pull request Dec 31, 2024

Modify Pub/Sub interface to common exporter interface #3497

Open

maxsmythe reviewed Jan 16, 2025

View reviewed changes

JaydipGabani force-pushed the exporter-interface branch from bf0c5cf to 1f2905e Compare January 24, 2025 22:12

JaydipGabani requested review from sozercan, maxsmythe and ritazh January 24, 2025 22:13

sozercan reviewed Jan 28, 2025

View reviewed changes

JaydipGabani added 2 commits January 28, 2025 11:39

updating pubsub system

64e2ae5

Signed-off-by: Jaydip Gabani <[email protected]>

replacing pub-sub with export everywhere

2fba5c2

Signed-off-by: Jaydip Gabani <[email protected]>

JaydipGabani force-pushed the exporter-interface branch from 1b578cb to 2fba5c2 Compare January 28, 2025 19:39

updating logs and docs

2323228

Signed-off-by: Jaydip Gabani <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: updating pubsub system #3646

chore: updating pubsub system #3646

JaydipGabani commented Oct 15, 2024 •

edited

Loading

codecov-commenter commented Oct 15, 2024 •

edited

Loading

JaydipGabani commented Oct 23, 2024

JaydipGabani Dec 5, 2024

maxsmythe Jan 16, 2025

sozercan Jan 23, 2025 •

edited

Loading

JaydipGabani Jan 24, 2025

JaydipGabani commented Dec 31, 2024

maxsmythe Jan 16, 2025

JaydipGabani Jan 16, 2025

sozercan Jan 23, 2025

maxsmythe Jan 16, 2025

JaydipGabani Jan 16, 2025

sozercan left a comment

sozercan Jan 23, 2025

sozercan Jan 23, 2025

sozercan Jan 23, 2025

sozercan Jan 28, 2025

JaydipGabani Jan 28, 2025 •

edited

Loading

JaydipGabani Jan 29, 2025

sozercan Jan 28, 2025

	This guide provides an overview of the driver interface, including details of its structure and functionality. Additionally, it offers instructions on adding a new driver and utilizing different backends to export violations.
	This guide provides an overview of the driver interface, including details of its structure and functionality. Additionally, it offers instructions on adding a new driver and utilizing different backends to export audit violations.

chore: updating pubsub system #3646

Are you sure you want to change the base?

chore: updating pubsub system #3646

Conversation

JaydipGabani commented Oct 15, 2024 • edited Loading

codecov-commenter commented Oct 15, 2024 • edited Loading

Codecov Report

JaydipGabani commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaydipGabani commented Dec 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaydipGabani Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaydipGabani commented Oct 15, 2024 •

edited

Loading

codecov-commenter commented Oct 15, 2024 •

edited

Loading

sozercan Jan 23, 2025 •

edited

Loading

JaydipGabani Jan 28, 2025 •

edited

Loading