test: adding unit tests for dapr and updating dapr sdk version #2846

JaydipGabani · 2023-06-22T21:01:48Z

What this PR does / why we need it: Adding unit tests for pubsub system and dapr driver

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 #2800

Special notes for your reviewer:

codecov-commenter · 2023-06-22T21:11:21Z

Codecov Report

Patch coverage: 10.33% and project coverage change: -0.54 ⚠️

Comparison is base (1076798) 53.60% compared to head (31afce7) 53.07%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2846      +/-   ##
==========================================
- Coverage   53.60%   53.07%   -0.54%     
==========================================
  Files         133      135       +2     
  Lines       11545    11790     +245     
==========================================
+ Hits         6189     6257      +68     
- Misses       4880     5047     +167     
- Partials      476      486      +10

Flag	Coverage Δ
unittests	`53.07% <10.33%> (-0.54%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/audit/manager.go	`9.83% <0.00%> (+0.19%)`	⬆️
pkg/controller/pubsub/pubsub_config_controller.go	`11.68% <ø> (ø)`
pkg/pubsub/provider/fake_provider.go	`0.00% <0.00%> (ø)`
pkg/pubsub/dapr/fake_dapr_client.go	`9.37% <9.37%> (ø)`
pkg/pubsub/system.go	`73.07% <50.00%> (+69.15%)`	⬆️
pkg/pubsub/dapr/dapr.go	`69.04% <100.00%> (+45.23%)`	⬆️

... 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.

pkg/pubsub/system_test.go

maxsmythe · 2023-07-01T00:37:48Z

pkg/audit/manager.go

@@ -253,7 +253,7 @@ func (am *Manager) audit(ctx context.Context) error {

 		err := am.addAuditResponsesToUpdateLists(updateLists, res, totalViolationsPerConstraint, totalViolationsPerEnforcementAction, timestamp)
 		if errs != nil {
-			return err
+			am.log.Error(err, "Auditing")


Why are we squashing errors here and below?

Before the pub-sub change addAuditResponsesToUpdateLists didn't really generate any error. By returning "Publishing" err and not "Audit" errors at line 256 and below, we are disrupting the audit, which results in constraintStatus not being updated with violations.

I am also open to ideas on handling publishing errors in other ways.

But we are not squashing pubsub errors, we are squashing any error returned by addAuditResponsesToUpdateLists(), which seems much broader in scope than what you're intending (merely swallowing pubsub errors).

Also, anticipating another comment:

What is our contract with users with regards to failing to publish an audit message?

The way this is written, it appears we are giving up on first failure, even if there is something like a transient networking issue.

What is our contract with users? When will events fail to be published?

But we are not squashing pubsub errors, we are squashing any error returned by addAuditResponsesToUpdateLists(), which seems much broader in scope than what you're intending (merely swallowing pubsub errors).

before pub-sub change, addAuditResponsesToUpdateLists didn't generate any error at all, it returned nil. So any errors returned now are only errors encountered in publishing the message. 🤔

@ritazh @sozercan would be helpful to have your voice as to user expectations here.

Few chains of thought -

Popular pubsub tools might already have a retry mechanism in place. Can we assume that when a driver fails to publish a message, the underlying provider/tool has exhausted the retries?

Implementing retries on top of tool's retries might cause overhead for the messages that are not getting delivered because of legit errors.

Sorry for the delay in looking at this.

@maxsmythe i think @JaydipGabani's point is addAuditResponsesToUpdateLists never returned an err before pubsub (it had the return type as an error, which is a bug I believe - let me know if I am missing something) https://github.com/open-policy-agent/gatekeeper/blob/v3.10.0/pkg/audit/manager.go#L693
So previously (pre pubsub), audit was fault tolerant and we always published to constraints

however, now that pubsub is actually returning an error as part of addAuditResponsesToUpdateLists this would have unintended consequences of actually returning an error and skipping the constraint update.
I think @JaydipGabani's change is intending to restore the previous behavior so we still update constraints even if there is an error.

I think at some point we stopped returning errors b/c it was poorly-behaved. In general, audit code could probably use a refactor at some point. The core point is that we probably shouldn't be swallowing all errors unless it is part of some principled design for audit (which, again, we should do at some point).

If all we are doing is avoiding rocking the boat, then we should swallow pubsub failures close to the source. Anything grander is getting into redesign territory, and requires an understanding of audit as a whole to write/review.

Regardless, the larger point of this discussion thread is: what are the user expectations with regard to failed pubsub requests?

IMO, the user expectation should be we return as many violations as possible even when we encounter errors. The audit process should be fault tolerant in that we log the errors, but an error should not impact all violations.

pkg/controller/pubsub/pubsub_config_controller.go

JaydipGabani · 2023-07-07T20:21:08Z

/benchmark

github-actions · 2023-07-07T20:21:28Z

Running benchmark here...

pkg/pubsub/system.go

pkg/audit/manager.go

maxsmythe

LGTM after mutex nit

pkg/pubsub/system_test.go

pkg/audit/manager.go

pkg/pubsub/system_test.go

sozercan

a few nits and needs rebase, otherwise LGTM

sozercan

a few nits and needs rebase, otherwise LGTM

ritazh · 2023-07-21T23:21:05Z

go.mod

 	contrib.go.opencensus.io/exporter/ocagent v0.7.0
 	contrib.go.opencensus.io/exporter/prometheus v0.4.2
 	contrib.go.opencensus.io/exporter/stackdriver v0.13.14
-	github.com/dapr/go-sdk v1.6.0
+	github.com/dapr/go-sdk v1.8.0


We may need to start documenting dapr version compatibilities for each version of GK. wdyt?

Yeah, I could add it to doc PR 🤔

Do you have any specific place in mind for this information?

For docs, might be good to add to the pubsub dapr provider docs. But we need to remember to update that doc whenever this dependency is bumped. Not very ideal.

For the daprClient, is there any minimum required version we need to test? Does the dapr sdk return its own version? If so, might be good to include that in the log as the daprClient is created.

As follow up when we implement batch publishing, 1.8.0 will be the minimum required version, and dapr 1.11 will be the minimum compatible version.

Does the dapr sdk return its own version?

yes it does, but I am not sure how user can figure out which dapr runtime to install before hand if daprClient version information is going to be logged when user has installed all prerequisites and trying to initiate a connection using dapr.

Can we just say in the provider doc that, "current dapr sdk version is [link to gk go mod file]" and point to dapr-version for more information?

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

ritazh

LGTM

maxsmythe reviewed Jun 23, 2023

View reviewed changes

pkg/pubsub/system_test.go Show resolved Hide resolved

pkg/pubsub/system_test.go Show resolved Hide resolved

JaydipGabani requested a review from maxsmythe June 30, 2023 22:50

maxsmythe reviewed Jul 1, 2023

View reviewed changes

JaydipGabani requested a review from maxsmythe July 5, 2023 22:04

maxsmythe reviewed Jul 7, 2023

View reviewed changes

pkg/controller/pubsub/pubsub_config_controller.go Outdated Show resolved Hide resolved

JaydipGabani requested a review from maxsmythe July 7, 2023 18:25

acpana changed the title ~~chore: adding unit tests for dapr and updating dapr sdk version~~ test: adding unit tests for dapr and updating dapr sdk version Jul 10, 2023

ritazh added this to the v3.13.0 milestone Jul 12, 2023

JaydipGabani force-pushed the pbtest branch from 2d2b6d5 to 665944c Compare July 14, 2023 23:16

JaydipGabani requested review from sozercan and ritazh July 19, 2023 16:44

maxsmythe reviewed Jul 20, 2023

View reviewed changes

pkg/pubsub/system.go Outdated Show resolved Hide resolved

ritazh reviewed Jul 20, 2023

View reviewed changes

pkg/audit/manager.go Outdated Show resolved Hide resolved

JaydipGabani requested review from ritazh and maxsmythe July 20, 2023 19:21

sozercan assigned JaydipGabani Jul 20, 2023

maxsmythe approved these changes Jul 21, 2023

View reviewed changes

pkg/pubsub/system_test.go Outdated Show resolved Hide resolved

sozercan reviewed Jul 21, 2023

View reviewed changes

pkg/audit/manager.go Outdated Show resolved Hide resolved

sozercan reviewed Jul 21, 2023

View reviewed changes

pkg/pubsub/system_test.go Outdated Show resolved Hide resolved

sozercan approved these changes Jul 21, 2023

View reviewed changes

ritazh reviewed Jul 21, 2023

View reviewed changes

JaydipGabani added 5 commits July 21, 2023 23:34

adding unit tests for dapr and updating dapr sdk version

9a5440a

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

adding fake clients, refining tests

b64f86e

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

not closing existing connection on upsert error

ccd6241

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

reverting back to abstracting providers using system properly

d19e883

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

removing merge since we are logging all the errors

68e3be4

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

JaydipGabani added 4 commits July 21, 2023 23:34

logging audit publishing error immediately

542a037

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

removing duplicat error logging

014479d

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

handling mutext locks properly

1236ec5

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

addressing nits

2631eb2

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

JaydipGabani force-pushed the pbtest branch from ae9f8ba to 2631eb2 Compare July 21, 2023 23:35

JaydipGabani requested a review from ritazh July 21, 2023 23:39

fixing lint

31afce7

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

ritazh approved these changes Jul 24, 2023

View reviewed changes

sozercan merged commit 5f04a2c into open-policy-agent:master Jul 24, 2023
14 of 15 checks passed

This was referenced Nov 22, 2023

Master 314 stolostron/gatekeeper#185

Closed

Replace master with 3.14 stolostron/gatekeeper#189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: adding unit tests for dapr and updating dapr sdk version #2846

test: adding unit tests for dapr and updating dapr sdk version #2846

JaydipGabani commented Jun 22, 2023

codecov-commenter commented Jun 22, 2023 •

edited

Loading

maxsmythe Jul 1, 2023

JaydipGabani Jul 5, 2023

maxsmythe Jul 5, 2023

maxsmythe Jul 5, 2023

JaydipGabani Jul 6, 2023

maxsmythe Jul 7, 2023

JaydipGabani Jul 10, 2023

sozercan Jul 14, 2023

maxsmythe Jul 19, 2023

ritazh Jul 19, 2023

JaydipGabani commented Jul 7, 2023

github-actions bot commented Jul 7, 2023

maxsmythe left a comment

sozercan left a comment

sozercan left a comment

ritazh Jul 21, 2023

JaydipGabani Jul 21, 2023

JaydipGabani Jul 21, 2023

ritazh Jul 24, 2023 •

edited

Loading

JaydipGabani Jul 24, 2023

ritazh left a comment

test: adding unit tests for dapr and updating dapr sdk version #2846

test: adding unit tests for dapr and updating dapr sdk version #2846

Conversation

JaydipGabani commented Jun 22, 2023

codecov-commenter commented Jun 22, 2023 • edited Loading

Codecov Report

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

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 commented Jul 7, 2023

github-actions bot commented Jul 7, 2023

maxsmythe left a comment

Choose a reason for hiding this comment

sozercan left a 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

ritazh Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritazh left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 22, 2023 •

edited

Loading

ritazh Jul 24, 2023 •

edited

Loading