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

Tink Server: Fix runtime panic #536

Merged
merged 1 commit into from
Sep 3, 2021

Conversation

jacobweinstock
Copy link
Member

Description

Fix runtime panic: The unary server interceptor was already set and may not be reset
The grpc server interceptors need to be chained to not cause a runtime panic.

Why is this needed

Fixes: #535

How Has This Been Tested?

make test
make run
docker logs tink_tinkerbell_1 - validate the server started up successfully.

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

…ady set and may not be reset

The grpc server interceptors need to be chained to not cause
a runtime panic.

Signed-off-by: Jacob Weinstock <[email protected]>
@jacobweinstock jacobweinstock added kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. kind/regression Categorizes issue or PR as related to a regression from a prior release. labels Sep 3, 2021
@codecov
Copy link

codecov bot commented Sep 3, 2021

Codecov Report

Merging #536 (151ab5a) into master (cbe2037) will increase coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #536      +/-   ##
==========================================
+ Coverage   33.59%   33.61%   +0.01%     
==========================================
  Files          44       44              
  Lines        3387     3385       -2     
==========================================
  Hits         1138     1138              
+ Misses       2152     2150       -2     
  Partials       97       97              
Impacted Files Coverage Δ
grpc-server/grpc_server.go 0.00% <0.00%> (ø)

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 cbe2037...151ab5a. Read the comment docs.

Copy link
Contributor

@mmlb mmlb left a comment

Choose a reason for hiding this comment

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

Is there a test we can add that would have caught this?

Copy link
Contributor

@tobert tobert left a comment

Choose a reason for hiding this comment

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

Whooooops. I didn't even notice that and am surprised the compiler didn't!

@tobert
Copy link
Contributor

tobert commented Sep 3, 2021

Is there a test we can add that would have caught this?

I should have started up the tink stack before submitting the PR. I'll get my rig set up before I dig in more (tink-worker and tink need some more otel bits). Ideally we can come up with a functional test that lets us exercise the tink binaries a ilttle without bringing up the whole thing.

@jacobweinstock jacobweinstock merged commit 38394a1 into tinkerbell:master Sep 3, 2021
@jacobweinstock jacobweinstock deleted the fix-runtime-panic branch September 3, 2021 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tink server panic: The unary server interceptor was already set and may not be reset.
3 participants