search

DMTF / Redfish-Service-Validator

The Redfish Service Validator is a Python3 tool for checking conformance of any "device" with a Redfish service interface against Redfish CSDL schema
Other
37 stars 33 forks source link

Added back flags to ignore URI checking for resources referenced by payload annotations #543

Closed mraineri closed 1 year ago

mraineri commented 1 year ago

Fix #542

mraineri commented 1 year ago

@tomasg2012 it may be worth noting that this doesn't seem to trigger an error consistently. For example, in public-rackmount1, the settings resource "/redfish/v1/Managers/BMC/EthernetInterfaces/eth0/SD" was not producing an error, but when I changed the mockup to be "/redfish/v1/Managers/BMC/EthernetInterfaces/eth0/Pending", I started seeing the same error reported by others.

mraineri commented 1 year ago

@JojoWu19 would you be able to try this fix on your system? I was able to reproduce it when I changed some mockups to use "Pending" as mentioned above.

tomasg2012 commented 1 year ago

I'm a little worried this code addition will introduce inconsistencies in the logic, as described in the PR that removes them. While this will fix this annotation bug, the inconsistency will be that it will not validate some URIs at all, falsely detecting them as annotations.

Do you think I could suggest a better change that accurately detects what an annotation would be? Any more examples of annotation links that would be consistent would be nice, and any Specification details on why these URIs are not to be validated. The fact that any link could be an annotation, and URIs are validated on a first-come basis means the current method is not consistent.

*edited for extra details

mraineri commented 1 year ago

As long as inAnnotation is set to true only if the URI is discovered inside of @Redfish.Settings, @Redfish.CollectionCapabilities, or @Redfish.ActionInfo, then the logic should be correct. Those three objects are the only annotations that contain special supplemental resources. If we do add more in the future, those would be part of this exception too (essentially if you found a resource from inside of an object named @Redfish.<Foo>, then do not validate the URI).

Some examples in existing mockups (from https://www.dmtf.org/sites/default/files/standards/documents/DSP2043_2022.3.zip):

mraineri commented 1 year ago

Added some breadcrumbs and it's not working the way I expected... I see it go into that if statement for most resources, such as /redfish/v1/Managers/BMC in public-rackmount1 (and it's certainly not found inside an annotation...)

mraineri commented 1 year ago

@tomasg2012 the change I just pushed seems to get things in the right direction where inAnnotation is set to true if the link is discovered inside of one of the listed payload annotations. Need to do more testing with @Redfish.CollectionCapabilities since that has additional links, but the URIs listed in my trails for when it steps into the if statement to disable URI checking in public-rackmount1 are limited to the URIs I listed above (and similar URIs found in annotations).

mraineri commented 1 year ago

As for the reason as to why they're not supposed to be validated in the first place is because we don't want to potentially quadruple the size of the openapi.yaml file we publish; OpenAPI documents all URIs, and payload annotations can theoretically be in any resource. So if we want to be super pedantic, we'd be adding a ton of bloat for little value, since it's expected clients will always be walking the resource itself to find these special annotation and never going directly to them based on an OpenAPI definition.

We have this statement in the Redfish spec:

All other Redfish service-supported URIs shall match the resource URI patterns definitions, except the supplemental resources that the @Redfish.Settings, @Redfish.ActionInfo, and @Redfish.CollectionCapabilities payload annotations reference.

tomasg2012 commented 1 year ago

This looks good now, but additional testing should help, thanks for catching this oversight.

JojoWu19 commented 1 year ago

@JojoWu19 would you be able to try this fix on your system? I was able to reproduce it when I changed some mockups to use "Pending" as mentioned above.

@mraineri , After trying this branch, there is no any error about checking URI, even the URI is incorrect, for example: 1 failRedfishUriStrict errors in /redfish/v1/Systems/1/Oem/Abcd/AbcdResource Because there is no URIs be defined in the schema of AbcdResource. Should this be reported an error?

mraineri commented 1 year ago

If there are no URIs defined, there is nothing to test against. I would only expect an error if the schema contains one or more URI patterns, and the resource doesn't match one of the patterns.

mraineri commented 1 year ago

Did some testing with the CollectionCapabilities term; it does not go deep into the annotation objects to mark the URIs in "Links" as "inAnnotation", which is good! Tried other mockups and other variations of payload annotations with supplemental resources and it's accurately marking them.