Do not block pod creating on internal error in webhook (#811)

* Do not block pod creating on internal error Signed-off-by: Pavol Loffay <[email protected]> * Fix Signed-off-by: Pavol Loffay <[email protected]> * Fix Signed-off-by: Pavol Loffay <[email protected]> * Add more docs Signed-off-by: Pavol Loffay <[email protected]> * Fix Signed-off-by: Pavol Loffay <[email protected]>
open-telemetry · Apr 25, 2022 · 5b1c7b5 · 5b1c7b5
1 parent 115cde6
commit 5b1c7b5
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 10 deletions.
diff --git a/internal/webhookhandler/webhookhandler.go b/internal/webhookhandler/webhookhandler.go
@@ -78,19 +78,30 @@ 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)
+		// 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
 	}
 
 	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
+			return res
 		}
 	}
 
 	marshaledPod, err := json.Marshal(pod)
 	if err != nil {
-		return admission.Errored(http.StatusInternalServerError, err)
+		res := admission.Errored(http.StatusInternalServerError, err)
+		res.Allowed = true
+		return res
 	}
 	return admission.PatchResponseFromRaw(req.Object.Raw, marshaledPod)
 }

diff --git 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)
 		})