Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the Accept Header to indicate that the response should be JSON #777

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

twink0r
Copy link

@twink0r twink0r commented Apr 21, 2022

Related Issue

#775

New Behavior

The Accept request-header field can be used to specify certain media types which are acceptable for the response. Accept headers can be used to indicate that the request is specifically limited to a small set of desired types, as in the case of a request for an in-line image.

Contrast to Current Behavior

Sends an Accept Header to indicate that's the client only accept JSON as return
...

Discussion: Benefits and Drawbacks

In our case this helps with our caching proxy because this strips all GET parameters, e.g. /api/docs/?format=openapi.
After the proxy strips the parameters we got a lot of HTML and the nb_inventory crashes (take a look here: #775)

Changes to the Documentation

Proposed Release Note Entry

Use the Accept Header to indicate that the response should be JSON

Double Check

  • I have read the comments and followed the CONTRIBUTING.md.
  • I have explained my PR according to the information in the comments or in a linked issue.
  • My PR targets the devel branch.

@twink0r twink0r changed the title Use the Accept Header to indicate that the response should be json Use the Accept Header to indicate that the response should be JSON Apr 21, 2022
@rodvand
Copy link
Contributor

rodvand commented Apr 22, 2022

@twink0r So it looks like this change does not go over well with the CI tests being run - any idea why?

sc68cal
sc68cal previously approved these changes Apr 22, 2022
@sc68cal
Copy link
Contributor

sc68cal commented Apr 22, 2022

  File "/tmp/ansible-test-e4138r2s/ansible/inventory/manager.py", line 290, in parse_source
    plugin.parse(self._inventory, self._loader, source, cache=cache)
  File "/tmp/ansible-test-e4138r2s/ansible/plugins/inventory/auto.py", line 58, in parse
    plugin.parse(inventory, loader, path, cache=cache)
  File "/home/runner/.ansible/collections/ansible_collections/netbox/netbox/plugins/inventory/nb_inventory.py", line 1888, in parse
    self.main()
  File "/home/runner/.ansible/collections/ansible_collections/netbox/netbox/plugins/inventory/nb_inventory.py", line 1759, in main
    self.fetch_api_docs()
  File "/home/runner/.ansible/collections/ansible_collections/netbox/netbox/plugins/inventory/nb_inventory.py", line 1387, in fetch_api_docs
    openapi = self._fetch_information(
  File "/home/runner/.ansible/collections/ansible_collections/netbox/netbox/plugins/inventory/nb_inventory.py", line 380, in _fetch_information
    raise AnsibleError(to_native(e.fp.read()))
  File "/tmp/ansible-test-e4138r2s/ansible/inventory/manager.py", line 290, in parse_source
    plugin.parse(self._inventory, self._loader, source, cache=cache)
  File "/tmp/ansible-test-e4138r2s/ansible/plugins/inventory/yaml.py", line 112, in parse
    raise AnsibleParserError('Plugin configuration YAML file, not YAML inventory')
  File "/tmp/ansible-test-e4138r2s/ansible/inventory/manager.py", line 290, in parse_source
    plugin.parse(self._inventory, self._loader, source, cache=cache)
  File "/tmp/ansible-test-e4138r2s/ansible/plugins/inventory/ini.py", line 136, in parse
    raise AnsibleParserError(e)

Looks like we need our parser to recognize that it's json?

@sc68cal sc68cal dismissed their stale review April 22, 2022 17:47

mistake

@nomaster
Copy link

I think the "Content-type" header really has to be removed; "Accept" is technically correct and sufficient.

The parser error indicates that Ansible tries to parse the returned data as YAML, which to me seems like a faulty test.

@sc68cal
Copy link
Contributor

sc68cal commented Apr 25, 2022

Yeah I see that the server returned an HTTP error 406

[WARNING]:  * Failed to parse /home/runner/.ansible/collections/ansible_collect
ions/netbox/netbox/tests/output/.tmp/integration/inventory-v2.11-vans_y50-ÅÑŚÌβ
ŁÈ/tests/integration/targets/inventory-v2.11/files/test-inventory.yml with auto
plugin: 406 Not Acceptable

https://github.com/netbox-community/ansible_modules/runs/6116117348?check_suite_focus=true#step%3A14%3A5289=

@rodvand
Copy link
Contributor

rodvand commented Apr 27, 2022

So I'm trying to run this on the NetBox demo instance with the following nb_inventory config:

plugin: netbox.netbox.nb_inventory
api_endpoint: https://demo.netbox.dev
token: 62b24bdf9ce08cfa3fb7ed15aa0d3e027a142df1

And running the ansible-inventory --list -i inventory.yml command results in this error:

Fetching: https://demo.netbox.dev/api/docs/?format=openapi
toml declined parsing /Users/rodvand/ansible_test/inventory.yml as it did not pass its verify_file() method
[WARNING]:  * Failed to parse /Users/rodvand/ansible_test/inventory.yml with auto plugin: 406 Not Acceptable

It fails the exact same way as the CI tests. Remove Content-type and just having Accept in the code does not make any difference for my install.

It would be useful to figure out your configurations which leads to this problem, because so far I have not been able to reproduce it with the demo instance.

Summarised: I think we need to work a bit more on this before merging it.

@rodvand
Copy link
Contributor

rodvand commented Apr 27, 2022

Even easier to reproduce my experience:

Works: curl -i -H "Content-Type: application/json" -H "Token: 62b24bdf9ce08cfa3fb7ed15aa0d3e027a142df1" https://demo.netbox.dev/api/docs/\?format\=openapi

Does not work:curl -i -H "Accept: application/json" -H "Content-Type: application/json" -H "Token: 62b24bdf9ce08cfa3fb7ed15aa0d3e027a142df1" https://demo.netbox.dev/api/docs/\?format\=openapi

@nomaster
Copy link

I tested the public NetBox with the same parameters as above and with some additional tests. The public NetBox behaves as it should. Note that when requesting with the 'Accept' header, with 'application/json' as well as with 'application/openapi+json', it returns JSON content.

What puzzles me a bit is the different behavior when adding the 'format=openapi' query parameter: setting it provokes a 406 response when also setting the 'Accept' header to 'application/json'. Without the query parameter, it works.

So this means that we can change the behavior of the Ansible inventory plugin to use that header. It should work correctly then.

See my results here:
https://gist.github.com/nomaster/26487448b1c9f4dbabc0b80bda27a94b

Copy link
Contributor

@rodvand rodvand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this, then we need to change the endpoint URL in the function fetching API docs. As shown in the tests done by @nomaster - https://gist.github.com/nomaster/26487448b1c9f4dbabc0b80bda27a94b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants