Update controller code to use the controller-runtime helper function to create or update resources #730

jvanz · 2024-04-24T01:59:26Z

Description

This PR updates the controller code responsible to update the policy server resources to use the controller-runtime CreateOrUpdate helper function to abstract the logic of detecting if the resource should be created or updated. Making the code more simple and easy to maintain.

During this process the unit tests testing the removed code have been removed as well. Which bring us to the spin off PR of this one, the integration tests reorganization #731 . The old tests have been reorganized to describe better the context of what the tests are validating. As well as updating the whole policy server controller tests to be independent as suggested by the Ginkgo documentation. After that, to ensure that the controller continue to works as before a bunch of integrations tests have been added covering the removed unit tests

~~NOTE: I've not added test to cover the metrics configurations yet. I'll add a future PR to cover that. I do not want to wait more in this PR to get some feedback from the team.~~ This is not necessary anymore. I've added tests for the metrics and tracing configuration here as well.

Fix #723

Test

make test

Additional information

This PR has the same content of the Create or update function #726 . That PR is closed just to get a better history and code coverage report.

codecov · 2024-04-24T02:42:36Z

Codecov Report

Attention: Patch coverage is 84.98024% with 38 lines in your changes are missing coverage. Please review.

Project coverage is 73.84%. Comparing base (66fac8e) to head (5adaba0).

Files	Patch %	Lines
internal/pkg/admission/policy-server-deployment.go	77.27%	28 Missing and 7 partials ⚠️
internal/pkg/admission/policy-server-service.go	91.66%	1 Missing and 1 partial ⚠️
internal/pkg/admission/policy-server-configmap.go	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #730      +/-   ##
==========================================
+ Coverage   71.02%   73.84%   +2.81%     
==========================================
  Files          28       28              
  Lines        2071     1908     -163     
==========================================
- Hits         1471     1409      -62     
+ Misses        459      381      -78     
+ Partials      141      118      -23

Flag	Coverage Δ
integration-tests	`62.36% <81.81%> (+2.88%)`	⬆️
unit-tests	`46.07% <41.89%> (+1.31%)`	⬆️

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.

jvanz · 2024-04-24T19:20:02Z

@kubewarden/kubewarden-developers I take this opportunity to verify some of the policy controller flaky test that we can see in the main branch. I could hit this test failing regularly in this branch. Therefore, I've started by reorganizing the tests to be independent. This way I can run the problematic test individually. That's why, you can see another test reorganization in the policy controller tests. ;)

jvanz · 2024-04-25T15:17:09Z

@kubewarden/kubewarden-developers I've split this PR into two. The second one, #731, has the test reorganization that I did while working on this change. As the tests added in this PR require the changes on #731, this PR is based on that changes. Thus, #731 should be merge before.

Moving this PR to block until we merge #731. Once it's merged, I'll rebase this on top of main again and we will have a cleaner diff.

internal/pkg/admission/policy-server-service_test.go

viccuad

LGTM!

fabriziosestito

Overall, LGTM.
Great work!
The reconciler code looks very simple now and we got rid of all the custom logic.

Just left some minor comments.

fabriziosestito · 2024-04-29T08:55:35Z

internal/pkg/admission/policy-server-configmap.go

@@ -200,7 +125,7 @@ func (policyConfigEntryMap PolicyConfigEntryMap) ToClusterAdmissionPolicyReconci
 	return res
 }

-func (r *Reconciler) createPoliciesMap(admissionPolicies []policiesv1.Policy) PolicyConfigEntryMap {
+func (r *Reconciler) CreatePoliciesMap(admissionPolicies []policiesv1.Policy) PolicyConfigEntryMap {


This does not need to be public, as it is not used elsewhere.
We could call it something like buildPoliciesMap since the current name can be ambiguous (one could think that a Create action is performed by a client in this function).
Also, this doesn't need to be a method, by using just a function we could simplify the unit test in the future, when we get rid/refactor of the other unit tests.

Sorry, I was using this function in the integration tests. But I've ended not using and generating the data to check the result there instead.

internal/pkg/admission/policy-server-configmap.go

controllers/policyserver_controller_test.go

fabriziosestito · 2024-04-29T11:20:33Z

controllers/policyserver_controller_test.go

+		}, timeout, pollInterval).Should(Succeed())
+	})
+
+	It("when the requests are updated should update the PolicyServer pod with the new requests", func() {


maybe rephrase (It)"should update the PolicyServer pod with the new requests when the requests are updated"

I've also moved the test to the context about updating policy servers. I believe it better to keep this test there. It's testing policy server updates.

jvanz · 2024-04-29T13:20:51Z

@fabriziosestito I've addressed your comments. Can you review it again? Thanks!

Updates the kubebuilder statements to build the RBAC roles in the controller to be in sync with the roles used in the Helm charts. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

Updates the reconciliation code to use the controller-runtime helper functions thats abstracts the logic to decide if the resource should be created or updated. Therefore, our code can be simplified and it is not necessary custom code to detect if the source should be updated or not. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

Updates the controller to use the controller-runtime help function CreateOrUpdate to reconcile the validation and mutating webhooks. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

As now the controller uses the controller-runtime helper function to create and update resources, the unit tests that validate our custom code has been removed. But it's still necessary to validate the behaviour of the controller. Therefore, this commit adds a bunch of integration test to cover the same verification done by the previous unit tests. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

Turns some methods in the reconciler used in the policy server configmap creation into functions. These function does not need access to information from the reconciler struct. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

Updates the integration tests grouping all the Policy Server creation tests in a context. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

Increases the timeout to run test because Github workers can be slow. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

flavio

That's a really nice cleanup! 👏

jvanz self-assigned this Apr 24, 2024

jvanz requested a review from a team as a code owner April 24, 2024 01:59

jvanz added the kind/tech-debt label Apr 24, 2024

jvanz force-pushed the create-or-update-function branch from 64fa644 to 083ceff Compare April 24, 2024 02:41

jvanz force-pushed the create-or-update-function branch 2 times, most recently from d9ce0e1 to 5e4bb58 Compare April 24, 2024 19:06

jvanz force-pushed the create-or-update-function branch from 5e4bb58 to 6723672 Compare April 24, 2024 19:23

jvanz mentioned this pull request Apr 25, 2024

Test reorganization #731

Merged

jvanz force-pushed the create-or-update-function branch 2 times, most recently from e7218af to a23f394 Compare April 25, 2024 15:15

viccuad reviewed Apr 26, 2024

View reviewed changes

internal/pkg/admission/policy-server-service_test.go Show resolved Hide resolved

jvanz force-pushed the create-or-update-function branch 2 times, most recently from 35ef552 to fdcdc08 Compare April 26, 2024 14:45

jvanz requested a review from viccuad April 26, 2024 14:52

jvanz force-pushed the create-or-update-function branch from fdcdc08 to 5e5c6c6 Compare April 26, 2024 15:02

This was referenced Apr 26, 2024

feat: use owner reference with policy server resources. #720

Merged

Increase the usage of metadata.ownerReferences #660

Closed

viccuad approved these changes Apr 29, 2024

View reviewed changes

fabriziosestito requested changes Apr 29, 2024

View reviewed changes

jvanz force-pushed the create-or-update-function branch from bad974a to 1b54aee Compare April 29, 2024 13:19

jvanz requested a review from fabriziosestito April 29, 2024 13:19

fabriziosestito approved these changes Apr 29, 2024

View reviewed changes

jvanz requested a review from flavio April 29, 2024 14:04

jvanz added 3 commits May 6, 2024 09:04

fix: sync RBAC config with Helm charts.

cdcc234

Updates the kubebuilder statements to build the RBAC roles in the controller to be in sync with the roles used in the Helm charts. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

fix: use CreateOrUpdate to reconcile webhooks.

4d2617a

Updates the controller to use the controller-runtime help function CreateOrUpdate to reconcile the validation and mutating webhooks. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

jvanz added 3 commits May 6, 2024 09:04

fix: turn methods into functions

bececfa

Turns some methods in the reconciler used in the policy server configmap creation into functions. These function does not need access to information from the reconciler struct. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

fix: group creation integration tests in a context.

204d2bc

Updates the integration tests grouping all the Policy Server creation tests in a context. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

jvanz force-pushed the create-or-update-function branch from 1b54aee to 204d2bc Compare May 6, 2024 12:06

fix: increase test timeout.

5adaba0

Increases the timeout to run test because Github workers can be slow. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

flavio approved these changes May 6, 2024

View reviewed changes

jvanz merged commit 75ff9bb into kubewarden:main May 7, 2024
9 checks passed

jvanz deleted the create-or-update-function branch May 7, 2024 02:01

jvanz mentioned this pull request May 27, 2024

Kubewarden controller potentially doing too many requests to apiserver #645

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update controller code to use the controller-runtime helper function to create or update resources #730

Update controller code to use the controller-runtime helper function to create or update resources #730

jvanz commented Apr 24, 2024 •

edited

Loading

codecov bot commented Apr 24, 2024 •

edited

Loading

jvanz commented Apr 24, 2024 •

edited

Loading

jvanz commented Apr 25, 2024 •

edited

Loading

viccuad left a comment

fabriziosestito left a comment

fabriziosestito Apr 29, 2024

jvanz Apr 29, 2024

fabriziosestito Apr 29, 2024

jvanz Apr 29, 2024

jvanz commented Apr 29, 2024

flavio left a comment

Update controller code to use the controller-runtime helper function to create or update resources #730

Update controller code to use the controller-runtime helper function to create or update resources #730

Conversation

jvanz commented Apr 24, 2024 • edited Loading

Description

Test

Additional information

codecov bot commented Apr 24, 2024 • edited Loading

Codecov Report

jvanz commented Apr 24, 2024 • edited Loading

jvanz commented Apr 25, 2024 • edited Loading

viccuad left a comment

Choose a reason for hiding this comment

fabriziosestito left a comment

Choose a reason for hiding this comment

fabriziosestito Apr 29, 2024

Choose a reason for hiding this comment

jvanz Apr 29, 2024

Choose a reason for hiding this comment

fabriziosestito Apr 29, 2024

Choose a reason for hiding this comment

jvanz Apr 29, 2024

Choose a reason for hiding this comment

jvanz commented Apr 29, 2024

flavio left a comment

Choose a reason for hiding this comment

jvanz commented Apr 24, 2024 •

edited

Loading

codecov bot commented Apr 24, 2024 •

edited

Loading

jvanz commented Apr 24, 2024 •

edited

Loading

jvanz commented Apr 25, 2024 •

edited

Loading