From 58d4d3c28ab12536d5d0b892fb35b12b43069992 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Thu, 6 Apr 2023 16:28:13 +0200 Subject: [PATCH] fix: add logs in webhook middlewares (#6797) --- pkg/config/config.go | 22 ++++--------------- pkg/engine/api/resolver_test.go | 2 +- pkg/tracing/attributes.go | 6 +++++ pkg/tracing/helpers.go | 5 +++++ pkg/webhooks/handlers/admission.go | 5 ----- pkg/webhooks/handlers/dump.go | 4 ++-- pkg/webhooks/handlers/enrich.go | 2 ++ pkg/webhooks/handlers/filter.go | 35 ++++++++++++++++++++---------- pkg/webhooks/handlers/protect.go | 4 ++-- pkg/webhooks/handlers/trace.go | 5 +++++ 10 files changed, 50 insertions(+), 40 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index be7afae758..5f0ebeef5d 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -197,27 +197,13 @@ func (cd *configuration) ToFilter(gvk schema.GroupVersionKind, subresource, name defer cd.mux.RUnlock() if !cd.skipResourceFilters { for _, f := range cd.filters { - if wildcard.Match(f.Group, gvk.Group) && - wildcard.Match(f.Version, gvk.Version) && - wildcard.Match(f.Kind, gvk.Kind) && - wildcard.Match(f.Subresource, subresource) && - wildcard.Match(f.Namespace, namespace) && - wildcard.Match(f.Name, name) { - return true - } - // [Namespace,kube-system,*] || [*,kube-system,*] - if gvk.Group == "" && gvk.Version == "v1" && gvk.Kind == "Namespace" { - if wildcard.Match(f.Group, gvk.Group) && - wildcard.Match(f.Version, gvk.Version) && - wildcard.Match(f.Kind, gvk.Kind) && - wildcard.Match(f.Namespace, name) { + if wildcard.Match(f.Group, gvk.Group) && wildcard.Match(f.Version, gvk.Version) && wildcard.Match(f.Kind, gvk.Kind) && wildcard.Match(f.Subresource, subresource) { + if wildcard.Match(f.Namespace, namespace) && wildcard.Match(f.Name, name) { return true } + // [Namespace,kube-system,*] || [*,kube-system,*] if gvk.Group == "" && gvk.Version == "v1" && gvk.Kind == "Namespace" { - if wildcard.Match(f.Group, gvk.Group) && - wildcard.Match(f.Version, gvk.Version) && - wildcard.Match(f.Kind, gvk.Kind) && - wildcard.Match(f.Namespace, name) { + if wildcard.Match(f.Namespace, name) { return true } } diff --git a/pkg/engine/api/resolver_test.go b/pkg/engine/api/resolver_test.go index a9e9fab3f5..1f8330f706 100644 --- a/pkg/engine/api/resolver_test.go +++ b/pkg/engine/api/resolver_test.go @@ -123,7 +123,7 @@ func Test_namespacedResourceResolverChain_Get(t *testing.T) { t.Run(tt.name, func(t *testing.T) { resolver, _ := NewNamespacedResourceResolver(tt.fields.resolvers...) got, err := resolver.Get(context.TODO(), tt.args.namespace, tt.args.name) - if !reflect.DeepEqual(err, tt.wantErr) { + if !reflect.DeepEqual(err, tt.wantErr) { //nolint:deepequalerrors t.Errorf("ConfigmapResolver.Get() error = %v, wantErr %v", err, tt.wantErr) return } diff --git a/pkg/tracing/attributes.go b/pkg/tracing/attributes.go index f86fe46e6e..2610235cfd 100644 --- a/pkg/tracing/attributes.go +++ b/pkg/tracing/attributes.go @@ -43,6 +43,12 @@ const ( RequestUserNameKey = attribute.Key("admission.request.user.name") RequestUserUidKey = attribute.Key("admission.request.user.uid") RequestUserGroupsKey = attribute.Key("admission.request.user.groups") + RequestRolesKey = attribute.Key("admission.request.roles") + RequestClusterRolesKey = attribute.Key("admission.request.clusterroles") + RequestGroupKey = attribute.Key("admission.request.group") + RequestVersionKey = attribute.Key("admission.request.version") + RequestKindKey = attribute.Key("admission.request.kind") + RequestFilteredKey = attribute.Key("admission.request.filtered") // admission response attributes ResponseUidKey = attribute.Key("admission.response.uid") ResponseAllowedKey = attribute.Key("admission.response.allowed") diff --git a/pkg/tracing/helpers.go b/pkg/tracing/helpers.go index 6f86e65049..7087f29d34 100644 --- a/pkg/tracing/helpers.go +++ b/pkg/tracing/helpers.go @@ -4,6 +4,7 @@ import ( "context" "net/http" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" semconv "go.opentelemetry.io/otel/semconv/v1.17.0" "go.opentelemetry.io/otel/trace" @@ -43,3 +44,7 @@ func IsInSpan(ctx context.Context) bool { func CurrentSpan(ctx context.Context) trace.Span { return trace.SpanFromContext(ctx) } + +func SetAttributes(ctx context.Context, kv ...attribute.KeyValue) { + CurrentSpan(ctx).SetAttributes(kv...) +} diff --git a/pkg/webhooks/handlers/admission.go b/pkg/webhooks/handlers/admission.go index 106bd7f4bb..397fef2bc6 100644 --- a/pkg/webhooks/handlers/admission.go +++ b/pkg/webhooks/handlers/admission.go @@ -62,10 +62,5 @@ func (inner AdmissionHandler) withAdmission(logger logr.Logger) HttpHandler { HttpError(request.Context(), writer, request, logger, err, http.StatusInternalServerError) return } - if admissionReview.Request.Kind.Kind == "Lease" { - logger.V(6).Info("admission review request processed", "time", time.Since(startTime).String()) - } else { - logger.V(4).Info("admission review request processed", "time", time.Since(startTime).String()) - } } } diff --git a/pkg/webhooks/handlers/dump.go b/pkg/webhooks/handlers/dump.go index bcadb90bfb..461b360e72 100644 --- a/pkg/webhooks/handlers/dump.go +++ b/pkg/webhooks/handlers/dump.go @@ -40,8 +40,8 @@ func dumpPayload( if err != nil { logger.Error(err, "Failed to extract resources") } else { - logger = logger.WithValues("AdmissionResponse", response, "AdmissionRequest", reqPayload) - logger.Info("Logging admission request and response payload ") + logger = logger.WithValues("admission.response", response, "admission.request", reqPayload) + logger.Info("admission request dump") } } diff --git a/pkg/webhooks/handlers/enrich.go b/pkg/webhooks/handlers/enrich.go index 59cc08dd7b..4b69968927 100644 --- a/pkg/webhooks/handlers/enrich.go +++ b/pkg/webhooks/handlers/enrich.go @@ -32,6 +32,7 @@ func (inner AdmissionHandler) withRoles( return func(ctx context.Context, logger logr.Logger, request AdmissionRequest, startTime time.Time) AdmissionResponse { roles, clusterRoles, err := userinfo.GetRoleRef(rbLister, crbLister, request.UserInfo) if err != nil { + logger.Error(err, "failed to get roles/cluster roles from user infos") return admissionutils.Response(request.UID, err) } request.Roles = roles @@ -50,6 +51,7 @@ func (inner AdmissionHandler) withTopLevelGVK( return func(ctx context.Context, logger logr.Logger, request AdmissionRequest, startTime time.Time) AdmissionResponse { gvk, err := client.GetGVKFromGVR(schema.GroupVersionResource(request.Resource)) if err != nil { + logger.Error(err, "failed to get top level GVK from GVR") return admissionutils.Response(request.UID, err) } request.GroupVersionKind = gvk diff --git a/pkg/webhooks/handlers/filter.go b/pkg/webhooks/handlers/filter.go index a71aaa0164..6c2a5945a5 100644 --- a/pkg/webhooks/handlers/filter.go +++ b/pkg/webhooks/handlers/filter.go @@ -6,6 +6,7 @@ import ( "github.com/go-logr/logr" "github.com/kyverno/kyverno/pkg/config" + "github.com/kyverno/kyverno/pkg/tracing" admissionutils "github.com/kyverno/kyverno/pkg/utils/admission" wildcard "github.com/kyverno/kyverno/pkg/utils/wildcard" webhookutils "github.com/kyverno/kyverno/pkg/webhooks/utils" @@ -25,45 +26,55 @@ func (inner AdmissionHandler) WithSubResourceFilter(subresources ...string) Admi return inner.withSubResourceFilter(subresources...).WithTrace("SUBRESOURCE") } +func filtered(ctx context.Context, logger logr.Logger, request AdmissionRequest, message string, keysAndValues ...interface{}) AdmissionResponse { + logger.V(2).Info(message, keysAndValues...) + tracing.SetAttributes(ctx, tracing.RequestFilteredKey.Bool(true)) + return admissionutils.ResponseSuccess(request.UID) +} + func (inner AdmissionHandler) withFilter(c config.Configuration) AdmissionHandler { return func(ctx context.Context, logger logr.Logger, request AdmissionRequest, startTime time.Time) AdmissionResponse { // filter by username - for _, username := range c.GetExcludedUsernames() { + excludeUsernames := c.GetExcludedUsernames() + for _, username := range excludeUsernames { if wildcard.Match(username, request.UserInfo.Username) { - return admissionutils.ResponseSuccess(request.UID) + return filtered(ctx, logger, request, "admission request filtered because user is excluded", "config.exlude.usernames", excludeUsernames) } } // filter by groups - for _, group := range c.GetExcludedGroups() { + excludeGroups := c.GetExcludedGroups() + for _, group := range excludeGroups { for _, candidate := range request.UserInfo.Groups { if wildcard.Match(group, candidate) { - return admissionutils.ResponseSuccess(request.UID) + return filtered(ctx, logger, request, "admission request filtered because group is excluded", "config.exlude.groups", excludeGroups) } } } // filter by roles - for _, role := range c.GetExcludedRoles() { + excludeRoles := c.GetExcludedRoles() + for _, role := range excludeRoles { for _, candidate := range request.Roles { if wildcard.Match(role, candidate) { - return admissionutils.ResponseSuccess(request.UID) + return filtered(ctx, logger, request, "admission request filtered because role is excluded", "config.exlude.roles", excludeRoles) } } } // filter by cluster roles - for _, clusterRole := range c.GetExcludedClusterRoles() { + excludeClusterRoles := c.GetExcludedClusterRoles() + for _, clusterRole := range excludeClusterRoles { for _, candidate := range request.ClusterRoles { if wildcard.Match(clusterRole, candidate) { - return admissionutils.ResponseSuccess(request.UID) + return filtered(ctx, logger, request, "admission request filtered because role is excluded", "config.exlude.cluster-roles", excludeClusterRoles) } } } // filter by resource filters if c.ToFilter(request.GroupVersionKind, request.SubResource, request.Namespace, request.Name) { - return admissionutils.ResponseSuccess(request.UID) + return filtered(ctx, logger, request, "admission request filtered because it apears in configmap resource filters") } // filter kyverno resources if webhookutils.ExcludeKyvernoResources(request.Kind.Kind) { - return admissionutils.ResponseSuccess(request.UID) + return filtered(ctx, logger, request, "admission request filtered because it is for a kyverno resource") } return inner(ctx, logger, request, startTime) } @@ -78,7 +89,7 @@ func (inner AdmissionHandler) withOperationFilter(operations ...admissionv1.Oper if allowed.Has(string(request.Operation)) { return inner(ctx, logger, request, startTime) } - return admissionutils.ResponseSuccess(request.UID) + return filtered(ctx, logger, request, "admission request filtered because operation is excluded") } } @@ -88,6 +99,6 @@ func (inner AdmissionHandler) withSubResourceFilter(subresources ...string) Admi if request.SubResource == "" || allowed.Has(request.SubResource) { return inner(ctx, logger, request, startTime) } - return admissionutils.ResponseSuccess(request.UID) + return filtered(ctx, logger, request, "admission request filtered because subresource is excluded") } } diff --git a/pkg/webhooks/handlers/protect.go b/pkg/webhooks/handlers/protect.go index de8fb7b28a..174fffa5de 100644 --- a/pkg/webhooks/handlers/protect.go +++ b/pkg/webhooks/handlers/protect.go @@ -33,14 +33,14 @@ func (inner AdmissionHandler) withProtection() AdmissionHandler { } newResource, oldResource, err := admissionutils.ExtractResources(nil, request.AdmissionRequest) if err != nil { - logger.Error(err, "Failed to extract resources") + logger.Error(err, "failed to extract resources") return admissionutils.Response(request.UID, err) } for _, resource := range []unstructured.Unstructured{newResource, oldResource} { resLabels := resource.GetLabels() if resLabels[kyvernov1.LabelAppManagedBy] == kyvernov1.ValueKyvernoApp { if request.UserInfo.Username != kyvernoUsername { - logger.Info("Access to the resource not authorized, this is a kyverno managed resource and should be altered only by kyverno") + logger.V(2).Info("access to the resource not authorized, this is a kyverno managed resource and should be altered only by kyverno") return admissionutils.Response(request.UID, errors.New("A kyverno managed resource can only be modified by kyverno")) } } diff --git a/pkg/webhooks/handlers/trace.go b/pkg/webhooks/handlers/trace.go index a9b8c53b5a..e59023cded 100644 --- a/pkg/webhooks/handlers/trace.go +++ b/pkg/webhooks/handlers/trace.go @@ -83,6 +83,11 @@ func (inner AdmissionHandler) WithTrace(name string) AdmissionHandler { tracing.RequestRequestResourceResourceKey.String(tracing.StringValue(request.RequestResource.Resource)), tracing.RequestUserNameKey.String(tracing.StringValue(request.UserInfo.Username)), tracing.RequestUserUidKey.String(tracing.StringValue(request.UserInfo.UID)), + tracing.RequestRolesKey.StringSlice(request.Roles), + tracing.RequestClusterRolesKey.StringSlice(request.ClusterRoles), + tracing.RequestGroupKey.String(request.GroupVersionKind.Group), + tracing.RequestVersionKey.String(request.GroupVersionKind.Version), + tracing.RequestKindKey.String(request.GroupVersionKind.Kind), tracing.RequestUserGroupsKey.StringSlice(request.UserInfo.Groups), ), )