fix: #3146 Support close open/fail for Ready Tracker & surface errors swallowed by grp.Wait() #3308

David-Jaeyoon-Lee · 2024-03-14T20:30:27Z

What this PR does / why we need it:
This change fixes Run() method in Ready Tracker to have a fail-close option so that if we fail to fetch expectations we surface those errors & fail. By default, this won't be enabled & we will fail-open.

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

Special notes for your reviewer: Refer to issue link for more information

pkg/readiness/ready_tracker.go

maxsmythe

Thank you for the PR! Added a couple comments.

pkg/readiness/ready_tracker.go

David-Jaeyoon-Lee · 2024-04-08T17:20:56Z

Separating out constraint template v1beta1 -> v1 changes in separate PR.

maxsmythe

LGTM after nit, thank you!

pkg/readiness/ready_tracker.go

codecov-commenter · 2024-04-10T03:18:10Z

Codecov Report

Attention: Patch coverage is 87.83784% with 36 lines in your changes missing coverage. Please review.

Project coverage is 47.83%. Comparing base (3350319) to head (bf49b3c).
Report is 98 commits behind head on master.

Files	Patch %	Lines
pkg/syncutil/concurrent_slice.go	0.00%	19 Missing ⚠️
pkg/readiness/ready_tracker.go	95.01%	11 Missing and 2 partials ⚠️
pkg/syncutil/single_runner.go	71.42%	4 Missing ⚠️

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

HEAD has 1 upload less than BASE

Flag BASE (3350319) HEAD (bf49b3c)

unittests 2 1

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3308      +/-   ##
==========================================
- Coverage   54.49%   47.83%   -6.67%     
==========================================
  Files         134      219      +85     
  Lines       12329    14884    +2555     
==========================================
+ Hits         6719     7120     +401     
- Misses       5116     6955    +1839     
- Partials      494      809     +315

Flag	Coverage Δ
unittests	`47.83% <87.83%> (-6.67%)`	⬇️

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.

pkg/readiness/ready_tracker.go

ritazh · 2024-04-13T01:09:18Z

Thanks for the PR @David-Jaeyoon-Lee
Few nits
I did not see unit test added for the new flag.

David-Jaeyoon-Lee · 2024-04-23T21:18:40Z

pkg/readiness/ready_tracker.go

@@ -45,7 +45,7 @@ import (
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )

-var crashOnFailureFetchingExpectations = flag.Bool("crash-on-failure-fetching-expectations", false, "When set (defaults to false), gatekeeper will ignore errors that occur when gathering expectations. This prevents bootstrapping errors from crashing Gatekeeper at the cost of increasing the risk Gatekeeper will under-enforce policy by serving before it has loaded in all policies. Enabling this will help prevent under-enforcement at the risk of crashing during startup for issues like network errors.")
+var crashOnFailureFetchingExpectations = flag.Bool("crash-on-failure-fetching-expectations", false, "When set (defaults to false), gatekeeper will ignore errors that occur when gathering expectations. This prevents bootstrapping errors from crashing Gatekeeper at the cost of increasing the risk Gatekeeper will under-enforce policy by serving before it has loaded in all policies. Enabling this will help prevent under-enforcement at the risk of crashing during startup for issues like network errors. Note that enabling this flag currently does not achieve the aforementioned effect since fetching expectations are set to retry until success so failures during fetching expectations currently do not occurr.")


@maxsmythe Just wanted to double check this wording

"When set" -> "unless set"

David-Jaeyoon-Lee · 2024-04-30T21:45:06Z

I still need to fix lint errors for the Error return value is not checked for the goroutine deleted object polling. Any ideas on how to fix this? @maxsmythe

maxsmythe

LGTM with nits

Thank you for figuring this out!

pkg/readiness/ready_tracker.go

maxsmythe · 2024-07-02T00:24:35Z

pkg/readiness/ready_tracker.go

@@ -46,6 +46,8 @@ import (

 var log = logf.Log.WithName("readiness-tracker")

+var crashOnFailureFetchingExpectations = flag.Bool("crash-on-failure-fetching-expectations", false, "Unless set (defaults to false), gatekeeper will ignore errors that occur when gathering expectations. This prevents bootstrapping errors from crashing Gatekeeper at the cost of increasing the risk Gatekeeper will under-enforce policy by serving before it has loaded in all policies. Enabling this will help prevent under-enforcement at the risk of crashing during startup for issues like network errors. Note that enabling this flag currently does not achieve the aforementioned effect since fetching expectations are set to retry until success so failures during fetching expectations currently do not occur.")


nit: I think this can be shortened

Do you mean the naming or the description or both?

Description

pkg/readiness/ready_tracker.go

Add fail-close support for fetching expectations by adding error handling & surfacing the errors being swallowed by grp.Wait(). Signed-off-by: David-Jaeyoon-Lee <[email protected]>

Remove the starting hyphen from the crash-on-failure-fetching-expectations flag Signed-off-by: David-Jaeyoon-Lee <[email protected]>

and add unit tests Fix comments and return error in trackConfigAndSyncSets. Add unit tests for each track method & each wait in Run method. Signed-off-by: David-Jaeyoon-Lee <[email protected]>

in test Signed-off-by: David-Jaeyoon-Lee <[email protected]>

Change errgroup logic such that we instead use waitgroups and errchannels. This is that we remove the idea of shared fate between each tracking/subtracking threads. Removes complexity of figuring out what the errgroups are doing as well as complicated logic with shared fate between tracking threads and singlerunners Signed-off-by: David-Jaeyoon-Lee <[email protected]>

Signed-off-by: David-Jaeyoon-Lee <[email protected]>

maxsmythe · 2024-07-16T05:31:39Z

@David-Jaeyoon-Lee Unit tests are failing

JaydipGabani

code LGTM. @David-Jaeyoon-Lee unit tests are failing.

JaydipGabani · 2024-07-18T17:41:12Z

Unit test passed, re-running to check for flakiness.

JaydipGabani · 2024-07-18T17:53:52Z

Running once more to check.

ritazh

LGTM

Thank you for the PR!

… & surface errors swallowed by grp.Wait() (open-policy-agent#3308) Signed-off-by: David-Jaeyoon-Lee <[email protected]> Co-authored-by: Sertaç Özercan <[email protected]> Co-authored-by: Jaydipkumar Arvindbhai Gabani <[email protected]>

David-Jaeyoon-Lee force-pushed the davjlee/refactor/ready-tracker branch 2 times, most recently from 0b9e1f4 to 96644d8 Compare March 14, 2024 20:39

David-Jaeyoon-Lee commented Mar 14, 2024

View reviewed changes

pkg/readiness/ready_tracker.go Outdated Show resolved Hide resolved

pkg/readiness/ready_tracker.go Outdated Show resolved Hide resolved

David-Jaeyoon-Lee force-pushed the davjlee/refactor/ready-tracker branch 2 times, most recently from 25e891e to 2c5000d Compare March 28, 2024 19:39

David-Jaeyoon-Lee marked this pull request as ready for review March 28, 2024 19:40

David-Jaeyoon-Lee requested a review from a team as a code owner March 28, 2024 19:40

maxsmythe reviewed Mar 28, 2024

View reviewed changes

pkg/readiness/ready_tracker.go Outdated Show resolved Hide resolved

pkg/readiness/ready_tracker.go Outdated Show resolved Hide resolved

David-Jaeyoon-Lee force-pushed the davjlee/refactor/ready-tracker branch 2 times, most recently from e1fa4a6 to cbfcfc8 Compare April 2, 2024 00:00

David-Jaeyoon-Lee changed the title ~~fix: #3146 Surface error swallowed by grp.Wait() & replace v1beta references for constraint templates in Ready Tracker~~ fix: #3146 Support close open/fail for Ready Tracker & surface errors swallowed by grp.Wait() Apr 8, 2024

David-Jaeyoon-Lee force-pushed the davjlee/refactor/ready-tracker branch 3 times, most recently from 77559ff to 7c5e5a0 Compare April 8, 2024 17:40

David-Jaeyoon-Lee requested a review from maxsmythe April 8, 2024 17:41

maxsmythe approved these changes Apr 10, 2024

View reviewed changes

pkg/readiness/ready_tracker.go Outdated Show resolved Hide resolved

David-Jaeyoon-Lee force-pushed the davjlee/refactor/ready-tracker branch 2 times, most recently from 1705b33 to aa2b049 Compare April 10, 2024 20:14

maxsmythe requested review from sozercan and ritazh April 10, 2024 20:18

David-Jaeyoon-Lee force-pushed the davjlee/refactor/ready-tracker branch from c9601c0 to 7d63dbf Compare April 10, 2024 20:31

ritazh reviewed Apr 13, 2024

View reviewed changes

pkg/readiness/ready_tracker.go Show resolved Hide resolved

ritazh reviewed Apr 13, 2024

View reviewed changes

pkg/readiness/ready_tracker.go Outdated Show resolved Hide resolved

David-Jaeyoon-Lee force-pushed the davjlee/refactor/ready-tracker branch 3 times, most recently from ab0fdbb to bf1ac93 Compare April 23, 2024 21:18

David-Jaeyoon-Lee commented Apr 23, 2024

View reviewed changes

David-Jaeyoon-Lee force-pushed the davjlee/refactor/ready-tracker branch 2 times, most recently from fc45d15 to 1b69eac Compare April 30, 2024 19:08

David-Jaeyoon-Lee requested a review from maxsmythe April 30, 2024 20:03

David-Jaeyoon-Lee force-pushed the davjlee/refactor/ready-tracker branch 2 times, most recently from ef710df to f6ee82e Compare May 2, 2024 20:32

maxsmythe mentioned this pull request May 21, 2024

fix: Create dynamic CEL object variable open-policy-agent/frameworks#432

Merged

David-Jaeyoon-Lee force-pushed the davjlee/refactor/ready-tracker branch 2 times, most recently from bbf995a to 6df8c51 Compare July 1, 2024 21:53

maxsmythe approved these changes Jul 2, 2024

View reviewed changes

David-Jaeyoon-Lee added 6 commits July 2, 2024 23:37

Add error handling & fail-close support in Ready Tracker

78d01f8

Add fail-close support for fetching expectations by adding error handling & surfacing the errors being swallowed by grp.Wait(). Signed-off-by: David-Jaeyoon-Lee <[email protected]>

Fix starting flag with hyphen

1917366

Remove the starting hyphen from the crash-on-failure-fetching-expectations flag Signed-off-by: David-Jaeyoon-Lee <[email protected]>

Add comment for constraint & data tracker, fix retErr nil comparison,

af869f0

and add unit tests Fix comments and return error in trackConfigAndSyncSets. Add unit tests for each track method & each wait in Run method. Signed-off-by: David-Jaeyoon-Lee <[email protected]>

Fix crashOnFailureFetchingExpectations flag description & race condition

5010682

in test Signed-off-by: David-Jaeyoon-Lee <[email protected]>

Removing race condition by giving each fake client its own scheme object

f874270

Signed-off-by: David-Jaeyoon-Lee <[email protected]>

David-Jaeyoon-Lee force-pushed the davjlee/refactor/ready-tracker branch from 4f8a07d to f874270 Compare July 2, 2024 23:38

ritazh added this to the v3.17.0 milestone Jul 3, 2024

Change naming of variables, refactor, and lint errors

212d84c

Signed-off-by: David-Jaeyoon-Lee <[email protected]>

JaydipGabani approved these changes Jul 17, 2024

View reviewed changes

Merge branch 'master' into davjlee/refactor/ready-tracker

b1a8531

JaydipGabani and others added 2 commits July 18, 2024 11:58

Merge branch 'master' into davjlee/refactor/ready-tracker

762134e

Merge branch 'master' into davjlee/refactor/ready-tracker

bf49b3c

ritazh approved these changes Jul 19, 2024

View reviewed changes

ritazh merged commit 2244cee into open-policy-agent:master Jul 19, 2024
19 checks passed

David-Jaeyoon-Lee mentioned this pull request Jul 24, 2024

feat: Add retry limit when fetching/listing expectations in ready tracker #3460

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: #3146 Support close open/fail for Ready Tracker & surface errors swallowed by grp.Wait() #3308

fix: #3146 Support close open/fail for Ready Tracker & surface errors swallowed by grp.Wait() #3308

David-Jaeyoon-Lee commented Mar 14, 2024 •

edited

Loading

maxsmythe left a comment

David-Jaeyoon-Lee commented Apr 8, 2024

maxsmythe left a comment

codecov-commenter commented Apr 10, 2024 •

edited

Loading

ritazh commented Apr 13, 2024 •

edited

Loading

David-Jaeyoon-Lee Apr 23, 2024

maxsmythe Apr 27, 2024

David-Jaeyoon-Lee commented Apr 30, 2024 •

edited

Loading

maxsmythe left a comment

maxsmythe Jul 2, 2024

David-Jaeyoon-Lee Jul 12, 2024

maxsmythe Jul 16, 2024

maxsmythe commented Jul 16, 2024

JaydipGabani left a comment •

edited

Loading

JaydipGabani commented Jul 18, 2024

JaydipGabani commented Jul 18, 2024 •

edited

Loading

ritazh left a comment

		@@ -46,6 +46,8 @@ import (

		var log = logf.Log.WithName("readiness-tracker")

		var crashOnFailureFetchingExpectations = flag.Bool("crash-on-failure-fetching-expectations", false, "Unless set (defaults to false), gatekeeper will ignore errors that occur when gathering expectations. This prevents bootstrapping errors from crashing Gatekeeper at the cost of increasing the risk Gatekeeper will under-enforce policy by serving before it has loaded in all policies. Enabling this will help prevent under-enforcement at the risk of crashing during startup for issues like network errors. Note that enabling this flag currently does not achieve the aforementioned effect since fetching expectations are set to retry until success so failures during fetching expectations currently do not occur.")

fix: #3146 Support close open/fail for Ready Tracker & surface errors swallowed by grp.Wait() #3308

fix: #3146 Support close open/fail for Ready Tracker & surface errors swallowed by grp.Wait() #3308

Conversation

David-Jaeyoon-Lee commented Mar 14, 2024 • edited Loading

maxsmythe left a comment

Choose a reason for hiding this comment

David-Jaeyoon-Lee commented Apr 8, 2024

maxsmythe left a comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 10, 2024 • edited Loading

Codecov Report

ritazh commented Apr 13, 2024 • edited Loading

David-Jaeyoon-Lee Apr 23, 2024

Choose a reason for hiding this comment

maxsmythe Apr 27, 2024

Choose a reason for hiding this comment

David-Jaeyoon-Lee commented Apr 30, 2024 • edited Loading

maxsmythe left a comment

Choose a reason for hiding this comment

maxsmythe Jul 2, 2024

Choose a reason for hiding this comment

David-Jaeyoon-Lee Jul 12, 2024

Choose a reason for hiding this comment

maxsmythe Jul 16, 2024

Choose a reason for hiding this comment

maxsmythe commented Jul 16, 2024

JaydipGabani left a comment • edited Loading

Choose a reason for hiding this comment

JaydipGabani commented Jul 18, 2024

JaydipGabani commented Jul 18, 2024 • edited Loading

ritazh left a comment

Choose a reason for hiding this comment

David-Jaeyoon-Lee commented Mar 14, 2024 •

edited

Loading

codecov-commenter commented Apr 10, 2024 •

edited

Loading

ritazh commented Apr 13, 2024 •

edited

Loading

David-Jaeyoon-Lee commented Apr 30, 2024 •

edited

Loading

JaydipGabani left a comment •

edited

Loading

JaydipGabani commented Jul 18, 2024 •

edited

Loading