search

openziti / edge

Application-embedded connectivity and zero-trust components
Apache License 2.0
75 stars 19 forks source link

host.v1 and host.v2 are out of sync #1616

Closed plorenz closed 12 months ago

plorenz commented 1 year ago

host.v1 has listenOptions.connectTimeoutSeconds, host.v2 has listenOptions.connectTimeout.

Should allow both, preferring connectTimeout if available.

scareything commented 1 year ago

Just IMO this will be at least slightly confusing to end users, and it isn't clear how this improves the software. I personally think we should have one field.

If we do have one field, IMO it should be connectTimeoutSeconds. connectTimeout will require tunnelers that aren't written in golang to implement golang's time.Duration.

plorenz commented 1 year ago

Just IMO this will be at least slightly confusing to end users, and it isn't clear how this improves the software. I personally think we should have one field.

If we do have one field, IMO it should be connectTimeoutSeconds. connectTimeout will require tunnelers that aren't written in golang to implement golang's time.Duration.

There are other fields in the health checks that already use duration, so requirement to parse durations is already there. This was connectTimeout in host.v2 and connectTimeoutSeconds in host.v1. Host.v2 code was broken when the types were merged. We can have both types, for backwards compatibility, or we could see if anyone is even using connectTimeoutSeconds and drop it.

Or we can un-merge them and override the attribute for host.v2.

Also, connectTimeoutSeconds is not very granular. If you're hosting something on the same machine, it should be able to respond within milliseconds, and you may want sub-second connect timeouts.

plorenz commented 1 year ago

Thinking about this a bit more, I think what we should do is:

  1. Add connectTimeout support to c-sdk
  2. Document as connectTimeoutSeconds as deprecated, to be removed in x months (or x years)
  3. Add migration to migrate connectTimeoutSeconds to connectTimeout