From 011c5a9102448f2ff816ec9ba9ab1e9836de013f Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Wed, 6 Apr 2022 19:05:09 +0200 Subject: [PATCH 1/5] Do not block pod creating on internal error Signed-off-by: Pavol Loffay --- internal/webhookhandler/webhookhandler.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/internal/webhookhandler/webhookhandler.go b/internal/webhookhandler/webhookhandler.go index 457ad37cf9..8f26dd6d3b 100644 --- a/internal/webhookhandler/webhookhandler.go +++ b/internal/webhookhandler/webhookhandler.go @@ -29,7 +29,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/config" ) -// +kubebuilder:webhook:path=/mutate-v1-pod,mutating=true,failurePolicy=ignore,groups="",resources=pods,verbs=create;update,versions=v1,name=mpod.kb.io,sideEffects=none,admissionReviewVersions=v1 +// +kubebuilder:webhook:path=/mutate-v1-pod,mutating=true,failurePolicy=fail,groups="",resources=pods,verbs=create;update,versions=v1,name=mpod.kb.io,sideEffects=none,admissionReviewVersions=v1 // +kubebuilder:rbac:groups="",resources=namespaces,verbs=list;watch // +kubebuilder:rbac:groups=opentelemetry.io,resources=opentelemetrycollectors,verbs=get;list;watch // +kubebuilder:rbac:groups=opentelemetry.io,resources=instrumentations,verbs=get;list;watch @@ -78,19 +78,22 @@ func (p *podSidecarInjector) Handle(ctx context.Context, req admission.Request) ns := corev1.Namespace{} err = p.client.Get(ctx, types.NamespacedName{Name: req.Namespace, Namespace: ""}, &ns) if err != nil { - return admission.Errored(http.StatusInternalServerError, err) + res := admission.Errored(http.StatusInternalServerError, err) + res.Allowed = true } for _, m := range p.podMutators { pod, err = m.Mutate(ctx, ns, pod) if err != nil { - return admission.Errored(http.StatusInternalServerError, err) + res := admission.Errored(http.StatusInternalServerError, err) + res.Allowed = true } } marshaledPod, err := json.Marshal(pod) if err != nil { - return admission.Errored(http.StatusInternalServerError, err) + res := admission.Errored(http.StatusInternalServerError, err) + res.Allowed = true } return admission.PatchResponseFromRaw(req.Object.Raw, marshaledPod) } From a729b1225f75e111e13403daba648d7447bf4009 Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Wed, 6 Apr 2022 22:42:30 +0200 Subject: [PATCH 2/5] Fix Signed-off-by: Pavol Loffay --- internal/webhookhandler/webhookhandler.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/webhookhandler/webhookhandler.go b/internal/webhookhandler/webhookhandler.go index 8f26dd6d3b..5dd3527b83 100644 --- a/internal/webhookhandler/webhookhandler.go +++ b/internal/webhookhandler/webhookhandler.go @@ -29,7 +29,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/config" ) -// +kubebuilder:webhook:path=/mutate-v1-pod,mutating=true,failurePolicy=fail,groups="",resources=pods,verbs=create;update,versions=v1,name=mpod.kb.io,sideEffects=none,admissionReviewVersions=v1 +// +kubebuilder:webhook:path=/mutate-v1-pod,mutating=true,failurePolicy=ignore,groups="",resources=pods,verbs=create;update,versions=v1,name=mpod.kb.io,sideEffects=none,admissionReviewVersions=v1 // +kubebuilder:rbac:groups="",resources=namespaces,verbs=list;watch // +kubebuilder:rbac:groups=opentelemetry.io,resources=opentelemetrycollectors,verbs=get;list;watch // +kubebuilder:rbac:groups=opentelemetry.io,resources=instrumentations,verbs=get;list;watch @@ -79,7 +79,10 @@ func (p *podSidecarInjector) Handle(ctx context.Context, req admission.Request) err = p.client.Get(ctx, types.NamespacedName{Name: req.Namespace, Namespace: ""}, &ns) if err != nil { res := admission.Errored(http.StatusInternalServerError, err) + // By default, admission.Errored sets Allowed to false which blocks pod creation even though the failurePolicy=ignore. + // Allowed set to true makes sure failure does not block pod creation in case of an error. res.Allowed = true + return res } for _, m := range p.podMutators { @@ -87,6 +90,7 @@ func (p *podSidecarInjector) Handle(ctx context.Context, req admission.Request) if err != nil { res := admission.Errored(http.StatusInternalServerError, err) res.Allowed = true + return res } } @@ -94,6 +98,7 @@ func (p *podSidecarInjector) Handle(ctx context.Context, req admission.Request) if err != nil { res := admission.Errored(http.StatusInternalServerError, err) res.Allowed = true + return res } return admission.PatchResponseFromRaw(req.Object.Raw, marshaledPod) } From 84082e2fe252176eafbd5e4486f959c373055138 Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Thu, 7 Apr 2022 11:34:48 +0200 Subject: [PATCH 3/5] Fix Signed-off-by: Pavol Loffay --- internal/webhookhandler/webhookhandler_test.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/internal/webhookhandler/webhookhandler_test.go b/internal/webhookhandler/webhookhandler_test.go index c439d40d4f..97b37b7cb5 100644 --- a/internal/webhookhandler/webhookhandler_test.go +++ b/internal/webhookhandler/webhookhandler_test.go @@ -403,15 +403,17 @@ func TestFailOnInvalidRequest(t *testing.T) { name string req admission.Request expected int32 + allowed bool }{ { - "empty payload", - admission.Request{}, - http.StatusBadRequest, + name: "empty payload", + req: admission.Request{}, + expected: http.StatusBadRequest, + allowed: false, }, { - "namespace doesn't exist", - func() admission.Request { + name: "namespace doesn't exist", + req: func() admission.Request { pod := corev1.Pod{} encoded, err := json.Marshal(pod) require.NoError(t, err) @@ -425,7 +427,8 @@ func TestFailOnInvalidRequest(t *testing.T) { }, } }(), - http.StatusInternalServerError, + expected: http.StatusInternalServerError, + allowed: true, }, } { t.Run(tt.name, func(t *testing.T) { @@ -442,7 +445,7 @@ func TestFailOnInvalidRequest(t *testing.T) { res := injector.Handle(context.Background(), tt.req) // verify - assert.False(t, res.Allowed) + assert.Equal(t, tt.allowed, res.Allowed) assert.NotNil(t, res.AdmissionResponse.Result) assert.Equal(t, tt.expected, res.AdmissionResponse.Result.Code) }) From c8167a843a903e94571d86adf9780f8100d584da Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Thu, 14 Apr 2022 15:48:10 +0200 Subject: [PATCH 4/5] Add more docs Signed-off-by: Pavol Loffay --- internal/webhookhandler/webhookhandler.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/webhookhandler/webhookhandler.go b/internal/webhookhandler/webhookhandler.go index 5dd3527b83..21d5a9004b 100644 --- a/internal/webhookhandler/webhookhandler.go +++ b/internal/webhookhandler/webhookhandler.go @@ -78,9 +78,12 @@ func (p *podSidecarInjector) Handle(ctx context.Context, req admission.Request) ns := corev1.Namespace{} err = p.client.Get(ctx, types.NamespacedName{Name: req.Namespace, Namespace: ""}, &ns) if err != nil { - res := admission.Errored(http.StatusInternalServerError, err) + res := admission.Errored(http.StatusBadRequest, err) // By default, admission.Errored sets Allowed to false which blocks pod creation even though the failurePolicy=ignore. // Allowed set to true makes sure failure does not block pod creation in case of an error. + // Using the http.StatusInternalServerError creates a k8s event associated with the replica set. + // The admission.Allowed("").WithWarnings(err.Error()) or http.StatusBadRequest does not + // create any event. Additionally, an event/log cannot be created explicitly because the pod name is not known. res.Allowed = true return res } From a0f168df1b5d95b39671707af3162cdbacbdb358 Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Fri, 22 Apr 2022 10:31:18 +0200 Subject: [PATCH 5/5] Fix Signed-off-by: Pavol Loffay --- internal/webhookhandler/webhookhandler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/webhookhandler/webhookhandler.go b/internal/webhookhandler/webhookhandler.go index 21d5a9004b..948717f5a2 100644 --- a/internal/webhookhandler/webhookhandler.go +++ b/internal/webhookhandler/webhookhandler.go @@ -78,7 +78,7 @@ func (p *podSidecarInjector) Handle(ctx context.Context, req admission.Request) ns := corev1.Namespace{} err = p.client.Get(ctx, types.NamespacedName{Name: req.Namespace, Namespace: ""}, &ns) if err != nil { - res := admission.Errored(http.StatusBadRequest, err) + res := admission.Errored(http.StatusInternalServerError, err) // By default, admission.Errored sets Allowed to false which blocks pod creation even though the failurePolicy=ignore. // Allowed set to true makes sure failure does not block pod creation in case of an error. // Using the http.StatusInternalServerError creates a k8s event associated with the replica set.