[Asterisk-code-review] res pjsip: Use a MD5 hash for static Contact IDs (asterisk[master])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: res_pjsip: Use a MD5 hash for static Contact IDs
......................................................................


res_pjsip: Use a MD5 hash for static Contact IDs

When 90d9a70789 was merged, it mostly tested dynamic contacts created as
a result of registering a PJSIP endpoint. Contacts generated in this
fashion typically have a long alphanumeric string as their object identifier,
which maps reasonably well for StatsD. Unfortunately, this doesn't work in the
general case. StatsD treats both '.' and ':' characters as special characters.
In particular, having a ':' appear in the middle of a StatsD metric will
result in the metric being rejected.

This causes some obvious issues with SIP URIs.

The StatsD API should not be responsible for escaping the metric name passed
to it. The metric is treated as a single long string, and it would be
challenging to know what to escape in the string passed to the function.
Likewise, we don't want to escape the metric in PJSIP, as that involves
overhead that is wasted when either res_statsd isn't loaded or enabled.

This patch takes an alternative approach. The Contact ID has been changed
to be "aor@@uri_hash" instead of "aor@@uri". This (a) won't contain any of the
aforementioned special characters, (b) can be done on Contact creation,
which has minimal impact on run-time performance, and (c) also conforms to an
earlier commit that changed the ID for dynamic contacts.

The downside of this is that StatsD users will have to map SHA1 hashes back to
the Contacts that are emitting the statistics. To that end, the CLI commands
have been updated to include the first 10 characters of the MD5 hash, which
should be enough to match what is shown in Graphite (or some other StatsD
backend).

ASTERISK-25595 #close

Change-Id: Ic674a3307280365b4a45864a3571c295b48a01e2
Reported-by: Matt Jordan
Tested-by: George Joseph
---
M res/res_pjsip/location.c
1 file changed, 12 insertions(+), 7 deletions(-)

Approvals:
  Richard Mudgett: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/res/res_pjsip/location.c b/res/res_pjsip/location.c
index 4471d7c..8f540cf 100644
--- a/res/res_pjsip/location.c
+++ b/res/res_pjsip/location.c
@@ -388,7 +388,8 @@
 	while ((contact_uri = strsep(&contacts, ","))) {
 		struct ast_sip_contact *contact;
 		struct ast_sip_contact_status *status;
-		char contact_id[strlen(aor_id) + strlen(contact_uri) + 2 + 1];
+		char hash[33];
+		char contact_id[strlen(aor_id) + sizeof(hash) + 2 + 1];
 
 		if (!aor->permanent_contacts) {
 			aor->permanent_contacts = ao2_container_alloc_list(AO2_ALLOC_OPT_LOCK_NOLOCK,
@@ -398,7 +399,8 @@
 			}
 		}
 
-		snprintf(contact_id, sizeof(contact_id), "%s@@%s", aor_id, contact_uri);
+		ast_md5_hash(hash, contact_uri);
+		snprintf(contact_id, sizeof(contact_id), "%s@@%s", aor_id, hash);
 		contact = ast_sorcery_alloc(ast_sip_get_sorcery(), "contact", contact_id);
 		if (!contact) {
 			return -1;
@@ -774,12 +776,12 @@
 {
 	struct ast_sip_cli_context *context = arg;
 	int indent = CLI_INDENT_TO_SPACES(context->indent_level);
-	int filler = CLI_LAST_TABSTOP - indent - 18;
+	int filler = CLI_LAST_TABSTOP - indent - 23;
 
 	ast_assert(context->output_buffer != NULL);
 
 	ast_str_append(&context->output_buffer, 0,
-		"%*s:  <Aor/ContactUri%*.*s>  <Status....>  <RTT(ms)..>\n",
+		"%*s:  <Aor/ContactUri%*.*s> <Hash....> <Status> <RTT(ms)..>\n",
 		indent, "Contact", filler, filler, CLI_HEADER_FILLER);
 
 	return 0;
@@ -792,23 +794,26 @@
 	struct ast_sip_cli_context *context = arg;
 	int indent;
 	int flexwidth;
+	const char *contact_id = ast_sorcery_object_get_id(contact);
+	const char *hash_start = contact_id + strlen(contact->aor) + 2;
 
 	RAII_VAR(struct ast_sip_contact_status *, status,
-		ast_sorcery_retrieve_by_id( ast_sip_get_sorcery(), CONTACT_STATUS, ast_sorcery_object_get_id(contact)),
+		ast_sorcery_retrieve_by_id( ast_sip_get_sorcery(), CONTACT_STATUS, contact_id),
 		ao2_cleanup);
 
 	ast_assert(contact->uri != NULL);
 	ast_assert(context->output_buffer != NULL);
 
 	indent = CLI_INDENT_TO_SPACES(context->indent_level);
-	flexwidth = CLI_LAST_TABSTOP - indent - 2 - strlen(contact->aor) + 1;
+	flexwidth = CLI_LAST_TABSTOP - indent - 9 - strlen(contact->aor) + 1;
 
-	ast_str_append(&context->output_buffer, 0, "%*s:  %s/%-*.*s  %-12.12s  %11.3f\n",
+	ast_str_append(&context->output_buffer, 0, "%*s:  %s/%-*.*s %-10.10s %-7.7s %11.3f\n",
 		indent,
 		"Contact",
 		contact->aor,
 		flexwidth, flexwidth,
 		contact->uri,
+		hash_start,
 		ast_sip_get_contact_short_status_label(status ? status->status : UNKNOWN),
 		(status && (status->status != UNKNOWN) ? ((long long) status->rtt) / 1000.0 : NAN));
 

-- 
To view, visit https://gerrit.asterisk.org/1758
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic674a3307280365b4a45864a3571c295b48a01e2
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>