refactor: policy common reconciliation logic in a subreconciler

[fabriziosestito]

Description

This PR refactors and groups the common policy reconciliation logic in a subreconciler, removing the admission package and the Reconciler "god class" leftover.
To learn more about the subreconciler pattern, refer to this

It also removes the packages policyserver and naming and fixes some issues with the setPolicyStatus error handling:

kubewarden-controller/internal/controller/policy_utils.go

Line 43 in 220be72

func setPolicyStatus(ctx context.Context, deploymentsNamespace string, apiReader client.Reader, policy policiesv1.Policy) error {

Fixes #774, fixes #773

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 69 lines in your changes missing coverage. Please review.

Project coverage is 70.25%. Comparing base (a331d8d) to head (0005c69).

Files	Patch %	Lines
internal/controller/policy_subreconciler.go	75.87%	33 Missing and 15 partials ⚠️
...nternal/controller/policy_subreconciler_webhook.go	83.63%	14 Missing and 4 partials ⚠️
api/policies/v1/policyserver_webhook.go	83.33%	1 Missing and 1 partial ⚠️
internal/controller/admissionpolicy_controller.go	90.90%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #796      +/-   ##
==========================================
+ Coverage   70.02%   70.25%   +0.22%     
==========================================
  Files          23       19       -4     
  Lines        1705     1708       +3     
==========================================
+ Hits         1194     1200       +6     
+ Misses        394      389       -5     
- Partials      117      119       +2     

Flag	Coverage Δ
integration-tests	61.88% <77.10%> (+<0.01%)	⬆️
unit-tests	39.86% <83.33%> (+7.96%)	⬆️

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]

LGTM. But I have a question about one field in the reconcilers

[jvanz]

Cannot we remove the Scheme field and get it, when necessary, from the client?

[fabriziosestito]

This comes from the kubebuilder scaffolding. I could find the rationale here: kubernetes-sigs/kubebuilder#986

[flavio]

It looks really good to me. I just left some minor comments

[viccuad]

I expected a different approach for the subreconciler pattern, one like the example from here:

  subreconcilersForPolicy := []subreconciler.FnWithRequest{
    r.setStatusToUnknown,
    r.addFinalizer,
    r.handleDelete,
    r.updateStatus,
  }

  // Run all subreconcilers sequentially
  for _, f := range subreconcilersForPolicy {
    if r, err := f(ctx, req); subreconciler.ShouldHaltOrRequeue(r, err) {
      return subreconciler.Evaluate(r, err)
    }
  }

I don't consider it a blocker though.

We need to also update docs/crds/Makefile for make generate to work, the resulting crd docs are part of the release artifacts.

[fabriziosestito]

I expected a different approach for the subreconciler pattern, one like the example from here:

  subreconcilersForPolicy := []subreconciler.FnWithRequest{
    r.setStatusToUnknown,
    r.addFinalizer,
    r.handleDelete,
    r.updateStatus,
  }

  // Run all subreconcilers sequentially
  for _, f := range subreconcilersForPolicy {
    if r, err := f(ctx, req); subreconciler.ShouldHaltOrRequeue(r, err) {
      return subreconciler.Evaluate(r, err)
    }
  }

This example is using a helper library. I linked the blog post to show the pattern of splitting and sharing reconciliation methods, if you look at the first example in the blog post a subreconciler is just a method. I personally don't want to introduce a dependency to a library, which is an opinionated take on the pattern just for this, but I am open to consider it in the future if the code becomes more complex (for instance, it could be useful when we will add the Policy Groups).

We need to also update docs/crds/Makefile for make generate to work, the resulting crd docs are part > of the release artifacts.

Done in: 5900152

[viccuad]

LGTM modulo the others.

[flavio]

I think we're good to go. Ship it 🚀