Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Adding pubsub interface #2538

Merged
merged 25 commits into from
May 25, 2023
Merged

Conversation

JaydipGabani
Copy link
Contributor

@JaydipGabani JaydipGabani commented Jan 25, 2023

Signed-off-by: Jaydip Gabani gabanijaydip@gmail.com

What this PR does / why we need it: This PR introduces a way to export violations and provides a way to consume all the violations without scanning the logs. For more details, please refer to this doc

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:

@JaydipGabani
Copy link
Contributor Author

Open questions:

  1. Should we include a way for us to batch the messages in the interface itself, or rely on the batching mechanism of individual underlying tools?
  2. How to implement per-request routing based on the relevant queues, for different enforcement actions?

@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2023

Codecov Report

Patch coverage: 16.51% and project coverage change: -0.79 ⚠️

Comparison is base (a5ebec1) 53.46% compared to head (17e0271) 52.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2538      +/-   ##
==========================================
- Coverage   53.46%   52.68%   -0.79%     
==========================================
  Files         127      131       +4     
  Lines       11373    11591     +218     
==========================================
+ Hits         6081     6107      +26     
- Misses       4817     5005     +188     
- Partials      475      479       +4     
Flag Coverage Δ
unittests 52.68% <16.51%> (-0.79%) ⬇️

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

Impacted Files Coverage Δ
pkg/audit/controller.go 0.00% <ø> (ø)
pkg/audit/manager.go 9.64% <0.00%> (-0.41%) ⬇️
pkg/pubsub/system.go 3.92% <3.92%> (ø)
pkg/controller/pubsub/pubsub_config_controller.go 11.68% <11.68%> (ø)
pkg/pubsub/dapr/dapr.go 23.80% <23.80%> (ø)
pkg/pubsub/provider/provider.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JaydipGabani
Copy link
Contributor Author

Here is a PR I came across that defined method definition for batching in dapr. This is a complete proposal for batching in dapr pub sub

@JaydipGabani
Copy link
Contributor Author

@maxsmythe according to this PPT, it seems I was wrong and at least with dapr, the subscriber will have a choice of bulk subscribe or simple subscribe (i.e how the subscriber can get messages).

@maxsmythe
Copy link
Contributor

It looks like Dapr's response to the possibility of some clients not supporting bulk publishing is to have their library turn a bulk publish into a loop over the non-supporting client's publish call.

@JaydipGabani
Copy link
Contributor Author

Yes! According to this comment, non-supporting client's publish call can be made either parallelly or serially (by default it will be made parallelly

pkg/pubsub/rabbitmq/rabbitmq.go Outdated Show resolved Hide resolved
pkg/pubsub/pubsub_client.go Outdated Show resolved Hide resolved
pkg/pubsub/dapr/dapr.go Outdated Show resolved Hide resolved
@davis-haba
Copy link
Contributor

We want to add this to the validation webhook too, right? Or did you plan to do this as a follow up?

@JaydipGabani
Copy link
Contributor Author

We want to add this to the validation webhook too, right? Or did you plan to do this as a follow up?

Initially we will enable this with audit and then, later on, can integrate to different parts such as validation webhook.

@JaydipGabani JaydipGabani marked this pull request as ready for review February 9, 2023 01:32
@JaydipGabani
Copy link
Contributor Author

JaydipGabani commented Feb 9, 2023

@sozercan @maxsmythe @nilekhc @ritazh I am working on fixing CI errors, but this PR is ready for review

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, added a few comments.

pkg/pubsub/pubsub_client.go Outdated Show resolved Hide resolved
pkg/pubsub/dapr/dapr.go Outdated Show resolved Hide resolved
pkg/pubsub/pubsub_client.go Outdated Show resolved Hide resolved
pkg/pubsub/pubsub_client.go Outdated Show resolved Hide resolved
pkg/pubsub/pubsub_init.go Outdated Show resolved Hide resolved
pkg/pubsub/pubsub_client.go Outdated Show resolved Hide resolved
pkg/pubsub/dapr/dapr.go Outdated Show resolved Hide resolved
pkg/pubsub/pubsub_init.go Outdated Show resolved Hide resolved
pkg/pubsub/pubsub_init.go Outdated Show resolved Hide resolved
pkg/pubsub/pubsub_init.go Outdated Show resolved Hide resolved
pkg/pubsub/client/pubsub_client.go Outdated Show resolved Hide resolved
pkg/audit/manager.go Outdated Show resolved Hide resolved
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
…ror reporting on publish

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, last nit is on the Helm chart

@JaydipGabani
Copy link
Contributor Author

@maxsmythe @sozercan made the change, PTAL.

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants