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.

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

fix(kuma-cp): don't panic in webhook if k8s object can't convert to core resource #4455

Merged
merged 2 commits into from
Jun 10, 2022

Conversation

michaelbeaumont
Copy link
Contributor

@michaelbeaumont michaelbeaumont commented Jun 10, 2022

Summary

Trying to set, for example, a negative weight on a TrafficRoute split results in a confusing error for users:

error: trafficroutes.kuma.io "route-all-default" could not be patched: Internal error occurred: failed calling webhook "validator.kuma-admission.kuma.io": failed to call webhook: Post "https://kuma-control-plane.kuma-system.svc:443/validate-kuma-io-v1alpha1?timeout=10s": EOF

and a panic in the webhook goroutine in kuma-cp:

2022/06/10 06:42:31 http: panic serving 172.17.0.1:8216: failed to unmarshal *v1alpha1.TrafficRoute: json: cannot unmarshal number -1 into Go value of type uint32
goroutine 1247 [running]:
net/http.(*conn).serve.func1()
	/usr/lib/go/src/net/http/server.go:1825 +0xbf
panic({0x1e1dde0, 0xc0013482b0})
	/usr/lib/go/src/runtime/panic.go:844 +0x258
github.com/kumahq/kuma/pkg/util/proto.MustUnmarshalJSON({0xc0006ca833?, 0xc001c9e1e8?, 0x40d987?}, {0x3031080?, 0xc00163f560?})
	/home/mike/projects/kuma/pkg/util/proto/proto.go:67 +0xc7
github.com/kumahq/kuma/pkg/plugins/resources/k8s/native/api/v1alpha1.(*TrafficRoute).GetSpec(0xc0006ca480?)
	/home/mike/projects/kuma/pkg/plugins/resources/k8s/native/api/v1alpha1/zz_generated.mesh.go:1689 +0x58
github.com/kumahq/kuma/pkg/plugins/resources/k8s.(*SimpleConverter).ToCoreResource(0xc001421530?, {0x3057300, 0xc001644b40}, {0x3042890, 0xc000651d40})
	/home/mike/projects/kuma/pkg/plugins/resources/k8s/converter.go:53 +0x112
github.com/kumahq/kuma/pkg/plugins/runtime/k8s/webhooks.(*validatingHandler).decode(_, {{{0xc001421530, 0x24}, {{0xc000ce2930, 0x7}, {0xc000ce2938, 0x8}, {0xc000ce2940, 0xc}}, {{0xc000ce2950, ...}, ...}, ...}})
	/home/mike/projects/kuma/pkg/plugins/runtime/k8s/webhooks/validation.go:105 +0x16a

Here we can't assume that GetSpec succeeds.

Now the user gets:

error: trafficroutes.kuma.io "route-all-default" could not be patched: admission webhook "validator.kuma-admission.kuma.io" denied the request: json: cannot unmarshal number -2 into Go value of type uint32

…ore resource

Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont michaelbeaumont requested a review from a team as a code owner June 10, 2022 07:18
@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2022

Codecov Report

Merging #4455 (62e1e4b) into master (260d813) will decrease coverage by 0.06%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master    #4455      +/-   ##
==========================================
- Coverage   55.42%   55.36%   -0.07%     
==========================================
  Files         947      947              
  Lines       57938    57976      +38     
==========================================
- Hits        32113    32097      -16     
- Misses      23284    23326      +42     
- Partials     2541     2553      +12     
Impacted Files Coverage Δ
...donothingpolicy/k8s/v1alpha1/zz_generated.types.go 0.00% <0.00%> (ø)
pkg/plugins/resources/k8s/caching_converter.go 0.00% <0.00%> (ø)
...lugins/resources/k8s/native/pkg/model/resources.go 100.00% <ø> (ø)
...ime/k8s/controllers/gateway_instance_controller.go 0.00% <0.00%> (ø)
tools/policy-gen/protoc-gen-kumapolicy/crd.go 0.00% <ø> (ø)
tools/resource-gen/main.go 0.00% <ø> (ø)
...urces/k8s/native/api/v1alpha1/zz_generated.mesh.go 45.73% <23.95%> (-0.39%) ⬇️
...ces/k8s/native/api/v1alpha1/zz_generated.system.go 44.31% <37.50%> (+0.13%) ⬆️
pkg/plugins/resources/k8s/converter.go 76.47% <50.00%> (-4.78%) ⬇️
...e/test/api/sample/v1alpha1/sample_types_helpers.go 82.75% <100.00%> (+0.61%) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 260d813...62e1e4b. Read the comment docs.

@jakubdyszkiewicz
Copy link
Contributor

jakubdyszkiewicz commented Jun 10, 2022

Uhhh, can we recover from the panic in webhook server?

We recover from panics in API Server. We should never crash a CP for invalid request. Otherwise, it's very easy to do DDoS a CP.

@michaelbeaumont
Copy link
Contributor Author

michaelbeaumont commented Jun 10, 2022

Uhhh, can we recover from the panic in webhook server?

Yeah it doesn't crash kuma-cp. It just results in the request failing inexplicably.

@jakubdyszkiewicz
Copy link
Contributor

I misunderstand the description then, great!

Signed-off-by: Mike Beaumont <mjboamail@gmail.com>
@michaelbeaumont michaelbeaumont enabled auto-merge (squash) June 10, 2022 13:45
@michaelbeaumont michaelbeaumont merged commit f4b94a0 into kumahq:master Jun 10, 2022
@michaelbeaumont michaelbeaumont deleted the fix/k8s_convert branch June 10, 2022 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants