-
Notifications
You must be signed in to change notification settings - Fork 80
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
fix: get tunnels working, fix tls, wf bug fix #2025
base: 02-12-fix_get_stuff_building
Are you sure you want to change the base?
fix: get tunnels working, fix tls, wf bug fix #2025
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR focuses on improving tunnel functionality, TLS certificate management, and workflow execution in the Rivet platform, with significant changes to DNS handling and security configurations.
- Reduced workflow polling interval from 120s to 5s in
/packages/common/chirp-workflow/core/src/worker.rs
, which could significantly impact system performance - Added new columns
api_cert_pem
andapi_private_key_pem
todatacenter_tls
table but missing down migration in/packages/core/services/cluster/db/cluster/migrations/20250213000643_api_cert.down.sql
- Disabled TLS certificate verification for CockroachDB and Redis in
/packages/core/services/cluster/src/workflows/server/install/install_scripts/components/rivet/worker.rs
, introducing potential security risks - Split DNS deletion into separate
api_dns_delete
andgg_dns_delete
workflows with different proxying configurations - Added HTTP/HTTPS firewall rules (ports 80/443) for worker pools but missing UDP rules present in GG pool configuration
17 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -6,7 +6,7 @@ use uuid::Uuid; | |||
use crate::{ctx::WorkflowCtx, db::DatabaseHandle, metrics, registry::RegistryHandle, utils}; | |||
|
|||
/// How often to pull workflows when polling. | |||
pub const TICK_INTERVAL: Duration = Duration::from_secs(120); | |||
pub const TICK_INTERVAL: Duration = Duration::from_secs(5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Reducing TICK_INTERVAL from 120s to 5s will significantly increase database load. Consider adding rate limiting or backoff mechanisms if many workers are running simultaneously.
|
||
/// Signifies a retryable executable entity in a workflow. For example: activity, tuple of activities (join), | ||
/// closure. | ||
#[async_trait] | ||
pub trait Executable: Send + Sized { | ||
pub trait Executable: Send + Sized + Sync { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: adding Sync bound may break existing code using non-Sync closures in workflows
let history_res = ctx | ||
.cursor() | ||
.compare_activity(self.version.unwrap_or(ctx.version()), &event_id)?; | ||
let location = ctx.cursor().current_location_for(&history_res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: potential race condition if cursor state changes between compare_activity and current_location_for calls
if let Some(inner) = self { | ||
inner.execute(&mut branch).await.map(Some) | ||
let mut branch = ctx.clone(); | ||
|
||
// Move to next event | ||
inner.shift_cursor(ctx).map_err(GlobalError::raw)?; | ||
|
||
let res = inner.execute(&mut branch).await?; | ||
|
||
Ok(Some(res)) | ||
} else { | ||
Ok(None) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Option::None case still increments cursor but doesn't create branch, could lead to cursor misalignment
branch: { | ||
let branch = ctx.clone(); | ||
$args.shift_cursor(ctx).map_err(GlobalError::raw)?; | ||
|
||
branch | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: cloning context and shifting cursor before execution could cause issues if execution fails
pub fn firewall_rules(&self) -> Vec<FirewallRule> { | ||
FirewallRule::base_rules() | ||
[ | ||
FirewallRule::base_rules(), | ||
vec![ | ||
// HTTP(S) | ||
FirewallRule { | ||
label: "http-tcp".into(), | ||
ports: "80".into(), | ||
protocol: "tcp".into(), | ||
inbound_ipv4_cidr: vec!["0.0.0.0/0".into()], | ||
inbound_ipv6_cidr: vec!["::/0".into()], | ||
}, | ||
FirewallRule { | ||
label: "https-tcp".into(), | ||
ports: "443".into(), | ||
protocol: "tcp".into(), | ||
inbound_ipv4_cidr: vec!["0.0.0.0/0".into()], | ||
inbound_ipv6_cidr: vec!["::/0".into()], | ||
}, | ||
], | ||
] | ||
.concat() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: HTTP/HTTPS firewall rules are added for TCP only, while ClusterPoolGg includes UDP rules for the same ports. Consider whether UDP rules should also be added here for consistency.
ADD COLUMN api_cert_pem TEXT, | ||
ADD COLUMN api_private_key_pem TEXT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding NOT NULL constraints with DEFAULT values or ensuring existing rows are handled in application code, as NULL certificates could cause runtime errors
if let Some(dns_record_id) = records_res.dns_record_id { | ||
ctx.activity(DeleteDnsRecordInput { | ||
dns_record_id, | ||
zone_id: zone_id.to_string(), | ||
}) | ||
.await?; | ||
} else { | ||
tracing::warn!("server has no primary dns record"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding a metric or structured log when DNS record is missing to help track frequency of this case
@@ -54,20 +54,20 @@ pub fn configure(config: &rivet_config::Config) -> GlobalResult<String> { | |||
}, | |||
cockroachdb: CockroachDb { | |||
url: Url::parse(&format!( | |||
"postgres://127.0.0.1:{TUNNEL_CRDB_PORT}/postgres?sslmode=verify-ca" | |||
"postgres://127.0.0.1:{TUNNEL_CRDB_PORT}/postgres?sslmode=require" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Changing from sslmode=verify-ca to sslmode=require disables certificate verification. This is a security downgrade that could enable MITM attacks. Consider fixing the certificate configuration instead.
))?, | ||
..server_config.cockroachdb.clone() | ||
}, | ||
redis: RedisTypes { | ||
ephemeral: Redis { | ||
url: Url::parse(&format!( | ||
"rediss://127.0.0.1:{TUNNEL_REDIS_EPHEMERAL_PORT}", | ||
"rediss://127.0.0.1:{TUNNEL_REDIS_EPHEMERAL_PORT}/#insecure", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Adding #insecure to Redis URL disables TLS certificate verification. This is a security risk that could enable MITM attacks. Consider fixing the certificate configuration instead.
290658d
to
55a330f
Compare
56f8e31
to
998ea52
Compare
55a330f
to
95ef81c
Compare
998ea52
to
70935b0
Compare
Deploying rivet-hub with
|
Latest commit: |
2e17256
|
Status: | ✅ Deploy successful! |
Preview URL: | https://c7c96a33.rivet-hub-7jb.pages.dev |
Branch Preview URL: | https://02-13-fix-get-tunnels-workin.rivet-hub-7jb.pages.dev |
95ef81c
to
08b4e1f
Compare
70935b0
to
29618f5
Compare
08b4e1f
to
d3ebd1b
Compare
61641ee
to
6a57b86
Compare
590a2ac
to
5bb2397
Compare
6a57b86
to
d8bf651
Compare
5bb2397
to
caf9cca
Compare
d8bf651
to
b9f0ded
Compare
caf9cca
to
2e17256
Compare
b9f0ded
to
d8bf651
Compare
2e17256
to
caf9cca
Compare
d8bf651
to
228ff44
Compare
caf9cca
to
20dfc77
Compare
228ff44
to
b9f0ded
Compare
20dfc77
to
2e17256
Compare
2e17256
to
caf9cca
Compare
b9f0ded
to
d8bf651
Compare
d8bf651
to
b9f0ded
Compare
caf9cca
to
2e17256
Compare
b9f0ded
to
d8bf651
Compare
2e17256
to
caf9cca
Compare
d8bf651
to
b9f0ded
Compare
caf9cca
to
2e17256
Compare
Changes