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

jvanz · 2024-04-15T19:01:34Z

Description

Updates the controller to set owner references for all the resources created to make the policy server works. Which includes: service, configmap, deployment and secret. Therefore, we can remove the reconciliation loop code to remove this resources when the user deletes the policy server and rely on Kubernetes garbage collector to do the job.

Fix #660

Test

make test
# manual testing with tilt
tilt up --stream
# install and uninstall policy server

codecov · 2024-04-15T19:06:21Z

Codecov Report

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

Project coverage is 73.17%. Comparing base (86ef275) to head (f80c28b).

Files	Patch %	Lines
internal/pkg/admission/policy-server-ca-secret.go	69.69%	17 Missing and 3 partials ⚠️
internal/pkg/admission/policy-server-deployment.go	55.55%	3 Missing and 5 partials ⚠️
internal/pkg/admission/policy-server-configmap.go	0.00%	1 Missing and 1 partial ⚠️
internal/pkg/admission/policy-server-service.go	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #720      +/-   ##
==========================================
+ Coverage   72.85%   73.17%   +0.32%     
==========================================
  Files          28       28              
  Lines        1908     1812      -96     
==========================================
- Hits         1390     1326      -64     
+ Misses        397      370      -27     
+ Partials      121      116       -5

Flag	Coverage Δ
integration-tests	`61.03% <56.38%> (-0.18%)`	⬇️
unit-tests	`50.19% <62.76%> (+4.12%)`	⬆️

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.

controllers/policyserver_controller_test.go

jvanz · 2024-04-26T17:28:23Z

This PR is based on top of #730. Waiting for that PR to get merged before moving this to pending review.

Updates the base CRDs generated after some recent dependencies updates. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

Updates the controller to set owner references for all the resources created to make the policy server works. Which includes: service, configmap, deployment and secret. Therefore, we can remove the reconciliation loop code to remove this resources when the user deletes the policy server and rely on Kubernetes garbage collector to do the job. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

jvanz · 2024-05-07T21:28:33Z

@kubewarden/kubewarden-developers the integration tests are failing but I do not think this is related to the changes here. It looks like something different. I'm moving this PR to be reviewed by the team while I investigate the issue. It may take some time because I cannot reproduce this in my local branch. 🤞

viccuad · 2024-05-08T12:47:55Z

internal/pkg/admission/policy-server_integration_test.go

-
-const port = "8181"
-
-func TestCAAndCertificateCreationInAHttpsServer(t *testing.T) {


Should we drop this test? Are we missing a test that checks for the cert creation on call to fetchOrInitializePolicyServerCASecret()?

Fair enough, I've re added this test in the internal/pkg/admission/policy-server-ca-secret_test.go file. ;)

viccuad

LGTM, just one question about the cert test refactor.

Uses the controller-runtime helper function CreateOrUpdate to reconcile the secrets storing the certificates used by policy server. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

Some tests needs to update resources in order to properly test resource deletion. However, this needs to happen inside an Eventually call to ensure that the test will update the latest version of the resource. Otherwise, it will fail. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

flavio

LGTM, I left one minor comment

flavio · 2024-05-09T10:57:37Z

config/crd/bases/policies.kubewarden.io_admissionpolicies.yaml

@@ -163,11 +163,13 @@ spec:
                          items:
                            type: string
                          type: array
+                          x-kubernetes-list-type: atomic


Where is this coming from?

This came from a recent update in the apimachinery package. As we use label selector objects in the policy spec, the CRDs will be updated with the latest changes there as well. I've added a commit here to fix that in our main branch and avoid generating noise every time that we run the controller locally.

If you think is better, I can open another PR just for that update in the CRDs.

jvanz self-assigned this Apr 15, 2024

github-actions bot added the kind/feature label Apr 15, 2024

jvanz force-pushed the issue660-owner-references branch from 7103329 to 3abe65b Compare April 15, 2024 19:02

jvanz force-pushed the issue660-owner-references branch 2 times, most recently from 99ad0f1 to 83049a3 Compare April 16, 2024 17:30

jvanz marked this pull request as ready for review April 16, 2024 17:30

jvanz requested a review from a team as a code owner April 16, 2024 17:31

flavio reviewed Apr 17, 2024

View reviewed changes

controllers/policyserver_controller_test.go Outdated Show resolved Hide resolved

jvanz force-pushed the issue660-owner-references branch 3 times, most recently from 9b77c8a to cbaa416 Compare April 17, 2024 17:54

jvanz force-pushed the issue660-owner-references branch 2 times, most recently from 341b80c to bf14af1 Compare April 26, 2024 17:27

jvanz force-pushed the issue660-owner-references branch from bf14af1 to d36cf05 Compare May 7, 2024 02:50

jvanz requested review from flavio and a team and removed request for a team and flavio May 7, 2024 11:34

jvanz force-pushed the issue660-owner-references branch 4 times, most recently from dc14e60 to 933d2fe Compare May 7, 2024 17:47

jvanz added 2 commits May 7, 2024 14:49

fix: update generated base CRDs.

bc7fc72

Updates the base CRDs generated after some recent dependencies updates. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

jvanz force-pushed the issue660-owner-references branch from 933d2fe to 5563f3d Compare May 7, 2024 17:50

jvanz added the kind/tech-debt label May 7, 2024

jvanz requested a review from flavio May 7, 2024 21:28

jvanz requested a review from a team May 7, 2024 21:28

jvanz force-pushed the issue660-owner-references branch from 5563f3d to dd3cd07 Compare May 7, 2024 21:45

viccuad reviewed May 8, 2024

View reviewed changes

jvanz added 2 commits May 8, 2024 16:40

fix: use CreateOrUpdate function to reconcile secrets.

da07b27

Uses the controller-runtime helper function CreateOrUpdate to reconcile the secrets storing the certificates used by policy server. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

jvanz force-pushed the issue660-owner-references branch from dd3cd07 to f80c28b Compare May 8, 2024 19:42

jvanz requested a review from viccuad May 8, 2024 19:44

flavio approved these changes May 9, 2024

View reviewed changes

jvanz merged commit f562dbe into kubewarden:main May 9, 2024
8 of 9 checks passed

jvanz deleted the issue660-owner-references branch May 9, 2024 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jvanz commented Apr 15, 2024

codecov bot commented Apr 15, 2024 •

edited

Loading

jvanz commented Apr 26, 2024

jvanz commented May 7, 2024

viccuad May 8, 2024 •

edited

Loading

jvanz May 8, 2024

viccuad left a comment

flavio left a comment

flavio May 9, 2024

jvanz May 9, 2024


		const port = "8181"

		func TestCAAndCertificateCreationInAHttpsServer(t *testing.T) {

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

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

Conversation

jvanz commented Apr 15, 2024

Description

Test

codecov bot commented Apr 15, 2024 • edited Loading

Codecov Report

jvanz commented Apr 26, 2024

jvanz commented May 7, 2024

viccuad May 8, 2024 • edited Loading

Choose a reason for hiding this comment

jvanz May 8, 2024

Choose a reason for hiding this comment

viccuad left a comment

Choose a reason for hiding this comment

flavio left a comment

Choose a reason for hiding this comment

flavio May 9, 2024

Choose a reason for hiding this comment

jvanz May 9, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 15, 2024 •

edited

Loading

viccuad May 8, 2024 •

edited

Loading