webui-vue hardcodes redfish addresses

[edtanous]

Describe the bug webui-vue hardcodes Redfish addresses, in such a way that is not compliant with the spec.

To Reproduce Edit bmcweb to change the name of the chassis handler from bmcweb. Attempt to load, and observe a 404 and error.

https://github.com/openbmc/webui-vue/blob/54c6bfc2d40def4db1bc3a13ab92ee71091b1e4f/src/store/modules/Health/ChassisStore.js#L41

Is one such place where a redfish path is hardcoded.

https://github.com/openbmc/webui-vue/blob/5918b48a0530a43a4dd9ee1a3f134846c948011e/src/store/modules/Health/PowerSupplyStore.js#L43

Is another.

https://github.com/openbmc/webui-vue/blob/61859097d8a129de1d8292f3ebde5e9228d82838/src/store/modules/Control/VirtualMediaStore.js#L40

Is another. This one is especially bad, because virtual media is optionally compiled, so if someone compiles without virtual media, I suspect this will break.

Considering an OpenBMC system might take many forms, some without a main chassis, and bmcweb at some point may need to move away from hardcoded path names to support etags, we should not be hardcoding them in the Redfish client. We should instead be following the hypermedia links as Redfish prescribes, and have bail outs if the functionality doesn't exit.

[edtanous]

Since this bug was written, we seem to have checked in more hardcoded resources. A quick code search shows 3 pages of them present in this repo now. Considering this is explicitly against the Redfish specification, and we seem to be moving in the wrong direction in that regard. Any update from the maintainers (or authors of those patches) on when this will get resolved?

There will likely be work in progress soon in redfish that adjusts the odata IDs to allow for ETag caching. I would prefer not to break webui-vue when that work happens, but considering I don't use the UI on my systems, its somewhat hard to test that.

[gtmills]

Query parameters would help the GUI a lot here. E.g. Calling $expand=1 on the Chassis collection to get all the Chassis. or a combination of $expand and $select to get the power / thermal resources

[edtanous]

Considering $expand isn't required by the specification, technically webui-vue would then need to implement both arbitrary tree walking and $expand to be compliant. Is a "walk the tree and expand" javascript helper method really that difficult to write? It seems like it should be ~60 lines of code to dump a specific section of the tree, then, when bmcweb gets $expand support (which is pretty involved), that helper method could simply call into $expand when its supported to get the same result.

FWIW, I built a quick and dirty python script that dumps the tree in the way you'd probably want to replicate in javascript, and it took about 60 lines of code. I would expect the js to be similar in size.

import requests
from collections import defaultdict
import json

import urllib3
urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning)

SERVER = "https://192.168.7.2"

def get_uri(path):
    r = requests.get(SERVER + path, auth=("root", "0penBmc"), verify=False)
    r.raise_for_status()
    return r.json()

def get_tree(path):
    elements = path.split("/")
    if elements[0] != "" or elements[1] != "redfish" or elements[2] != "v1":
        return
    elements.pop(0)
    elements.pop(0)
    elements.pop(0)

    # list of work, uri to get, location in the redfish tree to place it, and remaining elements that still need to be resolved
    current_uris = [("/redfish/v1", "/redfish/v1", elements)]
    result = {}
    while len(current_uris) != 0:
        this_uri = current_uris[0][0]
        j = get_uri(this_uri)
        this_elements = current_uris[0][2]
        this_id = current_uris[0][1]
        current_dict = result
        for this_split in this_id.split("/")[1:-1]:
            if not this_split in current_dict:
                current_dict[this_split] = {}
            current_dict = current_dict[this_split]
        last_id_element = this_id.split("/")[-1]
        if isinstance(current_dict, list):
            last_id_element = int(last_id_element)
        current_dict[last_id_element] = j

        current_uris.pop(0)

        if len(this_elements) == 0:
            continue
        this_element = this_elements[0]
        if this_element.endswith("*"):
            # special case for collections
            for e_index, element in enumerate(j[this_element[:-1]]):
                odata_id = element["@odata.id"]
                resultpath = this_id + "/" + this_element[:-1] + "/" + str(e_index)
                current_uris.append((odata_id, resultpath, this_elements[1:]))
        else:
            odata_id = j[this_element]["@odata.id"]
            current_uris.append((odata_id, this_id + "/" + this_element, this_elements[1:]))
        if this_elements[0] == "*":
            pass
    return result

def printer(string):
    print(json.dumps(string, indent=2))

if __name__ == "__main__":
    printer(get_tree("/redfish/v1/AccountService/Roles"))
    printer(get_tree("/redfish/v1/AccountService/Roles/Members*"))

[derick-montague]

Based on conversation at today's GUI Design Work Group, the first step is fixing a bug with Chasis and the Manage Power Operations page. Intel is going to be working on that and sharing up in Gerrit. I will also document in the Work Group meeting minutes that we agreed to not add any more hardcoded endpoints and to start using the odata links.

[edtanous]

Any progress on this? I would happy to help make this a higher priority for the maintainers by obfuscating the bmcweb URI identifiers, but I suspect it'd be better for everyone for the authors of the incorrect webui-vue patches to put up fixes ahead of time. We're rounding 11 months since this bug was opened, and thusfar we've only had negative progress, with more hardcoded identifiers in webui-vue than when this bug was first opened.

[gtmills]

/redfish/v1/Chassis

This is the Chassis Collection and the URI is defined here https://redfish.dmtf.org/schemas/v1/ChassisCollection_v1.xml

<Annotation Term="Redfish.Uris">
<Collection>
<String>/redfish/v1/Chassis</String>
</Collection>

Although webui-vue could start at the service root and walk the tree from there, I think hardcoding the Chassis collection, Manager collection, and ComputerSystem collection is a lot lower priority / a lot less of a violation than hardcoding Ids, e.g. '/redfish/v1/Chassis/chassis/Power' or '/redfish/v1/Managers/bmc/VirtualMedia'.

'/redfish/v1/Managers/bmc/VirtualMedia'

Manager and ComputerSystem Ids are hardcoded in bmcweb, webui-vue should still not rely on these (I would like to see webui-vue not require bmcweb but there is also other things we need to fix) but I think hardcoding the ChassisId is the worst offender here and should be fixed as soon as possible since the ChassisId differs between vendors/systems. '/redfish/v1/Chassis/chassis/Power'

https://github.com/openbmc/webui-vue/search?q=chassis

There is a commit here that does this for PowerControl, https://gerrit.openbmc-project.xyz/c/openbmc/webui-vue/+/44850 but we need more discussion around which chassis should have the Power Control because the commit as is, just uses the first. :/

I don't see any commits to fix ChassisId for Fans, etc. @mszopinx Are you planning on doing that as well?

[edtanous]

we agreed to not add any more hardcoded endpoints and to start using the odata links.

That's great news, and I think will help in the long run.

Although webui-vue could start at the service root and walk the tree from there, I think hardcoding the Chassis collection, Manager collection, and ComputerSystem collection is a lot lower priority / a lot less of a violation than hardcoding Ids, e.g. '/redfish/v1/Chassis/chassis/Power' or '/redfish/v1/Managers/bmc/VirtualMedia'.

While I don't disagree with you about the priorities, I think it's somewhat missing the greater design that Redfish is itself a tree, with various views into it, rather than just looking at it from a request/response perspective. If each individual page is manipulating the view ports, this means that as we go into the future and add $expand/$filter support, caching, and other tree manipulation stuff, each individual page will need to be updated with new logic. This stops scaling in a hurry as the webui gets bigger, and leads to individual pages having system level assumptions. The best solution to this at an internal API level that I've seen proposed is the redpath API I implemented above in the python script, and what libredfish uses, which requires walking the full tree from service root.

Also, on a real note parsing service root allows us to make sure that nonexistance is handled properly in the code, because they'll have to index into the tree, and handle the case where chassis service doesn't exist.

[leiyu-bytedance]

There is https://gerrit.openbmc-project.xyz/c/openbmc/webui-vue/+/44850 trying to fix the chassis case. It got two +1s but the maintainer had a concern about the change

[derick-montague]

There is https://gerrit.openbmc-project.xyz/c/openbmc/webui-vue/+/44850 trying to fix the chassis case. It got two +1s but the maintainer had a concern about the change

The hard coded path for Chasisis is updated, but then the there are two concerns:

1. Targeting the first item in the collection
2. Hard coding the path to Power

.get(`${collection[0]}/Power`)

[dixsie]

POC: 47141: Remove hardcoded api urls | https://gerrit.openbmc-project.xyz/c/openbmc/webui-vue/+/47141