Validate grpc port info in GRPCServer ctor #4728

ckeshava · 2023-09-25T21:36:50Z

High Level Overview of Change

Fix #4015, #4557: GRPCServer and ServerHandler are created during the construction of Application instance. Do not validate GRPC IP/Port/Protocol information in ServerHandler. Instead, pass the responsibility of grpc port information validation to GRPCServer constructor. This ensures that configuration file can specify the port_grpc section.

Context of Change

Based on suggestions from @ximinez , I implemented this code change. @cjcobb23 suggests an alternative solution to the same issue #4015

I welcome any other suggestions too. I felt this is a clean way to handle this problem without breaking backwards compatibility.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

This pertains to renaming of variables inside the codebase. It should suffice to test it through the existing unit tests.

…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

… test

ximinez

This is only a partial review after taking a quick glance

src/ripple/app/main/GRPCServer.cpp

ximinez

One minor suggestion. Also, please merge in the latest develop branch.

ximinez · 2023-10-09T18:42:00Z

src/ripple/core/ConfigSections.h

@@ -101,6 +101,8 @@ struct ConfigSection
 #define SECTION_SWEEP_INTERVAL "sweep_interval"
 #define SECTION_NETWORK_ID "network_id"

+#define SECTION_PORT_GRPC "port_grpc"


Most of the definitions in this list are alphabetical. Could you move this one to slot in alphabetically with the bulk of them?

The last 6 #defines are not in alphabetical order, I've fixed them too

… issue at hand)

intelliot · 2023-12-06T17:55:22Z

This is being held until 2.1.0 (so should be merged in ~Jan 2024)

intelliot · 2023-12-06T18:01:06Z

@ckeshava confirmed this is ready from his perspective; adding Passed label.

codecov-commenter · 2024-02-01T07:32:09Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (1e96a1d) 61.50% compared to head (f7dd7b8) 61.49%.

Files	Patch %	Lines
src/ripple/rpc/impl/ServerHandler.cpp	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4728      +/-   ##
===========================================
- Coverage    61.50%   61.49%   -0.01%     
===========================================
  Files          797      797              
  Lines        70131    70131              
  Branches     36244    36245       +1     
===========================================
- Hits         43136    43130       -6     
- Misses       19755    19759       +4     
- Partials      7240     7242       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

intelliot · 2024-02-01T17:16:45Z

@ckeshava @legleux - did you confirm that this PR, as-is, fully fixes the issue reported in #4557 (comment)?

Specifically:

All ports removed and isolated down to grpc_port - node stop

Isolated down to grpc_port and add protocol - node stop

grpc_port removed from server block - successful boot

In all of those cases, with this PR, the node will successfully boot with a correctly configured grpc_port, right?

intelliot · 2024-02-01T17:18:17Z

~~Suggested commit message:~~

See updated message below.

legleux · 2024-02-03T01:07:04Z

The commit message should not have [port_grpc] but just port_grpc in the [server] block.

👍 With this PR, rippled no longer stops with

Application:ERR Missing 'protocol' in [port_grpc]

when port_grpc appears in the [server] block which was a cause of confusion.

However, port_grpc can be omitted and it appears the server starts so the behavior is not consistent with the other [server] options, i.e. if you omit port_rpc from [server] yet leave the config, the rpc server does not appear to be running.

Here's output from startup completely omitting grpc_port in [server] showing it (at least logging) that it's starting, which was the previous behavior. The previous behavior prevented startup if the field was present.

2024-Feb-03 01:10:04.559472985 UTC Server:NFO Opened 'port_rpc' (ip=0.0.0.0:51234, admin nets:127.0.0.1/32, 10.30.97.0/32, 10.30.97.162/32, http)
2024-Feb-03 01:10:04.559508247 UTC Server:NFO Opened 'port_peer' (ip=0.0.0.0:51235, peer)
2024-Feb-03 01:10:04.559537538 UTC Server:NFO Opened 'port_ws_admin_local' (ip=127.0.0.1:6006, admin nets:127.0.0.1/32, ws)
   ...
2024-Feb-03 01:10:04.583550265 UTC gRPC Server:NFO Starting gRPC server at 127.0.0.1:50051

intelliot · 2024-02-03T23:13:29Z

Thanks. I think that's ok - in fact it's probably what we want, as the change should be 100% backward-compatible with any existing working cfg.

intelliot · 2024-02-03T23:14:54Z

Suggested commit message (updated):

feat: allow `port_grpc` to be specified in `[server]` stanza (#4728)

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

ckeshava · 2024-02-07T17:02:40Z

@legleux Thanks for verifying the output of this PR! I was not able to look into this one for the past few days

) 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

ckeshava mentioned this pull request Sep 25, 2023

Validate grpc port info in GRPCServer ctor #4560

Closed

7 tasks

fix the compiler error by including ConfigSections in ServerInfo unit…

54d443a

… test

ximinez requested changes Sep 27, 2023

View reviewed changes

src/ripple/app/main/GRPCServer.cpp Outdated Show resolved Hide resolved

use const& whereever possible, addressing Ed's suggestions

dc4a1be

intelliot requested review from ximinez, cjcobb23 and legleux October 4, 2023 17:09

ximinez requested changes Oct 9, 2023

View reviewed changes

ckeshava and others added 3 commits October 10, 2023 12:47

alphabetically order the #defines

429593d

alphabetically order all the #defines (not relevant to the GRPCServer…

f61e718

… issue at hand)

Merge branch 'develop' into grpc3

2f882af

ximinez approved these changes Oct 12, 2023

View reviewed changes

intelliot requested a review from mDuo13 October 15, 2023 02:14

intelliot added this to the 2.0 milestone Oct 16, 2023

intelliot assigned legleux Oct 16, 2023

intelliot modified the milestones: 2.0, 2024 release Oct 17, 2023

legleux approved these changes Oct 30, 2023

View reviewed changes

Merge branch 'develop' into grpc3

52beb3f

intelliot unassigned legleux Oct 31, 2023

Merge branch 'develop' into grpc3

33f51ca

intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Dec 6, 2023

Merge branch 'develop' into grpc3

9212481

intelliot added the Perf impact not expected Change is not expected to improve nor harm performance. label Feb 1, 2024

intelliot added the Testable label Feb 1, 2024

intelliot added the Will Need Documentation label Feb 1, 2024

intelliot added 2 commits February 5, 2024 08:10

Merge branch 'develop' into grpc3

724492d

Merge branch 'develop' into grpc3

f7dd7b8

intelliot merged commit 6d3c21e into XRPLF:develop Feb 7, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate grpc port info in GRPCServer ctor #4728

Validate grpc port info in GRPCServer ctor #4728

ckeshava commented Sep 25, 2023 •

edited

Loading

ximinez left a comment

ximinez left a comment

ximinez Oct 9, 2023

ckeshava Oct 10, 2023

intelliot commented Dec 6, 2023

intelliot commented Dec 6, 2023

codecov-commenter commented Feb 1, 2024 •

edited

Loading

intelliot commented Feb 1, 2024

intelliot commented Feb 1, 2024 •

edited

Loading

legleux commented Feb 3, 2024 •

edited

Loading

intelliot commented Feb 3, 2024

intelliot commented Feb 3, 2024

ckeshava commented Feb 7, 2024

Validate grpc port info in GRPCServer ctor #4728

Validate grpc port info in GRPCServer ctor #4728

Conversation

ckeshava commented Sep 25, 2023 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

ximinez left a comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

ximinez Oct 9, 2023

Choose a reason for hiding this comment

ckeshava Oct 10, 2023

Choose a reason for hiding this comment

intelliot commented Dec 6, 2023

intelliot commented Dec 6, 2023

codecov-commenter commented Feb 1, 2024 • edited Loading

Codecov Report

intelliot commented Feb 1, 2024

intelliot commented Feb 1, 2024 • edited Loading

legleux commented Feb 3, 2024 • edited Loading

intelliot commented Feb 3, 2024

intelliot commented Feb 3, 2024

ckeshava commented Feb 7, 2024

ckeshava commented Sep 25, 2023 •

edited

Loading

codecov-commenter commented Feb 1, 2024 •

edited

Loading

intelliot commented Feb 1, 2024 •

edited

Loading

legleux commented Feb 3, 2024 •

edited

Loading