feat: Implement config pod status #3544

abhipatnala · 2024-09-14T03:35:47Z

What this PR does / why we need it:

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

Special notes for your reviewer:

codecov-commenter · 2024-09-14T03:44:39Z

Codecov Report

Attention: Patch coverage is 48.97959% with 125 lines in your changes missing coverage. Please review.

Project coverage is 48.22%. Comparing base (3350319) to head (20cb2ca).
Report is 157 commits behind head on master.

Files with missing lines	Patch %	Lines
apis/status/v1beta1/zz_generated.deepcopy.go	0.00%	62 Missing ⚠️
...controller/configstatus/configstatus_controller.go	64.47%	17 Missing and 10 partials ⚠️
pkg/controller/config/config_controller.go	66.15%	15 Missing and 7 partials ⚠️
apis/status/v1beta1/configpodstatus_types.go	80.00%	2 Missing and 2 partials ⚠️
pkg/controller/expansion/expansion_controller.go	50.00%	2 Missing and 2 partials ⚠️
...er/constraintstatus/constraintstatus_controller.go	0.00%	1 Missing and 1 partial ⚠️
...platestatus/constrainttemplatestatus_controller.go	0.00%	1 Missing and 1 partial ⚠️
...ller/expansionstatus/expansionstatus_controller.go	0.00%	1 Missing and 1 partial ⚠️

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

HEAD has 1 upload less than BASE

Flag BASE (3350319) HEAD (20cb2ca)

unittests 2 1

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3544      +/-   ##
==========================================
- Coverage   54.49%   48.22%   -6.28%     
==========================================
  Files         134      221      +87     
  Lines       12329    15371    +3042     
==========================================
+ Hits         6719     7413     +694     
- Misses       5116     7119    +2003     
- Partials      494      839     +345

Flag	Coverage Δ
unittests	`48.22% <48.97%> (-6.28%)`	⬇️

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.

config/crd/bases/status.gatekeeper.sh_configpodstatuses.yaml

pkg/readiness/ready_tracker.go

pkg/controller/config/config_controller.go

pkg/controller/configstatus/configstatus_controller.go

maxsmythe · 2024-09-19T22:12:29Z

pkg/controller/configstatus/configstatus_controller.go

+
+// PodStatusToConfigMapper correlates a ConfigPodStatus with its corresponding Config.
+// `selfOnly` tells the mapper to only map statuses corresponding to the current pod.
+func PodStatusToConfigMapper(selfOnly bool) handler.TypedMapFunc[*v1beta1.ConfigPodStatus] {


It looks like selfOnly is not being used. This code was copied from expansion templates, which also did not use selfOnly. This is a bug. Here is an example of proper use of selfOnly in the constraint controller:

gatekeeper/pkg/controller/constraint/constraint_controller.go

Lines 194 to 198 in 07127f2

err = c.Watch(

source.Kind(mgr.GetCache(), &constraintstatusv1beta1.ConstraintPodStatus{}, handler.TypedEnqueueRequestsFromMapFunc(constraintstatus.PodStatusToConstraintMapper(true, util.EventPackerMapFunc()))))

if err != nil {

return err

}

Here is the basic reasoning:

status controllers want to watch all podStatus objects and the primary object because they want to make sure they respond to any changes (i.e. writing status changes, overwriting inappropriate deletes of the status field, etc.)

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.

As it stands, if a user were to delete the podStatus object, there is a risk that there would be no reconcile.

We should add the appropriate watches to the config and expansion controllers.

Could we have a PR that fixes this in the other controller?

If selfOnly is always required and true, why is this a variable? when does it need to be set to false?

pkg/controller/configstatus/configstatus_controller.go

maxsmythe · 2024-09-19T22:25:11Z

pkg/controller/configstatus/configstatus_controller.go

+
+// Add creates a new config Status Controller and adds it to the Manager. The Manager will set fields on the Controller
+// and Start it when the Manager is Started.
+func (a *Adder) Add(mgr manager.Manager) error {


It doesn't look like we're gating status controller on whether the status operation is enabled -- this is bad because it means this controller will not run as a singleton, which invites write conflicts particularly as the # of pods scales.

Example of the status gate:

gatekeeper/pkg/controller/constrainttemplate/constrainttemplate_controller.go

Lines 155 to 178 in 07127f2

if operations.IsAssigned(operations.Status) {

// statusEvents will be used to receive events from dynamic watches registered

// via the registrar below.

statusEvents := make(chan event.GenericEvent, 1024)

csAdder := constraintstatus.Adder{

CFClient: cfClient,

WatchManager: wm,

ControllerSwitch: cs,

Events: statusEvents,

IfWatching: statusW.IfWatching,

}

if err := csAdder.Add(mgr); err != nil {

return nil, err

}

ctsAdder := constrainttemplatestatus.Adder{

CfClient: cfClient,

WatchManager: wm,

ControllerSwitch: cs,

}

if err := ctsAdder.Add(mgr); err != nil {

return nil, err

}

}

Though a more appropriate code shape for this PR is probably the mutator status gate:

gatekeeper/pkg/controller/mutatorstatus/mutatorstatus_controller.go

Lines 60 to 62 in 07127f2

if !operations.IsAssigned(operations.MutationStatus) {

return nil

}

(however we should depend on Status, not MutatorStatus)

We should also fix this for the expansion template status controller (worth verifying it has a similar oversight first).

Long-term a more uniform design pattern for adding status controllers may help avoid similar oversights in the future, but that's beyond the scope of this PR.

Updated the logic to use status gate

Can we create PRs to add it to the other status controllers that do not have a similar gate? This would include expansion template status at the minimum.

pkg/controller/configstatus/configstatus_controller.go

apis/status/v1beta1/configpodstatus_types.go

maxsmythe

almost there! Minor nits and some follow-up items.

apis/status/v1beta1/configpodstatus_types.go

pkg/controller/config/config_controller.go

maxsmythe · 2024-09-25T02:01:02Z

pkg/controller/configstatus/configstatus_controller.go

+
+// Add creates a new config Status Controller and adds it to the Manager. The Manager will set fields on the Controller
+// and Start it when the Manager is Started.
+func (a *Adder) Add(mgr manager.Manager) error {


Can we create PRs to add it to the other status controllers that do not have a similar gate? This would include expansion template status at the minimum.

maxsmythe · 2024-09-25T02:01:43Z

pkg/controller/configstatus/configstatus_controller.go

+
+// PodStatusToConfigMapper correlates a ConfigPodStatus with its corresponding Config.
+// `selfOnly` tells the mapper to only map statuses corresponding to the current pod.
+func PodStatusToConfigMapper(selfOnly bool) handler.TypedMapFunc[*v1beta1.ConfigPodStatus] {


Could we have a PR that fixes this in the other controller?

pkg/controller/configstatus/configstatus_controller.go

maxsmythe

Code LGTM, pending resolution of either fixing readiness tracker tests in this PR or separating code to separate PR.

pkg/readiness/ready_tracker_unit_test.go

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

…ntrollers to use operation.status flag Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

maxsmythe

LGTM after final minor fix. Good job!

pkg/controller/mutatorstatus/mutatorstatus_controller.go

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

maxsmythe

LGTM

JaydipGabani

Thanks for the PR! changes looks good to me.

One q:
Issue mentiones adding gatekeeper_config metrics and status tag for the same as well. Do we want to include those changes in this PR or keep the original issue open and follow up with another PR?

maxsmythe · 2024-10-08T01:01:59Z

Let's make it a separate PR. This one is already sizeable and that's conceptually a different thing. Maybe file a separate issue for metrics and status tag?

JaydipGabani

LGTM

ritazh · 2024-10-09T23:57:47Z

pkg/controller/configstatus/configstatus_controller.go

+	log    logr.Logger
+}
+
+// +kubebuilder:rbac:groups=config.gatekeeper.sh,resources=*,verbs=get;list;watch;create;update;patch;delete


This overrides existing rbac rules per https://github.com/open-policy-agent/gatekeeper/blob/master/pkg/controller/config/config_controller.go#L130-L131

should the old ones be removed?

I think what it does is adds overlapping entries in the role rather than override?

IMO it's safest to just duplicate to avoid accidentally deleting necessary permissions due to code refactors, but not a huge deal either way for me.

I'm ok with keeping it. Just wanted to make sure this is what we want as it trumps the old rules.

groups=config.gatekeeper.sh,resources=*,verbs=get;list;watch;create;update;patch;delete

ritazh

LGTM

abhipatnala requested a review from a team as a code owner September 14, 2024 03:35

abhipatnala force-pushed the add_status_to_config branch from 99bfce6 to d1016c3 Compare September 16, 2024 17:41

maxsmythe reviewed Sep 16, 2024

View reviewed changes

config/crd/bases/status.gatekeeper.sh_configpodstatuses.yaml Outdated Show resolved Hide resolved

abhipatnala force-pushed the add_status_to_config branch 2 times, most recently from 0721779 to a240878 Compare September 19, 2024 18:18

abhipatnala requested a review from maxsmythe September 19, 2024 18:21

maxsmythe reviewed Sep 19, 2024

View reviewed changes

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

maxsmythe requested changes Sep 19, 2024

View reviewed changes

abhipatnala force-pushed the add_status_to_config branch from d545b1f to 60ec8d5 Compare September 20, 2024 23:21

maxsmythe reviewed Sep 23, 2024

View reviewed changes

pkg/controller/configstatus/configstatus_controller.go Outdated Show resolved Hide resolved

maxsmythe reviewed Sep 23, 2024

View reviewed changes

apis/status/v1beta1/configpodstatus_types.go Outdated Show resolved Hide resolved

abhipatnala requested a review from maxsmythe September 23, 2024 22:07

abhipatnala force-pushed the add_status_to_config branch from c7e45a5 to d7d2405 Compare September 24, 2024 23:26

maxsmythe requested changes Sep 25, 2024

View reviewed changes

maxsmythe reviewed Sep 27, 2024

View reviewed changes

pkg/readiness/ready_tracker_unit_test.go Show resolved Hide resolved

abhipatnala force-pushed the add_status_to_config branch from dabdc7b to eaf85ba Compare October 1, 2024 17:09

Avinash Patnala added 13 commits October 1, 2024 17:10

add config pod status crd

dfe3878

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

generate manifests

14d80aa

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

implement config status controller wip

369004c

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

implement config pod status logic

9c27e83

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

remove make comment from api docs

290b46e

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

fix go lint errors

2d6ccba

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

fix helm charts

b149d3f

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

fix double import

bc5ba07

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

add or delete pod status at the end

8806502

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

pass request.name as key when config is deleted

a2f6e9e

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

update config controller to pass request.NameSpace.Name

9cf8fe5

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

wrap fake client with mutex logic to resolve data race issues

00c3e41

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

use wrapped fake client in failing test

842b26a

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

Avinash Patnala added 10 commits October 1, 2024 17:10

Address review comments

0f431eb

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

fix lint errors

79c048b

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

add unit tests for config pod status types

9c5b5d7

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

write upsert error to status obj

3a4e7b2

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

update kubebuilder tag in config status controller

1d44b5f

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

pass namespace to reconcile request of config controller

f371fb3

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

address review comments

45c2da7

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

use namespace stored in config pod status

4dae25f

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

store namespace in configpod status name

b326059

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

address review comments

b75a3ea

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

abhipatnala force-pushed the add_status_to_config branch from eaf85ba to b75a3ea Compare October 1, 2024 17:10

wrap fake client with mutex in unit tests

8201cff

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

abhipatnala requested a review from maxsmythe October 1, 2024 18:17

fix expansion template status to use self only and gate all status co…

958be2c

…ntrollers to use operation.status flag Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

maxsmythe reviewed Oct 2, 2024

View reviewed changes

pkg/controller/mutatorstatus/mutatorstatus_controller.go Outdated Show resolved Hide resolved

remove gating in mutator status controller

a860f6c

Signed-off-by: Avinash Patnala <avinashpatnala@google.com>

abhipatnala requested a review from maxsmythe October 2, 2024 17:34

maxsmythe approved these changes Oct 3, 2024

View reviewed changes

maxsmythe requested a review from a team October 3, 2024 01:27

Merge branch 'master' into add_status_to_config

3600f97

JaydipGabani reviewed Oct 7, 2024

View reviewed changes

JaydipGabani approved these changes Oct 8, 2024

View reviewed changes

ritazh reviewed Oct 9, 2024

View reviewed changes

This was referenced Oct 10, 2024

Gate each status controller on the status operation to avoid conflict #3624

Closed

Use selfOnly in expansion controller #3625

Closed

Merge branch 'master' into add_status_to_config

20cb2ca

ritazh approved these changes Oct 11, 2024

View reviewed changes

ritazh merged commit 7d71ba2 into open-policy-agent:master Oct 11, 2024
21 checks passed

BrewTestBot mentioned this pull request Dec 12, 2024

gator 3.18.0 Homebrew/homebrew-core#200877

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement config pod status #3544

feat: Implement config pod status #3544

abhipatnala commented Sep 14, 2024

codecov-commenter commented Sep 14, 2024 •

edited

Loading

maxsmythe Sep 19, 2024

abhipatnala Sep 21, 2024

maxsmythe Sep 25, 2024

ritazh Oct 10, 2024

maxsmythe Sep 19, 2024

abhipatnala Sep 21, 2024

maxsmythe Sep 25, 2024

maxsmythe left a comment

maxsmythe Sep 25, 2024

maxsmythe Sep 25, 2024

maxsmythe left a comment

maxsmythe left a comment

maxsmythe left a comment

JaydipGabani left a comment

maxsmythe commented Oct 8, 2024

JaydipGabani left a comment

ritazh Oct 9, 2024

maxsmythe Oct 10, 2024

ritazh Oct 11, 2024 •

edited

Loading

ritazh left a comment

	err = c.Watch(
	source.Kind(mgr.GetCache(), &constraintstatusv1beta1.ConstraintPodStatus{}, handler.TypedEnqueueRequestsFromMapFunc(constraintstatus.PodStatusToConstraintMapper(true, util.EventPackerMapFunc()))))
	if err != nil {
	return err
	}

	if operations.IsAssigned(operations.Status) {
	// statusEvents will be used to receive events from dynamic watches registered
	// via the registrar below.
	statusEvents := make(chan event.GenericEvent, 1024)
	csAdder := constraintstatus.Adder{
	CFClient: cfClient,
	WatchManager: wm,
	ControllerSwitch: cs,
	Events: statusEvents,
	IfWatching: statusW.IfWatching,
	}
	if err := csAdder.Add(mgr); err != nil {
	return nil, err
	}

	ctsAdder := constrainttemplatestatus.Adder{
	CfClient: cfClient,
	WatchManager: wm,
	ControllerSwitch: cs,
	}
	if err := ctsAdder.Add(mgr); err != nil {
	return nil, err
	}
	}

	if !operations.IsAssigned(operations.MutationStatus) {
	return nil
	}

feat: Implement config pod status #3544

feat: Implement config pod status #3544

Conversation

abhipatnala commented Sep 14, 2024

codecov-commenter commented Sep 14, 2024 • 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

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

JaydipGabani left a comment

Choose a reason for hiding this comment

maxsmythe commented Oct 8, 2024

JaydipGabani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritazh Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

ritazh left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 14, 2024 •

edited

Loading

ritazh Oct 11, 2024 •

edited

Loading