nsqadmin: work-around bug for raw ipv6 addresses #1179

andyxning · 2019-08-25T08:25:26Z

Ipv6 address in broadcast_address should be returned with leading and trilling square bracket trimmed.

When we use go-nsq connect to nsqd by using lookupd's lookup api, go-nsq will compose the address by calling net.JoinHostPort. net.JoinHostPort will assume that if it is an ipv6 address, there should be no square brackets as net.JoinHostPort will add square brackets when it detects an ipv6 address is added.

ploxiln · 2019-08-29T06:13:28Z

This doesn't seem like the right place to fix this. Maybe it should be processed when the nsqd is registered, rather than when it is looked-up?

The default broadcast-address is the hostname (and it is often set to a dns name). So I think that if you get an ipv6 address for this, it was probably configured explicitly as the broadcast-address for that nsqd? So maybe the thing to do is to specify the ipv6 address without square brackets when you specify the broadcast-address to nsqd?

(Are there other places where we combine an address and a port and do not use net.JoinHostPort()?)

andyxning · 2019-08-30T11:53:15Z

So I think that if you get an ipv6 address for this, it was probably configured explicitly as the broadcast-address for that nsqd?

Yes. That's right.

So maybe the thing to do is to specify the ipv6 address without square brackets when you specify the broadcast-address to nsqd?

I have tried this but the api like http://127.0.0.1:4171/nodes/127.0.0.1:4151(actually 127.0.0.1 should be replaced with an ipv6 ip address), but something is wrong and i got nothing. I think this is possibly related with net.JoinHostPort.

Are there other places where we combine an address and a port and do not use net.JoinHostPort()?

Definitely we can write a function by ourself. But this may not be the right thing to do since i think we need to follow the programming language spec.

We use ip directly because we do not want to rely on dns. This is mainly for stability. Aslo, colon is not a valid character in a dns name. This should not cause any ambiguity.

ploxiln · 2019-08-30T17:23:11Z

Also, colon is not a valid character in a dns name. This should not cause any ambiguity.

You're right, this way works, at least for this specific case. But it is "hacky". These values should be correct earlier in the process.

I have tried this but the api like http://127.0.0.1:4171/nodes/127.0.0.1:4151 (actually 127.0.0.1 should be replaced with an ipv6 ip address), but something is wrong and i got nothing.

This is what should be figured out.

Are there other places where we combine an address and a port and do not use net.JoinHostPort()?

Definitely we can write a function by ourself. But this may not be the right thing to do since i think we need to follow the programming language spec.

Yeah, I was saying we should find places where we concatenate host and port with something like fmt.Sprintf(), there may be a couple such places.

andyxning · 2019-09-08T10:30:12Z

@ploxiln

I have tried this but the api like http://127.0.0.1:4171/nodes/127.0.0.1:4151 (actually 127.0.0.1 should be replaced with an ipv6 ip address), but something is wrong and i got nothing.

I have figured out that nsqadmin nodes pages will use

nsq/nsqadmin/static/js/views/nodes.hbs

Line 27 in 470346f

    
           <td><a class="link" href="{{basePath "/nodes"}}/{{broadcast_address}}:{{http_port}}">{{broadcast_address}}</a></td>

broadcast_address and http_port directly.

When we set an ipv6 address as the nsqd broadcast_address, the composite node link will be http://[fdbd:dc02:8:92::85]:4171/nodes/fdbd:dc02:8:92::85:4151. This will finally make a request to http://fdbd:dc02:8:92::85:4151/stats?format=json and this will be error because ipv6 address will be surrounded by a square bracket in a http request. But i have googled that the javascript template does not support complex directives to make us adjust this on the js file. So we need to adjust the nodes responses.

robincallahan89 · 2019-09-08T22:02:47Z

Message

ploxiln · 2019-09-08T22:40:14Z

There is a right way to do it, it's just that you and I don't understand the backbonejs architecture. Here's a crazy guess, untested and probably not working:

--- a/nsqadmin/static/js/collections/nodes.js
+++ b/nsqadmin/static/js/collections/nodes.js
@@ -18,6 +18,13 @@ var Nodes = Backbone.Collection.extend({
     },
 
     parse: function(resp) {
+        resp['nodes'].forEach(function(n) {
+            var jaddr = n['broadcast_address'];
+            if (jaddr.includes(':')) {
+                jaddr = '[' + jaddr + ']';  // ipv6 addr needs []
+            }
+            n['broadcast_address_http'] = jaddr + ":" + n['http_port'];
+        });
         return resp['nodes'];
     }
 });
--- a/nsqadmin/static/js/views/nodes.hbs
+++ b/nsqadmin/static/js/views/nodes.hbs
@@ -24,7 +24,7 @@
             {{#each collection}}
             <tr {{#if out_of_date}}class="warning"{{/if}}>
                 <td>{{hostname}}</td>
-                <td><a class="link" href="{{basePath "/nodes"}}/{{broadcast_address}}:{{http_port}}">{{broadcast_address}}</a></td>
+                <td><a class="link" href="{{basePath "/nodes"}}/{{broadcast_address_http}}">{{broadcast_address}}</a></td>
                 <td>{{tcp_port}}</td>
                 <td>{{http_port}}</td>
                 <td>{{version}}</td>

ploxiln · 2019-09-08T22:51:10Z

... and another "I have no idea what I'm doing" untested idea, but again showing the kind of change that fixes the problem where it actually is:

--- a/nsqadmin/static/js/models/node.js
+++ b/nsqadmin/static/js/models/node.js
@@ -12,6 +12,14 @@ var Node = Backbone.Model.extend({ //eslint-disable-line no-undef
         return AppState.apiPath('/nodes');
     },
 
+    url: function() {
+        var jaddr = this.get('broadcast_address');
+        if (jaddr.includes(':')) {
+            jaddr = '[' + jaddr + ']';  // ipv6 addr needs []
+        }
+        return AppState.apiPath('/nodes') + '/' + encodeURIComponent(jaddr + ':' + this.get('http_port'));
+    }
+
     tombstoneTopic: function(topic) {
         return this.destroy({
--- a/nsqadmin/static/js/views/nodes.hbs
+++ b/nsqadmin/static/js/views/nodes.hbs
@@ -24,7 +24,7 @@
             {{#each collection}}
             <tr {{#if out_of_date}}class="warning"{{/if}}>
                 <td>{{hostname}}</td>
-                <td><a class="link" href="{{basePath "/nodes"}}/{{broadcast_address}}:{{http_port}}">{{broadcast_address}}</a></td>
+                <td><a class="link" href="{{url}}">{{broadcast_address}}</a></td>
                 <td>{{tcp_port}}</td>
                 <td>{{http_port}}</td>

andyxning · 2019-09-09T03:33:22Z

@ploxiln I have thought about this. But the problem is that after display the broadcast_address in [fdbd:dc02:8:92::85]:4151 format, it will be passed in nsqadmin to request http://[fdbd:dc02:8:92::85]:4171/nodes/[fdbd:dc02:8:92::85]:4151. The api behands this request will only contains nsqd broadcast address of fdbd:dc02:8:92::85. This will make the request error without changing the backend logic.

ploxiln · 2019-09-09T03:49:48Z

testing locally, http://[::1]:4171/nodes/[::1]:4151 works for me

ploxiln · 2019-09-09T03:50:24Z

(started nsqd with nsqd --broadcast-address=::1 --lookupd-tcp-address=[::1]:4160)

andyxning · 2019-09-09T09:08:38Z

testing locally, http://[::1]:4171/nodes/[::1]:4151 works for me

Yes, this will work.

(started nsqd with nsqd --broadcast-address=::1 --lookupd-tcp-address=[::1]:4160)

You mean that you can successfully request testing locally, http://[::1]:4171/nodes/[::1]:4151 works for me under above configuration? Can you open nodes page and Counter page successfully?

ploxiln · 2019-09-09T17:32:45Z

The "nodes" page works, just the link to a "node" details page does not work. But I haven't tested ipv6 broadcast addr with the graphite integration at all ...

ploxiln · 2019-09-09T17:33:44Z

(I also tested nsq_tail using nsqlookupd and getting that ipv6 broadcast address though, that worked.)

andyxning · 2019-09-10T03:04:14Z

The "nodes" page works, just the link to a "node" details page does not work.

Yes, that's what i mean. Sorry for not making myself clear.

I also tested nsq_tail using nsqlookupd and getting that ipv6 broadcast address though, that worked.

That is because you set the broadcast address to ipv6 address only without square bracket, right? Once you enclose an ipv6 broadcast address with a square bracket, you can not do this correctly.

ploxiln · 2019-09-10T03:21:23Z

... because --broadcast-address=[::1] would be wrong. the square brackets are only used when the address is combined with a port (typically in a url, but nsq often omits the scheme, so I'm not sure what to call this but "pair"). which is why this proposed fix is wrong.

andyxning · 2019-09-10T03:47:02Z

Yes. I agreed. We should only set --broadcast-address=IPV6 and adjust other components to use this. I think this PR is the right way to do this. :)

ploxiln · 2019-09-10T04:31:08Z

oh, I see this has been updated ... yeah this is better ...

This is still not the correct place. This is just where it's easier for us to make changes. But this is passing an ipv6 address to the frontend js in a strange format, to trick it to include brackets where the frontend should know to put brackets on its own.

But at least this is contained to nsqadmin, and can't further confuse other components. So this can be fixed in the frontend js later.

ploxiln · 2019-09-10T04:58:22Z

can you just update the commit title/description plz

andyxning · 2019-09-10T12:10:56Z

@ploxiln I have changed commit title and description.

But at least this is contained to nsqadmin, and can't further confuse other components. So this can be fixed in the frontend js later.

So, IIUC, you mean that we can merge this, for now?

andyxning · 2019-09-10T12:13:45Z

@ploxiln I agree that display broadcast address in [::1] format on the nodes page is not reasonable. But for now, in the situation of not so easily change the frondend js code, we can for now merge this to resolve IPV6 adoption. :)

ploxiln · 2019-09-10T16:20:48Z

Yes, planning to merge this, and figure out the frontend js stuff later.

Another point of clarification: while I haven't tried it, I suspect that ipv6 was already supported if you use DNS names instead of raw addresses. What this PR does is work-around a UI bug with raw ipv6 addresses. It does not "add ipv6 support" for the normal case of using dns names to manage your servers.

ploxiln · 2019-09-10T16:32:38Z

It is important to me to fix bugs in the right place, whenever possible. Consider if I had merged your original proposal, where you specify ipv6 address with the wrong format to nsqd --broadcast-address=... and then it is fixed up in nsqlookupd, all to work-around a bug in nsqadmin. That may affect other places that deal with ipv6 addresses, and would be a tricky mess to untangle, a year or two in the future.

ploxiln · 2019-09-10T16:38:07Z

This change breaks the individual node page for me, it gets NOT FOUND now for /nodes/[::1]:4151

andyxning · 2019-09-11T02:58:11Z

This change breaks the individual node page for me, it gets NOT FOUND now for /nodes/[::1]:4151

What is the detailed error message and have you try to debug this further?

ploxiln · 2019-09-11T03:51:06Z

it's a plain 404 in the logs, and when I switched back to master branch that page worked

ploxiln · 2019-09-11T03:54:01Z

nsqadmin/http.go

+	for _, producer := range producers {
+		// Assume it is an ipv6 address if a colon is in it
+		if strings.Contains(producer.BroadcastAddress, ":") {
+			producer.BroadcastAddress = fmt.Sprintf("[%s]", producer.BroadcastAddress)


you should not apply the work-around here, this makes producers.Search(node) just below not find anything, and result in the 404 "NODE_NOT_FOUND"

ploxiln · 2019-09-11T04:03:02Z

nsqadmin/http.go

+	for _, producer := range producers {
+		// Assume it is an ipv6 address if a colon is in it
+		if strings.Contains(producer.BroadcastAddress, ":") {
+			producer.BroadcastAddress = fmt.Sprintf("[%s]", producer.BroadcastAddress)


looks like this place is also inappropriate because this is going to be used for GetNSQDStats(), not for web UI links

andyxning force-pushed the clear_ipv6_address branch from c4285ff to 5661079 Compare August 25, 2019 08:35

andyxning force-pushed the clear_ipv6_address branch from 5661079 to 721cc67 Compare September 8, 2019 10:25

ploxiln changed the title ~~nsqlookupd: clear ipv6 address in lookupd lookup api~~ nsqadmin: add brackets to ipv6 addresses Sep 10, 2019

add ipv6 support to nsqadmin

6282c50

andyxning force-pushed the clear_ipv6_address branch from 721cc67 to 6282c50 Compare September 10, 2019 10:54

andyxning changed the title ~~nsqadmin: add brackets to ipv6 addresses~~ nsqadmin: add ipv6 support for nsqadmin Sep 10, 2019

ploxiln changed the title ~~nsqadmin: add ipv6 support for nsqadmin~~ nsqadmin: work-around UI bug for raw ipv6 addresses Sep 10, 2019

ploxiln changed the title ~~nsqadmin: work-around UI bug for raw ipv6 addresses~~ nsqadmin: work-around bug for raw ipv6 addresses Sep 10, 2019

ploxiln reviewed Sep 11, 2019

View reviewed changes

ploxiln mentioned this pull request Sep 11, 2019

nsqadmin: fix nodes list links with ipv6 addresses #1186

Merged

andyxning closed this Sep 16, 2019

andyxning deleted the clear_ipv6_address branch February 27, 2020 16:56

mreiferson added the bug label Jun 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nsqadmin: work-around bug for raw ipv6 addresses #1179

nsqadmin: work-around bug for raw ipv6 addresses #1179

andyxning commented Aug 25, 2019

ploxiln commented Aug 29, 2019 •

edited

Loading

andyxning commented Aug 30, 2019 •

edited

Loading

ploxiln commented Aug 30, 2019

andyxning commented Sep 8, 2019 •

edited

Loading

robincallahan89 commented Sep 8, 2019

ploxiln commented Sep 8, 2019

ploxiln commented Sep 8, 2019

andyxning commented Sep 9, 2019 •

edited

Loading

ploxiln commented Sep 9, 2019

ploxiln commented Sep 9, 2019

andyxning commented Sep 9, 2019

ploxiln commented Sep 9, 2019

ploxiln commented Sep 9, 2019

andyxning commented Sep 10, 2019

ploxiln commented Sep 10, 2019

andyxning commented Sep 10, 2019

ploxiln commented Sep 10, 2019

ploxiln commented Sep 10, 2019

andyxning commented Sep 10, 2019

andyxning commented Sep 10, 2019

ploxiln commented Sep 10, 2019

ploxiln commented Sep 10, 2019

ploxiln commented Sep 10, 2019 •

edited

Loading

andyxning commented Sep 11, 2019

ploxiln commented Sep 11, 2019

ploxiln Sep 11, 2019

ploxiln Sep 11, 2019

nsqadmin: work-around bug for raw ipv6 addresses #1179

nsqadmin: work-around bug for raw ipv6 addresses #1179

Conversation

andyxning commented Aug 25, 2019

ploxiln commented Aug 29, 2019 • edited Loading

andyxning commented Aug 30, 2019 • edited Loading

ploxiln commented Aug 30, 2019

andyxning commented Sep 8, 2019 • edited Loading

robincallahan89 commented Sep 8, 2019

ploxiln commented Sep 8, 2019

ploxiln commented Sep 8, 2019

andyxning commented Sep 9, 2019 • edited Loading

ploxiln commented Sep 9, 2019

ploxiln commented Sep 9, 2019

andyxning commented Sep 9, 2019

ploxiln commented Sep 9, 2019

ploxiln commented Sep 9, 2019

andyxning commented Sep 10, 2019

ploxiln commented Sep 10, 2019

andyxning commented Sep 10, 2019

ploxiln commented Sep 10, 2019

ploxiln commented Sep 10, 2019

andyxning commented Sep 10, 2019

andyxning commented Sep 10, 2019

ploxiln commented Sep 10, 2019

ploxiln commented Sep 10, 2019

ploxiln commented Sep 10, 2019 • edited Loading

andyxning commented Sep 11, 2019

ploxiln commented Sep 11, 2019

ploxiln Sep 11, 2019

Choose a reason for hiding this comment

ploxiln Sep 11, 2019

Choose a reason for hiding this comment

ploxiln commented Aug 29, 2019 •

edited

Loading

andyxning commented Aug 30, 2019 •

edited

Loading

andyxning commented Sep 8, 2019 •

edited

Loading

andyxning commented Sep 9, 2019 •

edited

Loading

ploxiln commented Sep 10, 2019 •

edited

Loading