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): guard the nil version in metadata #3969

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

jakubdyszkiewicz
Copy link
Contributor

Summary

Guard the nil version.dependencies.

I went through code a couple of times and it seems that the only place where we can get a nil panic.

Issues resolved

Fix #3968

Documentation

No docs.

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

- [ ] Update UPGRADE.md with any steps users will need to take when upgrading.

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz force-pushed the fix/nil-check-metadata branch from 3d4d467 to 87ad464 Compare March 3, 2022 16:01
@lahabana
Copy link
Contributor

lahabana commented Mar 3, 2022

We have pentest coming soon these are the kind of bugs we'll find!

Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for backporting

@lahabana
Copy link
Contributor

lahabana commented Mar 3, 2022

Shouldn't the http endpoint catch panics btw?

@codecov-commenter
Copy link

Codecov Report

Merging #3969 (87ad464) into master (c1d2350) will increase coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3969      +/-   ##
==========================================
+ Coverage   56.02%   56.03%   +0.01%     
==========================================
  Files         917      917              
  Lines       54770    54771       +1     
==========================================
+ Hits        30684    30691       +7     
+ Misses      21640    21633       -7     
- Partials     2446     2447       +1     
Impacted Files Coverage Δ
pkg/core/xds/metadata.go 62.35% <0.00%> (+6.40%) ⬆️
pkg/core/tokens/default_signing_key.go 72.22% <0.00%> (-8.34%) ⬇️
pkg/kds/reconcile/reconciler.go 79.48% <0.00%> (-5.13%) ⬇️
pkg/core/secrets/manager/global_manager.go 39.39% <0.00%> (-3.04%) ⬇️
pkg/plugins/resources/postgres/store.go 74.68% <0.00%> (-0.43%) ⬇️
api/mesh/v1alpha1/dataplane_insight.pb.go 44.84% <0.00%> (+0.25%) ⬆️
pkg/defaults/components.go 88.88% <0.00%> (+3.70%) ⬆️
pkg/plugins/runtime/gateway/route/sorter.go 71.79% <0.00%> (+5.12%) ⬆️
pkg/core/resources/manager/cache.go 88.31% <0.00%> (+5.19%) ⬆️

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 c1d2350...87ad464. Read the comment docs.

@jakubdyszkiewicz
Copy link
Contributor Author

jakubdyszkiewicz commented Mar 3, 2022

Shouldn't the http endpoint catch panics btw?

Good question. It seems that the regular http package does recover from panic. I just checked it.
also golang/go#25245

But grpc code is different. Maybe we can also recover there somehow

edit: there is a way https://github.com/grpc-ecosystem/go-grpc-middleware let me do this in the follow up PR

@jakubdyszkiewicz jakubdyszkiewicz merged commit 7f5ae2b into master Mar 3, 2022
@jakubdyszkiewicz jakubdyszkiewicz deleted the fix/nil-check-metadata branch March 3, 2022 16:47
mergify bot pushed a commit that referenced this pull request Mar 3, 2022
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
(cherry picked from commit 7f5ae2b)
jakubdyszkiewicz pushed a commit that referenced this pull request Mar 4, 2022
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
SallyBlichWalkMe pushed a commit to SallyBlichWalkMe/kuma that referenced this pull request Apr 14, 2022
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Sally Blich <sally.blich@walkme.com>
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.

Fix defensiveness of nils when DP connects to the CP.
3 participants