Mixed tags for a specific location, from other server/location blocks

[CezarAntohe]

Hello all,

First of all, thank you for looking into this issue.

Description - please consider the following nginx configuration example, with 2 server blocks:

server {
    listen 9191;
    server_name tracing.test1;
    access_log /var/log/api-gateway/tracing1_access.log platform;
    error_log /var/log/api-gateway/tracing1_error.log debug;

    opentracing_tag server1 "server1";

    location /tracing11 {
        opentracing_propagate_context;
        opentracing on;
        opentracing_tag location11 "location11";
        content_by_lua_block {
            ngx.status = 200
            ngx.say("OK - tracing11")
        }
    }

    location /tracing12 {
        opentracing_propagate_context;
        opentracing on;
        opentracing_tag location12 "location12";
        content_by_lua_block {
            ngx.status = 200
            ngx.say("OK - tracing12")
        }
    }
}

server {
    listen 9191;
    server_name tracing.test2;
    access_log /var/log/api-gateway/tracing2_access.log platform;
    error_log /var/log/api-gateway/tracing2_error.log debug;

    opentracing_tag server2 "server2";

    location /tracing21 {
        opentracing_propagate_context;
        opentracing on;
        opentracing_tag location21 "location21";
        content_by_lua_block {
            ngx.status = 200
            ngx.say("OK - tracing21")
        }
    }
}

When curl-ing any of the locations from either tracing.test1 or tracing.test2, the resulting traces contain tags from the other server or other location as well.
I would expect a curl to tracing.test1, location /tracing12, to generate a trace that only contains the tags server1 and location12, not the tag location11. The same happens when sending requests for tracing.test1.

Looking over the code in ngx_http_opentracing_module.cpp, function merge_opentracing_loc_conf, I believe I found the issue there. I have added a patch with a possible fix for merging module configurations. The fix is tested and works correctly. It resembles the config merging from the opentelemetry nginx module, we use them both in our company.

Here is the patch:

diff --git a/opentracing/src/ngx_http_opentracing_module.cpp b/opentracing/src/ngx_http_opentracing_module.cpp
index acadae1..8b5f503 100644
--- a/opentracing/src/ngx_http_opentracing_module.cpp
+++ b/opentracing/src/ngx_http_opentracing_module.cpp
@@ -302,12 +302,40 @@ static char *merge_opentracing_loc_conf(ngx_conf_t *, void *parent,
   if (prev->tags && !conf->tags) {
     conf->tags = prev->tags;
   } else if (prev->tags && conf->tags) {
-    std::swap(prev->tags, conf->tags);
-    auto tags = static_cast<opentracing_tag_t *>(
-        ngx_array_push_n(conf->tags, prev->tags->nelts));
-    if (!tags) return static_cast<char *>(NGX_CONF_ERROR);
-    auto prev_tags = static_cast<opentracing_tag_t *>(prev->tags->elts);
-    for (size_t i = 0; i < prev->tags->nelts; ++i) tags[i] = prev_tags[i];
+    std::unordered_map<std::string, opentracing_tag_t> merged_tags;
+
+    for (ngx_uint_t i = 0; i < prev->tags->nelts; i++) {
+      opentracing_tag_t* tag = &((opentracing_tag_t*)prev->tags->elts)[i];
+      std::string key;
+      key.assign(reinterpret_cast<const char*>(tag->key_script.pattern_.data), tag->key_script.pattern_.len);
+      merged_tags[key] = *tag;
+    }
+
+    for (ngx_uint_t i = 0; i < conf->tags->nelts; i++) {
+      opentracing_tag_t* tag = &((opentracing_tag_t*)conf->tags->elts)[i];
+      std::string key;
+      key.assign(reinterpret_cast<const char*>(tag->key_script.pattern_.data), tag->key_script.pattern_.len);
+      merged_tags[key] = *tag;
+    }
+
+    ngx_uint_t index = 0;
+    for (const auto& kv : merged_tags) {
+      if (index == conf->tags->nelts) {
+        opentracing_tag_t* tag = (opentracing_tag_t*)ngx_array_push(conf->tags);
+
+        if (!tag) {
+          return (char*)NGX_CONF_ERROR;
+        }
+
+        *tag = kv.second;
+      }
+      else {
+        opentracing_tag_t* tag = (opentracing_tag_t*)conf->tags->elts;
+        tag[index] = kv.second;
+      }
+
+      index++;
+    }
   }

Thank you again for looking at this issue, please let me know what you think.
Cheers!

[miry]

@CezarAntohe Could you please send a PR with your changes and may be add some integration tests.

[CezarAntohe]

@miry Thanks for looking into this matter. Quick question - branching from this repo is not authorised for me. What would be the procedure to open a PR? Fork the master, and push there? Or obtain permissions to branch from master and open a PR from that branch?
Cheers!

[miry]

@CezarAntohe We use default opensource approach: Fork, create branch in the fork, when it is ready github will show you the button to create PR from the branch to this repo.

insturctions

[CezarAntohe]

Thanks a lot, will open the PR as soon as possible.
Cheers!

[CezarAntohe]

Hi @miry , I've opened a PR, as per your instructions:
#173
Thanks a lot, cheers.

[miry]

Fixed in #173