fix authconfig reconciliation

[eguzki]

What

After #303, the authpolicy controller watched for changes of the authconfig objects owned by the authpolicy.

When the desired authconfig is being generated and compared with the existing object, here, the comparison always gives "update needed" when some fields are empty maps. This is because the serialization omits empty maps, but the desired object has a map with 0 entries, which is not nil.

spec:   v1beta2.AuthConfigSpec{
  	... // 4 identical fields
  	Metadata:      nil,
  	Authorization: {"deny-all": {AuthorizationMethodSpec: {Opa: &{Rego: "allow = false"}}}},
  	Response: &v1beta2.ResponseSpec{
  		Unauthenticated: nil,
  		Unauthorized:    &{Headers: {"content-type": {Value: {Raw: `"application/json"`}}}, Body: &{Value: {Raw: `"{\n  \"error\": \"Forbidden\",\n  \"message\": \"Access denied `...}}},
  		Success: v1beta2.WrappedSuccessResponseSpec{
- 			Headers:         nil,
+ 			Headers:         map[string]v1beta2.HeaderSuccessResponseSpec{},
- 			DynamicMetadata: nil,
+ 			DynamicMetadata: map[string]v1beta2.SuccessResponseSpec{},
  		},
  	},
  	Callbacks: nil,
  }

The consequence is that the authpolicy controller is endlessly reconciling an object and also failing to update it with the following error log

{"level":"error","ts":"2023-11-27T14:17:03Z","logger":"kuadrant-operator.authpolicy","msg":"ReconcileResource failed to create/update AuthConfig resource","AuthPolicy":{"name":"gw-auth","namespace":"istio-system"},"error":"Operation cannot be fulfilled on authconfigs.authorino.kuadrant.io \"ap-istio-system-gw-auth\": the object has been modified; please apply your changes to the latest version and try again","stacktrace":"github.com/kuadrant/kuadrant-operator/controllers.(*AuthPolicyReconciler).reconcileAuthConfigs\n\t/workspace/controllers/authpolicy_authconfig.go:39\ngithub.com/kuadrant/kuadrant-operator/controllers.(*AuthPolicyReconciler).reconcileResources\n\t/workspace/controllers/authpolicy_controller.go:153\ngithub.com/kuadrant/kuadrant-operator/controllers.(*AuthPolicyReconciler).Reconcile\n\t/workspace/controllers/authpolicy_controller.go:103\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:119\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:316\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227"}
{"level":"error","ts":"2023-11-27T14:17:03Z","logger":"kuadrant-operator","msg":"Reconciler error","controller":"authpolicy","controllerGroup":"kuadrant.io","controllerKind":"AuthPolicy","AuthPolicy":{"name":"gw-auth","namespace":"istio-system"},"namespace":"istio-system","name":"gw-auth","reconcileID":"9d8f5a06-b9ae-4806-8cdd-41b553a8e8a3","error":"Operation cannot be fulfilled on authconfigs.authorino.kuadrant.io \"ap-istio-system-gw-auth\": the object has been modified; please apply your changes to the latest version and try again","stacktrace":"sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:329\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227"}

[codecov]

Codecov Report

Merging #341 (2143e51) into main (2740b79) will decrease coverage by 0.03%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #341      +/-   ##
==========================================
- Coverage   65.10%   65.07%   -0.03%     
==========================================
  Files          35       35              
  Lines        3797     3800       +3     
==========================================
+ Hits         2472     2473       +1     
- Misses       1130     1131       +1     
- Partials      195      196       +1     

Flag	Coverage Δ
integration	70.08% <0.00%> (-0.06%)	⬇️
unit	59.40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1beta1 (u)	∅ <ø> (∅)
pkg/common (u)	76.92% <ø> (ø)
pkg/istio (u)	30.24% <ø> (ø)
pkg/log (u)	31.81% <ø> (ø)
pkg/reconcilers (u)	33.33% <ø> (ø)
pkg/rlptools (u)	56.46% <ø> (ø)
controllers (i)	70.08% <0.00%> (-0.06%)	⬇️

Files	Coverage Δ
controllers/authpolicy_authconfig.go	67.11% <0.00%> (-0.55%)	⬇️

... and 3 files with indirect coverage changes

[guicassolato]

Would returning before even creating the map make sense perhaps?

if len(configs) == 0 {
	return nil
}

[eguzki]

Would work for this use case, it would had the same effect. As a general rule, I would say it is better to check before returning.