Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cleanup connection timers #604

Merged
merged 3 commits into from
Jan 2, 2024
Merged

Cleanup connection timers #604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d9f021
remove ziti context timeout setting
ekoby Dec 29, 2023
16ba7ab
use conn_req.dial_options.connect_timeout_seconds
ekoby Dec 29, 2023
1a1a597
check for valid dial_opts.identity and app_data
ekoby Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions inc_internal/zt_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@ struct ziti_write_req_s {

struct message_s *message;
ziti_write_cb cb;
uv_timer_t *timeout;
uint64_t start_ts;

void *ctx;
Expand Down Expand Up @@ -204,7 +203,6 @@ struct ziti_conn {
bool fin_sent;
int fin_recv; // 0 - not received, 1 - received, 2 - called app data cb
bool disconnecting;
int timeout;

TAILQ_HEAD(, message_s) in_q;
buffer *inbound;
Expand Down Expand Up @@ -303,9 +301,6 @@ struct ziti_ctx {

uint32_t conn_seq;

/* options */
int ziti_timeout;

/* context wide metrics */
uint64_t start;
rate_t up_rate;
Expand Down
22 changes: 2 additions & 20 deletions includes/ziti/ziti.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
// Copyright (c) 2022-2023. NetFoundry Inc.
// Copyright (c) 2022-2023. NetFoundry Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// You may obtain a copy of the License at
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
Expand Down Expand Up @@ -550,24 +550,6 @@ extern const ziti_identity *ziti_get_identity(ziti_context ztx);
ZITI_FUNC
extern void ziti_get_transfer_rates(ziti_context ztx, double *up, double *down);

/**
* @brief Sets connect and write timeouts(in millis).
*
* The #ZITI_DEFAULT_TIMEOUT is used if this function is not invoked prior to initializing connections. This value is only
* referenced when initializing new connections via ziti_conn_init(). Any connection initialized before this function will
* have the whatever timeout was set at the time of initialization.
*
* Note: There is no check to verify the timeout specified is not "too small". Setting this value to a very small value
* may lead to a large number of timeouts.
*
* @param ztx the Ziti Edge identity context to set a timeout on
* @param timeout the value in milliseconds of the timeout (must be > 0)
*
* @return #ZITI_OK or corresponding #ZITI_ERRORS
*/
ZITI_FUNC
extern int ziti_set_timeout(ziti_context ztx, int timeout);

/**
* @brief Shutdown Ziti Edge identity context and reclaim the memory from the provided #ziti_context.
*
Expand Down
116 changes: 37 additions & 79 deletions library/connect.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
conn->ziti_ctx->id, conn->conn_id, (int)sizeof(conn->marker), conn->marker, conn_state_str[conn->state], ##__VA_ARGS__)


#define DEFAULT_DIAL_OPTS (ziti_dial_opts){ \
.connect_timeout_seconds = ZITI_DEFAULT_TIMEOUT/1000, \
}

static const char *conn_state_str[] = {
#define state_str(ST) #ST ,
Expand All @@ -39,8 +42,7 @@
char *service_id;
ziti_net_session *session;
ziti_conn_cb cb;
ziti_dial_opts *dial_opts;
ziti_listen_opts *listen_opts;
ziti_dial_opts dial_opts;

int retry_count;
uv_timer_t *conn_timeout;
Expand Down Expand Up @@ -84,30 +86,24 @@
}
}

static ziti_dial_opts *clone_ziti_dial_opts(const ziti_dial_opts *dial_opts) {
if (dial_opts == NULL) {
ZITI_LOG(TRACE, "refuse to clone NULL dial_opts");
return NULL;
static void clone_ziti_dial_opts(ziti_dial_opts *dest, const ziti_dial_opts *dial_opts) {
*dest = DEFAULT_DIAL_OPTS;

dest->connect_timeout_seconds = dial_opts->connect_timeout_seconds;
if (dial_opts->identity != NULL && dial_opts->identity[0] != '\0') {
dest->identity = strdup(dial_opts->identity);
}
ziti_dial_opts *c = calloc(1, sizeof(ziti_dial_opts));
memcpy(c, dial_opts, sizeof(ziti_dial_opts));
if (dial_opts->identity != NULL) c->identity = strdup(dial_opts->identity);
if (dial_opts->app_data != NULL) {
c->app_data = malloc(dial_opts->app_data_sz);
c->app_data_sz = dial_opts->app_data_sz;
memcpy(c->app_data, dial_opts->app_data, dial_opts->app_data_sz);

if (dial_opts->app_data != NULL && dial_opts->app_data_sz > 0) {
dest->app_data = malloc(dial_opts->app_data_sz);
dest->app_data_sz = dial_opts->app_data_sz;
memcpy(dest->app_data, dial_opts->app_data, dial_opts->app_data_sz);
}
return c;
}

static void free_ziti_dial_opts(ziti_dial_opts *dial_opts) {
if (dial_opts == NULL) {
ZITI_LOG(TRACE, "refuse to free NULL dial_opts");
return;
}
FREE(dial_opts->identity);
FREE(dial_opts->app_data);
free(dial_opts);
}

static ziti_listen_opts *clone_ziti_listen_opts(const ziti_listen_opts *ln_opts) {
Expand Down Expand Up @@ -140,8 +136,7 @@
FREE(r->session);
}

free_ziti_dial_opts(r->dial_opts);
free_ziti_listen_opts(r->listen_opts);
free_ziti_dial_opts(&r->dial_opts);
FREE(r->service_id);
free(r);
}
Expand Down Expand Up @@ -223,12 +218,6 @@
}
CONN_LOG(TRACE, "status %d", status);

if (req->timeout != NULL) {
uv_timer_stop(req->timeout);
uv_close((uv_handle_t *) req->timeout, free_handle);
req->timeout = NULL;
}

if (status < 0) {
conn_set_state(conn, Disconnected);
CONN_LOG(DEBUG, "is now Disconnected due to write failure: %d", status);
Expand Down Expand Up @@ -342,8 +331,8 @@
CONN_LOG(WARN, "connect timeout: no suitable edge router for service[%s]", conn->service);
} else {

CONN_LOG(WARN, "failed to establish connection to service[%s] in %dms on ch[%d]",
conn->service, conn->timeout, ch->id);
CONN_LOG(WARN, "failed to establish connection to service[%s] in %ds on ch[%d]",
conn->service, conn->conn_req->dial_opts.connect_timeout_seconds, ch->id);
}
complete_conn_req(conn, ZITI_TIMEOUT);
ziti_disconnect(conn);
Expand All @@ -352,7 +341,7 @@
}
}

static int ziti_connect(struct ziti_ctx *ztx, const ziti_net_session *session, struct ziti_conn *conn) {
static int ziti_connect(struct ziti_ctx *ztx, ziti_net_session *session, struct ziti_conn *conn) {
// verify ziti context is still authorized
if (ztx->api_session == NULL) {
CONN_LOG(ERROR, "ziti context is not authenticated, cannot connect to service[%s]", conn->service);
Expand Down Expand Up @@ -495,7 +484,7 @@
req->conn_timeout = calloc(1, sizeof(uv_timer_t));
uv_timer_init(loop, req->conn_timeout);
req->conn_timeout->data = conn;
uv_timer_start(req->conn_timeout, connect_timeout, conn->timeout, 0);
uv_timer_start(req->conn_timeout, connect_timeout, req->dial_opts.connect_timeout_seconds * 1000, 0);

CONN_LOG(DEBUG, "starting %s connection for service[%s] with session[%s]",
ziti_session_types.name(req->session_type), conn->service, req->session->id);
Expand Down Expand Up @@ -531,14 +520,12 @@
req->session_type = ziti_session_types.Dial;
req->cb = conn_cb;

req->dial_opts = DEFAULT_DIAL_OPTS;
if (dial_opts != NULL) {
// clone dial_opts to survive the async request
req->dial_opts = clone_ziti_dial_opts(dial_opts);
clone_ziti_dial_opts(&req->dial_opts, dial_opts);

// override connection timeout if set in dial_opts
if (dial_opts->connect_timeout_seconds > 0) {
conn->timeout = dial_opts->connect_timeout_seconds * 1000;
}
}

conn->data_cb = data_cb;
Expand Down Expand Up @@ -567,21 +554,6 @@
return do_ziti_dial(conn, service, dial_opts, conn_cb, data_cb);
}

static void ziti_write_timeout(uv_timer_t *t) {
struct ziti_write_req_s *req = t->data;
struct ziti_conn *conn = req->conn;

req->timeout = NULL;
req->conn = NULL;

if (conn->state < Disconnected) {
conn_set_state(conn, Disconnected);
req->cb(conn, ZITI_TIMEOUT, req->ctx);
}

uv_close((uv_handle_t *) t, free_handle);
}

static void ziti_write_req(struct ziti_write_req_s *req) {
struct ziti_conn *conn = req->conn;

Expand All @@ -593,13 +565,6 @@
message *m = create_message(conn, ContentTypeStateClosed, 0);
send_message(conn, m, req);
} else {
if (req->cb) {
req->timeout = calloc(1, sizeof(uv_timer_t));
uv_timer_init(conn->ziti_ctx->loop, req->timeout);
req->timeout->data = req;
uv_timer_start(req->timeout, ziti_write_timeout, conn->timeout, 0);
}

message *m = req->message;
if (m == NULL) {
size_t total_len = req->len + (conn->encrypted ? crypto_secretstream_xchacha20poly1305_abytes() : 0);
Expand Down Expand Up @@ -697,15 +662,8 @@
uint8_t *peer_key;
bool peer_key_sent = message_get_bytes_header(msg, PublicKeyHeader, &peer_key, &peer_key_len);
if (!peer_key_sent) {
if (conn->encrypted) {
CONN_LOG(ERROR, "failed to establish crypto for encrypted service: did not receive peer key");
return ZITI_CRYPTO_FAIL;
} else {
CONN_LOG(VERBOSE, "encryption not set up: peer_key_sent[%d] conn->encrypted[%d]", (int) peer_key_sent,
(int) conn->encrypted);
// service is not required to be encrypted and hosting side did not send the key
return ZITI_OK;
}
CONN_LOG(ERROR, "failed to establish crypto for encrypted service: did not receive peer key");
return ZITI_CRYPTO_FAIL;
}

int rc = init_crypto(&conn->key_ex, &conn->key_pair, peer_key, conn->state == Accepting);
Expand Down Expand Up @@ -865,7 +823,7 @@
}
}

CATCH(crypto) {

Check warning on line 826 in library/connect.c

View workflow job for this annotation

GitHub Actions / Windows ARM64 

 unreachable code [D:\a\ziti-sdk-c\ziti-sdk-c\build\library\ziti.vcxproj]

Check warning on line 826 in library/connect.c

View workflow job for this annotation

GitHub Actions / Windows x86_64 

 unreachable code [D:\a\ziti-sdk-c\ziti-sdk-c\build\library\ziti.vcxproj]
FREE(plain_text);
conn_set_state(conn, Disconnected);
conn->data_cb(conn, NULL, ZITI_CRYPTO_FAIL);
Expand Down Expand Up @@ -1063,19 +1021,19 @@
init_key_pair(&conn->key_pair);
nheaders++;
}
if (req->dial_opts != NULL) {
if (req->dial_opts->identity != NULL) {
headers[nheaders].header_id = TerminatorIdentityHeader;
headers[nheaders].value = (uint8_t *) req->dial_opts->identity;
headers[nheaders].length = strlen(req->dial_opts->identity);
nheaders++;
}
if (req->dial_opts->app_data != NULL) {
headers[nheaders].header_id = AppDataHeader;
headers[nheaders].value = req->dial_opts->app_data;
headers[nheaders].length = req->dial_opts->app_data_sz;
nheaders++;
}

if (req->dial_opts.identity != NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth checking for empty string here too. I see a lot of configs with dial / listen options set to "".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the check in clone_dial_opts

headers[nheaders].header_id = TerminatorIdentityHeader;
headers[nheaders].value = (uint8_t *) req->dial_opts.identity;
headers[nheaders].length = strlen(req->dial_opts.identity);
nheaders++;
}

if (req->dial_opts.app_data != NULL) {
headers[nheaders].header_id = AppDataHeader;
headers[nheaders].value = req->dial_opts.app_data;
headers[nheaders].length = req->dial_opts.app_data_sz;
nheaders++;
}

req->waiter = ziti_channel_send_for_reply(ch, content_type, headers, nheaders,
Expand Down
12 changes: 0 additions & 12 deletions library/ziti.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,13 +207,13 @@
ziti_context ctx = NULL;
PREPF(ziti, ziti_errorstr);

if (options->config == NULL) {

Check warning on line 210 in library/ziti.c

View workflow job for this annotation

GitHub Actions / Linux x86_64 

'config' is deprecated: ignored, will be removed [-Wdeprecated-declarations]

Check warning on line 210 in library/ziti.c

View workflow job for this annotation

GitHub Actions / Linux ARM64 

'config' is deprecated: ignored, will be removed [-Wdeprecated-declarations]
ZITI_LOG(ERROR, "config or controller/tls has to be set");
return ZITI_INVALID_CONFIG;
}
ctx = calloc(1, sizeof(*ctx));

if (options->config != NULL) {

Check warning on line 216 in library/ziti.c

View workflow job for this annotation

GitHub Actions / Linux x86_64 

'config' is deprecated: ignored, will be removed [-Wdeprecated-declarations]
TRY(ziti, ziti_load_config(&ctx->config, options->config));
}

Expand All @@ -236,7 +236,6 @@
ctx->opts = *options;
ctx->tlsCtx = tls;
ctx->loop = loop;
ctx->ziti_timeout = ZITI_DEFAULT_TIMEOUT;
ctx->ctrl_status = ZITI_WTF;

STAILQ_INIT(&ctx->w_queue);
Expand Down Expand Up @@ -538,15 +537,6 @@
*down = metrics_rate_get(&ztx->down_rate);
}

int ziti_set_timeout(ziti_context ztx, int timeout) {
if (timeout > 0) {
ztx->ziti_timeout = timeout;
} else {
ztx->ziti_timeout = ZITI_DEFAULT_TIMEOUT;
}
return ZITI_OK;
}

static void free_ztx(uv_handle_t *h) {
ziti_context ztx = h->data;

Expand Down Expand Up @@ -758,7 +748,6 @@
NEWP(c, struct ziti_conn);
c->ziti_ctx = ztx;
c->data = data;
c->timeout = ctx->ziti_timeout;
c->conn_id = ztx->conn_seq++;

*conn = c;
Expand Down Expand Up @@ -1918,7 +1907,6 @@

ztx->tlsCtx = tls;
ztx->loop = loop;
ztx->ziti_timeout = ZITI_DEFAULT_TIMEOUT;
ztx->ctrl_status = ZITI_WTF;

STAILQ_INIT(&ztx->w_queue);
Expand Down
2 changes: 1 addition & 1 deletion tests/integ/bootstrap.exp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!expect

set timeout 20
set timeout 30
proc abort errs {
puts "test failed: $errs"
exit 2
Expand Down
Loading