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

[discussion] Revise configuration endpoints according to the RFC #4063

Closed
0x009922 opened this issue Nov 21, 2023 · 5 comments
Closed

[discussion] Revise configuration endpoints according to the RFC #4063

0x009922 opened this issue Nov 21, 2023 · 5 comments
Assignees
Labels
api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha Documentation Documentation changes iroha2-dev The re-implementation of a BFT hyperledger in RUST QA-confirmed This bug is reproduced and needs a fix

Comments

@0x009922
Copy link
Contributor

0x009922 commented Nov 21, 2023

Description

According to the Configuration Overhaul RFC (#2585), the current configuration endpoints should be revised.

Current Design

Iroha's API currently provides two endpoints related to configuration:

  • POST /configuration: Allows runtime updating of the configuration. As of now, it only supports modifying the log level using a request body structured as {"LogLevel":"<level>"}.
  • GET /configuration: Serves a dual purpose; it can either return the entire configuration in a JSON format or provide documentation for a specific field.

From the Configuration Endpoints section of the RFC

Detailed Proposal

For the reasoning please refer to the Proposal 9 of the RFC.

  • Remove documentation fetching via API
  • Use the same naming conventions as used in the config file (or ENV)
  • Use GET/POST /configuration for fetching and updating the subset of configuration parameters. The subset and format should be the same between the two endpoints for the sake of uniformity.
  • While updating the configuration accepts partial structure (i.e. user might specify only those fields they need to update), retrieval always returns the whole subset. Note: this is not relevant now because we only have a single dynamic parameter.

The only dynamic parameter as of now is logger.level (according to the updated naming of parameters). Therefore, the following JSON request might be used to update the value with POST /configuration:

{
  "logger": {
    "level": "DEBUG"
  }
}

The same will be returned by GET /configuration.

Caveats

In that place of the docs it is recommended to get peers' configuration in the network through the configuration endpoint. It suggests me that probably retrieval of the configuration might include more fields than only dynamic ones.

Also

There is an issue:

, which suggests using sub-routing in a way it is used in GET /status endpoint. In general, I don't see a conflict here and it might be adopted for the new configuration endpoints:

Samples

Getting whole configuration:

GET /configuration
{
  "logger": {
    "level": "DEBUG"
  }
}

Getting a logger part:

GET /configuration/logger
{
  "level": "DEBUG"
}

Getting logger.level precisely:

GET /configuration/logger/level
"DEBUG"

Updating logger parameters:

POST /configuration/logger
{
  "level": "DEBUG"
}
@0x009922 0x009922 added question Further information is requested iroha2-dev The re-implementation of a BFT hyperledger in RUST Documentation Documentation changes api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha labels Nov 21, 2023
@Erigara
Copy link
Contributor

Erigara commented Nov 21, 2023

In that place of the docs it is recommended to get peers' configuration in the network through the configuration endpoint. It suggests me that probably retrieval of the configuration might include more fields than only dynamic ones.

Consensus critical parameters are now configured on-chain, so i'm not sure this section is still relevant.

@Erigara
Copy link
Contributor

Erigara commented Nov 21, 2023

Other than that i have no objections or additional comments.

@Mingela
Copy link
Contributor

Mingela commented Nov 21, 2023

Fine to me as well

@0x009922 0x009922 self-assigned this Nov 22, 2023
@0x009922 0x009922 removed the question Further information is requested label Nov 22, 2023
@Arjentix
Copy link
Contributor

Looks fine to me too

0x009922 added a commit to 0x009922/iroha that referenced this issue Nov 23, 2023
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
0x009922 added a commit to 0x009922/iroha that referenced this issue Nov 23, 2023
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
@DCNick3
Copy link
Contributor

DCNick3 commented Nov 23, 2023

Use GET/POST /configuration for fetching and updating the subset of configuration parameters

If it's a subset, I think a PATCH request method makes more sense here.

Otherwise, I think it's a good design!

0x009922 added a commit to 0x009922/iroha that referenced this issue Dec 5, 2023
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
0x009922 added a commit to 0x009922/iroha that referenced this issue Dec 5, 2023
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
0x009922 added a commit to 0x009922/iroha that referenced this issue Dec 7, 2023
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
0x009922 added a commit to 0x009922/iroha that referenced this issue Dec 7, 2023
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
mversic pushed a commit to 0x009922/iroha that referenced this issue Dec 11, 2023
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
mversic pushed a commit to 0x009922/iroha that referenced this issue Dec 11, 2023
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
0x009922 added a commit that referenced this issue Dec 12, 2023
…figuration (#4100)

Co-authored-by: ⭐️NINIKA⭐️ <DCNick3@users.noreply.github.com>
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
@timofeevmd timofeevmd added the QA-confirmed This bug is reproduced and needs a fix label Dec 12, 2023
@timofeevmd timofeevmd self-assigned this Dec 12, 2023
Asem-Abdelhady pushed a commit to Asem-Abdelhady/iroha that referenced this issue Jan 9, 2024
…ha#4064, hyperledger-iroha#4079: re-architect logger and dynamic configuration (hyperledger-iroha#4100)

Co-authored-by: ⭐️NINIKA⭐️ <DCNick3@users.noreply.github.com>
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
Asem-Abdelhady pushed a commit to Asem-Abdelhady/iroha that referenced this issue Jan 22, 2024
…ha#4064, hyperledger-iroha#4079: re-architect logger and dynamic configuration (hyperledger-iroha#4100)

Co-authored-by: ⭐️NINIKA⭐️ <DCNick3@users.noreply.github.com>
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
Signed-off-by: Asem-Abdelhady <asemshawkey@gmail.com>
Asem-Abdelhady pushed a commit to Asem-Abdelhady/iroha that referenced this issue Feb 9, 2024
…ha#4064, hyperledger-iroha#4079: re-architect logger and dynamic configuration (hyperledger-iroha#4100)

Co-authored-by: ⭐️NINIKA⭐️ <DCNick3@users.noreply.github.com>
Signed-off-by: Dmitry Balashov <43530070+0x009922@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha Documentation Documentation changes iroha2-dev The re-implementation of a BFT hyperledger in RUST QA-confirmed This bug is reproduced and needs a fix
Projects
None yet
Development

No branches or pull requests

6 participants