refactor: certificates generation package and secrets #777

fabriziosestito · 2024-06-20T16:44:29Z

Description

This PR refactors the generation utilities in the admissionregistration package,
removing duplication and simplifying the current implementation.
It renames admissionregistration in certs and reactors the existing reconciler unit test in integration tests clauses.

Additional info

This PR also changes the golangci configuration to add an exception for the ok map index in the varnamelen linter. However we should uniform and clean up the golangci configurations across all the KW repos, this is tracked by: Uniform golangci configuration across all repositories #778
There are a few incongruences with the naming, for instance, this status

kubewarden-controller/pkg/apis/policies/v1/policyserver_types.go

Line 132 in 030edce

PolicyServerCASecretReconciled PolicyServerConditionType = "CASecretReconciled"

should be probably named CertSecretReconciled instead. Fixing this is out of scope for this PR and I will create an issue about it.

Fixes: #775, #664

codecov · 2024-06-20T16:47:48Z

Codecov Report

Attention: Patch coverage is 81.57895% with 14 lines in your changes missing coverage. Please review.

Project coverage is 72.97%. Comparing base (e3750b2) to head (c5a217e).

Files	Patch %	Lines
internal/pkg/admission/policy-server-ca-secret.go	73.33%	6 Missing and 2 partials ⚠️
internal/pkg/certs/certs.go	86.36%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #777      +/-   ##
==========================================
- Coverage   74.05%   72.97%   -1.09%     
==========================================
  Files          28       26       -2     
  Lines        1808     1780      -28     
==========================================
- Hits         1339     1299      -40     
- Misses        353      366      +13     
+ Partials      116      115       -1

Flag	Coverage Δ
integration-tests	`60.61% <81.57%> (-1.28%)`	⬇️
unit-tests	`37.87% <0.00%> (-12.33%)`	⬇️

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.

flavio

I think we should address some naming confusion about Policy Server CA, see my notes.

Overall LGTM

flavio · 2024-06-24T08:02:31Z

controllers/utils_test.go

@@ -164,6 +164,14 @@ func getTestPolicyServerService(policyServerName string) (*corev1.Service, error
 	return &service, nil
 }

+func getTestPolicyServerCASecret() (*corev1.Secret, error) {


This is retrieving the Root CA created by our controller. I suggest to rename this helper function to reflect that.

Maybe something like getInternalRootCASecret ?

flavio · 2024-06-24T08:05:58Z

internal/pkg/admission/policy-server-ca-secret.go

-type generateCertFunc = func(ca []byte, commonName string, extraSANs []string, CAPrivateKey *rsa.PrivateKey) ([]byte, []byte, error)
-
-func (r *Reconciler) fetchOrInitializePolicyServerCASecret(ctx context.Context, policyServer *policiesv1.PolicyServer, caSecret *corev1.Secret, generateCert generateCertFunc) error {
+func (r *Reconciler) fetchOrInitializePolicyServerCASecret(ctx context.Context, policyServer *policiesv1.PolicyServer, caSecret *corev1.Secret) error {


Again, there's a bit of naming confusion here we inherited from the past.

This method creates the certificate used by a specific Policy Server instance, there's no CA associated with a Policy Server

flavio · 2024-06-24T08:08:42Z

internal/pkg/admission/policy-server-ca-secret.go

 }

-func (r *Reconciler) fetchOrInitializePolicyServerCARootSecret(ctx context.Context, policyServer *policiesv1.PolicyServer, generateCA generateCAFunc, pemEncodeCertificate pemEncodeCertificateFunc) (*corev1.Secret, error) {
+func (r *Reconciler) fetchOrInitializePolicyServerCARootSecret(ctx context.Context, policyServer *policiesv1.PolicyServer) (*corev1.Secret, error) {


Same here, the naming is confusing since no CA is stored inside of the Secret that is returned

flavio · 2024-06-24T08:11:09Z

internal/pkg/admission/policy-server-ca-secret.go

 	return policyServerSecret, nil
 }

-func updateSecretCA(policyServerSecret *corev1.Secret, generateCA generateCAFunc, pemEncodeCertificate pemEncodeCertificateFunc) error {
-	caRoot, err := generateCA()
+func updateCASecret(policyServerSecret *corev1.Secret) error {


This function is always generating the CA, it doesn't look into the contents of the secret that is passed as parameter.

Why don't we change the signature of the function to be something like that?

func createInternalCASecret() (policyServerSecret *corev1.Secret, error)

flavio · 2024-06-24T08:11:49Z

internal/pkg/admission/policy-server-ca-secret.go

Maybe something for later, but I would also rename this file since it can mislead into thinking each Policy Server has a dedicated CA

internal/pkg/certs/ca.go

internal/pkg/certs/cert.go

fabriziosestito · 2024-06-24T12:50:52Z

@flavio I've addressed the comments in c3cbdf4.

I tracked all the naming-related comments in: #782

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

fabriziosestito · 2024-06-25T07:40:11Z

Blocked: waiting for 1.14 release

flavio

Approving, the naming changes I've suggested are going to be done inside of future PRs

fabriziosestito force-pushed the refactor/cert branch 4 times, most recently from fef8105 to bcb9c32 Compare June 21, 2024 12:03

fabriziosestito changed the title ~~wip refactor cert~~ refactor: certificates generation package and secrets Jun 21, 2024

fabriziosestito marked this pull request as ready for review June 21, 2024 12:04

fabriziosestito requested a review from a team as a code owner June 21, 2024 12:04

fabriziosestito self-assigned this Jun 21, 2024

fabriziosestito added this to the 1.15 milestone Jun 21, 2024

fabriziosestito added the kind/tech-debt label Jun 21, 2024

flavio requested changes Jun 24, 2024

View reviewed changes

fabriziosestito mentioned this pull request Jun 24, 2024

Certificates package naming clean-up #782

Closed

fabriziosestito requested a review from flavio June 24, 2024 12:49

jvanz approved these changes Jun 24, 2024

View reviewed changes

fabriziosestito added 7 commits June 25, 2024 08:42

refactor: rework cert package

8bead45

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

refactor: ca and certificates reconciler methods

bc8e152

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

test: remove ca secret unit test

7b9da28

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

chore: format policyserver_controller_test

4e213d0

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

test: update PolicyServer secrets integration test

82b12a9

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

ci: tune varnamelen linter

d4a3b61

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

chore: move certs to a single file; use constant for bit size

c5a217e

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

fabriziosestito force-pushed the refactor/cert branch from c3cbdf4 to c5a217e Compare June 25, 2024 07:38

flavio approved these changes Jun 26, 2024

View reviewed changes

fabriziosestito merged commit b79efed into kubewarden:main Jun 26, 2024
8 of 9 checks passed

jvanz mentioned this pull request Jul 10, 2024

Refactor the internal/pkg/admissionregistration package #664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: certificates generation package and secrets #777

refactor: certificates generation package and secrets #777

fabriziosestito commented Jun 20, 2024 •

edited

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading

flavio left a comment

flavio Jun 24, 2024

flavio Jun 24, 2024

flavio Jun 24, 2024

flavio Jun 24, 2024

flavio Jun 24, 2024

fabriziosestito commented Jun 24, 2024

fabriziosestito commented Jun 25, 2024

flavio left a comment

refactor: certificates generation package and secrets #777

refactor: certificates generation package and secrets #777

Conversation

fabriziosestito commented Jun 20, 2024 • edited Loading

Description

Additional info

codecov bot commented Jun 20, 2024 • edited Loading

Codecov Report

flavio left a comment

Choose a reason for hiding this comment

flavio Jun 24, 2024

Choose a reason for hiding this comment

flavio Jun 24, 2024

Choose a reason for hiding this comment

flavio Jun 24, 2024

Choose a reason for hiding this comment

flavio Jun 24, 2024

Choose a reason for hiding this comment

flavio Jun 24, 2024

Choose a reason for hiding this comment

fabriziosestito commented Jun 24, 2024

fabriziosestito commented Jun 25, 2024

flavio left a comment

Choose a reason for hiding this comment

fabriziosestito commented Jun 20, 2024 •

edited

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading