feat: Export Connection CR by nreisch · Pull Request #3999 · open-policy-agent/gatekeeper

nreisch · 2025-06-13T14:35:39Z

What this PR does / why we need it:
Details: https://docs.google.com/document/d/12P3LCaOAQO9Uts4cVljHXkRgukEyWqyLketDP0rFq8A/edit?tab=t.0
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 #2802

Special notes for your reviewer:

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

codecov-commenter · 2025-06-13T14:46:54Z

Codecov Report

❌ Patch coverage is 31.85841% with 231 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.91%. Comparing base (3350319) to head (de195a3).
⚠️ Report is 606 commits behind head on master.

Files with missing lines	Patch %	Lines
apis/status/v1alpha1/zz_generated.deepcopy.go	0.00%	71 Missing ⚠️
apis/connection/v1alpha1/zz_generated.deepcopy.go	0.00%	68 Missing ⚠️
...er/connectionstatus/connectionstatus_controller.go	62.99%	39 Missing and 8 partials ⚠️
pkg/audit/manager.go	27.50%	28 Missing and 1 partial ⚠️
apis/status/v1alpha1/connectionpodstatus_types.go	73.91%	4 Missing and 2 partials ⚠️
apis/addtoscheme_connection_v1alpha1.go	0.00%	3 Missing ⚠️
apis/addtoscheme_status_v1alpha1.go	0.00%	3 Missing ⚠️
apis/connection/v1alpha1/connection_types.go	0.00%	2 Missing ⚠️
pkg/controller/add_connectionstatus.go	0.00%	2 Missing ⚠️

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

HEAD has 1 upload less than BASE

Flag BASE (3350319) HEAD (de195a3)

unittests 2 1

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #3999       +/-   ##
===========================================
- Coverage   54.49%   38.91%   -15.59%     
===========================================
  Files         134      243      +109     
  Lines       12329    20382     +8053     
===========================================
+ Hits         6719     7931     +1212     
- Misses       5116    11862     +6746     
- Partials      494      589       +95

Flag	Coverage Δ
unittests	`38.91% <31.85%> (-15.59%)`	⬇️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

JaydipGabani

Thanks for working on this PR @nreisch. Still reviewing PR but posting partial review.

JaydipGabani · 2025-06-16T18:09:27Z

  version: v1alpha1
+- group: status
+  kind: ConnectionPodStatus
+  version: v1beta1


Should this be alpha as this is net new status type?

In the following comment, #2598 (comment) it was suggested to go to beta directly, since the pod status resource isn't interfaced with directly by user facing clients, and instead it's only operated on internally, although it still requires backward compatibility with the status.

However, I do think it make sense to start with alpha to be safe and align with the Connection version, changed to alpha.

I am fine either way based on Max's reasoning. @sozercan @ritazh any thoughts on this?

JaydipGabani · 2025-06-17T00:09:37Z

+
+	err = c.Watch(
+		source.Kind(
+			mgr.GetCache(), &statusv1beta1.ConnectionPodStatus{},


Do we need to watch for connectionPodStatus here? We don't have any operational steps to be reconciled on pod status for connection unless I am missing something.

There's a comment here by @maxsmythe describing the value of having the primary controller watching the pod status #3544 (comment).

Snippet from that comment:

primary controllers want to watch podStatus for the corresponding pod -- if someone deletes a podStatus resource the main object should be re-reconciled to avoid missing state.

I'm also not clear if the primary controller watching the pod status is required. My understanding is that even without the primary controller watching the pod status, if there's a delete or modification to the pod status, the status controller should pick that up, write the new/empty status to the primary resource, and as a result the primary resource should get reconciled by the primary controller.

Maybe Max can chime in on the above

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

JaydipGabani

Nothing major, just few nits and questions. Otherwise lgtm.

JaydipGabani · 2025-06-20T21:18:21Z

  version: v1alpha1
+- group: status
+  kind: ConnectionPodStatus
+  version: v1beta1


I am fine either way based on Max's reasoning. @sozercan @ritazh any thoughts on this?

JaydipGabani · 2025-06-20T22:40:01Z

+	err = r.system.UpsertConnection(ctx, connObj.Spec.Config.Value, request.Name, connObj.Spec.Driver)
+	if err != nil {
+		// Reset the active connection status to false if UpsertConnection fails
+		activeConnection := false


is there a need to change active here? this will be overwritten regardless from audit if publish fails for all messages. Thinking out loud.

I think this decision to set to false here is about having stronger consistency guarantees. The observedGeneration of the status should reflect the latest state of the updated connection spec, so if Reconcile gets called for an updated connection with active currently set to true, and Upsert fails we should set active to false, instead of - maintaining the existing true value if it was previously successful as the status gets updated with the new observedGeneration, and waiting some delay for it to get accurately reflected when the current or next Audit finishes. There's potentially another way of viewing this active property that it should only be managed by Audit when publishing but then we get the delays reconciling the property.

JaydipGabani · 2025-06-20T22:47:35Z

+		// Reset the active connection state when there are updates to the Connection object to ensure the active state is only true when the Publish succeeds for the current Connection
+		resetActiveConnection := false
+		activeConnection = &resetActiveConnection
+	} else if activeConnection == nil {


when will activeConnection == nil be true? Just curious.

In the code activeConnection is true if published messages are > 0 which is always either result in true or false, right?

At the end of the Audit when we update the connection status, if active gets set to true, this Reconcile will get triggered and an Upsert will occur on the connection, in that case, the connection spec hasn't changed so we don't want to reset active to false, instead just use the existing active value otherwise if we did reset it would cause thrashing. In other words - anytime there's a successful Upsert we don't know if active can be true only from Upsert perspective can we set active to false if there was a failure, so we pass nil on successful Upsert and then it will fall into the if and else if above to determine if it should maintain the existing or reset based on the observedGeneration.

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

JaydipGabani

lgtm

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

sozercan

minor comments, otherwise LGTM. Thank you!

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

feat: Export Connection CR

c8978ca

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

nreisch requested a review from a team as a code owner June 13, 2025 14:35

nreisch added 6 commits June 13, 2025 18:06

ci

323f023

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

ci

2a35c0d

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

lint

974e56d

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

lint

7009b60

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

test improvements

c56e68c

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

lint

c53dd35

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

JaydipGabani requested changes Jun 17, 2025

View reviewed changes

address comments

cc976b9

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

nreisch force-pushed the nreisch/auditExportCrdProd branch from e4108be to cc976b9 Compare June 17, 2025 20:00

nreisch added 2 commits June 19, 2025 16:03

reset active state

aec54ba

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

nit comment

08dc142

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

JaydipGabani requested changes Jun 20, 2025

View reviewed changes

address comments 2

d9ba430

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

JaydipGabani approved these changes Jun 24, 2025

View reviewed changes

address comments 3

7e50f5a

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

nreisch force-pushed the nreisch/auditExportCrdProd branch from 7f357a8 to 7e50f5a Compare June 24, 2025 19:50

nreisch added 2 commits June 24, 2025 15:50

Merge branch 'master' into nreisch/auditExportCrdProd

d5ab2bd

lint

6dbfd17

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

sozercan reviewed Jun 25, 2025

View reviewed changes

Comment thread website/docs/export.md Outdated

sozercan reviewed Jun 25, 2025

View reviewed changes

Comment thread website/docs/export.md

sozercan approved these changes Jun 25, 2025

View reviewed changes

nreisch added 2 commits June 25, 2025 21:56

address comments 4

eae2886

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

nit change name

de195a3

Signed-off-by: Noah Reisch <noahreisch4@gmail.com>

nreisch force-pushed the nreisch/auditExportCrdProd branch from 5b72d9c to de195a3 Compare June 25, 2025 22:03

JaydipGabani merged commit e8959c8 into open-policy-agent:master Jun 25, 2025
26 checks passed

Conversation

nreisch commented Jun 13, 2025

Uh oh!

codecov-commenter commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JaydipGabani left a comment

Choose a reason for hiding this comment

Uh oh!

JaydipGabani Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

nreisch Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaydipGabani Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JaydipGabani Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

nreisch Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JaydipGabani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JaydipGabani Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JaydipGabani Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

nreisch Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaydipGabani Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

nreisch Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaydipGabani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sozercan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jun 13, 2025 •

edited

Loading

nreisch Jun 17, 2025 •

edited

Loading

nreisch Jun 24, 2025 •

edited

Loading

nreisch Jun 24, 2025 •

edited

Loading