Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: loggers in webhook handlers #2786

Merged
merged 4 commits into from
May 30, 2023
Merged

refactor: loggers in webhook handlers #2786

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8598bf
refactor: loggers in webhook handlers
acpana Apr 19, 2023
7a25d94
rename logger to log
acpana May 24, 2023
59cdb99
revert pkg/audit/manager.go
acpana May 30, 2023
8a43cea
Merge branch 'master' into acpana/log-webhooks
acpana May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions pkg/webhook/mutation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"net/http"
"time"

"github.com/go-logr/logr"
"github.com/open-policy-agent/cert-controller/pkg/rotator"
"github.com/open-policy-agent/gatekeeper/v3/apis"
"github.com/open-policy-agent/gatekeeper/v3/pkg/controller/config/process"
Expand Down Expand Up @@ -63,6 +64,7 @@ func AddMutatingWebhook(mgr manager.Manager, deps Dependencies) error {
eventBroadcaster := record.NewBroadcaster()
kubeClient := kubernetes.NewForConfigOrDie(mgr.GetConfig())

log := log.WithValues("hookType", "mutation")
eventBroadcaster.StartRecordingToSink(&clientcorev1.EventSinkImpl{Interface: kubeClient.CoreV1().Events("")})
recorder := eventBroadcaster.NewRecorder(
scheme.Scheme,
Expand All @@ -80,6 +82,7 @@ func AddMutatingWebhook(mgr manager.Manager, deps Dependencies) error {
},
mutationSystem: deps.MutationSystem,
deserializer: codecs.UniversalDeserializer(),
log: log,
},
}

Expand All @@ -99,12 +102,12 @@ type mutationHandler struct {
webhookHandler
mutationSystem *mutation.System
deserializer runtime.Decoder
log logr.Logger
}

// Handle the mutation request
// nolint: gocritic // Must accept admission.Request to satisfy interface.
func (h *mutationHandler) Handle(ctx context.Context, req admission.Request) admission.Response {
log := log.WithValues("hookType", "mutation")
timeStart := time.Now()

if isGkServiceAccount(req.AdmissionRequest.UserInfo) {
Expand All @@ -124,15 +127,15 @@ func (h *mutationHandler) Handle(ctx context.Context, req admission.Request) adm
defer func() {
if h.reporter != nil {
if err := h.reporter.ReportMutationRequest(ctx, requestResponse, time.Since(timeStart)); err != nil {
log.Error(err, "failed to report request")
h.log.Error(err, "failed to report request")
}
}
}()

// namespace is excluded from webhook using config
isExcludedNamespace, err := h.skipExcludedNamespace(&req.AdmissionRequest, process.Mutation)
if err != nil {
log.Error(err, "error while excluding namespace")
h.log.Error(err, "error while excluding namespace")
}

if isExcludedNamespace {
Expand Down Expand Up @@ -164,13 +167,13 @@ func (h *mutationHandler) mutateRequest(ctx context.Context, req *admission.Requ
case req.AdmissionRequest.Namespace != "":
if err := h.client.Get(ctx, types.NamespacedName{Name: req.AdmissionRequest.Namespace}, ns); err != nil {
if !k8serrors.IsNotFound(err) {
log.Error(err, "error retrieving namespace", "name", req.AdmissionRequest.Namespace)
h.log.Error(err, "error retrieving namespace", "name", req.AdmissionRequest.Namespace)
return admission.Errored(int32(http.StatusInternalServerError), err)
}
// bypass cached client and ask api-server directly
err = h.reader.Get(ctx, types.NamespacedName{Name: req.AdmissionRequest.Namespace}, ns)
if err != nil {
log.Error(err, "error retrieving namespace from API server", "name", req.AdmissionRequest.Namespace)
h.log.Error(err, "error retrieving namespace from API server", "name", req.AdmissionRequest.Namespace)
return admission.Errored(int32(http.StatusInternalServerError), err)
}
}
Expand All @@ -180,7 +183,7 @@ func (h *mutationHandler) mutateRequest(ctx context.Context, req *admission.Requ
obj := unstructured.Unstructured{}
err := obj.UnmarshalJSON(req.Object.Raw)
if err != nil {
log.Error(err, "failed to unmarshal", "object", string(req.Object.Raw))
h.log.Error(err, "failed to unmarshal", "object", string(req.Object.Raw))
return admission.Errored(int32(http.StatusInternalServerError), err)
}

Expand All @@ -198,7 +201,7 @@ func (h *mutationHandler) mutateRequest(ctx context.Context, req *admission.Requ
}
mutated, err := h.mutationSystem.Mutate(mutable)
if err != nil {
log.Error(err, "failed to mutate object", "object", string(req.Object.Raw))
h.log.Error(err, "failed to mutate object", "object", string(req.Object.Raw))
return admission.Errored(int32(http.StatusInternalServerError), err)
}
if !mutated {
Expand All @@ -209,7 +212,7 @@ func (h *mutationHandler) mutateRequest(ctx context.Context, req *admission.Requ

newJSON, err := mutable.Object.MarshalJSON()
if err != nil {
log.Error(err, "failed to marshal mutated object", "object", obj)
h.log.Error(err, "failed to marshal mutated object", "object", obj)
return admission.Errored(int32(http.StatusInternalServerError), err)
}
resp := admission.PatchResponseFromRaw(req.Object.Raw, newJSON)
Expand Down
2 changes: 2 additions & 0 deletions pkg/webhook/mutation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ func TestWebhookAssign(t *testing.T) {
},
mutationSystem: sys,
deserializer: codecs.UniversalDeserializer(),
log: log,
}

raw := []byte(`{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "acbd","namespace": "ns1"}}`)
Expand Down Expand Up @@ -130,6 +131,7 @@ func TestWebhookAssignMetadata(t *testing.T) {
},
mutationSystem: sys,
deserializer: codecs.UniversalDeserializer(),
log: log,
}

raw := []byte(`{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "acbd","namespace": "ns1"}}`)
Expand Down
4 changes: 2 additions & 2 deletions pkg/webhook/mutator_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ spec:
}
for _, tt := range tc {
t.Run(tt.Name, func(t *testing.T) {
handler := validationHandler{webhookHandler: webhookHandler{}}
handler := validationHandler{webhookHandler: webhookHandler{}, log: log}
b, err := yaml.YAMLToJSON([]byte(tt.AssignMeta))
if err != nil {
t.Fatalf("Error parsing yaml: %s", err)
Expand Down Expand Up @@ -348,7 +348,7 @@ spec:
}
for _, tt := range tc {
t.Run(tt.Name, func(t *testing.T) {
handler := validationHandler{webhookHandler: webhookHandler{}}
handler := validationHandler{webhookHandler: webhookHandler{}, log: log}
b, err := yaml.YAMLToJSON([]byte(tt.Assign))
if err != nil {
t.Fatalf("Error parsing yaml: %s", err)
Expand Down
25 changes: 14 additions & 11 deletions pkg/webhook/policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"strings"
"time"

"github.com/go-logr/logr"
"github.com/open-policy-agent/cert-controller/pkg/rotator"
externaldataUnversioned "github.com/open-policy-agent/frameworks/constraint/pkg/apis/externaldata/unversioned"
constraintclient "github.com/open-policy-agent/frameworks/constraint/pkg/client"
Expand Down Expand Up @@ -92,6 +93,7 @@ func AddPolicyWebhook(mgr manager.Manager, deps Dependencies) error {
if err != nil {
return err
}
log := log.WithValues("hookType", "validation")
eventBroadcaster := record.NewBroadcaster()
kubeClient := kubernetes.NewForConfigOrDie(mgr.GetConfig())
eventBroadcaster.StartRecordingToSink(&clientcorev1.EventSinkImpl{Interface: kubeClient.CoreV1().Events("")})
Expand All @@ -110,6 +112,7 @@ func AddPolicyWebhook(mgr manager.Manager, deps Dependencies) error {
eventRecorder: recorder,
gkNamespace: util.GetNamespace(),
},
log: log,
}
threadCount := *maxServingThreads
if threadCount < 1 {
Expand All @@ -134,13 +137,12 @@ type validationHandler struct {
mutationSystem *mutation.System
expansionSystem *expansion.System
semaphore chan struct{}
log logr.Logger
}

// Handle the validation request
// nolint: gocritic // Must accept admission.Request as a struct to satisfy Handler interface.
func (h *validationHandler) Handle(ctx context.Context, req admission.Request) admission.Response {
log := log.WithValues("hookType", "validation")

timeStart := time.Now()

if isGkServiceAccount(req.AdmissionRequest.UserInfo) {
Expand Down Expand Up @@ -171,15 +173,15 @@ func (h *validationHandler) Handle(ctx context.Context, req admission.Request) a
isDryRun = "true"
}
if err := h.reporter.ReportValidationRequest(ctx, requestResponse, isDryRun, time.Since(timeStart)); err != nil {
log.Error(err, "failed to report request")
h.log.Error(err, "failed to report request")
}
}
}()

// namespace is excluded from webhook using config
isExcludedNamespace, err := h.skipExcludedNamespace(&req.AdmissionRequest, process.Webhook)
if err != nil {
log.Error(err, "error while excluding namespace")
h.log.Error(err, "error while excluding namespace")
}

if isExcludedNamespace {
Expand All @@ -189,15 +191,15 @@ func (h *validationHandler) Handle(ctx context.Context, req admission.Request) a

resp, err := h.reviewRequest(ctx, &req)
if err != nil {
log.Error(err, "error executing query")
h.log.Error(err, "error executing query")
requestResponse = errorResponse
return admission.Errored(http.StatusInternalServerError, err)
}

if *logStatsAdmission {
logging.LogStatsEntries(
h.opa,
log.WithValues(
h.log.WithValues(
logging.Process, "admission",
logging.EventType, "review_response_stats",
logging.ResourceGroup, req.AdmissionRequest.Kind.Group,
Expand Down Expand Up @@ -266,7 +268,7 @@ func (h *validationHandler) getValidationMessages(res []*rtypes.Result, req *adm
continue
}
if *logDenies {
log.WithValues(
h.log.WithValues(
logging.Process, "admission",
logging.EventType, "violation",
logging.ConstraintName, r.Constraint.GetName(),
Expand All @@ -280,7 +282,8 @@ func (h *validationHandler) getValidationMessages(res []*rtypes.Result, req *adm
logging.ResourceNamespace, req.AdmissionRequest.Namespace,
logging.ResourceName, resourceName,
logging.RequestUsername, req.AdmissionRequest.UserInfo.Username,
).Info(fmt.Sprintf("denied admission: %s", r.Msg))
).Info(
fmt.Sprintf("denied admission: %s", r.Msg))
}
if *emitAdmissionEvents {
annotations := map[string]string{
Expand Down Expand Up @@ -603,14 +606,14 @@ func (h *validationHandler) reviewRequest(ctx context.Context, req *admission.Re
func (h *validationHandler) review(ctx context.Context, review interface{}, trace bool, dump bool) (*rtypes.Responses, error) {
resp, err := h.opa.Review(ctx, review, drivers.Tracing(trace), drivers.Stats(*logStatsAdmission))
if resp != nil && trace {
log.Info(resp.TraceDump())
h.log.Info(resp.TraceDump())
}
if dump {
dump, err := h.opa.Dump(ctx)
if err != nil {
log.Error(err, "dump error")
h.log.Error(err, "dump error")
} else {
log.Info(dump)
h.log.Info(dump)
}
}

Expand Down
1 change: 1 addition & 0 deletions pkg/webhook/policy_benchmark_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ func BenchmarkValidationHandler(b *testing.B) {
client: c,
injectedConfig: cfg,
},
log: log,
}

// create T templates
Expand Down
11 changes: 8 additions & 3 deletions pkg/webhook/policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ func TestTemplateValidation(t *testing.T) {
if err != nil {
t.Fatalf("Could not initialize OPA: %s", err)
}
handler := validationHandler{opa: opa, webhookHandler: webhookHandler{}}
handler := validationHandler{opa: opa, webhookHandler: webhookHandler{}, log: log}

b, err := json.Marshal(tt.Template)
if err != nil {
Expand Down Expand Up @@ -359,6 +359,7 @@ func TestReviewRequest(t *testing.T) {
client: tt.CachedClient,
reader: tt.APIReader,
},
log: log,
}
if maxThreads > 0 {
handler.semaphore = make(chan struct{}, maxThreads)
Expand Down Expand Up @@ -431,6 +432,7 @@ func TestReviewDefaultNS(t *testing.T) {
reader: &nsGetter{},
processExcluder: pe,
},
log: log,
}
if maxThreads > 0 {
handler.semaphore = make(chan struct{}, maxThreads)
Expand Down Expand Up @@ -522,6 +524,7 @@ func TestConstraintValidation(t *testing.T) {
opa: opa,
expansionSystem: expansion.NewSystem(mutation.NewSystem(mutation.SystemOpts{})),
webhookHandler: webhookHandler{},
log: log,
}
b, err := yaml.YAMLToJSON([]byte(tt.Constraint))
if err != nil {
Expand Down Expand Up @@ -649,6 +652,7 @@ func TestTracing(t *testing.T) {
opa: opa,
expansionSystem: expansion.NewSystem(mutation.NewSystem(mutation.SystemOpts{})),
webhookHandler: webhookHandler{injectedConfig: tt.Cfg},
log: log,
}
if maxThreads > 0 {
handler.semaphore = make(chan struct{}, maxThreads)
Expand Down Expand Up @@ -825,6 +829,7 @@ func TestGetValidationMessages(t *testing.T) {
opa: opa,
expansionSystem: expansion.NewSystem(mutation.NewSystem(mutation.SystemOpts{})),
webhookHandler: webhookHandler{},
log: log,
}
if maxThreads > 0 {
handler.semaphore = make(chan struct{}, maxThreads)
Expand Down Expand Up @@ -875,7 +880,7 @@ func TestValidateConfigResource(t *testing.T) {

for _, tt := range tc {
t.Run(tt.TestName, func(t *testing.T) {
handler := validationHandler{}
handler := validationHandler{log: log}
req := &admission.Request{
AdmissionRequest: admissionv1.AdmissionRequest{
Name: tt.Name,
Expand Down Expand Up @@ -922,7 +927,7 @@ func TestValidateProvider(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
h := &validationHandler{}
h := &validationHandler{log: log}
b, err := yaml.YAMLToJSON([]byte(tt.provider))
if err != nil {
t.Fatalf("Error parsing yaml: %s", err)
Expand Down