fix: refactoring to remove pubsub flags to improve experience

[JaydipGabani]

What this PR does / why we need it:

• Removing --audit-connection and --audit-channel flags to reduce configuration complexity
• Adding support to define channel/topic to publish messages within dapr driver
• Publishing to all established connections

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 #

Special notes for your reviewer:

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 50.00000% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 46.72%. Comparing base (3350319) to head (acc6280).
Report is 57 commits behind head on master.

Files	Patch %	Lines
pkg/pubsub/dapr/dapr.go	40.00%	5 Missing and 1 partial ⚠️
pkg/pubsub/dapr/fake_dapr_client.go	16.66%	5 Missing ⚠️
pkg/audit/manager.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3339      +/-   ##
==========================================
- Coverage   54.49%   46.72%   -7.77%     
==========================================
  Files         134      218      +84     
  Lines       12329    14798    +2469     
==========================================
+ Hits         6719     6915     +196     
- Misses       5116     7079    +1963     
- Partials      494      804     +310     

Flag	Coverage Δ
unittests	46.72% <50.00%> (-7.77%)	⬇️

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.

[maxsmythe]

I'm confused as to how this improves UX. If a customer doesn't specify the flags, won't they get the default values, which is an equivalent effort on the users part?

If a user is using a custom queue value/name how would they be able to continue to do so with this change?

Are there any backwards compatibility concerns removing flags from the command line and Helm chart?

[maxsmythe]

Also, we probably want to ensure pubsub can still be disabled for those who do not want to use it.

[maxsmythe]

Sorry, that last comment was off-topic... you mistakenly removed enablePubsub from the Helm config's values file instead of the channel name flag.

[maxsmythe]

I think you removed the wrong value here?

[JaydipGabani]

yip! I will update that.

[shravankumar-12]

yea

[JaydipGabani]

I'm confused as to how this improves UX. If a customer doesn't specify the flags, won't they get the default values, which is an equivalent effort on the users part?

If a user is using a custom queue value/name how would they be able to continue to do so with this change?

Are there any backwards compatibility concerns removing flags from the command line and Helm chart?

Instead of using flags to define global connection and channel - ensure that --audit-connection must match name of the configMap to send messages, the channel/queue should now be defined as a part of spec in connectionConfig. So each connection will say on which queue these messages should be sent out to. And instead of using only one connection for all messages, now pubsub would be able to use all the configured connections to send messages as it will not be required to sepcify a connection using audit-connection flag.

[ritazh]

what is consuming this env var? it looks like it is no longer being used and got removed here: https://github.com/open-policy-agent/gatekeeper/pull/3339/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L228

[ritazh]

I'm confused as to how this improves UX. If a customer doesn't specify the flags, won't they get the default values, which is an equivalent effort on the users part?
If a user is using a custom queue value/name how would they be able to continue to do so with this change?
Are there any backwards compatibility concerns removing flags from the command line and Helm chart?

Instead of using flags to define global connection and channel - ensure that --audit-connection must match name of the configMap to send messages, the channel/queue should now be defined as a part of spec in connectionConfig. So each connection will say on which queue these messages should be sent out to. And instead of using only one connection for all messages, now pubsub would be able to use all the configured connections to send messages as it will not be required to sepcify a connection using audit-connection flag.

The concern here is by removing flags, it will definitely break in-place upgrade if users are using this alpha feature. are we ok with doing that for an alpha feature? @maxsmythe
If not, then we need to follow the same deprecation process as other flags to add deprecation message for 2-3 releases before we can remove it. e.g. the validate-template-rego flag lived for 3 releases before we can remove it in 3.16.

Separately, you should add some details in the docs for how users can use topic in the configmap to describe connections and queues they want to use for different purposes. Add few examples on top of the audit one maybe.

[maxsmythe]

Are we still looking at supporting multiple queues simultaneously? Possibly specifying which connection/topic gets pushed to as part of a constraint's enforcement action?

Example: send violation X to security queue, violation Y to ops queue?

If so, Publish() will need to support something more fine-grained than "broadcast everywhere"

[maxsmythe]

What part of the connection/channel config should be specify-able by downstream users (e.g. constraint authors) and what should be owned by the author of the constraint objects?

We should think about the personas who would be interacting with these knobs. Are the people setting up the infra always the same people writing the policy?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.