Skip to content

Commit

Permalink
Merge branch 'main' into kingoliver/dynamic4
Browse files Browse the repository at this point in the history
  • Loading branch information
@OliverMKing
OliverMKing authored Nov 3, 2023
2 parents 0988d6d + 39ccfc0 commit ef15092
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 12 deletions.
17 changes: 17 additions & 0 deletions pkg/controller/metrics/metrics.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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)
Expand Down
47 changes: 37 additions & 10 deletions pkg/webhook/nginxingress.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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(),
Expand Down Expand Up @@ -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()),
Expand Down Expand Up @@ -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)
Expand All @@ -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))
}

Expand All @@ -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))
}
Expand All @@ -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")
Expand All @@ -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))
}

Expand Down
48 changes: 46 additions & 2 deletions pkg/webhook/nginxingress_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand All @@ -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) {
Expand All @@ -278,6 +288,11 @@ func TestNginxIngressResourceMutator(t *testing.T) {
},
},
},
expected: admission.Response{
AdmissionResponse: admissionv1.AdmissionResponse{
Allowed: true,
},
},
},
{
name: "mutation",
Expand All @@ -297,6 +312,9 @@ func TestNginxIngressResourceMutator(t *testing.T) {
},
},
expected: admission.Response{
AdmissionResponse: admissionv1.AdmissionResponse{
Allowed: true,
},
Patches: []jsonpatch.JsonPatchOperation{
{
Operation: "add",
Expand All @@ -306,17 +324,41 @@ 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{
decoder: admission.NewDecoder(scheme),
}
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) {
Expand All @@ -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)
}

0 comments on commit ef15092

Please sign in to comment.