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

Migrate API Server #6426

Merged
merged 23 commits into from
Jun 15, 2020
Merged

Migrate API Server #6426

merged 23 commits into from
Jun 15, 2020

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Jun 12, 2020

Description

This PR revises the model in which a API (REST) server is started. The server is now started in process along with the application binary & Tendermint -- everything is in a single process. This drastically reduces various complexities and simplifies the mental model.

Operators may now enable/disable the API server along with various other options. In the future, they'll be able to provide Caddy/TLS/Proxy configurations as well.

One thing left to consider is if how to handle proof verifications (i.e. trust-node) -- I left a TODO for this.

From an app developer's POV, not much changes. They still provide module route registration, except instead of passing it to a command, they now pass it along with their app constructor to the start command.

To test:

  1. make build-docker-local-simapp (only needed to be invoked once to build the image)
  2. make clean localnet-start
  3. BUILDDIR=./build/macos make build-sim
  4. Invoke and test various routes (localhost:1317-1320)
  5. make localnet-stop

closes: #6408

/cc @jackzampolin @marbar3778


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #6426 into master will increase coverage by 0.99%.
The diff coverage is 56.25%.

@@            Coverage Diff             @@
##           master    #6426      +/-   ##
==========================================
+ Coverage   54.80%   55.79%   +0.99%     
==========================================
  Files         378      465      +87     
  Lines       22391    27800    +5409     
==========================================
+ Hits        12272    15512    +3240     
- Misses       9194    11183    +1989     
- Partials      925     1105     +180     

@alexanderbez alexanderbez added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Jun 12, 2020
server/api/root.go Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor Author

I have the basis of this feature working. We're going to need to rethink the design and use of Context a bit. E.g. does it make sense to have trust-node and rpc flags defined?

@alexanderbez alexanderbez marked this pull request as ready for review June 13, 2020 15:56
@alexanderbez alexanderbez added R4R and removed WIP labels Jun 13, 2020
Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

Long overdue. Huge improvement would like to test but afk today

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

utACK, code looks good. Will test on Monday

@@ -39,6 +39,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Client Breaking

* (api) [\#6426](https://github.com/cosmos/cosmos-sdk/pull/6426) The ability to start an out-of-process API REST server has now been removed. Instead, the API server is now started in-process along with the application and Tendermint. Configuration options have been added to `app.toml` to enable/disable the API server along with additional HTTP server options.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* (api) [\#6426](https://github.com/cosmos/cosmos-sdk/pull/6426) The ability to start an out-of-process API REST server has now been removed. Instead, the API server is now started in-process along with the application and Tendermint. Configuration options have been added to `app.toml` to enable/disable the API server along with additional HTTP server options.
* (api) [\#6426](https://github.com/cosmos/cosmos-sdk/pull/6426) The ability to start an out-of-process API REST server has now been removed. Instead, the API server is now started in-process along with the application and Tendermint. Configuration options, i.e legacy flags, have been migrated to `app.toml` to enable/disable the API server along with additional HTTP server options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Legacy flags? What legacy flags?

server/config/config.go Show resolved Hide resolved
server/start.go Outdated Show resolved Hide resolved
server/api/server.go Outdated Show resolved Hide resolved
server/config/config.go Outdated Show resolved Hide resolved
server/config/config.go Show resolved Hide resolved
server/constructors.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

This is brilliant, left suggestions for a couple of cosmetic changes.

alexanderbez and others added 3 commits June 15, 2020 09:53
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
@alexanderbez
Copy link
Contributor Author

I'll leave this PR open for another day or so to allow people to test, until I merge it.

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

tested ACK

@fedekunze
Copy link
Collaborator

It'd be nice if we could add the docs from Gaia and update them for the SDK. Maybe a follow-up issue/PR

@alexanderbez alexanderbez merged commit 6a05b83 into master Jun 15, 2020
@alexanderbez alexanderbez deleted the bez/6408-integrate-rest-svc branch June 15, 2020 17:39
// APIConfig defines the API listener configuration.
type APIConfig struct {
// Enable defines if the API server should be enabled.
Enable bool `mapstructure:"enable"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@alexanderbez just saw this is not used, is that correct?

Copy link
Contributor Author

@alexanderbez alexanderbez Jun 16, 2020

Choose a reason for hiding this comment

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

WDYM not used? It is used and is false by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, maybe I am wrong but inside here

func (s *Server) Start(cfg config.Config) error {

I dont see any if Config.API.Enabled and I am going crazy with it!

Copy link
Contributor Author

@alexanderbez alexanderbez Jun 16, 2020

Choose a reason for hiding this comment

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

https://github.com/cosmos/cosmos-sdk/blob/master/server/start.go#L195

We should probably create the config before that and use that instead. Mind creating a small PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave it in my hands!

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove REST Service from CLI
5 participants