-
Notifications
You must be signed in to change notification settings - Fork 17
remote control: add initial remote control gRPC server #154
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
remote control: add initial remote control gRPC server #154
Conversation
3f0e08d to
9cdc3b7
Compare
|
This pull request now has conflicts with the target branch. Please resolve these conflicts and force push the updated branch. |
Add a samba command object for smbstatus. Signed-off-by: John Mulligan <[email protected]>
Add exclusions to the paths that we will be using to store generated source code related to grpc. Signed-off-by: John Mulligan <[email protected]>
Configure mypy not to check the files we plan to generate from the grpc toolchain. Signed-off-by: John Mulligan <[email protected]>
Signed-off-by: John Mulligan <[email protected]>
The plan is to provide a basic gRPC related API for clouds that want to use samba containers and do not want to exec into the containers directly to run commands. This creates a place for that. Signed-off-by: John Mulligan <[email protected]>
Add a new control.proto file that will be the basis for a cloud-ready gRPC remote-control API for certain samba commands. Signed-off-by: John Mulligan <[email protected]>
9cdc3b7 to
58359ff
Compare
|
CI passes and I tested a few scenarios manually within containers I built. The basics seemed to work OK, although I didn't check with mtls enabled. I think this is a good enough start that we can iterate on any issues that occur as they occur. |
58359ff to
7493968
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I don't have very specific comments about the new feature or its implementation (yet),
I suggest that we establish the standard of not adding copyright+license comment-header blocks:
They are redundant and therefore provide no value:
- copyright information is cintained in the git log
- the license is store centrally in the repo, applicable to all source files.
GPL projects are requested by GNU to add disclaimer headers:
gnu.org/licenses/gpl-howto.html#copyright-disclaimer
But as a counter-example, the Apache Foundation asks Apache-licensed projects NOT to add these disclaimers to source files.
https://www.apache.org/legal/apply-license.html#contributor-copyright
As it adds no value, I request that we omit/remove those comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
|
@obnoxxx I am just following the established precedent from the beginning of the project. Furthermore, this is not apache licensed so I don't think the ASF recommendations apply. If you feel strongly enough about it please file a separate issue to track this topic and don't block this PR because of an "administrative issue". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, that's a well organized coding style..kudos.
Looks really good to me, thanks.
See below for some minor comments.
Add a new tox environment that is to be used for regenerated python source files from .proto files. Currently, it is up to the contributor to make sure these are up-to-date. A second env is provided for use on centos/fedora with packaged versions for grpc and protobuf in order to make sure the generated files match what the planned deployment containers will support. Signed-off-by: John Mulligan <[email protected]>
Commit the files that are generated. We don't expect to be changing the interface a lot and we already commit a few other generated files in the project. IMO it's cleaner to trying to rebuild on release or install in python-land. Signed-off-by: John Mulligan <[email protected]>
Create a new backend.py module for working with samba commands within the planned rpc server. This file is not grpc specific but since no other consumers exists it can live in the grpc dir. If we ever add other server types we could move it then. Signed-off-by: John Mulligan <[email protected]>
Add a new server.py module to set up and run a gRPC server. The server can be configured using a class provided by this module that supplies information like network and (m)tls configuration. The server requires a backend that can be swapped out for testing purposes Signed-off-by: John Mulligan <[email protected]>
Signed-off-by: John Mulligan <[email protected]>
Signed-off-by: John Mulligan <[email protected]>
Add a new command line support library for "remote control". Remote control acts as a bridge between local samba command line tools and APIs and remote services that want to manage our instances. Right now the only supported (sub) command is `serve`. The only supported server type is `--grpc`. However, creating a new module with allows for room to grow as well as not interfere with the core sambacc feature set. Signed-off-by: John Mulligan <[email protected]>
Add the new python modules we just created for grpc and remotecontrol. Add a new samba-remote-control script. Add a grpc extra for our _optional_ grpc dependencies (as core sambacc works fine w/o grpc). Signed-off-by: John Mulligan <[email protected]>
Enable the grpc server tests rather than skipping them in the automated test suite. Signed-off-by: John Mulligan <[email protected]>
Update the spec file so that a new `+grpc` package gets produced for the new grpc extra. Signed-off-by: John Mulligan <[email protected]>
7493968 to
a5a7bcc
Compare
Pull request has been modified.
This topic is not something I want to block this PR on, as it is doing nothing new with regards to file headers. Please file an issue or we can discuss it in a different forum. Thanks.
|
@phlogistonjohn wrote:
right, and I have basically proposed to adapt a new standard here. < Furthermore, this is not apache licensed right. I know and I am happy about it.
right, and I did not mean to imply that. I mainly gave the ASF recommendations/requirements If you feel strongly enough about it please file a separate issue to track this topic I was and stil am indeed planning to do this a suggestion for all our SINK projects.
Apologies for creating this impression. It was not my intention to block this but it was meant more as a suggestion. Let's discuss this further outside this PR which is merged now anyways. |
Establish a new sambacc feature: a server that lets outside services "remote control" samba. This server acts as a bridge between an external environment, such as a cloud control plane and samba's internal apis and command line tools.
The initial server type is based on gRPC and can be used to list connected clients and disconnect clients and shares.
Putting the logic inside sambacc avoids having to "pullute" the samba project and/or samba server code with things that are typicaly outside the scope of samba proper.