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

feat(forwarder): add interfaces to handle a remote terminal session #63

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

rgallor
Copy link
Contributor

@rgallor rgallor commented Sep 27, 2023

Two interfaces has been added to allow managing a remote terminal session:

  • io.edgehog.devicemanager.RemoteTerminalRequest: a server-owned interface carrying the configuration data necessary to open a remote terminal from a device to a certain host
  • io.edgehog.devicemanager.ForwarderSessionsState: a device-owned interface used by a device to provide information about the status of a remote terminal session (connected / disconnected)

close #62

Copy link
Collaborator

@harlem88 harlem88 left a comment

Choose a reason for hiding this comment

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

I'm not sure but maybe we could add other 2 endpoints for forward_port and protocol_type and call the interface ...DeviceForwarderRequest, so we could use one interface for both RemoteTerminal and DeviceForwarder feature. What do you think?

io.edgehog.devicemanager.RemoteTerminal.json Outdated Show resolved Hide resolved
io.edgehog.devicemanager.RemoteTerminal.json Outdated Show resolved Hide resolved
io.edgehog.devicemanager.RemoteTerminal.json Outdated Show resolved Hide resolved
io.edgehog.devicemanager.RemoteTerminal.json Outdated Show resolved Hide resolved
io.edgehog.devicemanager.RemoteTerminal.json Outdated Show resolved Hide resolved
io.edgehog.devicemanager.RemoteTerminal.json Outdated Show resolved Hide resolved
@harlem88
Copy link
Collaborator

harlem88 commented Oct 4, 2023

Linking the pull request to #62 in the description of PR.

close #62 

@rgallor rgallor changed the title feat(terminal): add interface to open a remote terminal feat(terminal): add interfaces to handle a remote terminal session Nov 30, 2023
@rgallor rgallor changed the title feat(terminal): add interfaces to handle a remote terminal session feat(forwarder): add interfaces to handle a remote terminal session Nov 30, 2023
@rbino rbino requested a review from szakhlypa November 30, 2023 10:40
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any chance we may need to unset session_token with "allow_unset": true?

Copy link
Contributor Author

@rgallor rgallor Nov 30, 2023

Choose a reason for hiding this comment

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

From my point of view, the device will either be disconnected (false) or connected (true) in a remote terminal session, so I didn't consider using the allow_unset option.

io.edgehog.devicemanager.ForwarderSessionsState.json Outdated Show resolved Hide resolved
io.edgehog.devicemanager.RemoteTerminalRequest.json Outdated Show resolved Hide resolved
io.edgehog.devicemanager.RemoteTerminalRequest.json Outdated Show resolved Hide resolved
io.edgehog.devicemanager.RemoteTerminalRequest.json Outdated Show resolved Hide resolved
@szakhlypa
Copy link
Contributor

Do we want to have some error reporting? E.g. when device received RemoteTerminalRequest with hostname it cannot resolve.

@rgallor
Copy link
Contributor Author

rgallor commented Nov 30, 2023

Do we want to have some error reporting? E.g. when device received RemoteTerminalRequest with hostname it cannot resolve.

At the moment only HTTP client errors cause the device to stop attempting to connect with the bridge, but no information is reported (since no interfaces to do so have been defined). @harlem88 @rbino do you think it could be a good idea to introduce this information exchange?

@harlem88
Copy link
Collaborator

harlem88 commented Dec 4, 2023

Do we want to have some error reporting? E.g. when device received RemoteTerminalRequest with hostname it cannot resolve.

At the moment only HTTP client errors cause the device to stop attempting to connect with the bridge, but no information is reported (since no interfaces to do so have been defined). @harlem88 @rbino do you think it could be a good idea to introduce this information exchange?

I think it is a good idea but for the first forwarder implementation we will keep it basic, so we will introduce the error reporting later.

Signed-off-by: rgallor <riccardo.gallo@secomind.com>
Signed-off-by: rgallor <riccardo.gallo@secomind.com>
@rbino rbino merged commit c7dfb17 into edgehog-device-manager:main Dec 5, 2023
1 check passed
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.

Define Interface for remote terminal/device forwarder feature
5 participants