Nested 'path' query param in console URL breaks serialproxy

I know tempest has a novnc console test, I wonder if the same is possible for ironic serial consoles in ironic CI testing so we could avoid these types of regressions in the future?

OK... I looked into this and I think I know what's going on. As Jason observed, the token nested in the 'path' parameter isn't working for the serial console. And I think I found in the code why this is, the 'path' parameter is specific to noVNC and I didn't realize this before. :(

Here is the code where 'path' is parsed out of the query string and is then appended to the url which websockify will receive [1]:

        const path = readQueryVariable('path', 'websockify');

        // | | | | | |
        // | | | Connect | | |
        // v v v v v v
        status("Connecting");
        // Build the websocket URL used to connect
        let url;
        if (window.location.protocol === "https:") {
            url = 'wss';
        } else {
            url = 'ws';
        }
        url += '://' + host;
        if(port) {
            url += ':' + port;
        }
        url += '/' + path;

So, given a url for noVNC that looks like this:

  https://foo.com/vnc_lite.html?path=%3Ftoken%3Dfoo

because of the processing in vnc_lite.html, the url the websockify server receives looks something like this:

  ...?token=foo

without any nesting. And so the parsing works fine.

But, if you're running the serial proxy and there's nothing processing the 'path' variable, the websockify server receives the original url with the token embedded in the 'path' variable and will not parse it correctly.

There are two ways we could fix this, I think.

(1) Revert much of change I2ddf0f4d768b698e980594dd67206464a9cea37b and add the 'path' variable back *only* for noVNC console access url generation

or

(2) Add additional parsing logic to nova/console/websocketproxy.py to also look for a 'token' query parameter nested in a 'path' query parameter

It seems like the right thing to do is (1) but I will ask what others think before going forward with that approach.

[1] https://github.com/novnc/noVNC/blob/v1.1.0/vnc_lite.html#L155-L174

Without knowing the full details or history behind why noVNC changed, it seems like nesting the token within the path is a hack and I agree that reducing the scope of that to only noVNC (option 1) seems ideal rather than build a hack in nova (option 2) to deal with the original hack. I don't know how simple option 1 is though.

Option 1 is simple and is how the original change should have been made. I'll put a patch together. Might just do a full revert of the original change and a redo on top of it. Because most of the original patch was changing every console type's url and now we're only going to change on (noVNC).

Also, for context, the details and history of why noVNC changed is that previously, in the vnc_lite.html file used to handle a 'token' query parameter and stash it in a cookie, which we would process on the nova console proxy side:

https://github.com/novnc/noVNC/commit/51f9f0098d306bbc67cc8e02ae547921b6f6585c#diff-1d6838e3812778e95699b90d530543a1L173

https://github.com/openstack/nova/blob/207d2c22538ddec4d82fafbc01e756c9d25f6e36/nova/console/websocketproxy.py#L168

But after that noVNC commit (v1.1.0) they stopped handling the 'token' query parameter and passing it in a cookie. So they directed us to use the 'path' query parameter instead:

https://github.com/novnc/noVNC/pull/1220

It's clear now that this is something specific to noVNC but somehow I confused it because noVNC and websockify are/used to be so closely connected.

I think the right thing to do is leave the noVNC specific handling to get_vnc_console only.

Fix proposed to branch: master
Review: https://review.opendev.org/685194

Fix proposed to branch: stable/stein
Review: https://review.opendev.org/686063

Fix proposed to branch: stable/train
Review: https://review.opendev.org/686066

Fix proposed to branch: stable/rocky
Review: https://review.opendev.org/686067

Reviewed: https://review.opendev.org/685194
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=54125a75fb056dbea115408610f90e7d6eee5139
Submitter: Zuul
Branch: master

commit 54125a75fb056dbea115408610f90e7d6eee5139
Author: melanie witt <email address hidden>
Date: Fri Sep 27 02:10:22 2019 +0000

    Reduce scope of 'path' query parameter to noVNC consoles

    This is a partial revert of commit
    9606c80402f6db20d62b689c58aa8f024183628a which added the 'path' query
    parameter to work with noVNC v1.1.0. This broke all other console types
    using websockify server (serial, spice) because the websockify server
    itself doesn't know how to handle the 'path' query parameter. It is the
    noVNC vnc_lite.html file which parses the 'path' variable and uses it
    as the url to the websockify server. So, all other console types should
    *not* be generating a console access url with a 'path' query parameter,
    only noVNC.

    Closes-Bug: #1845243

    TODO(melwitt): Figure out how to test serial and/or spice console in
    the gate

    Change-Id: I9521f21a685edc44121d75bdf534c201fa87c2d7

Reviewed: https://review.opendev.org/686066
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=e736babdfea277720634ac00fd9dc119042837b9
Submitter: Zuul
Branch: stable/train

commit e736babdfea277720634ac00fd9dc119042837b9
Author: melanie witt <email address hidden>
Date: Fri Sep 27 02:10:22 2019 +0000

    Reduce scope of 'path' query parameter to noVNC consoles

    This is a partial revert of commit
    9606c80402f6db20d62b689c58aa8f024183628a which added the 'path' query
    parameter to work with noVNC v1.1.0. This broke all other console types
    using websockify server (serial, spice) because the websockify server
    itself doesn't know how to handle the 'path' query parameter. It is the
    noVNC vnc_lite.html file which parses the 'path' variable and uses it
    as the url to the websockify server. So, all other console types should
    *not* be generating a console access url with a 'path' query parameter,
    only noVNC.

    Closes-Bug: #1845243

    TODO(melwitt): Figure out how to test serial and/or spice console in
    the gate

    Change-Id: I9521f21a685edc44121d75bdf534c201fa87c2d7
    (cherry picked from commit 54125a75fb056dbea115408610f90e7d6eee5139)

Reviewed: https://review.opendev.org/686063
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=ff29f7018fdd4fc12b6b029ebef25904f9455aa9
Submitter: Zuul
Branch: stable/stein

commit ff29f7018fdd4fc12b6b029ebef25904f9455aa9
Author: melanie witt <email address hidden>
Date: Fri Sep 27 02:10:22 2019 +0000

    Reduce scope of 'path' query parameter to noVNC consoles

    This is a partial revert of commit
    9606c80402f6db20d62b689c58aa8f024183628a which added the 'path' query
    parameter to work with noVNC v1.1.0. This broke all other console types
    using websockify server (serial, spice) because the websockify server
    itself doesn't know how to handle the 'path' query parameter. It is the
    noVNC vnc_lite.html file which parses the 'path' variable and uses it
    as the url to the websockify server. So, all other console types should
    *not* be generating a console access url with a 'path' query parameter,
    only noVNC.

    Closes-Bug: #1845243

    TODO(melwitt): Figure out how to test serial and/or spice console in
    the gate

    NOTE(melwitt): The difference in nova/tests/unit/console/test_websocketproxy.py
    from the Train change is because change
    I0498599fd636aa9e30df932f0d893db5efa23260 is not in Stein.

    Change-Id: I9521f21a685edc44121d75bdf534c201fa87c2d7
    (cherry picked from commit 54125a75fb056dbea115408610f90e7d6eee5139)
    (cherry picked from commit e736babdfea277720634ac00fd9dc119042837b9)

This issue was fixed in the openstack/nova 19.0.3 release.

Reviewed: https://review.opendev.org/686067
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=f3eac7232cabe139ad34945b3be021af1f0d1599
Submitter: Zuul
Branch: stable/rocky

commit f3eac7232cabe139ad34945b3be021af1f0d1599
Author: melanie witt <email address hidden>
Date: Fri Sep 27 02:10:22 2019 +0000

    Reduce scope of 'path' query parameter to noVNC consoles

    This is a partial revert of commit
    9606c80402f6db20d62b689c58aa8f024183628a which added the 'path' query
    parameter to work with noVNC v1.1.0. This broke all other console types
    using websockify server (serial, spice) because the websockify server
    itself doesn't know how to handle the 'path' query parameter. It is the
    noVNC vnc_lite.html file which parses the 'path' variable and uses it
    as the url to the websockify server. So, all other console types should
    *not* be generating a console access url with a 'path' query parameter,
    only noVNC.

    Closes-Bug: #1845243

    TODO(melwitt): Figure out how to test serial and/or spice console in
    the gate

     Conflicts:
     nova/tests/unit/console/test_websocketproxy.py

    NOTE(melwitt): The conflict is because change
    I7f5f08691ca3f73073c66c29dddb996fb2c2b266 is not in Rocky.

    Change-Id: I9521f21a685edc44121d75bdf534c201fa87c2d7
    (cherry picked from commit 54125a75fb056dbea115408610f90e7d6eee5139)
    (cherry picked from commit e736babdfea277720634ac00fd9dc119042837b9)
    (cherry picked from commit ff29f7018fdd4fc12b6b029ebef25904f9455aa9)

This issue was fixed in the openstack/nova 20.0.0.0rc2 release candidate.

This issue was fixed in the openstack/nova 18.2.3 release.

Fix proposed to branch: stable/queens
Review: https://review.opendev.org/692581

Change abandoned by Lee Yarwood (<email address hidden>) on branch: stable/queens
Review: https://review.opendev.org/692581