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.

Sign up for GitHub

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

Jump to bottom

Rename the port_grpc section to avoid confusion #4015

Closed
cjcobb23 opened this issue Dec 7, 2021 · 1 comment · Fixed by #4728
Closed

Rename the port_grpc section to avoid confusion #4015

cjcobb23 opened this issue Dec 7, 2021 · 1 comment · Fixed by #4728
Assignees
@mDuo13 @ckeshava
Labels
Bug Will Need Documentation

Comments

@cjcobb23
Copy link
Contributor

cjcobb23 commented Dec 7, 2021

The gRPC related config info needs to be in a section called port_grpc. However, this section cannot be added to the server section, or else rippled will fail to start. This is because the code that handles the other ports (and reads from the server section) does not handle gRPC. I think the cleanest thing here would be to change the name from port_grpc to just gRPC, though there are a variety of other solutions here. Regardless, this trips a lot of people up and it's not clear to them what's wrong.
@legleux legleux mentioned this issue Jun 5, 2023
Closed
@intelliot intelliot added Good First Issue Great issue for a new contributor Bug Will Need Documentation labels Jun 5, 2023
@ckeshava
Copy link
Collaborator

ckeshava commented Jun 6, 2023

Hello @cjcobb23 ,
Thank you for your analysis, it is helpful.

I'm looking into a related issue - #4557 and I'd like to get your opinion.

You are suggesting that:

  • we change the configuration-section name
  • update documentation to indicate [server] section must not include [port_grpc] right?

I'm new to XRPL and I'm trying to chalk out the viability of this solution. What if a user wants to only set [server] and [port_grpc] sections and ignore/default the other sections like [port_ws_admin_local], [port_peer] etc? Is this situation possible?

@ckeshava ckeshava self-assigned this Jun 6, 2023
ckeshava added a commit to ckeshava/rippled that referenced this issue Jun 7, 2023
@ckeshava
Fix XRPLF#4015, XRPLF#4557: Do not validate GRPC IP/Port/Protocol inf…
8e122c5 
…ormation in ServerHandler. Pass the responsibility of grpc port information validation to GRPCServer constructor. Both GRPCServer and ServerHandler are created during the construction of Application instance.
@ckeshava ckeshava mentioned this issue Jun 7, 2023
Closed
7 tasks
@intelliot intelliot assigned cjcobb23 and unassigned ckeshava Jul 2, 2023
@intelliot intelliot assigned ckeshava and unassigned cjcobb23 Aug 30, 2023
@intelliot intelliot removed the Good First Issue Great issue for a new contributor label Aug 30, 2023
ckeshava added a commit to ckeshava/rippled that referenced this issue Sep 22, 2023
@ckeshava
Fix XRPLF#4015, XRPLF#4557: Do not validate GRPC IP/Port/Protocol inf…
7c78fd2 
…ormation in ServerHandler. Pass the responsibility of grpc port information validation to GRPCServer constructor. Both GRPCServer and ServerHandler are created during the construction of Application instance.
ckeshava added a commit to ckeshava/rippled that referenced this issue Sep 25, 2023
@ckeshava
Fix XRPLF#4015, XRPLF#4557: Do not validate GRPC IP/Port/Protocol inf…
34df740 
…ormation in ServerHandler. Pass the responsibility of grpc port information validation to GRPCServer constructor. Both GRPCServer and ServerHandler are created during the construction of Application instance.
ckeshava added a commit to ckeshava/rippled that referenced this issue Sep 25, 2023
@ckeshava
Fix XRPLF#4015, XRPLF#4557: Do not validate GRPC IP/Port/Protocol inf…
ba099f5 
…ormation in ServerHandler. Pass the responsibility of grpc port information validation to GRPCServer constructor. Both GRPCServer and ServerHandler are created during the construction of Application instance.
ckeshava added a commit to ckeshava/rippled that referenced this issue Sep 25, 2023
@ckeshava
Fix XRPLF#4015, XRPLF#4557: Do not validate GRPC IP/Port/Protocol inf…
8c4a920 
…ormation in ServerHandler. Pass the responsibility of grpc port information validation to GRPCServer constructor. Both GRPCServer and ServerHandler are created during the construction of Application instance.
ckeshava added a commit to ckeshava/rippled that referenced this issue Sep 25, 2023
@ckeshava
Fix XRPLF#4015, XRPLF#4557: Do not validate GRPC IP/Port/Protocol inf…
0e406c3 
…ormation in ServerHandler. Pass the responsibility of grpc port information validation to GRPCServer constructor. Both GRPCServer and ServerHandler are created during the construction of Application instance.

Introduce a constant (SECTION_PORT_GRPC) for port_grpc configuration section

Replace all usages of "port_grpc" raw string with a constant

Created macros for "port_ws", "port_rpc" and "port_peer" for unit test files.
Removed unused imports in some files
ckeshava added a commit to ckeshava/rippled that referenced this issue Sep 25, 2023
@ckeshava
Fix XRPLF#4015, XRPLF#4557: Do not validate GRPC IP/Port/Protocol inf…
47f55ae 
…ormation in ServerHandler. Pass the responsibility of grpc port information validation to GRPCServer constructor. Both GRPCServer and ServerHandler are created during the construction of Application instance.

Introduce a constant (SECTION_PORT_GRPC) for port_grpc configuration section

Replace all usages of "port_grpc" raw string with a constant

Created macros for "port_ws", "port_rpc" and "port_peer" for unit test files.
Removed unused imports in some files
@ckeshava ckeshava mentioned this issue Sep 25, 2023
Merged
7 tasks
intelliot pushed a commit that referenced this issue Feb 7, 2024
@ckeshava
feat: allow port_grpc to be specified in [server] stanza (#4728)
6d3c21e 
Prior to this commit, `port_grpc` could not be added to the [server]
stanza. Instead of validating gRPC IP/Port/Protocol information in
ServerHandler, validate grpc port info in GRPCServer constructor. This
should not break backwards compatibility.

gRPC-related config info must be in a section (stanza) called
[port_gprc].

* Close #4015 - That was an alternate solution. It was decided that with
  relaxed validation, it is not necessary to rename port_grpc.
* Fix #4557
sophiax851 pushed a commit to sophiax851/rippled that referenced this issue Jun 12, 2024
@ckeshava @sophiax851
feat: allow port_grpc to be specified in [server] stanza (XRPLF#4728
33fa7d5 
)

Prior to this commit, `port_grpc` could not be added to the [server]
stanza. Instead of validating gRPC IP/Port/Protocol information in
ServerHandler, validate grpc port info in GRPCServer constructor. This
should not break backwards compatibility.

gRPC-related config info must be in a section (stanza) called
[port_gprc].

* Close XRPLF#4015 - That was an alternate solution. It was decided that with
  relaxed validation, it is not necessary to rename port_grpc.
* Fix XRPLF#4557
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mDuo13 mDuo13

@ckeshava ckeshava

Labels
Bug Will Need Documentation
Projects
None yet
Milestone
No milestone
5 participants
@intelliot @mDuo13 @ckeshava @cjcobb23 and others