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

feat(debug): add APIs to allow forcing exceptions/prints/etc #277

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

jansegre
Copy link
Member

Unresolved questions:

  • Should these endpoints only by added when enabled from the command line? (like --enable-debug-api)
  • Is the prefix _debug/ OK? I wanted it to look as something clearly made for internal use, that's why the _
  • Should these endpoints be registered to the openapi generator? I just added them without putting much thought into it, they're all missing the parameters supported and/or examples, like we have on other endpoints
  • Is there any possibly useful functionality missing?

@jansegre jansegre requested review from msbrogli and luislhl July 23, 2021 02:42
@jansegre jansegre self-assigned this Jul 23, 2021
@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #277 (946e05d) into dev (7271e0c) will decrease coverage by 0.20%.
The diff coverage is 52.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #277      +/-   ##
==========================================
- Coverage   82.41%   82.21%   -0.21%     
==========================================
  Files         149      150       +1     
  Lines       14116    14220     +104     
  Branches     2014     2026      +12     
==========================================
+ Hits        11634    11691      +57     
- Misses       2065     2113      +48     
+ Partials      417      416       -1     
Impacted Files Coverage Δ
hathor/debug_resources.py 52.88% <52.88%> (ø)
hathor/consensus.py 93.10% <0.00%> (ø)
hathor/p2p/node_sync.py 83.54% <0.00%> (+0.49%) ⬆️

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 7271e0c...946e05d. Read the comment docs.

@jansegre jansegre requested a review from pedroferreira1 July 23, 2021 18:52
@luislhl
Copy link
Collaborator

luislhl commented Jul 23, 2021

  • Should these endpoints only by added when enabled from the command line? (like --enable-debug-api)

I think so, otherwise, if a full-node has its API exposed, someone will be able to crash it. Even with the possibility of blocking requests to the _debug path in another level, the default should be the safest option to avoid mistakes.

@jansegre
Copy link
Member Author

  • Should these endpoints only by added when enabled from the command line? (like --enable-debug-api)

I think so, otherwise, if a full-node has its API exposed, someone will be able to crash it. Even with the possibility of blocking requests to the _debug path in another level, the default should be the safest option to avoid mistakes.

Makes sense. I agree. Added --enable-debug-api now.

@jansegre jansegre force-pushed the feat/debug-exception-api branch from 6d9be29 to 48fb0d8 Compare July 23, 2021 21:21
Copy link
Member

@pedroferreira1 pedroferreira1 left a comment

Choose a reason for hiding this comment

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

I think we should add more protections for those endpoints. We've discussed to allow --test-mode only with another parameter --unsafe-mode. Maybe we should do the same here.

I am especially worried about the last two endpoints, the one that exit the full node and corrupts the database and the other that sets storage to None. We have to make sure none of our public nodes have those endpoints available (even if it's private, we need to make sure we won't call this API by mistake in our wallets/explorer nodes).

What do you think?

About the API docs, I like having it, I think this should be as complete as possible, but maybe the public docs could have only the public endpoints, I don't know.

hathor/debug_resources.py Show resolved Hide resolved
hathor/debug_resources.py Show resolved Hide resolved
@jansegre
Copy link
Member Author

I think we should add more protections for those endpoints. We've discussed to allow --test-mode only with another parameter --unsafe-mode. Maybe we should do the same here.

I am especially worried about the last two endpoints, the one that exit the full node and corrupts the database and the other that sets storage to None. We have to make sure none of our public nodes have those endpoints available (even if it's private, we need to make sure we won't call this API by mistake in our wallets/explorer nodes).

What do you think?

I'm not sure. I think --enable-debug-api and --unsafe-mode --enable-debug-api are just as likely to be enabled by mistake. If we do decide to add a --unsafe-mode for --test-mode, it makes sense to include this parameter and maybe others, but I think we should discuss the --unsafe-mode separately.

About the API docs, I like having it, I think this should be as complete as possible, but maybe the public docs could have only the public endpoints, I don't know.

I agree. Currently I can either register the endpoints or not. Do you think it's worth it to add a mechanism to selectively register endpoints so we can optionally generate a full API doc? Or do you think it's enough for the class to have the OpenAPI description but simply not register it? @pedroferreira1

@pedroferreira1
Copy link
Member

If we do decide to add a --unsafe-mode for --test-mode, it makes sense to include this parameter and maybe others, but I think we should discuss the --unsafe-mode separately.

Agree

Do you think it's worth it to add a mechanism to selectively register endpoints so we can optionally generate a full API doc? Or do you think it's enough for the class to have the OpenAPI description but simply not register it?

I think it's enough for now just not to register it.

@jansegre jansegre force-pushed the feat/debug-exception-api branch 3 times, most recently from c06b044 to f050479 Compare July 28, 2021 00:22
@jansegre jansegre force-pushed the feat/debug-exception-api branch from f050479 to 946e05d Compare July 28, 2021 02:32
@jansegre jansegre merged commit 7c334a5 into dev Jul 28, 2021
@jansegre jansegre deleted the feat/debug-exception-api branch July 28, 2021 03:00
@jansegre jansegre mentioned this pull request Aug 4, 2021
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.

4 participants