Admin Port #725
Admin Port #725
Conversation
WalkthroughAdds separate admin port support (ARKD_ADMIN_PORT) throughout config, CLI, service initialization, validation, and gateways; renames a gRPC handler constructor; refactors credential/TLS handling to propagate Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Cfg as Config Loader
participant CLI as cmd/arkd
participant Svc as gRPC Service
participant MainSrv as Main HTTP/gRPC Server
participant AdminSrv as Admin HTTP/gRPC Server
Cfg->>Cfg: Read PORT and ADMIN_PORT (env/flags)
Cfg-->>CLI: Config{Port, AdminPort}
CLI->>Svc: NewService(version, svcConfig{Port, AdminPort}, appConfig)
alt AdminPort != Port
Svc->>Svc: Validate(): attempt listeners (main & admin)
Svc->>MainSrv: Init main gRPC + gateway (public handlers)
Svc->>AdminSrv: Init admin gRPC + gateway (admin handlers)
else AdminPort == Port
Svc->>MainSrv: Init single gRPC + gateway (combined handlers)
end
CLI->>Svc: Start()
par Servers
Svc->>MainSrv: Listen on :Port
alt AdminPort != Port
Svc->>AdminSrv: Listen on :AdminPort
end
end
sequenceDiagram
autonumber
participant User as Client (public)
participant Admin as Client (admin)
participant MainGW as Main Gateway (:Port)
participant AdminGW as Admin Gateway (:AdminPort)
participant PubSvc as Public Services
participant AdmSvc as Admin Services
rect rgb(240,245,255)
User->>MainGW: Request on :Port
MainGW->>PubSvc: Route to public handlers
PubSvc-->>User: Response
end
rect rgb(245,240,255)
Admin->>AdminGW: Request on :AdminPort
AdminGW->>AdmSvc: Route to admin handlers
AdmSvc-->>Admin: Response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)cmd/arkd/main.go(1 hunks)internal/config/config.go(3 hunks)internal/interface/grpc/config.go(3 hunks)internal/interface/grpc/handlers/arkservice.go(1 hunks)internal/interface/grpc/service.go(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
internal/config/config.go (1)
internal/interface/grpc/config.go (1)
Config(13-21)
internal/interface/grpc/config.go (1)
internal/config/config.go (3)
AdminPort(143-143)Config(59-123)Port(142-142)
internal/interface/grpc/handlers/arkservice.go (1)
internal/core/application/types.go (1)
Service(12-43)
cmd/arkd/main.go (1)
internal/config/config.go (1)
AdminPort(143-143)
internal/interface/grpc/service.go (11)
internal/config/config.go (1)
Config(59-123)internal/interface/grpc/config.go (1)
Config(13-21)internal/interface/grpc/handlers/arkservice.go (1)
NewAppServiceHandler(31-43)api-spec/protobuf/gen/ark/v1/admin_grpc.pb.go (1)
RegisterAdminServiceServer(161-163)api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet_grpc.pb.go (1)
RegisterWalletServiceServer(543-545)api-spec/protobuf/gen/ark/v1/wallet_grpc.pb.go (1)
RegisterWalletInitializerServiceServer(119-121)api-spec/protobuf/gen/ark/v1/signer_manager_grpc.pb.go (1)
RegisterSignerManagerServiceServer(63-65)api-spec/protobuf/gen/ark/v1/admin.pb.gw.go (1)
RegisterAdminServiceHandler(466-468)api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.gw.go (1)
RegisterWalletServiceHandler(1502-1504)api-spec/protobuf/gen/ark/v1/wallet.pb.gw.go (1)
RegisterWalletInitializerServiceHandler(483-485)api-spec/protobuf/gen/ark/v1/signer_manager.pb.gw.go (1)
RegisterSignerManagerServiceHandler(121-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: integration tests
- GitHub Check: Build and Scan
- GitHub Check: unit tests
🔇 Additional comments (18)
internal/interface/grpc/handlers/arkservice.go (1)
31-31: LGTM! Constructor rename improves clarity.The rename from
NewHandlertoNewAppServiceHandlerbetter distinguishes the app service handler from admin handlers, improving API clarity without changing behavior.cmd/arkd/main.go (1)
40-40: LGTM! AdminPort correctly wired into service config.The AdminPort field is properly sourced from
cfg.AdminPortand passed to the gRPC service configuration, enabling the admin port feature.internal/config/config.go (4)
62-62: LGTM! AdminPort field added to Config.The AdminPort field is correctly added to the Config struct, enabling configuration of a separate admin port.
143-143: LGTM! AdminPort constant defined.The constant follows the existing naming pattern and enables environment variable configuration via
ARKD_ADMIN_PORT.
279-282: LGTM! AdminPort defaults to Port when not set.The fallback logic ensures backward compatibility: when
ARKD_ADMIN_PORTis not set (zero), the admin services listen on the same port as public services.
290-290: LGTM! AdminPort assigned to Config.The AdminPort value is correctly propagated into the returned Config instance.
internal/interface/grpc/config.go (4)
16-16: LGTM! AdminPort field added to gRPC Config.The AdminPort field enables separate admin port configuration at the gRPC service level.
31-39: LGTM! Admin port validation added.The validation correctly checks port availability when an admin port is configured, following the same pattern as the main port validation.
92-98: LGTM! Admin address helpers added.The
adminAddress()andadminGatewayAddress()methods follow the existing pattern and provide clean abstractions for admin port addressing.
100-102: LGTM! hasAdminPort() helper added.The helper method provides a clean way to check whether a separate admin port is configured.
internal/interface/grpc/service.go (8)
48-50: LGTM! Admin server fields added to service struct.The
adminServerandadminGrpcSrvrfields enable separate admin port support without affecting the main server.
99-109: LGTM! Service initialization updated.The service struct initialization correctly includes the new admin server fields, all initialized to nil by default.
118-120: LGTM! Admin server startup logged.The logging correctly indicates when the admin server is listening on a separate port.
157-175: LGTM! Admin server startup implemented.The admin server is correctly started alongside the main server when configured, using the same TLS settings.
188-196: LGTM! Admin server shutdown implemented.The admin server and gRPC server are correctly shut down when configured, preventing resource leaks.
270-284: LGTM! Service registration correctly implements admin port routing.The admin services (admin, wallet, wallet_initializer, signer_manager, health) are correctly registered on the admin gRPC server when a separate admin port is configured, with proper fallback to the main server for backward compatibility. Health service is correctly registered on both servers.
324-347: LGTM! Gateway registration correctly routes services.Admin gateway handlers are correctly excluded from the main gateway when a separate admin port is configured. Public services (ark, indexer) remain on the main gateway as expected.
367-419: LGTM! Admin gateway correctly implemented.The admin gateway setup correctly mirrors the main gateway structure while using a separate connection to the admin address. Only admin services are registered on the admin gateway, maintaining proper separation from public services.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.md(1 hunks)cmd/arkd/commands.go(26 hunks)cmd/arkd/flags.go(2 hunks)cmd/arkd/main.go(2 hunks)cmd/arkd/utils.go(5 hunks)docker-compose.regtest.yml(1 hunks)envs/arkd.dev.env(1 hunks)envs/arkd.light.env(1 hunks)internal/config/config.go(5 hunks)internal/interface/grpc/config.go(3 hunks)pkg/arkd-wallet-btcwallet/interface/grpc/service.go(1 hunks)pkg/arkd-wallet/interface/grpc/service.go(1 hunks)test/e2e/e2e_test.go(5 hunks)test/e2e/utils_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- envs/arkd.dev.env
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- internal/interface/grpc/config.go
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/arkd/flags.go (1)
internal/config/config.go (1)
DefaultAdminPort(184-184)
internal/config/config.go (1)
internal/interface/grpc/config.go (1)
Config(13-21)
cmd/arkd/main.go (1)
internal/config/config.go (1)
AdminPort(143-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit tests
- GitHub Check: integration tests
🔇 Additional comments (15)
envs/arkd.light.env (1)
8-8: LGTM: Explicit loopback address.The change from
localhostto127.0.0.1makes the binding more explicit and avoids potential IPv6 resolution ambiguity.docker-compose.regtest.yml (1)
74-74: LGTM: Admin port exposure for testing.Adding port 7071 correctly exposes the new admin port for the regtest environment, aligning with the admin port feature.
cmd/arkd/flags.go (3)
15-15: LGTM: Macaroon flag constant.The new constant follows the established naming convention for flag names.
39-43: LGTM: Admin port as default.Changing the default URL to point to the admin port (127.0.0.1:7071) is appropriate since the CLI commands primarily interact with admin endpoints.
49-52: LGTM: Macaroon flag definition.The flag is appropriately defined for optional authentication with a clear hex format usage description.
internal/config/config.go (6)
62-62: LGTM: Admin port field addition.The
AdminPortfield is correctly added to the publicConfigstruct.
143-143: LGTM: Environment variable constant.The constant follows the established naming pattern for environment variables.
184-184: LGTM: Default admin port value.The default value of 7071 for the admin port matches the requirement from issue #716.
216-216: LGTM: Default configuration.Setting the default for
AdminPortin the configuration loading ensures the value is always initialized.
281-285: LGTM: Backward-compatible fallback.The fallback logic ensures backward compatibility by using the service port when the admin port is not explicitly configured (zero value).
293-293: LGTM: Configuration assignment.The
AdminPortis correctly assigned to the returned configuration with the computed value (either explicit or fallback).pkg/arkd-wallet/interface/grpc/service.go (1)
153-153: LGTM: Explicit loopback for gateway.Using
127.0.0.1instead oflocalhostprovides explicit IPv4 loopback addressing and avoids potential resolution ambiguity.test/e2e/utils_test.go (1)
371-371: LGTM: Test updated for admin port.The test endpoint correctly targets the new admin port (7071) for the admin note endpoint.
cmd/arkd/commands.go (1)
152-603: LGTM: TLS config refactor.The refactor from
getCredentialPathstogetCredentials(returning*tls.Configinstead of cert path) centralizes TLS configuration construction and improves security by building the config once rather than passing paths around. The change is applied consistently across all command actions.pkg/arkd-wallet-btcwallet/interface/grpc/service.go (1)
142-142: LGTM: Explicit loopback for gateway.Using
127.0.0.1instead oflocalhostprovides explicit IPv4 loopback addressing and avoids potential resolution ambiguity, consistent with other wallet service changes.
This adds option to run Admin RPC’s on separate port.
This closes #716
@altafan please review.
Summary by CodeRabbit
New Features
Documentation
Tests