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

Ports API: Workaround for ifNames with slashes #10502

Merged
merged 2 commits into from Aug 12, 2019

Conversation

@murrant
Copy link
Member

commented Aug 7, 2019

Basically,

/api/v0/devices/3/ports/0%2f4 -> /api/v0/devices/3/ports/ifName?ifName=0/4
/api/v0/devices/3/ports/0%2f4/port_bits -> /api/v0/devices/3/ports/ifName/port_bits?ifName=0/4

Or you can just use port_id. If ifName doesn't contain / it will still work.

Also adds port_id to the default output for the ports list.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

Workaround for ifNames with slashes
Basically,

/api/v0/devices/3/ports/0%2f4 -> /api/v0/devices/3/ports/ifName?ifName=0/4
/api/v0/devices/3/ports/0%2f4/port_bits -> /api/v0/devices/3/ports/ifName/port_bits?ifName=0/4

Or you can just use port_id.
@murrant

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/api-issue-url-encoding-issue-with-port-graph/9146/7

@murrant murrant changed the title Workaround for ifNames with slashes Ports API: Workaround for ifNames with slashes Aug 9, 2019

@kate66

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

This is still not working as it was previously, the API call is getting a 404.

api.ip - - [09/Aug/2019:09:05:29 -0400] "GET /api/v0/devices/19/ports/ge-0%2F0%2F41/port_bits?from=1565269528&to=1565355928&width=1000 HTTP/1.1" 404 254 "-" "-"

@laf

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Is this backwards compatible? If not, I'd say this is a breaking change that shouldn't got in without a version bump of the API

@murrant

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

@laf it is a breaking change that we can't fix without breaking bc. Slashes in routes breaks the rfc :(. Additionally, there is a workaround for the last route segment, but we can't use that because of a collision with the other route... So bad stuff all around.

@laf

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Is this because the api is now going through Laravel I assume?

@murrant

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

Yes. I might have another work around, basically subverting the Laravel routing.

Fix API ports using ifName with slashes
hand parse the path for the ports graph endpoints
this way we can respect the original way of handling slashes (%2F)

@murrant murrant force-pushed the murrant:workaround-slashes branch from f5985ae to d11e9bd Aug 12, 2019

@murrant

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

@laf ok, should restore original functionality.

@murrant

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/api-issue-url-encoding-issue-with-port-graph/9146/14

@murrant murrant merged commit f1c67ac into librenms:master Aug 12, 2019

5 of 6 checks passed

codeclimate 3 issues to fix
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@murrant murrant deleted the murrant:workaround-slashes branch Aug 12, 2019

@laf

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Awesome. Thanks for keeping it compatible. Makes it easier at work :)

@murrant

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-55-release-changelog-august-2019/9428/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.