diff --git a/pkg/controller/metrics/metrics.go b/pkg/controller/metrics/metrics.go index 095ff4de..2cd0bd5f 100644 --- a/pkg/controller/metrics/metrics.go +++ b/pkg/controller/metrics/metrics.go @@ -6,6 +6,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/metrics" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) var ( @@ -50,6 +51,22 @@ func HandleControllerReconcileMetrics(controllerName controllername.ControllerNa } } +// HandleWebhookHandlerMetrics is meant to be called within a defer for each webhook handler func. +func HandleWebhookHandlerMetrics(controllerName controllername.ControllerNamer, result admission.Response, err error) { + cn := controllerName.MetricsName() + + switch { + case err != nil && !apierrors.IsNotFound(err): + AppRoutingReconcileTotal.WithLabelValues(cn, LabelError).Inc() + AppRoutingReconcileErrors.WithLabelValues(cn).Inc() + case result.Allowed == false: + AppRoutingReconcileTotal.WithLabelValues(cn, LabelError).Inc() + AppRoutingReconcileErrors.WithLabelValues(cn).Inc() + default: + AppRoutingReconcileTotal.WithLabelValues(cn, LabelSuccess).Inc() + } +} + func InitControllerMetrics(controllerName controllername.ControllerNamer) { cn := controllerName.MetricsName() AppRoutingReconcileTotal.WithLabelValues(cn, LabelError).Add(0) diff --git a/pkg/webhook/nginxingress.go b/pkg/webhook/nginxingress.go index 9ba46bfb..66ecd2eb 100644 --- a/pkg/webhook/nginxingress.go +++ b/pkg/webhook/nginxingress.go @@ -7,6 +7,8 @@ import ( "net/http" approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "github.com/Azure/aks-app-routing-operator/pkg/controller/controllername" + "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" "github.com/Azure/aks-app-routing-operator/pkg/manifests" "github.com/Azure/aks-app-routing-operator/pkg/util" "github.com/go-logr/logr" @@ -27,9 +29,15 @@ const ( mutationPath = "/mutate-nginx-ingress-controller" ) +var ( + nginxResourceValidationName = controllername.New("nginx", "ingress", "resource", "validator") + nginxResourceMutationName = controllername.New("nginx", "ingress", "resource", "mutator") +) + func init() { Validating = append(Validating, Webhook[admissionregistrationv1.ValidatingWebhook]{ AddToManager: func(mgr manager.Manager) error { + metrics.InitControllerMetrics(nginxResourceValidationName) mgr.GetWebhookServer().Register(validationPath, &webhook.Admission{ Handler: &nginxIngressResourceValidator{ client: mgr.GetClient(), @@ -68,6 +76,7 @@ func init() { Mutating = append(Mutating, Webhook[admissionregistrationv1.MutatingWebhook]{ AddToManager: func(mgr manager.Manager) error { + metrics.InitControllerMetrics(nginxResourceMutationName) mgr.GetWebhookServer().Register(mutationPath, &webhook.Admission{ Handler: &nginxIngressResourceMutator{ decoder: admission.NewDecoder(mgr.GetScheme()), @@ -157,11 +166,17 @@ var sarAuthenticate = func(ctx context.Context, lgr logr.Logger, cl client.Clien return "", nil } -func (n *nginxIngressResourceValidator) Handle(ctx context.Context, req admission.Request) admission.Response { - lgr := logr.FromContextOrDiscard(ctx).WithValues("resourceName", req.Name, "namespace", req.Namespace, "operation", req.Operation).WithName("nginxIngressResourceValidator") +func (n *nginxIngressResourceValidator) Handle(ctx context.Context, req admission.Request) (resp admission.Response) { + var err error + defer func() { + metrics.HandleWebhookHandlerMetrics(nginxResourceValidationName, resp, err) + }() + + lgr := logr.FromContextOrDiscard(ctx).WithValues("resourceName", req.Name, "namespace", req.Namespace, "operation", req.Operation).WithName(nginxResourceValidationName.LoggerName()) // ensure user has permissions required - cantPerform, err := n.authenticate(ctx, lgr, n.client, req) + var cantPerform string + cantPerform, err = n.authenticate(ctx, lgr, n.client, req) if err != nil { lgr.Error(err, "checking permissions") return admission.Errored(http.StatusInternalServerError, err) @@ -177,7 +192,8 @@ func (n *nginxIngressResourceValidator) Handle(ctx context.Context, req admissio lgr.Info("decoding NginxIngressController resource") var nginxIngressController approutingv1alpha1.NginxIngressController - if err := n.decoder.Decode(req, &nginxIngressController); err != nil { + if err = n.decoder.Decode(req, &nginxIngressController); err != nil { + lgr.Error(err, "decoding nginx ingress controller") return admission.Errored(http.StatusBadRequest, fmt.Errorf("decoding NginxIngressController: %w", err)) } @@ -196,18 +212,21 @@ func (n *nginxIngressResourceValidator) Handle(ctx context.Context, req admissio }, } lgr.Info("attempting to get IngressClass " + ic.Name) - err := n.client.Get(ctx, client.ObjectKeyFromObject(ic), ic) + err = n.client.Get(ctx, client.ObjectKeyFromObject(ic), ic) if err == nil { + lgr.Info("denied because IngressClass already exists") return admission.Denied(fmt.Sprintf("IngressClass %s already exists. Delete or use a different spec.IngressClassName field", ic.Name)) } + // err can still be populated as not found after this so be careful to overwrite for accurate metrics if needed if !k8serrors.IsNotFound(err) { + lgr.Error(err, "get IngressClass") return admission.Errored(http.StatusInternalServerError, fmt.Errorf("getting IngressClass %s: %w", ic.Name, err)) } // list nginx ingress controllers lgr.Info("listing NginxIngressControllers to check for collisions") var nginxIngressControllerList approutingv1alpha1.NginxIngressControllerList - if err := n.client.List(ctx, &nginxIngressControllerList); err != nil { + if err = n.client.List(ctx, &nginxIngressControllerList); err != nil { lgr.Error(err, "listing NginxIngressControllers") return admission.Errored(http.StatusInternalServerError, fmt.Errorf("listing NginxIngressControllers: %w", err)) } @@ -228,8 +247,13 @@ type nginxIngressResourceMutator struct { decoder *admission.Decoder } -func (n nginxIngressResourceMutator) Handle(ctx context.Context, request admission.Request) admission.Response { - lgr := logr.FromContextOrDiscard(ctx).WithValues("resourceName", request.Name, "namespace", request.Namespace, "operation", request.Operation).WithName("nginxIngressResourceMutator") +func (n nginxIngressResourceMutator) Handle(ctx context.Context, request admission.Request) (resp admission.Response) { + var err error + defer func() { + metrics.HandleWebhookHandlerMetrics(nginxResourceMutationName, resp, err) + }() + + lgr := logr.FromContextOrDiscard(ctx).WithValues("resourceName", request.Name, "namespace", request.Namespace, "operation", request.Operation).WithName(nginxResourceMutationName.LoggerName()) if request.Operation == admissionv1.Delete { lgr.Info("delete operation, skipping mutation") @@ -238,15 +262,18 @@ func (n nginxIngressResourceMutator) Handle(ctx context.Context, request admissi lgr.Info("decoding NginxIngressController resource") var nginxIngressController approutingv1alpha1.NginxIngressController - if err := n.decoder.Decode(request, &nginxIngressController); err != nil { + if err = n.decoder.Decode(request, &nginxIngressController); err != nil { + lgr.Error(err, "decoding nginx ingress controller") return admission.Errored(http.StatusBadRequest, fmt.Errorf("decoding NginxIngressController: %w", err)) } lgr.Info("defaulting NginxIngressController resource") nginxIngressController.Default() - marshalled, err := json.Marshal(nginxIngressController) + var marshalled []byte + marshalled, err = json.Marshal(nginxIngressController) if err != nil { + lgr.Error(err, "encoding nginx ingress controller") return admission.Errored(http.StatusInternalServerError, fmt.Errorf("marshalling NginxIngressController: %w", err)) } diff --git a/pkg/webhook/nginxingress_test.go b/pkg/webhook/nginxingress_test.go index 5136d715..35ac360d 100644 --- a/pkg/webhook/nginxingress_test.go +++ b/pkg/webhook/nginxingress_test.go @@ -6,10 +6,13 @@ import ( "encoding/json" "errors" "fmt" + "net/http" "reflect" "testing" approutingv1alpha1 "github.com/Azure/aks-app-routing-operator/api/v1alpha1" + "github.com/Azure/aks-app-routing-operator/pkg/controller/metrics" + "github.com/Azure/aks-app-routing-operator/pkg/controller/testutils" "github.com/go-logr/logr" "github.com/stretchr/testify/require" "gomodules.xyz/jsonpatch/v2" @@ -237,6 +240,10 @@ func TestNginxIngressResourceValidator(t *testing.T) { }, } + metrics.InitControllerMetrics(nginxResourceValidationName) + beforeErrCount := testutils.GetErrMetricCount(t, nginxResourceValidationName) + beforeSuccessCount := testutils.GetReconcileMetricCount(t, nginxResourceValidationName, metrics.LabelSuccess) + for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { validator := nginxIngressResourceValidator{ @@ -255,6 +262,9 @@ func TestNginxIngressResourceValidator(t *testing.T) { } }) } + + require.Greater(t, testutils.GetErrMetricCount(t, nginxResourceValidationName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, nginxResourceValidationName, metrics.LabelSuccess), beforeSuccessCount) } func TestNginxIngressResourceMutator(t *testing.T) { @@ -278,6 +288,11 @@ func TestNginxIngressResourceMutator(t *testing.T) { }, }, }, + expected: admission.Response{ + AdmissionResponse: admissionv1.AdmissionResponse{ + Allowed: true, + }, + }, }, { name: "mutation", @@ -297,6 +312,9 @@ func TestNginxIngressResourceMutator(t *testing.T) { }, }, expected: admission.Response{ + AdmissionResponse: admissionv1.AdmissionResponse{ + Allowed: true, + }, Patches: []jsonpatch.JsonPatchOperation{ { Operation: "add", @@ -306,8 +324,32 @@ func TestNginxIngressResourceMutator(t *testing.T) { }, }, }, + { + name: "mutation fails to decode bad input", + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: []byte{0, 0, 1, 2, 3}, + }, + }, + }, + expected: admission.Response{ + AdmissionResponse: admissionv1.AdmissionResponse{ + Allowed: false, + Result: &metav1.Status{ + Code: http.StatusBadRequest, + Message: fmt.Errorf("decoding NginxIngressController: %w", errors.New("failed decode")).Error(), + }, + }, + }, + }, } + metrics.InitControllerMetrics(nginxResourceMutationName) + beforeErrCount := testutils.GetErrMetricCount(t, nginxResourceMutationName) + beforeSuccessCount := testutils.GetReconcileMetricCount(t, nginxResourceMutationName, metrics.LabelSuccess) + for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { mutator := nginxIngressResourceMutator{ @@ -315,8 +357,8 @@ func TestNginxIngressResourceMutator(t *testing.T) { } actual := mutator.Handle(context.Background(), tc.req) - if actual.Allowed != true { - t.Errorf("expected allowed %v, got %v", true, actual.Allowed) + if actual.Allowed != tc.expected.Allowed { + t.Errorf("expected allowed %v, got %v", tc.expected.Allowed, actual.Allowed) } if len(actual.Patches) != len(tc.expected.Patches) { @@ -331,4 +373,6 @@ func TestNginxIngressResourceMutator(t *testing.T) { }) } + require.Greater(t, testutils.GetErrMetricCount(t, nginxResourceMutationName), beforeErrCount) + require.Greater(t, testutils.GetReconcileMetricCount(t, nginxResourceMutationName, metrics.LabelSuccess), beforeSuccessCount) }