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.

Sign up for GitHub

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

Jump to bottom

NAS-129920 / 24.10 / jsonrpc server #13979

Merged
merged 1 commit into from
Aug 15, 2024
Merged

NAS-129920 / 24.10 / jsonrpc server #13979

merged 1 commit into from
Aug 15, 2024

Conversation

themylogin
Copy link
Contributor

No description provided.

@bugclerk
Copy link
Contributor

bugclerk commented Jul 5, 2024

Jira URL: https://ixsystems.atlassian.net/browse/NAS-129920

@bugclerk bugclerk changed the title jsonrpc server NAS-129920 / 24.10 / jsonrpc server Jul 5, 2024
@bugclerk
Copy link
Contributor

bugclerk commented Jul 5, 2024

Jira URL: https://ixsystems.atlassian.net/browse/NAS-129920

@themylogin themylogin force-pushed the jsonrpc branch 3 times, most recently from 2772182 to 5364e85 Compare July 5, 2024 08:02
@anodos325
Copy link
Contributor

Are there any developer docs for this?

@anodos325
Copy link
Contributor

The FreeIPA JSON RPC server provides a principal key in RPC response. It might be nice for API consumers if we provided similarly terse key in response object indicating who operation was performed as.

@themylogin
Copy link
Contributor Author

The FreeIPA JSON RPC server provides a principal key in RPC response. It might be nice for API consumers if we provided similarly terse key in response object indicating who operation was performed as.

What will this non-standard field be useful for? Clients already know if they tried to authorize and whether they did it successfully.

@themylogin themylogin force-pushed the jsonrpc branch 14 times, most recently from 7316bab to 7ba5002 Compare July 12, 2024 10:03
@themylogin
Copy link
Contributor Author

HA tests pass: http://jenkins.eng.ixsystems.net:8080/job/master/job/api_tests/1394/

@themylogin themylogin removed the WIP label Jul 12, 2024
@yocalebo yocalebo requested a review from a team July 12, 2024 10:24
creatorcary
creatorcary requested changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@creatorcary creatorcary left a comment

Choose a reason for hiding this comment

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

Dropping my two cents here. Good work!

src/middlewared/middlewared/api/base/server/ws_handler/rpc.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/api/base/server/ws_handler/rpc.py Outdated Show resolved Hide resolved
src/middlewared/middlewared/api/base/server/ws_handler/rpc.py Show resolved Hide resolved
creatorcary
creatorcary reviewed Jul 18, 2024
View reviewed changes
src/middlewared/middlewared/api/base/server/app.py
self.authenticated_credentials: SessionManagerCredentials | None = None
self.py_exceptions = False
self.websocket = False
self.rest = False
Copy link
Contributor

@creatorcary creatorcary Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If websocket and rest are mutually exclusive, only one is necessary. It might be even better to cut both and just use isinstance(app, RpcWebSocketApp) and isinstance(app, Application).

In either case, it would be a good idea to rename Application to RestApp or something similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we could have an enum for session type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's like this for backwards compatibility with the existing code. This will be refactored when existing server code goes away.

@themylogin
Copy link
Contributor Author

http://jenkins.eng.ixsystems.net:8080/job/master/job/custom/430/

@themylogin themylogin force-pushed the jsonrpc branch 2 times, most recently from 6c9f2f6 to 51bbe9c Compare August 14, 2024 14:47
yocalebo
yocalebo approved these changes Aug 15, 2024
View reviewed changes
Copy link
Contributor

@yocalebo yocalebo left a comment

Choose a reason for hiding this comment

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

Link to test suite (1 failure that are unrelated to these changes) http://jenkins.eng.ixsystems.net:8080/job/tests/job/api_tests/123/testReport/

@themylogin themylogin merged commit b70d2c7 into master Aug 15, 2024
2 of 3 checks passed
@themylogin themylogin deleted the jsonrpc branch August 15, 2024 16:00
@bugclerk
Copy link
Contributor

JIRA ticket https://ixsystems.atlassian.net/browse/NAS-129920 is targeted to the following versions which have not received their corresponding PRs: 24.10-RC.1, 25.04

@bugclerk
Copy link
Contributor

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.

@truenas truenas locked as resolved and limited conversation to collaborators Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@creatorcary creatorcary creatorcary approved these changes

@aiden3c aiden3c aiden3c approved these changes

@anodos325 anodos325 Awaiting requested review from anodos325

@sonicaj sonicaj Awaiting requested review from sonicaj

@yocalebo yocalebo Awaiting requested review from yocalebo

Assignees
No one assigned
Labels
jira no-time-tracked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@themylogin @bugclerk @anodos325 @yocalebo @aiden3c @creatorcary