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.

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

refresh ziti_session.edge_routers #620

Merged
merged 7 commits into from
Feb 13, 2024
Merged

refresh ziti_session.edge_routers #620

merged 7 commits into from
Feb 13, 2024

Conversation

ekoby
Copy link
Member

@ekoby ekoby commented Feb 12, 2024

[fixes #614]

@ekoby ekoby requested a review from a team as a code owner February 12, 2024 21:44
@ekoby ekoby self-assigned this Feb 13, 2024
@@ -1339,7 +1340,8 @@ static void edge_routers_cb(ziti_edge_router_array ers, const ziti_error *err, v
// check if it is already in the list
if (model_map_remove(&curr_routers, tls) == NULL) {
ZTX_LOG(TRACE, "connecting to %s(%s)", er->name, tls);
ziti_channel_connect(ztx, er->name, tls, NULL, NULL);
ziti_channel_connect(ztx, er->name, tls);
ers_changed = true;
Copy link
Member

Choose a reason for hiding this comment

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

should this be done when the ER is successfully (re)connected?

Copy link
Member Author

Choose a reason for hiding this comment

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

that would cause (maybe a lot) more frequent session refreshes that are probably unnecessary

for (int i = 0; i < meta->field_count; i++) {
if (strncmp(meta->fields[i].path, json + tok->start, tok->end - tok->start) == 0) {
if (strlen(meta->fields[i].path) == token_len &&
Copy link
Member

Choose a reason for hiding this comment

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

This change seems behaviorally equivalent to me, but now the strncmp only happens if a match cannot be ruled out based on length. So I might think it's an optimization... but doesn't using strlen only guarantee that the entire string will be scanned once before possibly being scanned again in strncmp?

Copy link
Member Author

Choose a reason for hiding this comment

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

the issue was with an JSON having both service and serviceId field.
token(service) matched against serviceId field

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 reworked matching to avoid extra string scan

make sure path length matches token being checked
@ekoby ekoby merged commit 4271e3e into main Feb 13, 2024
13 checks passed
@ekoby ekoby deleted the session-ers-refresh branch February 13, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edge router not removed by ZET when deleted from network and timeouts
2 participants