add tests for webhook/admission pkg #123
Conversation
k8s-ci-robot
added
size/XL
pkg/webhook/admission/webhook.go
Outdated
| @@ -130,37 +135,44 @@ func (w *Webhook) kvMapToContext(ctx context.Context) (context.Context, error) { | |||
| } | |||
|
|
|||
| func (w *Webhook) handleMutating(ctx context.Context, req Request) Response { | |||
| var resp Response | |||
| var patches []jsonpatch.JsonPatchOperation | |||
| r := Response{ | |||
There was a problem hiding this comment.
You can move this declaration to line 155
| Expect(w.response).To(Equal(expected)) | ||
| Expect(alwaysDeny.invoked).To(BeTrue()) | ||
| Expect(alwaysDeny.valueFromContext).To(Equal("bar")) | ||
| Expect(alwaysAllow.invoked).To(BeFalse()) |
There was a problem hiding this comment.
So the handlers are executed with respect to their order in the Handlers slice. Is that something users need to be aware of? Do you need to add some doc for that?
| } | ||
| wh := &Webhook{ | ||
| Type: types.WebhookTypeMutating, | ||
| Handlers: []Handler{patcher1, patcher2}, |
There was a problem hiding this comment.
Do you check conflict among different handlers?
There was a problem hiding this comment.
No. I can't find a easy way to do that.
I added a comment in the struct saying that user should ensure that their handlers should not mutate same field and generate conflicting JSON patches.
pkg/webhook/admission/http_test.go
Outdated
| }) | ||
| }) | ||
|
|
||
| type fakeResponseWriter struct { |
There was a problem hiding this comment.
Did you look at httptest pkg https://golang.org/pkg/net/http/httptest/ It provides some nice utilities for testing http handlers without running the server.
| logf "sigs.k8s.io/controller-runtime/pkg/runtime/log" | ||
| ) | ||
|
|
||
| func TestSource(t *testing.T) { |
There was a problem hiding this comment.
TestSource or we can call it something like TestAdmissionWebhook() ?
pkg/webhook/admission/decode_test.go
Outdated
| var err error | ||
| decoder, err = NewDecoder(scheme.Scheme) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(decoder).NotTo(BeNil()) |
There was a problem hiding this comment.
Do we want to create decoder in BeforeEach ? I think it can be shared for the tests below. WDYT ?
pkg/webhook/admission/http_test.go
Outdated
| w := &fakeResponseWriter{} | ||
| expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"request body is empty","code":400}}} | ||
| `) | ||
| It("should return a response with an error", func() { |
There was a problem hiding this comment.
should return an error with bad-request status code ?
pkg/webhook/admission/http_test.go
Outdated
| w := &fakeResponseWriter{} | ||
| expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"contentType=application/foo, expect application/json","code":400}}} | ||
| `) | ||
| It("should return a response with an error", func() { |
|
PTAL |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
droot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: mengqiy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Make order of generated yaml from kubebuilder create config --crds deterministic
Fix several nil pointer issues when testing the unhappy path in the unit test.