broker mode schema changes - #3509
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR extends the transport configuration schema to add a ChangesBroker Clustering Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Confidence Score: 5/5Schema-only change that adds new optional fields with no breaking modifications to existing cluster config; existing mesh configs are unaffected. The change is purely additive to a JSON Schema definition. Existing No files require special attention beyond Important Files Changed
Reviews (2): Last reviewed commit: "broker mode schema changes" | Re-trigger Greptile |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
transports/config.schema.json (1)
3421-3446: ⚡ Quick winConsider adding conditional validation and aligning defaults.
A few observations on the broker object:
auth_token: Other sensitive fields in this schema documentenv.VAR_NAMEsupport (e.g.,proxy_config.password). Consider adding this to the description to discourage hardcoding secrets.
listen_port: The description says "(default: 50051)" but there's nodefaultproperty in the schema. This inconsistency could cause tooling or documentation drift.Conditional requirement: When
type: "broker", thebrokerobject should likely be required. Consider adding anif/thenblock (similar toscim_config) to enforce this at schema level.Suggested improvements
"broker": { "type": "object", "description": "Broker settings, used when type is 'broker'", "properties": { "address": { "type": "string", "description": "host:port of the broker that nodes dial" }, "tls": { "type": "boolean", - "description": "Whether to dial the broker over TLS" + "description": "Whether to dial the broker over TLS", + "default": false }, "auth_token": { "type": "string", - "description": "Optional shared secret sent on connect" + "description": "Optional shared secret sent on connect. Supports env.VAR_NAME for environment variable reference." }, "listen_port": { "type": "integer", "minimum": 1, "maximum": 65535, - "description": "Port the broker process serves on (default: 50051)" + "description": "Port the broker process serves on", + "default": 50051 } }, "required": ["address"], "additionalProperties": false }For conditional validation, add at the end of
cluster_config:"if": { "properties": { "type": { "const": "broker" } }, "required": ["type"] }, "then": { "required": ["broker"] }As per coding guidelines, config.schema.json is the authoritative source—ensuring schema completeness helps keep handlers and docs aligned.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/config.schema.json` around lines 3421 - 3446, The broker object schema is missing sensitive-env guidance, a declared default for listen_port, and conditional validation to require broker when type is "broker"; update the broker.properties.auth_token description to mention env.VAR_NAME support (like proxy_config.password), add "default": 50051 to broker.properties.listen_port, and add an if/then block under the parent cluster_config (mirroring scim_config) that checks if properties.type const "broker" and then requires ["broker"] so the broker object is mandatory when type is "broker".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@transports/config.schema.json`:
- Around line 3421-3446: The broker object schema is missing sensitive-env
guidance, a declared default for listen_port, and conditional validation to
require broker when type is "broker"; update the broker.properties.auth_token
description to mention env.VAR_NAME support (like proxy_config.password), add
"default": 50051 to broker.properties.listen_port, and add an if/then block
under the parent cluster_config (mirroring scim_config) that checks if
properties.type const "broker" and then requires ["broker"] so the broker object
is mandatory when type is "broker".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd659c46-a604-4ce3-99a6-c6a1b146c227
📒 Files selected for processing (1)
transports/config.schema.json
99651d2 to
38a7d5f
Compare
4de215a to
35f562f
Compare
Merge activity
|
The base branch was changed.
## Summary
Adds a `broker` clustering mode to the transport configuration schema, enabling deployments on platforms that lack peer-to-peer connectivity (e.g. Cloud Run) to route cluster traffic through a central broker process instead of using the default memberlist gossip mesh.
## Changes
- Added a `type` field to the cluster config with two options: `"mesh"` (default, existing peer-to-peer behavior via memberlist gossip) and `"broker"` (routes all cluster traffic through a central broker process).
- Added a `broker` config object with the following fields:
- `address` (required): `host:port` of the broker that nodes dial.
- `tls`: Whether to dial the broker over TLS.
- `auth_token`: Optional shared secret sent on connect.
- `listen_port`: Port the broker process serves on (default: 50051).
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
Validate the schema accepts and rejects configurations as expected:
```sh
# Verify schema changes are valid
go test ./transports/...
```
Example valid broker config:
```json
{
"enabled": true,
"type": "broker",
"broker": {
"address": "broker.internal:50051",
"tls": true,
"auth_token": "secret",
"listen_port": 50051
}
}
```
- `type: "mesh"` should continue to work without any `broker` block.
- `type: "broker"` without an `address` field should fail schema validation.
- `broker` block with unknown fields should fail due to `additionalProperties: false`.
## Breaking changes
- [ ] Yes
- [x] No
## Security considerations
The `auth_token` field is a shared secret transmitted on broker connection. Ensure this value is stored and passed securely (e.g. via environment variable or secrets manager) and that TLS is enabled when using `auth_token` over untrusted networks.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary
Adds a `broker` clustering mode to the transport configuration schema, enabling deployments on platforms that lack peer-to-peer connectivity (e.g. Cloud Run) to route cluster traffic through a central broker process instead of using the default memberlist gossip mesh.
## Changes
- Added a `type` field to the cluster config with two options: `"mesh"` (default, existing peer-to-peer behavior via memberlist gossip) and `"broker"` (routes all cluster traffic through a central broker process).
- Added a `broker` config object with the following fields:
- `address` (required): `host:port` of the broker that nodes dial.
- `tls`: Whether to dial the broker over TLS.
- `auth_token`: Optional shared secret sent on connect.
- `listen_port`: Port the broker process serves on (default: 50051).
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
Validate the schema accepts and rejects configurations as expected:
```sh
# Verify schema changes are valid
go test ./transports/...
```
Example valid broker config:
```json
{
"enabled": true,
"type": "broker",
"broker": {
"address": "broker.internal:50051",
"tls": true,
"auth_token": "secret",
"listen_port": 50051
}
}
```
- `type: "mesh"` should continue to work without any `broker` block.
- `type: "broker"` without an `address` field should fail schema validation.
- `broker` block with unknown fields should fail due to `additionalProperties: false`.
## Breaking changes
- [ ] Yes
- [x] No
## Security considerations
The `auth_token` field is a shared secret transmitted on broker connection. Ensure this value is stored and passed securely (e.g. via environment variable or secrets manager) and that TLS is enabled when using `auth_token` over untrusted networks.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary
Adds a `broker` clustering mode to the transport configuration schema, enabling deployments on platforms that lack peer-to-peer connectivity (e.g. Cloud Run) to route cluster traffic through a central broker process instead of using the default memberlist gossip mesh.
## Changes
- Added a `type` field to the cluster config with two options: `"mesh"` (default, existing peer-to-peer behavior via memberlist gossip) and `"broker"` (routes all cluster traffic through a central broker process).
- Added a `broker` config object with the following fields:
- `address` (required): `host:port` of the broker that nodes dial.
- `tls`: Whether to dial the broker over TLS.
- `auth_token`: Optional shared secret sent on connect.
- `listen_port`: Port the broker process serves on (default: 50051).
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
Validate the schema accepts and rejects configurations as expected:
```sh
# Verify schema changes are valid
go test ./transports/...
```
Example valid broker config:
```json
{
"enabled": true,
"type": "broker",
"broker": {
"address": "broker.internal:50051",
"tls": true,
"auth_token": "secret",
"listen_port": 50051
}
}
```
- `type: "mesh"` should continue to work without any `broker` block.
- `type: "broker"` without an `address` field should fail schema validation.
- `broker` block with unknown fields should fail due to `additionalProperties: false`.
## Breaking changes
- [ ] Yes
- [x] No
## Security considerations
The `auth_token` field is a shared secret transmitted on broker connection. Ensure this value is stored and passed securely (e.g. via environment variable or secrets manager) and that TLS is enabled when using `auth_token` over untrusted networks.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary
Adds a `broker` clustering mode to the transport configuration schema, enabling deployments on platforms that lack peer-to-peer connectivity (e.g. Cloud Run) to route cluster traffic through a central broker process instead of using the default memberlist gossip mesh.
## Changes
- Added a `type` field to the cluster config with two options: `"mesh"` (default, existing peer-to-peer behavior via memberlist gossip) and `"broker"` (routes all cluster traffic through a central broker process).
- Added a `broker` config object with the following fields:
- `address` (required): `host:port` of the broker that nodes dial.
- `tls`: Whether to dial the broker over TLS.
- `auth_token`: Optional shared secret sent on connect.
- `listen_port`: Port the broker process serves on (default: 50051).
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
Validate the schema accepts and rejects configurations as expected:
```sh
# Verify schema changes are valid
go test ./transports/...
```
Example valid broker config:
```json
{
"enabled": true,
"type": "broker",
"broker": {
"address": "broker.internal:50051",
"tls": true,
"auth_token": "secret",
"listen_port": 50051
}
}
```
- `type: "mesh"` should continue to work without any `broker` block.
- `type: "broker"` without an `address` field should fail schema validation.
- `broker` block with unknown fields should fail due to `additionalProperties: false`.
## Breaking changes
- [ ] Yes
- [x] No
## Security considerations
The `auth_token` field is a shared secret transmitted on broker connection. Ensure this value is stored and passed securely (e.g. via environment variable or secrets manager) and that TLS is enabled when using `auth_token` over untrusted networks.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable

Summary
Adds a
brokerclustering mode to the transport configuration schema, enabling deployments on platforms that lack peer-to-peer connectivity (e.g. Cloud Run) to route cluster traffic through a central broker process instead of using the default memberlist gossip mesh.Changes
typefield to the cluster config with two options:"mesh"(default, existing peer-to-peer behavior via memberlist gossip) and"broker"(routes all cluster traffic through a central broker process).brokerconfig object with the following fields:address(required):host:portof the broker that nodes dial.tls: Whether to dial the broker over TLS.auth_token: Optional shared secret sent on connect.listen_port: Port the broker process serves on (default: 50051).Type of change
Affected areas
How to test
Validate the schema accepts and rejects configurations as expected:
Example valid broker config:
{ "enabled": true, "type": "broker", "broker": { "address": "broker.internal:50051", "tls": true, "auth_token": "secret", "listen_port": 50051 } }type: "mesh"should continue to work without anybrokerblock.type: "broker"without anaddressfield should fail schema validation.brokerblock with unknown fields should fail due toadditionalProperties: false.Breaking changes
Security considerations
The
auth_tokenfield is a shared secret transmitted on broker connection. Ensure this value is stored and passed securely (e.g. via environment variable or secrets manager) and that TLS is enabled when usingauth_tokenover untrusted networks.Checklist
docs/contributing/README.mdand followed the guidelines